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(&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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200509/1cf36503/attachment.htm>