GNOME Bugzilla – Bug 761543
frames: don't force dark theme to all windows
Last modified: 2016-02-04 14:22:21 UTC
Created attachment 320415 [details] [review] frames: don't force dark theme to all windows Use theme override only if _GTK_THEME_VARIANT property does not exist on window. This allows applications to request default theme variant when global theme is enabled. This requires GTK+ master. https://git.gnome.org/browse/gtk+/commit/?id=8eb261988869608604c78ed90de5579beb4ef2b0
Review of attachment 320415 [details] [review]: ::: src/ui/frames.c @@ +485,2 @@ else + variant = *variant != '\0' ? variant : NULL; This is harder to read than it needs to. How about: if (variant == NULL) variant = get_theme_variant_overide (frame->frames); if (variant == NULL || *variant == '\0' || strcmp(variant, "normal") == 0) ... (We could also revert the gtk+ patch and change gtk_window_set_theme_variant() instead: gdk_x11_window_set_theme_variant (gdk_window, dark_theme_requested ? "dark" : "normal"); It's a bit silly to have two different ways to explicitly ask for the default variant (empty string and "normal") ...
Do you have some info / link about "normal" variant? When I looked in GTK+ I did not see normal... For default variant it is always NULL, or "dark" for dark variant. Maybe I missed that. I think with normal it will try to load gtk-normal.css (as example). As it does not exist it will try without variant.
Created attachment 320424 [details] [review] frames: don't force dark theme to all windows
(In reply to Alberts Muktupāvels from comment #2) > Do you have some info / link about "normal" variant? No. It's not an actual variant in fact, but an explicit value you can set the _GTK_THEME_VARIANT property to to explicitly ask for the default variant (i.e. what you added to gtk with "") > I think with normal it will try to load gtk-normal.css (as example). As it > does not exist it will try without variant. No, just like it won't look for gtk-.css when you set the property to the empty string - gtk-+ only uses the property to communicate the theme variant to the WM, not to determine which variant to use itself. So one of the issues here is that mutter and gtk+ now have different ways to explicitly express "use the default variant". Rather than making mutter handle both ways, we should just pick one (the mutter one has been around since 3.4 or so, but fwiw I'm not aware of any program that sets the hint, so changing it is probably safe as well)
(In reply to Florian Müllner from comment #4) > (In reply to Alberts Muktupāvels from comment #2) > > Do you have some info / link about "normal" variant? > > > No. It's not an actual variant in fact, but an explicit value you can set > the _GTK_THEME_VARIANT property to to explicitly ask for the default variant > (i.e. what you added to gtk with "") > > > > I think with normal it will try to load gtk-normal.css (as example). As it > > does not exist it will try without variant. > > No, just like it won't look for gtk-.css when you set the property to the > empty string - gtk-+ only uses the property to communicate the theme variant > to the WM, not to determine which variant to use itself. Oh, right. > So one of the issues here is that mutter and gtk+ now have different ways to > explicitly express "use the default variant". Rather than making mutter > handle both ways, we should just pick one (the mutter one has been around > since 3.4 or so, but fwiw I'm not aware of any program that sets the hint, > so changing it is probably safe as well) Quick search in google does not show any results where _GTK_THEME_VARIANT has been set to "normal". Personally I think that empty string for default makes more sense than "normal". Should I update patch?
Review of attachment 320424 [details] [review]: (In reply to Alberts Muktupāvels from comment #5) > Should I update patch? Sure - please split out the protocol change, then: ::: src/ui/frames.c @@ +483,2 @@ + if (variant == NULL) + variant = get_theme_variant_override (frame->frames); It's no longer an override when only used as fallback - should rename the function to get_global_theme_variant() or so.
Created attachment 320431 [details] [review] frames: default theme variant now is set as empty string
Created attachment 320432 [details] [review] frames: don't force dark theme to all windows
Review of attachment 320431 [details] [review]: The commit message should mention the reason for this change (i.e. at least reference the GTK+ change), otherwise looks good to me
Review of attachment 320432 [details] [review]: OK
Thanks! P.S. Do you plan to move .ssd to window node (bug 760714)?
(In reply to Alberts Muktupāvels from comment #11) > P.S. Do you plan to move .ssd to window node (bug 760714)? That was still sitting on the theme-updates branch, pushed that now.