GNOME Bugzilla – Bug 732959
GtkApplication: Load theme from resource path
Last modified: 2018-05-02 16:10:18 UTC
This commit makes GTK+ look for a theme css file in $resource_base/theme/$name.css. This can be used to provide theme-specific theming for custom widgets.
Created attachment 280296 [details] [review] GtkApplication: Load theme from resource path
Review of attachment 280296 [details] [review]: I'm not crazy about having the guts of this code in GtkApplication and it presents a semi-serious practical problem: The user may change their resource base path (for god knows what reason) after their app has started up. They probably don't consider the possibility that the theme will change in the future... This patch will re-read the base path on each change, possibly ending up in the theme disappearing out from under the app. I'd prefer something more like the icon theme where we can just feed in a base path and have that used. Apps could then also use that directly for themselves. If we are to take a page from the icon theme stuff, however, maybe a better solution would be to rework the way we consider resource paths for theme components. Why do we have theme names appearing inside of that? Do we really intend for the app to respond to theme changes in a meaningful way by knowing the names of a bunch of themes (beyond just 'light' and 'dark')? Also: since this is really gtk-specific stuff, maybe we should be loading this part from outside of the gtk/ subdir? So perhaps we could use /org/example/app/gtk/custom.css and /org/example/app/gtk/custom-dark.css or something like that? Maybe we could do it a bit like the menus and have also "custom-light.css" so that: - for light theme, you get custom-light.css (if exists) and custom.css (if exists), in that priority ordering - for dark theme, you get custom-dark.css (if exists) and custom.css (if exists)
(In reply to comment #2) > The user may change their resource base path (for god knows what reason) after > their app has started up. Why should that be a supported mode of operation? Esp. if it complicates stuff like this here. However I see a different problem with this patch, and also with the proposal in comment 2, in that there is only one path per app, so you can't e.g. use this in a library that wants to provide css for its widgets.
This is tied to GtkApplication anyway, so not something that you'll see in a library.
Any progress on this? It would be really convenient to not have to wire the css (re)loading in every app I work on.
Created attachment 293140 [details] [review] GtkCssProvider: make 'reset' method public We already have loaders that make this functionality public as part of what they do (since they reset before loading the new content), so we may as well let people also do this directly.
Created attachment 293141 [details] [review] GtkApplication: support for app theme in resources Add support for the application to install some files named: - gtk/app-theme.css - gtk/app-theme-light.css - gtk/app-theme-dark.css which define application-specific theme content. The first one is always loaded if it is available. The other two are loaded (at higher priority) if available, depending on the theme variant. This provides a mechanism to override some variant-specific settings without rewriting the whole thing.
Created attachment 293142 [details] [review] bloatpad: use an app theme resource Add some ridiculously conspicuous app theme tweaks to bloatpad.
Review of attachment 293140 [details] [review]: LGTM, just needs precondition check. ::: gtk/gtkcssprovider.c @@ +1980,3 @@ + * Since: 3.16 + */ +void This function needs a g_return_if_fail() macro.
Review of attachment 293141 [details] [review]: Couple nits, LGTM ::: gtk/gtksettings.c @@ +2022,2 @@ case PROP_APPLICATION_PREFER_DARK_THEME: + settings_update_app_variant (settings); Is it worth it to fall through or just duplicate the reload? @@ +3412,3 @@ + const gchar *path) +{ + g_free (settings->priv->app_resource_path); Generally speaking I prefer: if (settings->priv->app_resource_path != path) but I don't think we'd necessarily hit that anyway how this is currently structured. More of just a defence against future changes.
Review of attachment 293142 [details] [review]: LGTM
Ryan, as far as I can see, one significant difference between Matthias' approach and this is that his allows applications to install different CSS snippets for different themes. I think that's a very desirable feature for this issue.
Review of attachment 293141 [details] [review]: ::: gtk/gtksettings.c @@ +3228,3 @@ + } + else + } Should be priv->app_provider
Review of attachment 293141 [details] [review]: ::: gtk/gtksettings.c @@ +3228,3 @@ + } + else + gtk_css_provider_reset (settings->priv->app_variant_provider); Good catch. Thanks!
(In reply to comment #12) > Ryan, as far as I can see, one significant difference between Matthias' > approach and this is that his allows applications to install different CSS > snippets for different themes. I think that's a very desirable feature for this > issue. I agree that this might be helpful. I was chatting with Christian last night and this came up. Our idea was that we might add a mechanism to also provide per-theme settings in addition to what is here (which was meant to be a starting point). That way you could have any combination of: - app-theme - app-theme-dark - app-theme-Adwaita - etc. However, we also chatted with Benjamin and there seems to be some continuing disagreement about exactly what a theme is and how apps should expect to interact with it, so I'm not exactly sure what the right thing to do here is. Benjamin raised the point that depending on Adwaita might only make sense if you depend on Adwaita 3.16. The theme called "Adwaita" in 3.18 could end up totally different again. I suspect this is going to end up starting a long conversation that may not be finished until we all meet face to face in a month. I hope that we can resolve to have this bug closed (one way or another) by the end of that meeting, however.
(In reply to comment #15) > (In reply to comment #12) > > Ryan, as far as I can see, one significant difference between Matthias' > > approach and this is that his allows applications to install different CSS > > snippets for different themes. I think that's a very desirable feature for this > > issue. > > I agree that this might be helpful. I was chatting with Christian last night > and this came up. Our idea was that we might add a mechanism to also provide > per-theme settings in addition to what is here (which was meant to be a > starting point). > > That way you could have any combination of: > > - app-theme > - app-theme-dark > - app-theme-Adwaita > - etc. > > However, we also chatted with Benjamin and there seems to be some continuing > disagreement about exactly what a theme is and how apps should expect to > interact with it, so I'm not exactly sure what the right thing to do here is. > Benjamin raised the point that depending on Adwaita might only make sense if > you depend on Adwaita 3.16. The theme called "Adwaita" in 3.18 could end up > totally different again. > > I suspect this is going to end up starting a long conversation that may not be > finished until we all meet face to face in a month. I hope that we can resolve > to have this bug closed (one way or another) by the end of that meeting, > however. I won't be meeting face-to-face, so here some opinions: a) We already have a mechanism for per-theme settings b) I don't see why you would want to complicate things by another round through settings when you can just load a theme-named resource from an app-specific path (as my original patch does) c) We load themes from versioned subdirectories now: $theme/gtk-3.16/gtk.css, $theme/gtk-3.14/... and so on. If you apply this to the per-app theme fragments as well, you have an answer for Benjamins "Adwaita 3.18" scenario.
This would still be a great thing to have done for us. Are there any updated thoughts on its chances of getting in?
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/496.