GNOME Bugzilla – Bug 749903
YelpWindow: In Unity, use the header bar as a toolbar
Last modified: 2015-07-15 13:14:11 UTC
I hope you'll consider this patch to use the header bar as a toolbar under Unity. The headerbar works quite well as a toolbar. We also have to do some fiddling with the title to set it on the window instead of the headerbar when in Unity mode.
Created attachment 304006 [details] [review] YelpWindow: In Unity, use the header bar as a toolbar Unity prefers not to use headerbars. Ours is suitable for use as a toolbar, so use it as such.
Review of attachment 304006 [details] [review]: Patch looks OK, but it does not build with the strict compiler warnings that Yelp uses. :-( ::: src/yelp-window.c @@ +282,3 @@ +gboolean +in_desktop (const gchar *name) This function needs a prototype, or to be placed after the first usage (and should be static, I suppose). @@ +344,3 @@ + gtk_style_context_remove_class (headerbar_context, "header-bar"); + gtk_style_context_add_class (headerbar_context, "toolbar"); + gtk_style_context_add_class (headerbar_context, "primary-toolbar"); Would be good to use the symbolic constants for these. For example, GTK_STYLE_CLASS_TOOLBAR and GTK_STYLE_CLASS_PRIMARY_TOOLBAR (bot nothing for "header-bar", sadly). @@ +1192,3 @@ + if (in_desktop ("Unity")) { + if (page_title && root_title) { + title = g_strdup_printf ("%s - %s", page_title, root_title); title is const, but g_strdup_printf() returns a non-const string. Also, I am not sure if this should be a translatable string, as I can imagine that the title/subtitle order would be different for other languages (and possibly the separator character would also be different). It probably needs clarification from translators, so please ask gnome-i18n@gnome.org for advice.
You might consider using only one title instead of trying to combine them. Pre-headerbar Yelp only used the root title. But I'll defer to Unity designers on that.
Created attachment 304859 [details] [review] YelpWindow: In Unity, use the header bar as a toolbar Unity prefers not to use headerbars. Ours is suitable for use as a toolbar, so use it as such.
(In reply to Shaun McCance from comment #3) > You might consider using only one title instead of trying to combine them. > Pre-headerbar Yelp only used the root title. But I'll defer to Unity > designers on that. I did this, thanks. It seems simpler, but let me know if you want to try going for the other thing. (In reply to David King from comment #2) > Review of attachment 304006 [details] [review] [review]: > > Patch looks OK, but it does not build with the strict compiler warnings that > Yelp uses. :-( Hm, it did build for me. I think I fixed it all but I don't know why jhbuild isn't using the right flags so sorry if it still is broken. :( > [ more comments ] Should be fixed or no longer relevant now that we're just using the root title (as long as you agree with this change).
I talked with Dave about this on IRC. We agreed it would be better to use the gtk-dialogs-use-header setting, rather than XDG_CURRENT_DESKTOP. That way, we don't have to chase down the right behavior in other environments like XFCE. If your GTK+ dialogs are doing the right thing, so is Yelp. https://developer.gnome.org/gtk3/stable/GtkSettings.html#GtkSettings--gtk-dialogs-use-header
I wish there was a proper setting - we have GNOME apps implementing this kind of check in different ways now (in Unity, not in GNOME, dialogs-use-header), but okay. I think that's what we did with Cheese too. Thanks for the feedback!
Created attachment 305685 [details] [review] YelpWindow: In Unity, use the header bar as a toolbar Unity prefers not to use headerbars. Ours is suitable for use as a toolbar, so use it as such.
Review of attachment 305685 [details] [review]: I applied a slight formatting fix, and merged this to master as commit b479f660d607312f713a1896253dbcf409e9f53b. Thanks!
*** Bug 752399 has been marked as a duplicate of this bug. ***