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

[Kurt Hindenburg]

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, cblack, broulik, cfeck, kde-frameworks-devel, LeGast00n, 
michaelh, ngraham, bruns

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

[Friedrich W. H. Kossebau]

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.cpp:253
> +list.reserve(d->colorList.size());
> +for (auto color : d->colorList) {
> +list.append({color.second});

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

> kcolorcombo.cpp:264
> +ColorList list;
> +for (int index = 0; index < STANDARD_PALETTE_SIZE; ++index) {
> +list += {QString(), standardColor(index)};

list.reserve(STANDARD_PALETTE_SIZE);

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  named_color_support

REVISION DETAIL
  https://phabricator.kde.org/D29502

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham, #vdg
Cc: kossebau, cblack, broulik, cfeck, kde-frameworks-devel, LeGast00n, 
michaelh, ngraham, bruns

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

[Friedrich W. H. Kossebau]

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 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. 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). "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.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  named_color_support

REVISION DETAIL
  https://phabricator.kde.org/D29502

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham, #vdg
Cc: kossebau, cblack, broulik, cfeck, kde-frameworks-devel, LeGast00n, 
michaelh, ngraham, bruns

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

[Gustavo Carneiro]

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, ngraham, 
bruns

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

[Tomaz Canabrava]

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-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

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

[Gustavo Carneiro]

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
  https://phabricator.kde.org/D29502

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h
  tests/kcolorcombotest.cpp

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.

[Gustavo Carneiro]

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, ngraham, 
bruns

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

[patrick j pereira]

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, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

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

[patrick j pereira]

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 innerColor = ((cv.type() == QVariant::Color) ? cv.value() 
> : QPalette::Base);
> +

missing const.

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.

[Gustavo Carneiro]

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()) 
> / 255) > 0.5 ? Qt::black : Qt::white`

Done!

> cfeck wrote in kcolorcombo.cpp:89
> Please don't use "value" component to calculate the brightness of a color. 
> #81 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

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.

[Gustavo Carneiro]

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.

[Gustavo Carneiro]

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.

[Gustavo Carneiro]

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

REVISION DETAIL
  https://phabricator.kde.org/D29502

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h
  tests/kcolorcombotest.cpp

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.

[Gustavo Carneiro]

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.

[Gustavo Carneiro]

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.

[Gustavo Carneiro]

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 optionally named colours.

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.

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.

[Gustavo Carneiro]

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.

[Carson Black]

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 designers like, a better data structure would encapsulate named groups of 
optionally named colours.

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.

[Gustavo Carneiro]

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 a problem, consider the following code block:

  for (int i = 0; i  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.

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.

[Carson Black]

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 * color.green() + 0.114 * color.blue()) / 
255) > 0.5 ? Qt::black : Qt::white`

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.

[Gustavo Carneiro]

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
  https://phabricator.kde.org/D29502

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h
  tests/kcolorcombotest.cpp

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.

[Gustavo Carneiro]

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
  https://phabricator.kde.org/D29502

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h
  tests/kcolorcombotest.cpp

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.

[Gustavo Carneiro]

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.

[Gustavo Carneiro]

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.

[Gustavo Carneiro]

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.

[Gustavo Carneiro]

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 KComboColor {
>   Q_PROPERTY(QColor color MEMBER color)
>   Q_PROPERTY(QString name MEMBER name)
>   public:
>   QColor color;
>   QString name;
>   }

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

> 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?)

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.

[Gustavo Carneiro]

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 in kcolorcombo.cpp:83
> missing reference for innercolor ?

Yes, reference not found for innercolor or innerColor

> patrickelectric wrote in kcolorcombo.cpp:107
> It's missing const

Done!

> patrickelectric wrote in kcolorcombo.cpp:107
> 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);

Done!

> patrickelectric wrote in kcolorcombo.cpp:107
> 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);

Done!

> patrickelectric wrote in kcolorcombo.cpp:107
> It's missing const

Done!

> cfeck wrote in kcolorcombo.cpp:277
> Variables declared in 'for' are local, so just use 'color'.

Done!

> cfeck wrote in kcolorcombo.h:91
> typos: of; selection

Done!

> cfeck wrote in kcolorcombo.h:110
> typo: selection

Done!

> cfeck wrote in kcolorcombo.h: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).

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.

[Gustavo Carneiro]

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
  named_color_support

REVISION DETAIL
  https://phabricator.kde.org/D29502

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h
  tests/kcolorcombotest.cpp

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.

[Gustavo Carneiro]

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
  named_color_support

REVISION DETAIL
  https://phabricator.kde.org/D29502

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h
  tests/kcolorcombotest.cpp

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.

[Gustavo Carneiro]

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
  named_color_support

REVISION DETAIL
  https://phabricator.kde.org/D29502

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h
  tests/kcolorcombotest.cpp

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.

[Gustavo Carneiro]

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
  https://phabricator.kde.org/D29502

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h
  tests/kcolorcombotest.cpp

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.

[Gustavo Carneiro]

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
  named_color_support

REVISION DETAIL
  https://phabricator.kde.org/D29502

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h
  tests/kcolorcombotest.cpp

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.

[Gustavo Carneiro]

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

BRANCH
  named_color_support

REVISION DETAIL
  https://phabricator.kde.org/D29502

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h
  tests/kcolorcombotest.cpp

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.

[Gustavo Carneiro]

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=82476

BRANCH
  named_color_support

