GNOME Bugzilla – Bug 706376
Set the top bar as our titlebar and add a close button
Last modified: 2013-08-20 17:41:45 UTC
See patches.
Created attachment 252365 [details] [review] toolbar: port GtkToolbar to GtkBox and add close button
Created attachment 252366 [details] [review] window: make the top bar as titlebar
Created attachment 252367 [details] Screenshot
The screenshots look great to me, and this will bring nautilus in line with the other core apps we've ported to the headerbar style for 3.10.
Created attachment 252412 [details] [review] build: bump GTK requirement
Review of attachment 252365 [details] [review]: Looks mostly good - some coding style comments. ::: src/nautilus-toolbar.c @@ +455,1 @@ + if (rtl) Curly brace should be on the same line as the if here. @@ +476,3 @@ + gtk_widget_set_valign (button, GTK_ALIGN_CENTER); + gtk_container_add (GTK_CONTAINER (toolbar), button); + if (rtl) Same here. @@ +478,3 @@ + if (rtl) + { + gtk_widget_set_margin_right (button, 76); Why 76? Shouldn't you use the same 12px margin as before? @@ +521,3 @@ + gtk_container_add (GTK_CONTAINER (toolbar), button); + if (rtl) Coding style @@ +532,3 @@ + gtk_container_add (GTK_CONTAINER (toolbar), GTK_WIDGET (separator)); + + gtk_widget_set_margin_left (button, 12); You can re-use the existing "rtl" variable here... @@ +546,3 @@ + gtk_container_add (GTK_CONTAINER (toolbar), GTK_WIDGET (button)); + + if (gtk_widget_get_direction (GTK_WIDGET (separator)) == GTK_TEXT_DIR_RTL) ...and here.
Review of attachment 252366 [details] [review]: ::: src/nautilus-window.c @@ +1489,3 @@ G_CALLBACK (nautilus_window_load_extension_menus), window, G_CONNECT_SWAPPED); + + event_box = gtk_event_box_new (); Hmm - I'd rather you created the event box as part of NautilusToolbar, taking the place of the previously existing GtkToolbar. Actually, thinking about it, why do you need to port away from GtkToolbar at all? Can you just use that as titlebar instead of rewriting it as a GtkBox? @@ +1497,3 @@ + gtk_container_add (GTK_CONTAINER (event_box), window->details->toolbar); + gtk_window_set_titlebar (GTK_WINDOW (window), event_box); + gtk_event_box_set_visible_window (GTK_EVENT_BOX (event_box), TRUE); The event box is visible by default, so you shouldn't need this.
Review of attachment 252412 [details] [review]: This is fine, pending the comments on the previous patches.
Review of attachment 252365 [details] [review]: ::: src/nautilus-toolbar.c @@ +87,3 @@ gint toolbar_size_px, menu_size_px; + g_object_get (gtk_settings_get_default (), "gtk-toolbar-icon-size", &toolbar_size, NULL); I wonder if we couldn't get rid of this icon margin hack and just rely on gtk_button_new_from_icon_name() setting the right style class and the theme applying the right padding instead.
(In reply to comment #7) > Review of attachment 252366 [details] [review]: > > ::: src/nautilus-window.c > @@ +1489,3 @@ > G_CALLBACK (nautilus_window_load_extension_menus), window, > G_CONNECT_SWAPPED); > + > + event_box = gtk_event_box_new (); > > Hmm - I'd rather you created the event box as part of NautilusToolbar, taking > the place of the previously existing GtkToolbar. > Actually, thinking about it, why do you need to port away from GtkToolbar at > all? Can you just use that as titlebar instead of rewriting it as a GtkBox? Maybe move this to create_toolbar ()? And realy, I don't remember why I ported GtkToolbar to GtkBox. I remember have reason. maybe because bug #706050. > > @@ +1497,3 @@ > + gtk_container_add (GTK_CONTAINER (event_box), window->details->toolbar); > + gtk_window_set_titlebar (GTK_WINDOW (window), event_box); > + gtk_event_box_set_visible_window (GTK_EVENT_BOX (event_box), TRUE); > > The event box is visible by default, so you shouldn't need this. ok.
Created attachment 252426 [details] [review] toolbar: port GtkToolbar to GtkBox and add close button The margin is change to 76px after comment from Allan Day. I removed the function get_icon_margin (), for nautilus will consistent with the other apps.
(In reply to comment #10) > Maybe move this to create_toolbar ()? > And realy, I don't remember why I ported GtkToolbar to GtkBox. > I remember have reason. maybe because bug #706050. I think you should be able to make the eventbox part of NautilusToolbar directly instead.
(In reply to comment #12) > (In reply to comment #10) > > > Maybe move this to create_toolbar ()? > > And realy, I don't remember why I ported GtkToolbar to GtkBox. > > I remember have reason. maybe because bug #706050. > > I think you should be able to make the eventbox part of NautilusToolbar > directly instead. ok.
Review of attachment 252426 [details] [review]: ::: src/nautilus-toolbar.c @@ +385,3 @@ + + toplevel = gtk_widget_get_toplevel (GTK_WIDGET (button)); + gtk_window_close (GTK_WINDOW (toplevel)); Indentation here should follow the rest of the file. Also, instead of using gtk_widget_get_toplevel(), you should use the window stored in the toolbar's private structure.
Allan, why the 76px margin on the search icon? Shouldn't it be the same as the space between search and the next icon?
(In reply to comment #15) > Allan, why the 76px margin on the search icon? Shouldn't it be the same as the > space between search and the next icon? We need to leave some header bar exposed so you can drag the window...
(In reply to comment #16) > We need to leave some header bar exposed so you can drag the window... Oh hmm, good point, I guess it's fine then. In most cases, the pathbar won't extend that far towards the icon anyway.
Created attachment 252439 [details] [review] Test patch The patch for tested - the top bar is not opacity!
(In reply to comment #17) > (In reply to comment #16) > > > We need to leave some header bar exposed so you can drag the window... > > Oh hmm, good point, I guess it's fine then. In most cases, the pathbar won't > extend that far towards the icon anyway. Have location-entry...
As we're approaching UI freeze and I have to release a tarball before tonight, I reworked your last patch to use a GtkFrame, together with some small cleanups, and pushed it to master. FWIW, your test patch wasn't working because you forgot to change the parent GType inside the G_DEFINE_TYPE macro, in nautilus-toolbar.c. Eventually, I would like this toolbar to be a propert GtkHeaderBar subclass, but that's for a separate cleanup, as it doesn't require UI changes. Thanks for your work.
Created attachment 252458 [details] screenshot Thanks Cosimo and Yosef! I'm really happy to have this in for 3.10. I just tested it and saw some visual glitches: the HeaderBar has a 2px border running around it, the buttons seem to be slightly less tall than before, and the button icons don't seem to be quite vertically centered. Not sure if it's something to do with my JHBuild or not (I have reverted the gnome-themes-standard commit that's mentioned in bug 706418), but I thought I'd let you know.
I don't see those issues here... except for buttons being slightly less tall - I just pushed a fix for the buttons to git master (unfortunately I just released 3.9.90 without it...).