Bug 315061 - Places panel doesn't handle runtime color scheme changes correctly
Summary: Places panel doesn't handle runtime color scheme changes correctly
Status: RESOLVED FIXED
Alias: None
Product: dolphin
Classification: Applications
Component: panels: places (show other bugs)
Version: 2.2
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Dolphin Bug Assignee
URL:
Keywords: investigated, reproducible
: 323917 334697 (view as bug list)
Depends on:
Blocks: 335175
  Show dependency treegraph
 
Reported: 2013-02-13 13:06 UTC by Eike Hein
Modified: 2014-10-01 09:10 UTC (History)
7 users (show)

See Also:
Latest Commit:
Version Fixed In: 4.13


Attachments
Places panel with color role test color scheme (33.58 KB, image/png)
2013-02-13 13:07 UTC, Eike Hein
Details
Places panel with openSUSE 12.3 colors (25.17 KB, image/png)
2013-02-13 13:07 UTC, Eike Hein
Details
openSUSE 12.3 default color scheme (2.32 KB, text/plain)
2013-02-13 13:08 UTC, Eike Hein
Details
before restart (169.68 KB, image/png)
2013-09-24 22:13 UTC, Pavel Nedrigailov
Details
after restart (122.07 KB, image/png)
2013-09-24 22:13 UTC, Pavel Nedrigailov
Details
Screencast of said problem (1.19 MB, video/ogg)
2014-05-12 20:31 UTC, Richard Llom
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eike Hein 2013-02-13 13:06:12 UTC
The places panel doesn't adapt to runtime color scheme changes correctly, leading to invisible text labels with KDE color schemes that don't have contrasting Window Background and View Text colors. The correct colors are applied initially on startup, but not during runtime color scheme changes.

Attached are screenshots using the color role test color scheme and openSUSE 12.3's default color scheme, highlighting the problem.

I fixed a similar bug in Okular today by porting its sidebar delegate to KColorScheme, for reference: http://commits.kde.org/okular/a3b3545cabf1c52ce6c0a065dc415dc6fe97bf28

Reproducible: Always
Comment 1 Eike Hein 2013-02-13 13:07:20 UTC
Created attachment 77250 [details]
Places panel with color role test color scheme
Comment 2 Eike Hein 2013-02-13 13:07:45 UTC
Created attachment 77251 [details]
Places panel with openSUSE 12.3 colors
Comment 3 Eike Hein 2013-02-13 13:08:15 UTC
Created attachment 77252 [details]
openSUSE 12.3 default color scheme
Comment 4 Eike Hein 2013-02-13 13:09:09 UTC
Adding Will Stephenson to CC who supplied me with the color scheme file.
Comment 5 Christoph Feck 2013-02-13 13:15:59 UTC
Actually, this also affects the main file view (at least in icon mode).
Comment 6 Eike Hein 2013-02-13 13:23:34 UTC
The main view also has another problem in List mode: The alternate row color doesn't immediately update with color scheme changes, only after causing a repaint.

Responding to color scheme changes tends to be forgotten sadly ... runner up: responding to icon theme changes.
Comment 7 Frank Reininghaus 2013-02-13 14:59:25 UTC
Thanks for the bug report and for the analysis! If I understand you correctly, all we have to do is use KColorScheme to determine the colors everywhere where painting is done?
Comment 8 Eike Hein 2013-02-13 15:04:31 UTC
KColorScheme's the way to get at the correct colors, but whenever one stores a color derived/taken from the scheme for later use (and calling setPalette() with a modified palette means "storing for later use" in a way) one also has to make sure to react to the KGlobalSettings::appearanceChanged() signal to update the cached color. In the Okular example that wasn't necessary because appearanceChanged() already causes a repaint to happen and the code in question was a list view delegate which takes the colors from the scheme on every paint instead of caching them somewhere, and the same might be true for the affected Dolphin code, haven't checked.
Comment 9 Frank Reininghaus 2013-02-13 15:14:10 UTC
Thanks for the quick reply. So it's mostly clear how to fix this. I can't make any promises regarding a schedule though - I spend hardly any time in front of my development machine nowadays and have a couple of items on the TODO list already. If anyone else reads this and feels like working on it, please go ahead.
Comment 10 Frank Reininghaus 2013-02-14 15:13:12 UTC
Just for the record, Albert remarked that creating a KColorScheme every time paint() is called might be too expensive:

