kwidgetsaddons: Add a named colors support in KColorCombo.
AcceptedPublic

Authored by araujoluis on Thu, May 7, 7:04 AM.

Details

Summary

Signed-off-by: Gustavo Carneiro <gcarneiroa@hotmail.com>

This PR adds a feature to name colors in KColorCombo in special cases where, for example, the color of the main theme must be in the list, or some other important information must appear to the user of the application.

All functionality has been maintained for compatibility with applications in previous versions.

The developer can use something like:

KColorCombo *colorCombo = new KColorCombo;
colorCombo.insertNamedColor(0, {i18n("info xyz ..."), Qt::Blue});

There, all colors will be kept and the custom color with an explanation will be added in the position informed in the first parameter.

For compatibility reasons and to keep the features close to the current ones, the following functions have also been added:

void setColors(const QList<QColor> &colors);
void setNamedColors(const QList<KNamedColor> &colors);
QList<KNamedColor> namedColors() const;

Functions that are self explanatory.

This PR was created with the intention of being used in the future in PR: 77

This will facilitate some issues related to the colors that are configured in the tabs, see PR 77.

I am available for further clarifications and adaptations if they deem necessary.

Diff Detail

Repository
R236 KWidgetsAddons
Branch
named_color_support
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26701
Build 26719: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
araujoluis marked 7 inline comments as done.Sat, May 9, 9:05 AM

Changes made.

cfeck added inline comments.Sat, May 9, 10:27 AM
src/kcolorcombo.cpp
72

Please don't use "value" component to calculate the brightness of a color. #000081 is much darker than #808080. To decide, use qGray(). The threshold also needs to be higher than 128; I use 170, but this mostly depends on correctness of monitor gamma settings.

See https://stackoverflow.com/questions/3942878/how-to-decide-font-color-in-white-or-black-depending-on-background-color

253

Variables declared in 'for' are local, so just use 'color'.

src/kcolorcombo.h
61

The comment still says "struct". Maybe clarify that this list is actually used as a map.