REVISION DETAIL
  https://phabricator.kde.org/D29502

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h
  tests/kcolorcombotest.cpp

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.

[Gustavo Carneiro]

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

REVISION DETAIL
  https://phabricator.kde.org/D29502

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h
  tests/kcolorcombotest.cpp

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.

[patrick j pereira]

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);
> -} else {
> -innercolor = option.palette.color(QPalette::Base);
> -}
> +QColor innercolor = option.palette.color( isSelected ? 
> QPalette::Highlight : QPalette::Base);
> +

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

> kcolorcombo.cpp:83
> +
> +auto textColorLambda = [, , , 
> innercolor]() -> QColor {
> +QColor textColor;

missing reference for innercolor ?

> kcolorcombo.cpp:107
>  painter->setRenderHint(QPainter::Antialiasing);
> -painter->drawRoundedRect(innerrect, 2, 2);
> +QRect colorRect = !tv.toString().isEmpty() ? 
> QRect(innerrect.x(), innerrect.y() + innerrect.height() / 2, 
> innerrect.width(), innerrect.height() / 2) : innerrect;
> +painter->drawRoundedRect(colorRect, 2, 2);

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);

> kcolorcombo.cpp:107
>  painter->setRenderHint(QPainter::Antialiasing);
> -painter->drawRoundedRect(innerrect, 2, 2);
> +QRect colorRect = !tv.toString().isEmpty() ? 
> QRect(innerrect.x(), innerrect.y() + innerrect.height() / 2, 
> innerrect.width(), innerrect.height() / 2) : innerrect;
> +painter->drawRoundedRect(colorRect, 2, 2);

It's missing const

> kcolorcombo.h:52
>  Q_PROPERTY(QList colors READ colors WRITE setColors)
> +Q_PROPERTY(QList> namedColors READ namedColors 
> WRITE setNamedColors)
>  

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;
  }

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.

[Christoph Feck]

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

> kcolorcombo.cpp:277
> +list.reserve(d->colorList.size());
> +for (auto iColor : d->colorList) {
> +list.append({iColor.second});

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

> kcolorcombo.h:61
>  
> +/** Struct used in named colors list */
> +using ColorList = QList>;

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?)

> kcolorcombo.h:91
> + * standard list.
> + * @param colors map os named colors. If empty, the selction list
> + *  reverts to the standard list.

typos: of; selection

> kcolorcombo.h:97
> +/**
> + * Inserts a named color at a specific position in the standard list
> + * @param index position in the list

Sentence misses a full stop.

> kcolorcombo.h:110
> +/**
> + * Return the list of named colors available for selecion.
> + * If no named color, return namedColor is empty.

typo: selection

> kcolorcombo.h:111
> + * Return the list of named colors available for selecion.
> + * If no named color, return namedColor is empty.
> + * @return list of named colors

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).

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.

[Gustavo Carneiro]

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, cblack, michaelh, ngraham, 
bruns

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

[Gustavo Carneiro]

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:288
> why is there a for running with all code comented out?

Done!

> tcanabrava wrote in kcolorcombo.h:60
> QPair - no need for a struct.

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.

[Gustavo Carneiro]

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
  https://phabricator.kde.org/D29502

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h
  tests/kcolorcombotest.cpp

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.

[Tomaz Canabrava]

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; colorIndex < colors.count(); colorIndex++) {
> +namedColors.insert(colorIndex, { QString(), colors[colorIndex] });

for(auto color : colors)

> kcolorcombo.cpp:288
> +for (int index = 0; index < STANDARD_PALETTE_SIZE; ++index) {
> +//list += {QString(), standardColor(index)};
> +}

why is there a for running with all code comented out?

> kcolorcombo.cpp:291
> +return list;
>  } else {
>  return d->colorList;

remove the else, just return directly.

> araujoluis wrote in kcolorcombo.h:60
> broulik try a solution in several ways, but it looks like this was the most 
> convenient one found so far.

QPair - no need for a struct.

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.

[Gustavo Carneiro]

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 background?
  >
  >
  > not yet, I talked to gustavo and he's working in an updated version of the 
patch.
  >
  > This is the current - not in the diff yet - version:
  >  F8293330: image.png 
  
  
  Tomaz the new patch is now available for review

INLINE COMMENTS

> broulik wrote in kcolorcombo.h:60
> Do we really want to leak this `struct` into public API?

broulik try a solution in several ways, but it looks like this was the most 
convenient one found so far.

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.

[Gustavo Carneiro]

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.

[Gustavo Carneiro]

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 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29502?vs=82179=82200

BRANCH
  named_color_support

REVISION DETAIL
  https://phabricator.kde.org/D29502

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h
  tests/kcolorcombotest.cpp

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.

[Kai Uwe Broulik]

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

[Tomaz Canabrava]

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.
  
  This is the current - not in the diff yet - version:
  F8293330: image.png 

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D29502

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

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

[Christoph Feck]

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-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

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

[Gustavo Carneiro]

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
  https://phabricator.kde.org/D29502

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

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

[Gustavo Carneiro]

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
  https://phabricator.kde.org/D29502

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

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

[Gustavo Carneiro]

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.

[Gustavo Carneiro]

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.

[Gustavo Carneiro]

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.

[Gustavo Carneiro]

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.

[Gustavo Carneiro]

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 

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  named_color_support

REVISION DETAIL
  https://phabricator.kde.org/D29502

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns