The selected color is always printed as "#RRGGBB" in hex. A new printf-style "--format" option could allow different outputs. Default is "#%02X%02X%02X", but users might want to use "QColor(%d, %d, %d)" instead for coding purposes. Bonus points if it detects floating point specifiers (%e, %f, %g) and converts to 0...1 range.
Created attachment 91130 [details] KDialog patch The patch kdialog.diff applies to the file kde-baseapps/kdialog/kdialog.cpp. Two color outputs are allowed now: hex (or default) and QColor(%d,%d,%d). It is possible to specify the %f key for converting color components to 0...1 range.
The idea was to use the sprintf family of functions (not sure if there is a Qt equivalent), so that the user can use any (valid) format string.
Created attachment 104072 [details] allow set color format use regx for strictly format check for (e, g, f, x, d) with specific precision. e.g. allowed: %3f%4f%5f, %2e%2e%4e not allowed: %2e%2d%2e, %1f%1f%1f%1f
Please read comment #2.
Created attachment 104233 [details] v2_kdialog_allow_set_output_color_format Hi Christoph, New patch is based on QString's sprintf instead of cout concatenate in the previous patch and format specifier is more robost (e.g. "%.6f, %+2.f %3.3f") Please take a look, thx
Thanks, that is already an improvement, but not ready yet. I would have really preferred if you used our code review tools instead of adding the patch to this feature request, but I will spare you that for now and add my review comments here. - The format should be extracted from a separate '--format' argument via the arguments parser. If other xdialog type command line tools use "--getcolor [format]" instead, please add a reference. - QString::sprintf() is an obsolete member. Use QString::asprintf() or QString::vasprintf() instead. - Format strings are expected to be UTF-8. Use .toUtf8().constData() instead of .toLocal8Bit().data() - Your regexes do not parse '$' positional arguments (to reorder R,G,B), nor '%o', '%i', and '%u'. Flags '#' and '-' should be handled, too. - Length specifiers such as 'h', 'll', 'L' etc. as well as variable specifiers '*' need to be stripped for safety. Otherwise it could reference wrong memory when reading the values from the stack. - You are using contains(QRegularExpression("[dDxX]")) but _all_ your regexes contain 'd' for digits. Maybe use [0-9] explicitely. - The format string can also contain ordinary characters before, between, and after the values, e.g. --format="QColor(%d, %d, %d)" or --format="#%02X%02X%02X". These need to be visible in the result. - If possible, refactor the code to loop over number of arguments with a regexp for each of them. This is optional for now, and not required for passing review, but helps when alpha support is added to kdialog, which needs to handle 3 or 4 arguments for later RGB vs ARGB usage. If you need a reference for printf-style format strings, please see http://pubs.opengroup.org/onlinepubs/9699919799/functions/sprintf.html
Hi Christoph, firstly thanks for the above advice. (In reply to Christoph Feck from comment #6) > Thanks, that is already an improvement, but not ready yet. > > I would have really preferred if you used our code review tools instead of > adding the patch to this feature request, but I will spare you that for now > and add my review comments here. OK, is https://git.reviewboard.kde.org the right place? > - Your regexes do not parse '$' positional arguments (to reorder R,G,B), nor > '%o', '%i', and '%u'. Flags '#' and '-' should be handled, too. > > - Length specifiers such as 'h', 'll', 'L' etc. as well as variable > specifiers '*' need to be stripped for safety. Otherwise it could reference > wrong memory when reading the values from the stack. will revise the regx to suite more specifiers > - You are using contains(QRegularExpression("[dDxX]")) but _all_ your > regexes contain 'd' for digits. Maybe use [0-9] explicitely. I think [d] is only matched when your specifier is char 'd' instead of \d. and when I feed "%0002d" to sprintf, I got warned. That's why I choose it as [1-9] for decimal case. right? > - The format string can also contain ordinary characters before, between, > and after the values, e.g. --format="QColor(%d, %d, %d)" or > --format="#%02X%02X%02X". These need to be visible in the result. OK, that means release "^" and "$" check for head and tail. > - If possible, refactor the code to loop over number of arguments with a > regexp for each of them. This is optional for now, and not required for > passing review, but helps when alpha support is added to kdialog, which > needs to handle 3 or 4 arguments for later RGB vs ARGB usage. sounds like use "(%[1-9]+\\.?\\d*d|%d)" to loop over the arguments.
> OK, is https://git.reviewboard.kde.org the right place? Or Phabricator: https://phabricator.kde.org/differential/diff/create/
Git commit c78683dde1641f26d2bbcff9f27493bf12dbb4b3 by Christoph Feck, on behalf of Yuehang Wu. Committed on 10/03/2017 at 22:06. Pushed by cfeck into branch 'master'. Allow setting output format for --getcolor FIXED-IN: 17.04 Differential Revision: https://phabricator.kde.org/D4966 M +35 -3 src/kdialog.cpp https://commits.kde.org/kdialog/c78683dde1641f26d2bbcff9f27493bf12dbb4b3