(I guess since mapping would happen in both directions, using a QMap isn't useful?)

91

typos: of; selection

97

Sentence misses a full stop.

110

typo: selection

111

This sentence is confusing. I guess you wanted to write "if there are no named colors, the returned list is empty." (to clarify that it won't return the standard list).

Just some small tips.

src/kcolorcombo.cpp
58–59

Missing const, also the name should be innerColor with a capital C if we are following the code style from this file.

66

missing reference for innercolor ?

83–86

Maybe changing the logic to make it more simpler:

QRect colorRect = tv.toString().isEmpty() ? innerrect : QRect(innerrect.x(), innerrect.y() + innerrect.height() / 2, innerrect.width(), innerrect.height() / 2);
83–86

It's missing const

src/kcolorcombo.h
52

This is just a suggestion and not something that's necessary to do, but maybe it could help to create a simple class to replace QPair:

class KComboColor {
    Q_PROPERTY(QColor color MEMBER color)
    Q_PROPERTY(QString name MEMBER name)
public:
    QColor color;
    QString name;
}
  • kwidgetsaddons: kcolorcombo: rename a set constant for innerColor
araujoluis marked 2 inline comments as done.
  • kwidgetsaddons: kcolorcombo: set QRect colorRect as constant
araujoluis marked an inline comment as done.
  • kwidgetsaddons: kcolorcombo: rename innerrect to innerRect
araujoluis marked an inline comment as done.
  • kwidgetsaddons: update comments
  • kwidgetsaddons: kcolorcombo: fix comments
araujoluis marked an inline comment as done.
  • kwidgetsaddons: kcolorcombo: fix comments
araujoluis marked an inline comment as done.
  • kwidgetsaddons: kcolorcombo: fix comments
araujoluis marked an inline comment as done.
  • kwidgetsaddons: kcolorcombo: rename variable
araujoluis marked an inline comment as done.Mon, May 11, 12:42 AM
araujoluis added inline comments.
src/kcolorcombo.cpp
58–59

Done!

66

Yes, reference not found for innercolor or innerColor

83–86

Done!

83–86

Done!

83–86

Done!

83–86

Done!

253

Done!

src/kcolorcombo.h
91

Done!

110

Done!

111

Done!

araujoluis marked 14 inline comments as done.Mon, May 11, 12:47 AM
araujoluis added inline comments.
src/kcolorcombo.h
52

Good suggestion, I will be thinking how to do it as this would alter the use of lists within the code.

61

Done!

araujoluis marked 3 inline comments as done.Mon, May 11, 12:48 AM
araujoluis marked an inline comment as done.Mon, May 11, 12:50 AM
  • kwidgetsaddons: kcolorcombo: fix comments
araujoluis marked an inline comment as done.Mon, May 11, 12:52 AM
  • kwidgetsaddons: kcolorcombo: fix comments
cblack added a subscriber: cblack.Mon, May 11, 1:00 AM
cblack added inline comments.
src/kcolorcombo.cpp
73

I would probably do what Kirigami does for ColorUtils::brightnessForColor here: ((0.299 * color.red() + 0.587 * color.green() + 0.114 * color.blue()) / 255) > 0.5 ? Qt::black : Qt::white

araujoluis added inline comments.Mon, May 11, 1:03 AM
src/kcolorcombo.h
61

Using QMap would cause me a problem, consider the following code block:

for (int i = 0; i <STANDARD_PALETTE_SIZE; ++ i) {
                 if (standardColor (i) == color) {
                     q-> setCurrentIndex (i + 1);

I need variable i for iteration with the call q-> setCurrentIndex (i + 1) and this method would not be available with QMap.

cblack added inline comments.Mon, May 11, 1:05 AM
src/kcolorcombo.h
52

Is a simple tuple list really the best way to represent colours? Knowing what most designers like, a better data structure would encapsulate named groups of optionally named colours.

araujoluis marked an inline comment as done.Mon, May 11, 1:25 AM
araujoluis marked an inline comment as done.Mon, May 11, 1:28 AM
araujoluis added inline comments.
src/kcolorcombo.h
52

I believe that yes, each color has its own representation, and the way it was proposed, if the color is not available, the name used is the hex value of the color.

araujoluis marked 2 inline comments as done.Mon, May 11, 1:29 AM
araujoluis edited the summary of this revision. (Show Details)Mon, May 11, 12:31 PM
  • kwidgetsaddons: kcolorcombo: fix paint font colors and simplify painting
araujoluis marked 2 inline comments as done.Mon, May 11, 12:36 PM
araujoluis added inline comments.
src/kcolorcombo.cpp
72

Done!

73

Done!

araujoluis marked 2 inline comments as done.Mon, May 11, 12:37 PM
araujoluis marked 2 inline comments as done.
src/kcolorcombo.cpp
67

Missing const.

68

missing const.

patrickelectric accepted this revision.Mon, May 11, 12:43 PM
This revision is now accepted and ready to land.Mon, May 11, 12:43 PM
  • kwidgetsaddons: kcolorcombo: set a constant variables.
araujoluis marked 2 inline comments as done.Mon, May 11, 12:45 PM

+1. @cfeck do you think this can land now?

Isn't the recommendation to rather avoid using things like QPair, and instead used properly defined structs? And ideally non-nested ones, to help with cases of forward declarations?
Even QPair's API dox says so:
"The advent of C++11 automatic variable type deduction (auto) shifts the emphasis from the type name to the name of functions and members. Thus, QPair, like std::pair and std::tuple, is mostly useful in generic (template) code, where defining a dedicated type is not possible."

Code which uses ".first" and ".second" is harder to understand. And any users of the new API who want to pass in named colors but who cannot make use of auto-derived type name or init-list constructors, so have to explicitly write the name would also prefer some named type over QPair<QString, QColor>. So please reconsider using some non-nested struct, perhaps named e.g. "KNamedColor".
The alias "ColorList" might be also confusing, as it misses to point out this is a list of named colors, not just a list of colors (one might naively think of QList<QColor>). "NamedColorList" would be less ambiguous.

And as long as we are Qt5 at least., QVector would also be favourable here over QList given the size of the list item type and given that insertions are not expected to be typically done on this type.

kossebau added inline comments.Sat, May 16, 12:54 PM
src/kcolorcombo.cpp
222

auto& as we just want a reference, avoids a copy constructor call each time.

253

auto& as we just want a reference, avoids a copy constructor call each time.

264

list.reserve(STANDARD_PALETTE_SIZE);