GNOME Bugzilla – Bug 730903
HeaderBar as toolbar in composer looks out-of-place in some themes
Last modified: 2014-06-04 00:50:53 UTC
Created attachment 277410 [details] Screenshot I'm using (a variant of) the Ambiance theme from Ubuntu. Under this, HeaderBars are set to a dark slate gray to mimic the window bars of server-decorated windows. This looks very out-of-place when used in the middle of a window, as it is in the composer (see screenshot). I don't know if there's some switch to throw to tell it to style itself as a normal toolbar. If not, perhaps we should switch it back to a standard toolbar. (Why was it changed to a headerbar in the first place?)
The original code to give the main window's toolbar "pill" shapes was reused for the composer window's toolbar. When we moved the main window's toolbar to GtkHeaderBar, the composer window's toolbar inherited it as well. I agree, the toolbar shouldn't be a GtkHeaderBar. We should port the toolbar back to a GtkToolbar.
(In reply to comment #1) > The original code to give the main window's toolbar "pill" shapes was reused > for the composer window's toolbar. When we moved the main window's toolbar to > GtkHeaderBar, the composer window's toolbar inherited it as well. > > I agree, the toolbar shouldn't be a GtkHeaderBar. We should port the toolbar > back to a GtkToolbar. I think a better solution is just to remove the headerbar style from the toolbar in the composer, and added instead the toolbar style.
If that works, fine, but my point is that GtkHeaderBar seems to be designed for placement at the top of a window as its main toolbar (among other things), not to be placed in the middle of a window below other controls. My concern about changing its style is that the GtkHeaderBar code will remain and perhaps introduce other side-effects. But at this point I'll take a style patch and see what else shakes out.
Created attachment 277739 [details] [review] Patch using a mixin Solving this with styles would scare me a bit. Sure, it might work now, but who knows what future version of GTK will add a HeaderBar feature than we don't want in a toolbar. Instead, I think we should provide all the pill-styling stuff in a mixin, which can be combined with a HeaderBar, a Toolbar, or whatever else we want. That's what this patch does. Thanks to Vala's inheritance rules, the mixin has to be an interface, which means that it can't provide attributes, properties, or a constructor. So there's a little bit of boilerplate that this requires, but it's not too bad. There's now a PillHeaderBar, which mixes into the HeaderBar, and a PillToolbar. The latter actually uses a Gtk.Box, rather than a Toolbar, as I originally planned. It looked like it'd be a bit of work to make a Toolbar act like we want it to, and we wouldn't be using any of its Toolbar-ish feature, so I figured it'd be easer just to use a Box. By default, there wasn't enough spacing in the PillToolbar, so I added 6px between the groups. I'm not sure what the HeaderBar does, but it looks pretty similar. We may want to add a bit of padding above and below the items in the Toolbar. By the way, this patch was attached with git-bz, so you can see how it works.
Yes -- Gtk.Box was the right way to go. Gtk.Toolbar is an overly-complicated beastie and best avoided. The only change I would say to make is the new interface's class requirements, i.e. ": Object" at pill-toolbar.vala:14. That indicates the interface must be attached to a Object. That's technically okay, but it makes more sense for the requirement to be Gtk.Widget. Gtk.Container is tempting, but that might be too specific. In either case, it shouldn't be Object. I'll let you make the call between Container and Widget. Make that change and commit!
I didn't understand what the ": Object" meant; I just put it there so it would compile. I think Gtk.Container is appropriate; that's what I said in the comments. BTW, I closed this bug using git-bz. You'll notice that the commit message doesn't have the bug number in the title, but it does have the full bugzilla link at the bottom of the message. If you don't like this, I can go back to the old way (or hack git-bz to change that.) It also doesn't include the commit reference in this message, so I'll put it in by hand: 39ef1360