D27017: [KColorUtils] Add hue(), chroma(), getHcyColor() and update documentation

[Matthew Woehlke]

mwoehlke added inline comments.

INLINE COMMENTS

> kcolorutils.cpp:60
>      *c = khcy.c;
>      *h = khcy.h;
>      *y = khcy.y;

I guess it will be less confusing if this is also normalized?

  *h = khcy.h + (khcy.h < 0.0 ? 1.0 : 0.0);

> kcolorutils.h:36
> +/**
> + * Calculate the hue of a color. The range is 0.0 (red) to 0.999999 (slightly blue red).
> + * This is based on linear RGB.

"0.999..." is a little odd, and I wonder if it is accurate for e.g. `QColor::fromRgba64(0xffff, 0, 1}`? Maybe something like "just under 1.0 (also red)" would be more useful? (If you want to get technical, "to (asymptotically) 1.0" would also work. Maybe "0.0 //approaching// 1.0"?)

The range is of course [0, 1), but I don't know how to say that in straight-forward English 🙂.

OTOH, I wonder if it's worth bothering, vs. just saying "0.0 to 1.0" and ignoring that it will never be //exactly// 1.0.

> kcolorutils.h:37
> + * Calculate the hue of a color. The range is 0.0 (red) to 0.999999 (slightly blue red).
> + * This is based on linear RGB.
> + * 

Suggestion: "The result is computed in linear (not sRGB) color space and may differ slightly from QColor::hue()."

For the others, drop "and may differ...".

> kcolorutils.h:69
> + * 
> + * The range of hue is cyclical. Examples: 0.0 and 1.0 are both red, -0.166667 and 0.833333 are both magenta.
> + * The range of chroma is from 0.0 (none) to 1.0 (full).

I guess here should use the same text as `hue`?

> kcolorutils.h:82
> + * 
> + * The range of hue is cyclical. Examples: 0.0 and 1.0 are both red, -0.166667 and 0.833333 are both magenta.
> + * The range of chroma is from 0.0 (none) to 1.0 (full).

This reads a little odd. I think "For example, 0.0 and ..." would be better.

> kcolorutils.h:83
> + * The range of hue is cyclical. Examples: 0.0 and 1.0 are both red, -0.166667 and 0.833333 are both magenta.
> + * The range of chroma is from 0.0 (none) to 1.0 (full).
> + * The range of luma is from 0.0 (black) to 1.0 (white).

It might be useful to add (to luma also) "Out of range values will be clamped.".

> kcolorutils.h:90
> + */
> +KGUIADDONS_EXPORT QColor getHcyColor(qreal hue, qreal chroma, qreal luma, qreal alpha = 1.0);
> +

This name feels a little odd, but I'd appreciate some broader input. Maybe `hcyColor`?

REPOSITORY
  R273 KGuiAddons

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

To: ndavis, #frameworks, dfaure
Cc: mwoehlke, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200204/33b1ac7e/attachment.html>