Bug 308802

in this particular case, we have 3 (!) different wrong colors on the same background.

the expansion arrow is dark blue instead of dark green, the line next to it is black instead of dark green, and the text is white instead of dark green.

Thanks for that kde-look link.

I think this bug might be misleading.

Upon code inspection there _is_ a bug that the contact list won't handle changes to the style immediately.
(now opened a new bug at https://bugs.kde.org/show_bug.cgi?id=308820 about that issue)

Upon restarting the contact list after changing the colour scheme, the colours are all appeared correct to me. Could you confirm?

I can confirm the colors are fine once you restart the contactlist

Git commit d57901ab55947c1f3bae152c7d30bd6704b056b7 by Martin Klapetek.
Committed on 23/10/2012 at 14:14.
Pushed by mklapetek into branch 'master'.

Use option's palette instead of storing app's palette in class var

Reviewed-by: Rohan Garg
REVIEW: 107008
Related: bug 308820

M  +4    -6    abstract-contact-delegate.cpp
M  +0    -1    abstract-contact-delegate.h
M  +4    -4    contact-delegate-compact.cpp
M  +4    -4    contact-delegate.cpp

http://commits.kde.org/telepathy-contact-list/d57901ab55947c1f3bae152c7d30bd6704b056b7

ok, that was another bug and indeed my screenshot was misleading.

but contrary to what david and rohan said, even after a restart, the color roles are still wrong. after restarting, it’s still purple/blue on green instead of green on green:

http://imgur.com/JglwO

listen closely: what’s happening here is that one widget type widget using the button color role (purple, the line) and three types using the windows color role (blue, group text, 
contact text, and group expansion arrow) are rendered with transparent backgrounds on top of a widget with the view color role (green, the contact list background).

so basically we forget to set the arrows’, labels’, and lines’ color-role to “view”, when we set their background to be transparent.

————————

this bug will be only fixed once no background has another basic color (blue, yellow, green, purple) than a widget drawn over it.

note that the selected entry has the right text color, but still the wrong arrow/line colors, so we need to take care of that, too, when selecting stuff.

that one is fixed as soon as above screenshot can be replicated with everything in the green area being green, except (the icons and) the yellow area, in which everything has to be yellow.

That was really confusing.

Are there any other tree views that you consider "correct" ?

I'll make a list of all the items and what roles they are currently using, you can tell me what they should be using.

Selected name text:
   QPalette::Active, QPalette::HighlightedText
Normal name text:
   QPalette::Active, QPalette::WindowText
Selected presence message text:
   QPalette::Disabled, QPalette::HighlightedText
normal presence message text:
   QPalette::Disabled, QPalette::WindowText
The line:
   QPalette::Inactive, QPalette::Button;
Group Arrows:
   using QStyle::drawPrimitive() so definitely right

Ideally please use names from:
http://doc.qt.digia.com/4.5/qpalette.html#ColorGroup-enum 
and http://doc.qt.digia.com/4.5/qpalette.html#ColorRole-enum

so we're all talking the same language.

Fwiw, there's a patch changing the group headers completely, can you perhaps try it out? 

https://git.reviewboard.kde.org/r/106763/

> tell me what they should be using

When using QPalette, only the following combinations are allowed (Fg on Bg):

    WindowText on Window (window surface, usually black on gray)
    Text on Base (input fields, usually black on white)
    Text on AlternateBase (for alternating row coloring, usually black on light gray)
    HighlightedText on Highlight (selections, usually white on blue)
    ButtonText on Button (usually black on gray)
    ToolTipText on ToolTipBase (usually black on yellow)

Some more foreground colors exists (BrightText, Link, LinkVisited), but since it is unclear to which background they apply, you should not use them.

Additionally, be careful when mixing different color groups (Disabled, Active, Inactive). You cannot assume that a Disabled foreground is visible on an Active background color, for example. If in doubt, use reduced alpha for less important items, instead of possibly using an unreadable combination.

If you need more control for color combinations, use KColorScheme.

I'm not convinced that changing opacity is a suitable workaround. I've had to fix stuff in Plasma where there were different levels of opacity being used. Thus resulting in 50 shades of grey.

Also grey and black at 50% transparency aren't the same thing, the background should not be showing through the end pixel result is a different colour depending on what it's on - which is wrong. It's a bad hacky workaround at best.

We can maybe use Active::Inactive text for the less important text.

all of that is really not as important as ensuring what i and comment #7 say:

1.  each color role has a background color, and no foreground color from one role should appear on top of the background color of another

2. this is easily assessed by using abobe linked color scheme

i don’t get how that could possibly confuse anyone :)

I am still waiting on an explicit list of all items which are wrong as requested earlier.

As far as I can tell, all text is correct. 

Only other item is we use the button background on the view, but it's perfectly OK to embed buttons in a view (see KPluginLoader or Get New Stuff), so I consider that perfectly OK to do.

Looking at the screen shot from comment #5, also the text has wrong color, which the most serious issue, because it can easily lead to unreadable text with special color schemes.

Also, it is certainly OK to use buttons within item views. But unless they also use the Button background, they should not be rendered using ButtonText.

> Looking at the screen shot from comment #5, also the text has wrong color

Which text are you referring to? The main text for names is a standard colors text (I think at least), it uses QPalette::Active, QPalette::WindowText. What's wrong with that?

Created attachment 74807 [details]
The Colours Dialog in KDE

I don't see how the text colours are wrong.

Attached is a screenshot of the theme selection dialog in KDE. I think it's safe to assume the colour selection dialog would using the right roles. To me this looks the same.

> Also, it is certainly OK to use buttons within item views. But unless they
> also use the Button background, they should not be rendered using ButtonText.

We are rendering button background onto the view, nothing uses button text.

....and that is the line, which I'm trying to get rid of completely.

/me points again to https://git.reviewboard.kde.org/r/106763/

 
> Attached is a screenshot of the theme selection dialog in KDE. I think it's
> safe to assume the colour selection dialog would using the right roles. To
> me this looks the same.

Oh, it's the normal text that's apparently wrong. not the highlight.

I finally understand.

Me too.

And I'll answer myself - QPalette::WindowText is the widget background, not the view background, which seems to be QPalette::Base.

Erm, got confused a bit apparently :D QPalette::WindowText is text on widgets and we should be using QPalette::Text, which is to be used with QPalette::Base, which is background for views.

Git commit 0cd69ab53ecc4189a0827b7a80b1bba884197f2b by David Edmundson.
Committed on 26/10/2012 at 13:16.
Pushed by davidedmundson into branch 'kde-telepathy-0.5'.

Use correct colour role when painting text delegates on a view

REVIEWED-BY: Martin Klapetek
FIXED-IN: 0.5.2

M  +1    -1    abstract-contact-delegate.cpp
M  +2    -2    contact-delegate-compact.cpp
M  +2    -2    contact-delegate.cpp

http://commits.kde.org/telepathy-contact-list/0cd69ab53ecc4189a0827b7a80b1bba884197f2b

looks good!

note also that the lines between text and arrow in this screenshot are pink: http://imgur.com/JglwO

that’s also wrong: it is seemingly drawn with the ButtonText colorrole

For the third time I'd like to point you to https://git.reviewboard.kde.org/r/106763/

Those lines are going to be removed, just don't mind them.

(In reply to comment #23)
> For the third time I'd like to point you to
> https://git.reviewboard.kde.org/r/106763/
> 
> Those lines are going to be removed, just don't mind them.

oh, sorry, i misinterpretted it – awesome that it’s restructured.