After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 761543 - frames: don't force dark theme to all windows
frames: don't force dark theme to all windows
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2016-02-04 10:42 UTC by Alberts Muktupāvels
Modified: 2016-02-04 14:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
frames: don't force dark theme to all windows (1.36 KB, patch)
2016-02-04 10:42 UTC, Alberts Muktupāvels
none Details | Review
frames: don't force dark theme to all windows (1.48 KB, patch)
2016-02-04 12:18 UTC, Alberts Muktupāvels
none Details | Review
frames: default theme variant now is set as empty string (918 bytes, patch)
2016-02-04 13:31 UTC, Alberts Muktupāvels
committed Details | Review
frames: don't force dark theme to all windows (1.67 KB, patch)
2016-02-04 13:31 UTC, Alberts Muktupāvels
committed Details | Review

Description Alberts Muktupāvels 2016-02-04 10:42:36 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
Comment 1 Florian Müllner 2016-02-04 12:07:18 UTC
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") ...
Comment 2 Alberts Muktupāvels 2016-02-04 12:15:16 UTC
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.
Comment 3 Alberts Muktupāvels 2016-02-04 12:18:48 UTC
Created attachment 320424 [details] [review]
frames: don't force dark theme to all windows
Comment 4 Florian Müllner 2016-02-04 12:43:45 UTC
(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)
Comment 5 Alberts Muktupāvels 2016-02-04 13:01:52 UTC
(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?
Comment 6 Florian Müllner 2016-02-04 13:15:40 UTC
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.
Comment 7 Alberts Muktupāvels 2016-02-04 13:31:12 UTC
Created attachment 320431 [details] [review]
frames: default theme variant now is set as empty string
Comment 8 Alberts Muktupāvels 2016-02-04 13:31:24 UTC
Created attachment 320432 [details] [review]
frames: don't force dark theme to all windows
Comment 9 Florian Müllner 2016-02-04 13:51:05 UTC
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
Comment 10 Florian Müllner 2016-02-04 13:51:08 UTC
Review of attachment 320432 [details] [review]:

OK
Comment 11 Alberts Muktupāvels 2016-02-04 14:07:11 UTC
Thanks!

P.S. Do you plan to move .ssd to window node (bug 760714)?
Comment 12 Florian Müllner 2016-02-04 14:22:21 UTC
(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.