http://lists.kde.org/?l=kde-commits&m=136079574606134&w=2
Comment 11 Eike Hein 2013-02-14 15:32:02 UTC
Yup, and here's a patch pending review that moves KColorScheme instanciation out of the paint path: http://git.reviewboard.kde.org/r/108958/
Comment 12 Christoph Feck 2013-02-24 01:15:20 UTC
Note that it is not required to use KColorScheme here. Reacting to Qt's PaletteChanged events is sufficient to fix this.
Comment 13 Frank Reininghaus 2013-08-23 11:26:38 UTC
*** Bug 323917 has been marked as a duplicate of this bug. ***
Comment 14 Pavel Nedrigailov 2013-09-24 22:13:03 UTC
Created attachment 82481 [details]
before restart
Comment 15 Pavel Nedrigailov 2013-09-24 22:13:51 UTC
Created attachment 82482 [details]
after restart
Comment 16 Pavel Nedrigailov 2013-09-24 22:15:03 UTC
having same issue. in attachments - dolphin before its restart and after.

kde 4.11.1, fedora 20
Comment 17 Emmanuel Pescosta 2014-02-22 20:35:36 UTC
Review-Request: https://git.reviewboard.kde.org/r/115958/
Comment 18 Emmanuel Pescosta 2014-02-24 13:19:33 UTC
Git commit f40b80a960de8a2db6110b6fa14db2d7182ad6c3 by Emmanuel Pescosta.
Committed on 24/02/2014 at 13:17.
Pushed by emmanuelp into branch 'master'.

Handle font and palette changes in Dolphin list views.

Also update the font of the meta data widget in InformationPanelContent (smallest readable font).
Related: bug 329186
FIXED-IN: 4.13
REVIEW: 115958

M  +37   -4    dolphin/src/kitemviews/kitemlistview.cpp
M  +3    -0    dolphin/src/kitemviews/kitemlistview.h
M  +4    -0    dolphin/src/panels/information/informationpanelcontent.cpp
M  +20   -17   dolphin/src/views/dolphinitemlistview.cpp
M  +2    -1    dolphin/src/views/dolphinitemlistview.h

http://commits.kde.org/kde-baseapps/f40b80a960de8a2db6110b6fa14db2d7182ad6c3
Comment 19 Richard Llom 2014-05-03 13:01:46 UTC
Emmanuel, thanks a lot for your work. I'm already enjoying it. :-)
Comment 20 Richard Llom 2014-05-10 10:13:23 UTC
Hmm, I noticed that the color change only applies successfully until the tab is changed, ie. when you have only one tab open you have to open a new tab.

Can this be improved?
Comment 21 Frank Reininghaus 2014-05-11 21:22:07 UTC
(In reply to comment #20)
> Hmm, I noticed that the color change only applies successfully until the tab
> is changed, ie. when you have only one tab open you have to open a new tab.
> 
> Can this be improved?

I'm not entirely sure if I understand exactly what you mean, but since this seems to be different from the original problem, can you file a new bug report and provide detailed steps to reproduce?
Comment 22 Richard Llom 2014-05-12 20:31:57 UTC
Created attachment 86599 [details]
Screencast of said problem

(In reply to comment #21)
> I'm not entirely sure if I understand exactly what you mean, but since this
> seems to be different from the original problem, can you file a new bug
> report and provide detailed steps to reproduce?
> 
It is the same issue, see attached screencast.
Comment 23 Frank Reininghaus 2014-05-12 20:56:47 UTC
Thanks for the update and the screencast.

(In reply to comment #22)
> Created attachment 86599 [details]
> Screencast of said problem
> 
> (In reply to comment #21)
> > I'm not entirely sure if I understand exactly what you mean, but since this
> > seems to be different from the original problem, can you file a new bug
> > report and provide detailed steps to reproduce?
> > 
> It is the same issue, see attached screencast.

Even though the symptoms look similar to you, it is not exactly the same issue if I'm not mistaken. The original bug report was about Dolphin not updating the colors at all. The problem that you mention is only about the alternating row colors if I understand correctly (might be related to bug 334180 but since that one is about inactive window color effects, the root cause might be different).

I would appreciate it if you could file a new bug report (unless you think that it's the same as bug 334180) - even though recycling "fixed" reports for new problems or different variations of the old problem may look easier at first sight, this approach tends to turn bug reports into a big unreadable mess.

Thanks for your help.
Comment 24 Emmanuel Pescosta 2014-05-13 11:26:51 UTC
*** Bug 334697 has been marked as a duplicate of this bug. ***
Comment 25 Richard Llom 2014-05-22 08:47:49 UTC
(In reply to comment #22)
> Even though the symptoms look similar to you, it is not exactly the same
> issue if I'm not mistaken. The original bug report was about Dolphin not
> updating the colors at all. The problem that you mention is only about the
> alternating row colors if I understand correctly (might be related to bug
> 334180 but since that one is about inactive window color effects, the root
> cause might be different).
> 
> I would appreciate it if you could file a new bug report (unless you think
> that it's the same as bug 334180) - even though recycling "fixed" reports
> for new problems or different variations of the old problem may look easier
> at first sight, this approach tends to turn bug reports into a big
> unreadable mess.
> 
> Thanks for your help.

Ok, I filled now bug 335175.