D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

hindenburg added a comment. What's the status of this? Did it get moved to invent MR? REPOSITORY R236 KWidgetsAddons BRANCH named_color_support REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham, #vdg Cc: kossebau,

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

kossebau added inline comments. INLINE COMMENTS > kcolorcombo.cpp:222 > +namedColors.reserve(colors.size()); > +for (auto color : colors) { > +namedColors.append({QString(), color}); `auto& ` as we just want a reference, avoids a copy constructor call each time. >

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

kossebau added a comment. 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

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis added a reviewer: VDG. REPOSITORY R236 KWidgetsAddons BRANCH named_color_support REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham, #vdg Cc: cblack, broulik, cfeck, kde-frameworks-devel, LeGast00n, michaelh,

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

tcanabrava added a comment. +1. @cfeck do you think this can land now? REPOSITORY R236 KWidgetsAddons BRANCH named_color_support REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: cblack, broulik, cfeck,

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis updated this revision to Diff 82520. araujoluis added a comment. - kwidgetsaddons: kcolorcombo: set a constant variables. REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29502?vs=82517=82520 BRANCH named_color_support REVISION DETAIL

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis marked 2 inline comments as done. REPOSITORY R236 KWidgetsAddons BRANCH named_color_support REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: cblack, broulik, cfeck, kde-frameworks-devel, LeGast00n,

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

patrickelectric accepted this revision. This revision is now accepted and ready to land. REPOSITORY R236 KWidgetsAddons BRANCH named_color_support REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: cblack, broulik,

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

patrickelectric added inline comments. INLINE COMMENTS > kcolorcombo.cpp:67 > // inner color > +QVariant tv = index.data(Qt::DisplayRole); > QVariant cv = index.data(ColorRole); Missing const. > kcolorcombo.cpp:69 > QVariant cv = index.data(ColorRole); > +QColor

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis marked 2 inline comments as done. araujoluis added inline comments. INLINE COMMENTS > cblack wrote in kcolorcombo.cpp:90 > I would probably do what Kirigami does for `ColorUtils::brightnessForColor` > here: `((0.299 * color.red() + 0.587 * color.green() + 0.114 * color.blue()) > /

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis marked 2 inline comments as done. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: cblack, broulik, cfeck, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis marked 2 inline comments as done. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: cblack, broulik, cfeck, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis updated this revision to Diff 82517. araujoluis added a comment. - kwidgetsaddons: kcolorcombo: fix paint font colors and simplify painting REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29502?vs=82484=82517 BRANCH named_color_support

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis edited the summary of this revision. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: cblack, broulik, cfeck, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis marked 2 inline comments as done. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: cblack, broulik, cfeck, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis marked an inline comment as done. araujoluis added inline comments. INLINE COMMENTS > cblack wrote in 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

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis marked an inline comment as done. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: cblack, broulik, cfeck, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

cblack added inline comments. INLINE COMMENTS > kcolorcombo.h:52 > Q_PROPERTY(QList colors READ colors WRITE setColors) > +Q_PROPERTY(QList> namedColors READ namedColors > WRITE setNamedColors) > Is a simple tuple list really the best way to represent colours? Knowing what most

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis added inline comments. INLINE COMMENTS > cfeck wrote in 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?) Using QMap would cause me

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

cblack added inline comments. INLINE COMMENTS > kcolorcombo.cpp:90 > +innerColor.getHsv(, , ); > +textColor = v > 128 ? Qt::black : Qt::white; > +} I would probably do what Kirigami does for `ColorUtils::brightnessForColor` here: `((0.299 * color.red() + 0.587 *

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis updated this revision to Diff 82484. araujoluis added a comment. - kwidgetsaddons: kcolorcombo: fix comments REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29502?vs=82483=82484 BRANCH named_color_support REVISION DETAIL

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis updated this revision to Diff 82483. araujoluis added a comment. - kwidgetsaddons: kcolorcombo: fix comments REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29502?vs=82482=82483 BRANCH named_color_support REVISION DETAIL

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis marked an inline comment as done. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: broulik, cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis marked an inline comment as done. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: broulik, cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis marked 3 inline comments as done. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: broulik, cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis marked 14 inline comments as done. araujoluis added inline comments. INLINE COMMENTS > patrickelectric wrote in 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

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis marked an inline comment as done. araujoluis added inline comments. INLINE COMMENTS > patrickelectric wrote in kcolorcombo.cpp:74 > Missing const, also the name should be `innerColor` with a capital C if we > are following the code style from this file. Done! > patrickelectric

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis updated this revision to Diff 82482. araujoluis marked an inline comment as done. araujoluis added a comment. - kwidgetsaddons: kcolorcombo: rename variable REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29502?vs=82481=82482 BRANCH

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis updated this revision to Diff 82481. araujoluis marked an inline comment as done. araujoluis added a comment. - kwidgetsaddons: kcolorcombo: fix comments REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29502?vs=82480=82481 BRANCH

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis updated this revision to Diff 82480. araujoluis marked an inline comment as done. araujoluis added a comment. - kwidgetsaddons: kcolorcombo: fix comments REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29502?vs=82479=82480 BRANCH

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis updated this revision to Diff 82479. araujoluis added a comment. - kwidgetsaddons: kcolorcombo: fix comments REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29502?vs=82478=82479 BRANCH named_color_support REVISION DETAIL

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis updated this revision to Diff 82478. araujoluis marked an inline comment as done. araujoluis added a comment. - kwidgetsaddons: update comments REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29502?vs=82477=82478 BRANCH

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis updated this revision to Diff 82477. araujoluis marked an inline comment as done. araujoluis added a comment. - kwidgetsaddons: kcolorcombo: rename innerrect to innerRect REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29502?vs=82476=82477

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis updated this revision to Diff 82476. araujoluis marked 2 inline comments as done. araujoluis added a comment. - kwidgetsaddons: kcolorcombo: set QRect colorRect as constant REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis updated this revision to Diff 82475. araujoluis added a comment. - kwidgetsaddons: kcolorcombo: rename a set constant for innerColor REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29502?vs=82219=82475 BRANCH named_color_support

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

patrickelectric added a comment. Just some small tips. INLINE COMMENTS > kcolorcombo.cpp:74 > bool paletteBrush = (k_colorcombodelegate_brush(index, > Qt::BackgroundRole).style() == Qt::NoBrush); > -if (isSelected) { > -innercolor =

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

cfeck added inline comments. INLINE COMMENTS > kcolorcombo.cpp:89 > +int unused, v; > +innercolor.getHsv(, , ); > +textColor = v > 128 ? Qt::black : Qt::white; Please don't use "value" component to calculate the brightness of a color. #81 is much darker

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis marked 7 inline comments as done. araujoluis added a comment. Changes made. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: broulik, cfeck, kde-frameworks-devel, LeGast00n,

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis marked 7 inline comments as done. araujoluis added inline comments. INLINE COMMENTS > tcanabrava wrote in kcolorcombo.cpp:244 > namedColors.reserve(colors.size()); Done! > tcanabrava wrote in kcolorcombo.cpp:245 > for(auto color : colors) Done! > tcanabrava wrote in

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis updated this revision to Diff 82219. araujoluis added a comment. - kwidgetsaddons: kcolorcombo: code refactoring REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29502?vs=82200=82219 BRANCH named_color_support REVISION DETAIL

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

tcanabrava added inline comments. INLINE COMMENTS > kcolorcombo.cpp:244 > +{ > +QList namedColors; > +for (int colorIndex = 0; colorIndex < colors.count(); colorIndex++) { namedColors.reserve(colors.size()); > kcolorcombo.cpp:245 > +QList namedColors; > +for (int colorIndex =

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis added a comment. In D29502#665600 , @tcanabrava wrote: > In D29502#665582 , @cfeck wrote: > > > Does the delegate ensure the text is rendered in a color visible over the colored

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis edited the summary of this revision. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: broulik, cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis updated this revision to Diff 82200. araujoluis added a comment. - kwidgetsaddons: kcolorcombo: fix colors() out of range - kwidgetsaddons: kcolorcombo: tests: add example for use named colors. - kwidgetsaddons: kcolorcombo: update named colow view. REPOSITORY R236

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

broulik added inline comments. INLINE COMMENTS > kcolorcombo.h:60 > +/** Struct used in NamedColors list */ > +struct KNamedColor > +{ Do we really want to leak this `struct` into public API? REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29502

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

tcanabrava added a comment. In D29502#665582 , @cfeck wrote: > Does the delegate ensure the text is rendered in a color visible over the colored background? not yet, I talked to gustavo and he's working in an updated version of the

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

cfeck added a comment. Does the delegate ensure the text is rendered in a color visible over the colored background? REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: cfeck,

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis updated this revision to Diff 82179. araujoluis added a comment. - restore code deleted by mistake REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29502?vs=82178=82179 BRANCH named_color_support REVISION DETAIL

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis updated this revision to Diff 82178. araujoluis added a comment. - Restore code deleted by mistake REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29502?vs=82173=82178 BRANCH named_color_support REVISION DETAIL

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis edited the summary of this revision. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis edited the summary of this revision. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis edited the summary of this revision. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis edited the summary of this revision. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29502 To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

araujoluis created this revision. araujoluis added reviewers: tcanabrava, patrickelectric, hindenburg, ngraham. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. araujoluis requested review of this revision. REVISION SUMMARY Signed-off-by: Gustavo Carneiro