GNOME Bugzilla – Bug 770424
Themes: insensitive menu item are not readable
Last modified: 2018-04-18 19:21:47 UTC
Well it's an exageration, but the shadow on the insensitive menu items make them hard to read on the new themes. Could we get rid of this shadow? Let's just show the insensitive items another way (some faded color for instance).
On IRC, Patrick David (and others) documented themselves about it, and if not mistaken (that was already a few days/weeks ago), it may be a limitation of GTK+2. Basically the color of the shadow (or its presence) may not be settable through the theme. :-/ If anyone has any clue about this, patches welcome (to our themes, to GTK+, whatever…).
I've had a look around in a relatively recent copy of the GTK+2 code, and in the pixbuf rendering engine code: modules/engines/pixbuf/pixbuf-draw.c the draw_string function has an if (state == GTK_STATE_INSENSITIVE) section that sets the style to white_gc (???) I don't have a development environment so I can't personally play with it to confirm if it's the correct area or not.
(In reply to Jehan from comment #0) > Well it's an exageration, but the shadow on the insensitive menu items make > them hard to read on the new themes. > Could we get rid of this shadow? Let's just show the insensitive items > another way (some faded color for instance). I noticed this as well and would very much like it fixed. I don't know anything about GTK+, nor have I ever dove into the source code of GIMP myself, but I just wanted to put in my 2 cents on this. Thanks for all your work guys :)
Created attachment 364686 [details] [review] override default gtk draw_layout, ugly proof of concept It seems that the Gtk2 default draw_layout is used to draw menu items and it is here: https://git.gnome.org/browse/gtk+/tree/gtk/gtkstyle.c?h=gtk-2-24#n5287 An ugly way to override its implementation in GIMP is to overwrite the pointer of the PixbufStyle class with the address of a local function. Attached a quick and dirty patch that hard-writes a different style for insensitive texts.
Wow Massimo! As usual you are awesome. :-) I will try and review this as soon as possible. > Attached a quick and dirty patch that hard-writes a different style for insensitive texts. So does that mean that all themes would still have the same color for insensitive text (simply another color than the current)? If so, we will need to make it customizable by the themes so that depending on whether that's a dark or light theme, it can have an appropriate color for insensitive texts. Apart from this implementation detail, I don't mind the quick and dirty for something which (as far as I understand) has an impact on GTK+2 only, considering we will drop GTK+2 after 2.10 release.
(In reply to Jehan from comment #5) > Wow Massimo! As usual you are awesome. :-) > I will try and review this as soon as possible. > > > Attached a quick and dirty patch that hard-writes a different style for insensitive texts. > > So does that mean that all themes would still have the same color for > insensitive text (simply another color than the current)? it mixes (3/4, 1/4) the foreground color with its inverse. > If so, we will need to make it customizable by the themes so that depending > on whether that's a dark or light theme, it can have an appropriate color > for insensitive texts. > > Apart from this implementation detail, I don't mind the quick and dirty for > something which (as far as I understand) has an impact on GTK+2 only, > considering we will drop GTK+2 after 2.10 release. Anyway in case you base something on this beware I did not put check when dereferencing the class pointer, that in theory (Win, Mac) could be missing.
(In reply to Massimo from comment #6) > Anyway in case you base something on this beware I did not put check > when dereferencing the class pointer, that in theory (Win, Mac) could > be missing. Are you saying that: > g_type_class_ref (g_type_from_name ("PixbufStyle")) could return NULL? Also I was wondering. If we g_type_class_ref(), shouldn't we g_type_class_unref() too?
Created attachment 364710 [details] Before (left) - after fix (right) Ok done! I tested, it looks just great. I attach a screenshot of before/after with the default Dark theme. It looks good too with light themes. I pushed as is, and just did a bit of reorganization, adding a check (though I'm not sure g_type_class_ref() can return NULL, but just in case) and an unref(). Closing as FIXED. Please tell me if you spot any issue and don't hesitate to reopen, if so. commit 337dba62fddbb538ca6de6a05cf7935653535942 Author: Massimo Valentini <mvalentini@src.gnome.org> Date: Thu Nov 30 15:40:28 2017 +0100 Bug 770424 - Themes: insensitive menu item are not readable Comment by Jehan after review: "Quick and dirty hack" but a working one. Since the bug will likely disappear with the GTK+3 port (to be verified) which uses another theme system, let's just do it this way. app/gui/themes.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) commit 18a96d92682fbeddfa48883d17f46c50cf15d7eb (HEAD -> master, origin/master, origin/HEAD) Author: Jehan <jehan@girinstud.io> Date: Fri Dec 1 03:05:14 2017 +0100 Bug 770424 - Themes: insensitive menu item are not readable. Not sure if g_type_class_ref() can actually return NULL here, but let's add a check, just in case. Also unref() after since we ref-ed it ourselves. Finally reorganize a bit to keep the private functions together and named appropriately, clean some tabs and add a comment to remind of further check/cleanup once we port to GTK+3. app/gui/themes.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------ 1 file changed, 79 insertions(+), 56 deletions(-)
(In reply to Jehan from comment #7) > (In reply to Massimo from comment #6) > > Anyway in case you base something on this beware I did not put check > > when dereferencing the class pointer, that in theory (Win, Mac) could > > be missing. > > Are you saying that: > > > g_type_class_ref (g_type_from_name ("PixbufStyle")) > > could return NULL? > Well, I think that PixbufStyle is implemented in a module part of gtk-2 and I'm not sure it is used also on Windows or Mac, so I thought g_type_from_name could return G_TYPE_INVALID and g_type_class_ref NULL. > Also I was wondering. If we g_type_class_ref(), shouldn't we > g_type_class_unref() too? I did not because being a module I did not want it to be unloaded
(In reply to Massimo from comment #9) > (In reply to Jehan from comment #7) > > Also I was wondering. If we g_type_class_ref(), shouldn't we > > g_type_class_unref() too? > > I did not because being a module I did not want it to be unloaded Ok. From what I understand, it seems the only risk is if this g_type_class_ref() was the first time the class was loaded, in which case, g_type_class_unref() just after would unload it immediately and the assignment of draw_layout to a local function is rendered useless. So I propose instead to unref() it in themes_exit(). Cf. commit ba8dca5f47a7cf40bf63ac2f26b079c3069327dc. If you really think we should not unref() it at all, just tell me. I'll get rid of the unref(). I just don't like that we ref() something without unref(). But you may know better than I do. I don't know much about modules.
(In reply to Jehan from comment #10) > (In reply to Massimo from comment #9) > > (In reply to Jehan from comment #7) > > > Also I was wondering. If we g_type_class_ref(), shouldn't we > > > g_type_class_unref() too? > > > > I did not because being a module I did not want it to be unloaded > > Ok. From what I understand, it seems the only risk is if this > g_type_class_ref() was the first time the class was loaded, in which case, > g_type_class_unref() just after would unload it immediately and the > assignment of draw_layout to a local function is rendered useless. > > So I propose instead to unref() it in themes_exit(). > Cf. commit ba8dca5f47a7cf40bf63ac2f26b079c3069327dc. > > If you really think we should not unref() it at all, just tell me. I'll get > rid of the unref(). I just don't like that we ref() something without > unref(). But you may know better than I do. I don't know much about modules. The commit seems fine to me.
Fix the fix: commit 2dd2f2f371120a6eda501a3c3230b1f394eccb7a Author: Michael Natterer <mitch@gimp.org> Date: Sat Apr 7 12:27:53 2018 +0200 Bug 770424 - Themes: insensitive menu item are not readable Need to check if we must override PixbufStyle's draw_layout() after each theme change, not only at the beginning of themes_init(), so it works also when the the pixbuf engine was not already loaded at startup. app/gui/themes.c | 83 ++++++++++++++++++++++++++++++++++++--------------------------- 1 file changed, 48 insertions(+), 35 deletions(-)
*** Bug 793892 has been marked as a duplicate of this bug. ***