Bug 303133 - wrong text color in places
Summary: wrong text color in places
Status: RESOLVED FIXED
Alias: None
Product: dolphin
Classification: Applications
Component: panels: places (show other bugs)
Version: 2.1
Platform: Arch Linux Linux
: NOR normal
Target Milestone: ---
Assignee: Frank Reininghaus
URL:
Keywords: regression, reproducible
Depends on:
Blocks:
 
Reported: 2012-07-07 00:36 UTC by splash
Modified: 2012-08-15 13:08 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In: 4.9.1


Attachments
A screenshot of how it looks like (125.22 KB, image/png)
2012-07-07 00:36 UTC, splash
Details
where dolphin fail with the color scheme (297.49 KB, image/png)
2012-07-10 00:08 UTC, splash
Details
After patch (120.30 KB, image/png)
2012-08-02 22:01 UTC, Hrvoje Senjan
Details
Fixes the wrong text color in places group header (3.49 KB, patch)
2012-08-13 19:25 UTC, Emmanuel Pescosta
Details
Fix Item Group Header Text Color (7.11 KB, patch)
2012-08-14 20:17 UTC, Emmanuel Pescosta
Details

Note You need to log in before you can comment on or make changes to this bug.
Description splash 2012-07-07 00:36:10 UTC
Created attachment 72349 [details]
A screenshot of how it looks like

Since the 2.1 version of Dolphin the text color used in place panel is the same as the one used in display window.
So basically the problem is that we can no more read the text if we're using a dark color scheme.
Comment 1 Frank Reininghaus 2012-07-09 08:25:56 UTC
Thanks for the bug report! Which color scheme do you use?
Comment 2 splash 2012-07-09 11:35:08 UTC
(In reply to comment #1)
> Thanks for the bug report! Which color scheme do you use?

I'm using the homemade 'Wrath of the Muffin King' but the bug apply to all dark color scheme with white display color (and so black display text)
Comment 3 Frank Reininghaus 2012-07-09 12:11:07 UTC
Thanks for the quick reply. I have to admit that I'm not familiar with color scheme settings. I tried the dark schemes which are pre-installed here, and those work fine. So I just experimented a bit and could reproduce this, but only with settings which make the entire desktop unusable.

If you can provide detailed information about the settings required to reproduce this, that would be very helpful. Thanks.
Comment 4 splash 2012-07-10 00:08:22 UTC
Created attachment 72413 [details]
where dolphin fail with the color scheme
Comment 5 splash 2012-07-10 00:16:42 UTC
Steps to reproduce:

-White color for view background
-Black color(something dark) for view text
-Black color(something dark) for window background
-Window text in white(which is normal if we want to read on a dark window background)

Problem: dolphin 2.1 will use the view text color instead of the window text color in dolphin place.
===> unable to read what is write in places
Comment 6 Frank Reininghaus 2012-07-10 13:48:36 UTC
Thanks for the detailed description! I can confirm that this makes the text in the Places Panel impossible to read (but not in the old Places Panel implementation).
Comment 7 Christoph Feck 2012-07-12 20:09:39 UTC
Try setting foregroundRole() to QPalette::WindowText
Comment 8 Emmanuel Pescosta 2012-08-02 21:06:08 UTC
https://git.reviewboard.kde.org/r/105832/
Comment 9 Hrvoje Senjan 2012-08-02 22:01:02 UTC
Created attachment 72912 [details]
After patch

Emanuel, i confirming your patch fixes the issue! Thanks :)
Comment 10 Emmanuel Pescosta 2012-08-13 17:24:58 UTC
I'm not allowed to change the bug status to resolved. Can you please do it for me?

Git commit 5ba759b22104fb903c963155fa5fe2d225740c75 by Emmanuel Pescosta.
Committed on 13/08/2012 at 18:40.
Pushed by emmanuelp into branch 'KDE/4.9'.

FIXED-IN: 4.9.1
REVIEW: 105832
Comment 11 Christoph Feck 2012-08-13 18:22:38 UTC
Thanks, only the headlines (that say "Places" etc.) still use the View Text color.
Comment 12 Emmanuel Pescosta 2012-08-13 19:25:53 UTC
Created attachment 73145 [details]
Fixes the wrong text color in places group header

Fix wrong text color in Places Group Header. Use QPalette::Window for base color and QPalette::WindowText for text color. Also changed m_roleColor color mixing to 60% (from 70%) -> Better visible color difference when base color is darker than text color.
Comment 13 Frank Reininghaus 2012-08-14 11:08:58 UTC
Thanks for the new patch! Looks good, just two small questions:

