Bug 202276 – [GTK] Fix logic of dark theme detection

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Patrick Griffis]

[GTK] Fix logic of dark theme detection

[Patrick Griffis]

Created attachment 379656 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

Comment on attachment 379656 [details]
Patch

Now that you are a committer, you should have a trac account and can add this to https://trac.webkit.org/wiki/WebKitGTK/2.26.x. Otherwise your change might not reach users until 2.28.

[WebKit Commit Bot]

Comment on attachment 379656 [details]
Patch

Clearing flags on attachment: 379656

Committed r250388: <https://trac.webkit.org/changeset/250388>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Comment on attachment 379656 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379656&action=review

> Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:569
>      if (g_str_has_suffix(themeName.get(), "-dark"))

Um, well ":dark" isn't handled here. Surely this is a mistake? Looks like a mistake to me.

Nowadays we also have PageClientImpl::themeName that would need to be fixed as well.

[Michael Catanzaro]

(Assuming this is a mistake....)

[Michael Catanzaro]

Created attachment 394977 [details]
Patch

[Michael Catanzaro]

Comment on attachment 394977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394977&action=review

> Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:574
>      GUniqueOutPtr<char> themeName;
>      g_object_get(settings, "gtk-theme-name", &themeName.outPtr(), nullptr);
> -    if (g_str_has_suffix(themeName.get(), "-dark"))
> +    if (g_str_has_suffix(themeName.get(), "-dark") || g_str_has_suffix(themeName.get(), ":dark"))
>          return true;

Patrick, is this OK?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #9)
> Comment on attachment 394977 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394977&action=review
> 
> > Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:574
> >      GUniqueOutPtr<char> themeName;
> >      g_object_get(settings, "gtk-theme-name", &themeName.outPtr(), nullptr);
> > -    if (g_str_has_suffix(themeName.get(), "-dark"))
> > +    if (g_str_has_suffix(themeName.get(), "-dark") || g_str_has_suffix(themeName.get(), ":dark"))
> >          return true;
> 
> Patrick, is this OK?

I don't think it is. Just check GTK code instead of guessing:

https://gitlab.gnome.org/GNOME/gtk/-/blob/gtk-3-24/gtk/gtksettings.c#L3255

':' is only used to check the theme variant for the GTK_THEME env variable.

[Patrick Griffis]

Comment on attachment 394977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394977&action=review

>>> Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:574
>>>          return true;
>> 
>> Patrick, is this OK?
> 
> I don't think it is. Just check GTK code instead of guessing:
> 
> https://gitlab.gnome.org/GNOME/gtk/-/blob/gtk-3-24/gtk/gtksettings.c#L3255
> 
> ':' is only used to check the theme variant for the GTK_THEME env variable.

As carlos said :dark is a variant and not part of the theme name.

[Michael Catanzaro]

OK!