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. #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 > 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<QPair<QString, QColor>>; 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
