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, cbla

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. > kcolorcombo.cp

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 dedu

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, ngra

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, kde-framewor

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&id=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, michaelh,

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, cfeck

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 innerColo

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()) > / 25

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&id=82517 BRANCH named_color_suppor

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 op

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 desi

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(&unused, &unused, &v); > +textColor = v > 128 ? Qt::black : Qt::white; > +} I would probably do what Kirigami does for `ColorUtils::brightnessForColor` here: `((0.299 * color.

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&id=82484 BRANCH named_color_support REVISION DETAIL https://ph

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&id=82483 BRANCH named_color_support REVISION DETAIL https://ph

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 KCom

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 wrote

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&id=82482 BRANCH n

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&id=82481 BRANCH name

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&id=82480 BRANCH name

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&id=82479 BRANCH named_color_support REVISION DETAIL https://ph

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&id=82478 BRANCH named_color_su

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&id=824

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 https://phabricator.kde.org/D29502?vs=82475&id=8

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&id=82475 BRANCH named_color_support REVI

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 = option.palette.color(QPalette::Highlight);

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

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

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, c

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 kcolorcombo.cpp

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&id=82219 BRANCH named_color_support REVISION DETAIL https:

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 = 0;

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 backgroun

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 KWidgetsAdd

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 To:

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 patch

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, kde-framewor

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&id=82179 BRANCH named_color_support REVISION DETAIL https://phabricator.

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&id=82178 BRANCH named_color_support REVISION DETAIL https://phabricator.

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 RE