KColorScheme

[Matthew Woehlke]

On 2012-08-09 07:43, David Faure wrote:
> On Friday 20 July 2012 14:19:47 Matthew Woehlke wrote:
>> I think going with flags is probably a good idea; I would probably do
>> that by keeping the existing sets, roles and shades as distinct, but
>> allow combining them into one variable, e.g.:
>>
>> QPalette::ViewSet | QPalette::Background | QPalette::LinkRole |
>> QPalette::DarkShade | QPalette::Disabled
>
> Good to see we have the same idea.
>
> (Disabled is already orthogonal, that's ColorGroup, see below)

Yes, keeping that as-is is probably best; you're not going to be mixing 
and matching much. (Nor sets, which is why they're in KColorScheme's 
ctor. Not sure if it is preferable to handle those with a 
setCurrentColorSet, or as part of ColorRole.)

Mostly I'd really like to see Inactive+Disabled added as a group... :-)

> In practice though, this means making some of the current flags orthogonal, no?
> (I'm only looking at which changes are BIC, i.e. need to be done for Qt-5.0)
>
> If I just add Q_DECLARE_FLAGS(ColorRoles, ColorRole) and switch the api to use ColorRoles instead of ColorRole everywhere, then this doesn't seem to be enough, we also need to change some of the current enum values.
> E.g. Button becomes Button = Background | ButtonStuff (?) and
> ButtonText becomes Foreground | ButtonStuff (?), right?
>
> Could you make a complete proposal for how the current ColorRole enum should be changed?

ColorGroup -> flags
   0 = Active
   1 = Disabled
   2 = Inactive
  (3 = Disabled | Inactive)
...probably some special value for Current; will let you decide on that.

ColorRole -> flags (ColorRoleFlag, and keep ColorRole as the QFlags, as 
"ColorRoles" doesn't sound right IMHO)
   2 bits:
     0 = Background
     1 = Foreground
     2 = Decoration
     3 // reserved
   4 bits:
     0 = Normal
     1 = Positve
     2 = Negative
     3 = Neutral
     4 = Alternate (bg) / Inactive (fg)
     5 = Active (fg,bg) / Hover (d)
     6 = Visited
     7 = Link (fg,bg) / Focus (d)
   3 bits:
     0 = Window
     1 = View
     2 = Button
     3 // reserved
     4 = ToolTip
     5 = Selection
     6 // reserved
     7 // reserved
   3 bits:
     0 = Normal
     1 = Midlight
     2 = Light
     4 = Shadow
     6 = Dark
     7 = Mid

Shift multipliers are omitted from the above; bit order doesn't really 
matter. (I'd recommend 1-2 bits between bit groups for possible future 
expansion.) Masks should probably also be included.

I picked the second group values to make some vague sense if you combine 
bits (e.g. Neutral = Positive|Negative), though that's not strictly 
necessary. The third group could be part of roles or as mentioned above 
use a currentColorSet. Likewise the fourth group could use a separate 
shade() method as in KColorScheme. However, both would necessitate SIC 
between Qt4 -> Qt5, as some colors that have a ColorRole in Qt4 would no 
longer be accessible the same way. Putting them in ColorRole implies 
that you can ask for e.g. ToolTip | Link | ShadowShade, which isn't bad, 
but centralizes the code to generate that on demand. (I'd like to see 
the 4-arg KColorScheme::shade regardless, as well as all of KColorUtils, 
really, though probably in a different class.)

On a tangential thought, it might be nice if setForegroundRole(), etc. 
enforced the first group, with an overload to allow bypassing that (as a 
means of "helping" developers pay attention when to a11y violations, but 
it's also convenient).

> The current stuff is
>      enum ColorGroup { Active, Disabled, Inactive, NColorGroups, Current, All, Normal = Active };
>      enum ColorRole { WindowText, Button, Light, Midlight, Dark, Mid,
>                       Text, BrightText, ButtonText, Base, Window, Shadow,
>                       Highlight, HighlightedText,
>                       Link, LinkVisited,
>                       AlternateBase,
>                       NoRole,
>                       ToolTipBase, ToolTipText,
>                       NColorRoles = ToolTipText + 1,
>                       Foreground = WindowText, Background = Window
>                     };
>
> No need to introduce unused orthogonal concepts for now (such as "ViewSet" or "DarkShade",
> if I understand these correctly), but we at least need to split the background and text colors
> for the same things into "one value in the main enum | background or foreground".

If you want to map all of the old roles directly (and a few won't be 
possible), the set and shade bits need to be part of it. Some examples:

WindowText -> Window|Foreground|Normal
Link -> View|Foreground|Link
Shadow -> Button|Background|Normal|Shadow

I think you're going to have to either:
a. Accept a SIC and just let some roles be wrong for old code
b. Have a suffix on all the new flags
c. Put the new flags in some namespace that isn't QPalette, and keep a 
method taking the old enum for compatibility

For (b), I'd also consider having the "old" flags in a different enum 
with a compatibility method.

If you made Button=0, the shade roles could map correctly (if the names 
are overloaded), but Foreground, Background, and Link would then give 
the color for buttons. As-is, the shade roles are all wrong as is Link 
(all giving stuff from the Window set). However the actual results 
should be acceptable for many color schemes.

I'm not exactly sure what BrightText is... at any rate, it has no 
analogue (not by naming convention, anyway) in KColorScheme.

I'm probably forgetting something, but really need to get back to paid 
work :-). Questions welcomed.

> PS: re "qteDebug(xMyArea) << {...};", there's already something for this, currently a separate addon but planned
> for inclusion into Qt-5.1, see https://bugreports.qt-project.org/browse/QTBUG-19536
>
>> Since (AFAICT?) the 'active' state of a debug area cannot be changed at run-time,
>
> Actually in KDE4 it can, there's a dbus call to reload the kdebug settings. This probably won't be there in Qt5 though
> (no QtDBus dependency in QtCore).
>
> All in all, we're rather covered in the qDebug topic, while I really need your help on the QPalette topic :)

Sure... mostly, was thinking I've got mine down to where the cost of a 
disabled area is a handful of loads and function calls, and a few 
conditional jumps. No locks, no atomic operations, and it doesn't 
execute any of the operator<<.

-- 
Matthew