a) Maybe replacing
styleOption().palette.brush(group, normalBaseColorPalette()).color()
by
styleOption().palette.color(group, normalBaseColorPalette())
would be slightly easier and more efficient? Or is there a reason why the brush is needed?

b) One could rename normal*ColorPalette() to normal*ColorRole() because these functions return a color role rather than a palette. But maybe that's a matter of taste ;-)

(In reply to comment #10)
> I'm not allowed to change the bug status to resolved. Can you please do it
> for me?

Please ask the sysadmins to enable 'editbugs' permissions for you, then you can close this report when you commit your new patch.
Comment 14 Emmanuel Pescosta 2012-08-14 20:17:14 UTC
Created attachment 73167 [details]
Fix Item Group Header Text Color

a) and b) fixed ;)

Should I create a review request for this patch? Or can I ship it directly?
Comment 15 Frank Reininghaus 2012-08-15 08:51:02 UTC
Thanks, very nice! No review request needed - the review has already been done here. Please go ahead and push to 4.9 and master.
Comment 16 Emmanuel Pescosta 2012-08-15 13:03:44 UTC
Git commit b3c3da985159a9627c079ad615cd77fc5f7bb72a by Emmanuel Pescosta.
Committed on 14/08/2012 at 22:02.
Pushed by emmanuelp into branch 'master'.

Fix wrong text color in Places Group Header. Use QPalette::Window for base color and QPalette::WindowText for text color. Also changed m_roleColor color mixing to 60% (from 70%) -> Better visible color difference when base color is darker than text color. Also changed styleOption().palette.brush(group, role).color() to styleOption().palette.color(group, role) in KStandardItemListWidget -> should be more efficient.

M  +25   -3    dolphin/src/kitemviews/kitemlistgroupheader.cpp
M  +6    -0    dolphin/src/kitemviews/kitemlistgroupheader.h
M  +3    -3    dolphin/src/kitemviews/kstandarditemlistwidget.cpp
M  +1    -1    dolphin/src/kitemviews/kstandarditemlistwidget.h
M  +1    -1    dolphin/src/panels/folders/foldersitemlistwidget.cpp
M  +1    -1    dolphin/src/panels/folders/foldersitemlistwidget.h
M  +5    -0    dolphin/src/panels/places/placesitemlistgroupheader.cpp
M  +2    -0    dolphin/src/panels/places/placesitemlistgroupheader.h
M  +1    -1    dolphin/src/panels/places/placesitemlistwidget.cpp
M  +1    -1    dolphin/src/panels/places/placesitemlistwidget.h

http://commits.kde.org/kde-baseapps/b3c3da985159a9627c079ad615cd77fc5f7bb72a
Comment 17 Emmanuel Pescosta 2012-08-15 13:08:48 UTC
Git commit 23194dde3bda67f15f8247ca742640666781e02e by Emmanuel Pescosta.
Committed on 14/08/2012 at 22:02.
Pushed by emmanuelp into branch 'KDE/4.9'.

Fix wrong text color in Places Group Header. Use QPalette::Window for base color and QPalette::WindowText for text color. Also changed m_roleColor color mixing to 60% (from 70%) -> Better visible color difference when base color is darker than text color. Also changed styleOption().palette.brush(group, role).color() to styleOption().palette.color(group, role) in KStandardItemListWidget -> should be more efficient.
FIXED-IN: 4.9.1
(cherry picked from commit b3c3da985159a9627c079ad615cd77fc5f7bb72a)

M  +25   -3    dolphin/src/kitemviews/kitemlistgroupheader.cpp
M  +6    -0    dolphin/src/kitemviews/kitemlistgroupheader.h
M  +3    -3    dolphin/src/kitemviews/kstandarditemlistwidget.cpp
M  +1    -1    dolphin/src/kitemviews/kstandarditemlistwidget.h
M  +1    -1    dolphin/src/panels/folders/foldersitemlistwidget.cpp
M  +1    -1    dolphin/src/panels/folders/foldersitemlistwidget.h
M  +5    -0    dolphin/src/panels/places/placesitemlistgroupheader.cpp
M  +2    -0    dolphin/src/panels/places/placesitemlistgroupheader.h
M  +1    -1    dolphin/src/panels/places/placesitemlistwidget.cpp
M  +1    -1    dolphin/src/panels/places/placesitemlistwidget.h

http://commits.kde.org/kde-baseapps/23194dde3bda67f15f8247ca742640666781e02e