GNOME Bugzilla – Bug 761098
[PATCH] Update theming
Last modified: 2016-01-28 09:43:08 UTC
Created attachment 319693 [details] [review] nautilus-list-view: set a css name These patches update some aspects of the nautilus theming. The first patch re-enables the borders in the list view, as the "NautilusListView" selector no longer works with the latest GTK+. This was done by setting a custom css name and updating the Adwaita.css stylesheet accordingly. Here is a comparison Screenshot without patch: http://ibin.co/2UhCbMhbvnMg Screenshot with patch: http://ibin.co/2UhCLRT8WIvo The second patch just updates the button css selector from ".button" to "button". The third patch sets a name for NautilusWindow, since the "NautilusWindow" selector no longer works. This way themes can provide their own nautilus specific theming by using the "#nautilus-window" selector without affecting other applications.
Created attachment 319694 [details] [review] css: update button selector
Created attachment 319695 [details] [review] nautilus-window: set a widget name
Review of attachment 319693 [details] [review]: LGTM
Review of attachment 319694 [details] [review]: ok
(In reply to horst3180 from comment #2) > Created attachment 319695 [details] [review] [review] > nautilus-window: set a widget name I cannot find this "name" property, only in GladeWidget. If you want to use it for css, why don't set a css_name?
Sorry, I'm not very experienced with GTK/GObject, but I think it's this property: https://developer.gnome.org/gtk3/stable/GtkWidget.html#GtkWidget--name It does the same as gtk_widget_set_name (https://git.gnome.org/browse/gtk+/tree/gtk/gtkwidget.c#n3707). I didn't use gtk_widget_class_set_css_name, because it would change the "window" node name to "nautilus-window". Themes may expect the "window" node to be part of every window. In fact, Adwaita does this at one point (https://git.gnome.org/browse/gtk+/tree/gtk/theme/Adwaita/_common.scss#n1344) Although it doesn't affect nautilus, I thought it wouldn't be a good idea to change it.
(In reply to horst3180 from comment #6) > Sorry, I'm not very experienced with GTK/GObject, but I think it's this > property: > https://developer.gnome.org/gtk3/stable/GtkWidget.html#GtkWidget--name > > It does the same as gtk_widget_set_name > (https://git.gnome.org/browse/gtk+/tree/gtk/gtkwidget.c#n3707). > > I didn't use gtk_widget_class_set_css_name, because it would change the > "window" node name to "nautilus-window". Themes may expect the "window" node > to be part of every window. > In fact, Adwaita does this at one point > (https://git.gnome.org/browse/gtk+/tree/gtk/theme/Adwaita/_common.scss#n1344) > Although it doesn't affect nautilus, I thought it wouldn't be a good idea to > change it. Thanks for taking a deeper look. I asked to a gtk+ developer and I understand what you mean. However, for this case seems it's better to just set a style class rather than use "names" because it's simpler to understand and setting a name doesn't provide us any benefit in this case. Also, could you try the same thing with the nautilus-list-view patch? So you will have .nautilus-list-view .view {} and use gtk_style_context_add_class?
Created attachment 319813 [details] [review] nautilus-list-view: add a styleclass
Created attachment 319814 [details] [review] css: update button selector
Created attachment 319815 [details] [review] nautilus-window: add a styleclass
You're right, styleclasses seem like a better solution. I changed my patches accordingly.
Review of attachment 319813 [details] [review]: yes
Review of attachment 319814 [details] [review]: yep
Review of attachment 319815 [details] [review]: and the css change? is not here. Also, provide a better commit message, why we need these changes etc.
(In reply to Carlos Soriano from comment #15) > Review of attachment 319815 [details] [review] [review]: > > and the css change? is not here. > Also, provide a better commit message, why we need these changes etc. This patch is meant to give themes that are not Adwaita a way to add styling that specifically targets Nautilus. Adwaita.css doesn't need any modifications. So the css change, i.e. changing the NautilusWindow selector to .nautilus-window, needs to happen in third party themes.
Created attachment 319848 [details] [review] nautilus-window: add a styleclass I added a more in-depth explanation in the commit message.
(In reply to horst3180 from comment #16) > (In reply to Carlos Soriano from comment #15) > > Review of attachment 319815 [details] [review] [review] [review]: > > > > and the css change? is not here. > > Also, provide a better commit message, why we need these changes etc. > > This patch is meant to give themes that are not Adwaita a way to add styling > that specifically targets Nautilus. Adwaita.css doesn't need any > modifications. > > So the css change, i.e. changing the NautilusWindow selector to > .nautilus-window, needs to happen in third party themes. Ah now I get your comment 0 in the bug report. However, why would a theme provide some specific styling in nautilus window? Anyway, I'm fine with it. But add a comment to that line, if not I will probably remove it on the future for being unused :) Something like: /* Allow third party themes to match nautilus windows specifically to provide different styling to nautilus than other gtk+ applications. This was a request by some theme authors */ With a good indentation etc. Otherwise looks good.
Created attachment 319871 [details] [review] nautilus-window: add a styleclass Okay, thanks. I added a comment.
Created attachment 319872 [details] [review] nautilus-window: add a styleclass Sorry, I used the wrong comment style. Should be fine now.
Review of attachment 319871 [details] [review]: ::: src/nautilus-window.c @@ +2445,3 @@ + /* Allow third party themes to match nautilus windows to provide + specific styling for nautilus without affecting other gtk+ applications. + This was a request by some theme authors */ one nitpick: when I said good indentation etc I mean the format as well. Looks how is done in other multiline comments. I cannot format it correctly on bugzilla. Feel free to commit with that fixed. (do you have gnome git rights access?)
Review of attachment 319872 [details] [review]: aha! you realized it :) Now looks fine. Thanks for the patches!
No, I don't have access to gnome git.
Pushed to master Attachment 319813 [details] pushed as c3555a5 - nautilus-list-view: add a styleclass Attachment 319814 [details] pushed as af40f7b - css: update button selector Attachment 319872 [details] pushed as 87193d7 - nautilus-window: add a styleclass