GNOME Bugzilla – Bug 739768
Use a traditional title bar on non-GNOME environments
Last modified: 2015-06-01 13:07:34 UTC
Other environments (e.g. Unity, XFCE) prefer to use traditional window decorations. In these situations we can just keep the header bar as a toolbar. I didn't reindent the .ui file so that the diff is readable; you might want to do that before commiting.
Created attachment 290143 [details] [review] Use a traditional title bar on non-GNOME environments, keeping the header bar as a toolbar
Review of attachment 290143 [details] [review]: I'm fine with this workaround for your poorly-maintained GTK+ theme if it only applies to Unity. It shouldn't apply to all non-GNOME environments.
Note: XDG_CURRENT_DESKTOP is a list, so checking it properly turns out to be slightly more involved than a strcmp, see bug #739592.
Created attachment 304058 [details] [review] Use a traditional title bar on non-GNOME environments, keeping the header bar as a toolbar
Created attachment 304059 [details] [review] GduWindow: If we're in Unity, don't set the headerbar's title Always set the window title
Created attachment 304060 [details] [review] GduWindow: When using the headerbar as a toolbar, apply the right style classes So that themes can style it as they do other toolbars.
Thanks for the review. Here's an updated version, together with some further tweak patches which are separate as I think you might want to review them independently.
Review of attachment 304058 [details] [review]: Sigh... I might have been hoping you wouldn't respond to this. :) This is OK to commit with nits *and UI file indentation* fixed; let me know if you need me to commit. ::: src/disks/gduwindow.c @@ +984,3 @@ + + names = g_strsplit (desktop_name_list, ":", -1); + for (i = 0; names[i] && !in_list; i++) You should either get rid of the redundant !in_list check or the break, whichever you prefer. Also, I prefer to have unnecessary braces around the entire for loop body, for readability. Please add them (unless David Z used this style and you are copying that -- I haven't checked -- in which case it's fine). @@ +1027,2 @@ window->header = create_header (window); + if (!in_desktop("Unity")) Don't forget the space before the opening parentheses.
Review of attachment 304060 [details] [review]: Hm... isn't it reasonable to expect Ambiance to style the header bar appropriately when it is not set as the window title bar? I am not a theme person, but I'm pretty sure that is possible and a better way to handle this. Then again, maybe it is wrong to assume a header bar outside the title bar is a toolbar. And it's a Unity-specific codepath, and it would be silly to accept the rest of your changes but not this.
Review of attachment 304059 [details] [review]: Also OK to commit with issues fixed: ::: src/disks/gduwindow.c @@ +2044,3 @@ } + + if (!in_desktop("Unity")) { Missing space here too @@ +2050,3 @@ + + title = g_strdup_printf ("%s — %s", udisks_object_info_get_description (info), str->str); + gtk_window_set_title (GTK_WINDOW (window), title); This needs to be in an else statement, otherwise you append str->str to the window title even though it's already present in the subtitle. (gtk_header_bar_set_title and gtk_window_set_title are equivalent when the header bar is the title bar.) @@ +2277,3 @@ + + title = g_strdup_printf ("%s — %s", udisks_object_info_get_description (info), device_desc); + gtk_window_set_title (GTK_WINDOW (window), title); Same here. @@ +2336,3 @@ + + title = g_strdup_printf ("%s — %s", udisks_object_info_get_description (info), device_desc); + gtk_window_set_title (GTK_WINDOW (window), title); And here
Created attachment 304347 [details] [review] Use a traditional title bar on non-GNOME environments, keeping the header bar as a toolbar
Created attachment 304348 [details] [review] GduWindow: If we're in Unity, don't set the headerbar's title
Created attachment 304349 [details] [review] GduWindow: When using the headerbar as a toolbar, apply the right style classes So that themes can style it as they do other toolbars.
Fixed, I hope. I had to resolve a conflict in the patch which was acn because I fixed the bracketing style to be consistent with the rest of the file, so that's reattached. UI file is reindented. No git account, sorry.
Review of attachment 304347 [details] [review]: I guess it's OK. Can't review the UI file; you were right to submit it without indentation the first time. :)
Review of attachment 304348 [details] [review]: Been away from GNOME programming for too long... I'd forgotten how nice it is to have null-safe g_free().
Review of attachment 304349 [details] [review]: Sigh :)
Attachment 304347 [details] pushed as c54d7fa - Use a traditional title bar on non-GNOME environments, keeping the header bar as a toolbar Attachment 304348 [details] pushed as af5c778 - GduWindow: If we're in Unity, don't set the headerbar's title Attachment 304349 [details] pushed as 3859385 - GduWindow: When using the headerbar as a toolbar, apply the right style classes