GNOME Bugzilla – Bug 645355
Use correct GTK+ colors when the application prefers a dark variant
Last modified: 2011-05-18 21:11:02 UTC
Patch series relies on GTK+ bug 645354.
Created attachment 183894 [details] [review] frame: Delay updating the background until the frame is ready To determine the correct background style, the UI needs to access some frame properties via meta_core_get(); this call will bail out early if window->frame is unset, so delay the call until the association is made.
Created attachment 183895 [details] [review] theme: Get GTK+ colors from style context Rather than using a single widget's style for GTK+ colors in themes, use the style context parameter of the drawing functions for those colors. Right now, a single style context is shared between frames, but this will change to support different style variants.
Created attachment 183896 [details] [review] window: Parse _GTK_THEME_VARIANT property Since version 3.0, GTK+ has support for style variants. At the moment, themes may provide a dark variant, which can be requested by applications via GtkSettings. The requested variant is exported to X11 via the _GTK_THEME_VARIANT property - support this property, in order to pick up the correct style variant in the future.
Created attachment 183897 [details] [review] core: Allow retrieving the theme variant via core_get() To associate frames with the correct style variant, the UI will need access to the window's theme variant property.
Created attachment 183898 [details] [review] ui-frame: Add support for style variants Rather than sharing a single style context between all frames, use a default style and one style per encountered variant. The value of the _GTK_THEME_VARIANT property should determine which style is attached to a particular frame, though for the time being the default style is used for every frame, as the window property cannot be accessed at the time the style is attached. This will be fixed in a later commit.
Created attachment 183899 [details] [review] ui: Add meta_ui_update_frame_style() This method allows forcing a style update of a particular frame from the core, so that it can pick up style variants.
Created attachment 183900 [details] [review] core: Update frame style when _GTK_THEME_VARIANT changes When the _GTK_THEME_VARIANT property changes, rather than just updating the window's theme_variant property, update its frame style as well, so that the window decoration reflects the requested variant. As the initial properties of a window may be read before its frame is created, there will be cases where the change is not picked up initially.
Created attachment 183901 [details] [review] ui-frames: Delay attaching the style to new frames Like the setting of new frames' background is delayed until the frame is associated with its window, delay attaching the initial style, so that the correct style variant is picked.
Oh, a quick note regarding freeze: while this series is a huge improvement regarding polish, the GTK+ patch won't make it in a 3.0 point release, so there's no point in reviewing this now.
Created attachment 184184 [details] [review] frame: Delay updating the background until the frame is ready Reattaching to maintain patch order.
Created attachment 184187 [details] [review] theme: Get GTK+ colors from style context Fixed minor issue where a parameter was used before its g_return_if_fail() check.
Created attachment 184188 [details] [review] window: Parse _GTK_THEME_VARIANT property Reattaching.
Created attachment 184189 [details] [review] core: Allow retrieving the theme variant via core_get() Reattaching.
Created attachment 184190 [details] [review] ui-frame: Add support for style variants Use the GtkSettings object associated with the frame rather than the default one.
Created attachment 184191 [details] [review] ui: Add meta_ui_update_frame_style() Reattaching.
Created attachment 184192 [details] [review] core: Update frame style when _GTK_THEME_VARIANT changes Reattaching.
Created attachment 184193 [details] [review] ui-frames: Delay attaching the style to new frames Reattaching.
Created attachment 184195 [details] [review] theme: Do not create temporary GtkStyle objects In order to pick up colors from a GtkStyleContext, a temporary GtkStyle object was created from the context and destroyed after copying the requested GdkColor. This is slightly inefficient, so get the appropriate GdkRGBA from the context and translate it to a GdkColor, based on the compatibility code in gtkstyle.c. Could possibly be merged with attachment 183895 [details] [review], but there's one doubt I have with this patch: the GtkStyle colors are initialized to default values, so when gtk_style_context_get() in the compatibility code fails, the color will still end up with something reasonable. We don't have that fallback here, not sure if that could cause problems.
Review of attachment 184187 [details] [review]: Patch looks safe to me but unclear about performance impact ::: src/ui/theme.c @@ +1428,3 @@ + style = g_object_new (GTK_TYPE_STYLE, + "context", context, + NULL); Hmm, I'm concerned about performance here - creating a GtkStyle from a GtkStyleContext is far from free, and we're doing it separately for every color used in drawing a frame? I'd really like to see some stats - maybe hack up a 'static int count; if (++count % 100 == 0) g_print ("* %d\n", count);' and see how much that gets hit if you start the shell and interactively resize a window a bit. What would it take to retrieve colors directly from the GtkStyleContext so we don't allocate a GObject and compute 5 states * 4 colors in order to get a single color?
Review of attachment 184188 [details] [review]: ::: src/core/window-private.h @@ +99,3 @@ char *startup_id; char *mutter_hints; + char *gtk_theme_variant; Is leaked ::: src/core/window-props.c @@ +1539,3 @@ + } + + if (changed) All of the above replaceable with: if (g_strcmp0 (requested_variant, current_variant) != 0) { } @@ +1602,3 @@ { display->atom__MOTIF_WM_HINTS, META_PROP_VALUE_MOTIF_HINTS, reload_mwm_hints, TRUE, FALSE }, { XA_WM_TRANSIENT_FOR, META_PROP_VALUE_WINDOW, reload_transient_for, TRUE, FALSE }, + { display->atom__GTK_THEME_VARIANT, META_PROP_VALUE_UTF8, reload_gtk_theme_variant, TRUE, FALSE }, looks like it could be better aligned ::: src/core/window.c @@ +10116,3 @@ +meta_window_get_gtk_theme_variant (MetaWindow *window) +{ + return window->gtk_theme_variant; This seems like it isn't that useful without notification, since it can change at any time. (Maybe somewhere else in the patch set? will keep reading.)
Review of attachment 184190 [details] [review]: Commit message seems to be from an earlier version of this patch ::: src/ui/frames.c @@ +210,3 @@ + gtk_widget_get_path (GTK_WIDGET (frames))); + + if (theme_name && *theme_name) You leak the theme name if it's "" @@ +238,3 @@ + g_object_get (gtk_settings_get_for_screen (screen), + "gtk-theme-name", &theme_name, + NULL); Seems like a utility function would be useful to avoid ~15 lines of code duplication between this and meta_frames_get_theme_variant() @@ +246,3 @@ + gtk_widget_get_path (GTK_WIDGET (frames))); + + if (theme_name && *theme_name) Duplicate bug @@ +259,3 @@ + { + style = meta_frames_get_theme_variant (frames, (char *)variant->data); + g_hash_table_insert (frames->style_variants, variant->data, style); This is going to crash - because g_hash_table_insert() over the old key will the new key and the old value, so you need to strdup() one and ref() the other. Which implies that this code path needs testing.
Review of attachment 184188 [details] [review]: ::: src/core/window.c @@ +10116,3 @@ +meta_window_get_gtk_theme_variant (MetaWindow *window) +{ + return window->gtk_theme_variant; I see you call an internal update function in a later version of this patch. Maybe this should just be in window-private.h and not exported publicly if you don't want to add public notification?
Review of attachment 184195 [details] [review]: This looks much better to me than the old code; more useful to get this in than to do the testing I suggested for the other code. ::: src/ui/theme.c @@ +1476,3 @@ + color->blue = CLAMP ((guint) (rgba_color->blue * 65535), 0, 65535); + gdk_rgba_free (rgba_color); + } after this point, if rgba_color isn't set, you'll be accessing uninitialized memory, which I think a C-standard compliant compiler is allowed to turn into writing poetry over your boot sector. Would suggest setting the color to magenta and maybe a g_warning(). (Not sure under what circumstances the style_context_get() calls would fail.)
This code looks generally excellent but the kind of changes of ordering involved here often result in subtle bugs that are hard to find in testing. I'm not comfortable with this landing after hard code freeze. Crashing window managers are nobody's friend. It would be better for this feature to wait until 3.2 in my opinion.
Review of attachment 184193 [details] [review]: Grammar nit in commit message "Similar to how ... " not "Like"
Review of attachment 184192 [details] [review]: Looks good
Review of attachment 184184 [details] [review]: Looks reasonable as far as I can tell, though there's some possibility for subtle stuff going on here.
Review of attachment 184191 [details] [review]: Looks good
Review of attachment 184189 [details] [review]: Looks good
(In reply to comment #25) > Review of attachment 184193 [details] [review]: > > Grammar nit in commit message "Similar to how ... " not "Like" Fixed locally.
Created attachment 184291 [details] [review] window: Parse _GTK_THEME_VARIANT property (In reply to comment #20) > Review of attachment 184188 [details] [review]: > > + char *gtk_theme_variant; > > Is leaked Fixed. > ::: src/core/window-props.c > All of the above replaceable with: > > if (g_strcmp0 (requested_variant, current_variant) != 0) Done. > looks like it could be better aligned Mmh. I hope the current version is slightly better, though proper alignment would require adjusting the other elements. > ::: src/core/window.c > @@ +10116,3 @@ > +meta_window_get_gtk_theme_variant (MetaWindow *window) > +{ > + return window->gtk_theme_variant; > > This seems like it isn't that useful without notification, since it can change > at any time. (Maybe somewhere else in the patch set? will keep reading.) Yeah, looks like I ended up not using it at all myself, so I just removed the function as suggested in comment 22.
Created attachment 184293 [details] [review] ui-frame: Add support for style variants (In reply to comment #21) > Review of attachment 184190 [details] [review]: > > Commit message seems to be from an earlier version of this patch > > ::: src/ui/frames.c > @@ +210,3 @@ > You leak the theme name if it's "" Fixed. > @@ +238,3 @@ > Seems like a utility function would be useful to avoid ~15 lines of code > duplication between this and meta_frames_get_theme_variant() Done. > @@ +259,3 @@ > + { > + style = meta_frames_get_theme_variant (frames, (char *)variant->data); > + g_hash_table_insert (frames->style_variants, variant->data, style); > > This is going to crash - because g_hash_table_insert() over the old key will > the new key and the old value, so you need to strdup() one and ref() the other. > Which implies that this code path needs testing. Gah, sorry. I didn't strdup() to avoid having to free the list data before g_list_free(), but of course the data is owned by the hash table. Fixed. The other error is even more wrong - the intention of the function is to update the contexts after a style change, so assuming that the style is newly allocated, the hash table can take ownership of it. Of course you are right that when we hit this path, the style may be already in the hash table and we need to ref() it, but having the lookup in the first place is wrong, so always create a new style context here (another use for the new helper function!). And yes, this code path is completely untested - it's supposed to be triggered by GTK+ style changes, but GtkStyleContext apparently is harder to fool than GtkStyle ... a separate bug, probably introduced when porting to GtkStyleContext.
Created attachment 184294 [details] [review] theme: Do not create temporary GtkStyle objects (In reply to comment #23) > Review of attachment 184195 [details] [review]: > Would suggest setting the color to magenta and maybe a g_warning(). Done. (I couldn't get myself to use magenta all the way, so I picked black for META_GTK_COLOR_TEXT ...)
Several patches here have the status "accepted-commit_after_freeze". Assuming this refered to 3.0 Hardcode Freeze, can this get a commit now?
(In reply to comment #34) > Several patches here have the status "accepted-commit_after_freeze". Assuming > this refered to 3.0 Hardcode Freeze, can this get a commit now? No, not until we branch for 3.2.
Review of attachment 184291 [details] [review]: Looks good
Review of attachment 184293 [details] [review]: Commit still doesn't make sense to me given your use of META_CORE_GET_THEME_VARIANT, &variant Otherwise looks good to me. ::: src/ui/frames.c @@ +217,3 @@ + + if (theme_name) + g_free (theme_name); Don't need the if (theme_name)
Review of attachment 184294 [details] [review]: Looks good
Attachment 184184 [details] pushed as 37aeb5b - frame: Delay updating the background until the frame is ready Attachment 184187 [details] pushed as da4486b - theme: Get GTK+ colors from style context Attachment 184189 [details] pushed as 0cdac78 - core: Allow retrieving the theme variant via core_get() Attachment 184191 [details] pushed as eb17cd9 - ui: Add meta_ui_update_frame_style() Attachment 184192 [details] pushed as be67757 - core: Update frame style when _GTK_THEME_VARIANT changes Attachment 184193 [details] pushed as 7577437 - ui-frames: Delay attaching the style to new frames Attachment 184291 [details] pushed as 4f3b03e - window: Parse _GTK_THEME_VARIANT property Attachment 184293 [details] pushed as 4affd22 - ui-frame: Add support for style variants Attachment 184294 [details] pushed as 0d9a9b8 - theme: Do not create temporary GtkStyle objects