GNOME Bugzilla – Bug 724480
use a title-combobox instead of a gear menu
Last modified: 2014-04-07 14:08:41 UTC
It’s really better to see the “change mode” thing in the headerbar and not in the app’menu. But, trying it: * you don’t know what it does before tryinig it; I initially wanted to file a bug for replacing it by a “Mode” combobox (would look bad on the simple mode, but…); * it’s quite strange to have a gear-menu with no “options”, only a choose-your-mode thing (in Files for example, the options are in the gear-menu, the display is in an other button); * the gear-menu is usually at the right of the headerbar (would look bad here I think, but…); and, the left of the headerbar is used for main actions. So, I suggest using the “title-combobox”[1] way: * you immediatly know what clicking here will do; * one less square button (:D); * better consistancy with other apps. [1] how is that called? http://imgur.com/6SOJIok
Could you attach that image here -- imgur doesn't seem to be working right now (or for me) FWIW I agree the gear menu is non-ideal, especially off on the left, but I wasn't sure how better to handle this. I wonder if using the title as a combo box might not be an anti-pattern, though; are any other GNOME apps doing that?
Created attachment 269355 [details] gedit sidebar’s headerbar with popover I’m sorry, I didn’t well explained myself, because of the link to the image. It is from the development version of Gedit (it’s not mine, and I can’t reproduce it on Rawhide without jhbuilding: in my Gedit, it’s a real old-style combobox that is used). It’s “only” for the sidebar, but it replaced a (bad) notebook and it really looks cool, even with the small width of the sidebar. Having a look on the API, what I suggest is a “button with popover”[1], or a “combobox” if the combobox widget is begining to use this look. Calculator is the only app’ that has different window mode of different sizes (and a simple one with a really small width); no pattern here. But, it’s usual to have the “change mode” widget as a centered title, with the 3.10’s GtkStack[2] (System Monitor, Clocks, Softwares, and probably others). [1] https://developer.gnome.org/gtk3/unstable/GtkPopover.html [2] https://developer.gnome.org/gtk3/stable/GtkStack.html
I don't think a GtkStack makes sense for a simple app like Calculator, with such a tiny window (particularly in Basic Mode). But the gedit sidebar is a good example for why your idea makes sense. I suspect that would work better than the gear menu.
Created attachment 270413 [details] [review] Depend on GTK+ 3.11.6 For GtkPopover, including integration with GtkMenuModel
Created attachment 270414 [details] [review] Depend on Vala 0.23.3 For latest GTK+ 3.11.6 bindings
Created attachment 270415 [details] [review] Use a popover to change modes, not a gear menu This packs the mode label (presently the header bar title) into a GtkMenuButton, which uses a popover for changing the mode. Requires a freeze exception. We can *probably* get one if we act fast (and if you like the change, Arth). I'm not sure that I like the radio buttons, and not only because Adwaita seems to be having some trouble with them. We can avoid those if we use a GtkButtonBox instead of a GtkMenuButton, and manually create buttons for the different modes. I'll probably work on that only if asked to.
Created attachment 270416 [details] screenshot
(In reply to comment #6) > I'm not sure that I like the radio buttons, and not only because Adwaita seems > to be having some trouble with them. We can avoid those if we use a > GtkButtonBox instead of a GtkMenuButton, and manually create buttons for the > different modes. I'll probably work on that only if asked to. I'm thinking this would be a superior approach. (It's what gedit does.)
Michael, Great work. In my personal opinion, it'll be better if we delay the update for the next iteration of Calculator. There are a few advantages in waiting. We will have stable version of Vala and GTK+ in that period. Also, we are planning to change the calculation back-end in the next iteration. Which will make perfect sense for the distros which don't want the extra dependencies (or newer version of dependencies), to stay with the 3.12 fork.
I'm inclined to agree, especially since we're now much closer to release than we were when I posted the patch, and I'm not planning on trying out the ButtonBox approach right away.
Sorry for the late comment, holidays are the time where I’m not on computer. ^^’ It’s very nice here with your patch, and I really think it’s the way to go. In your screenshot, there’s a button ‘and’ a radio thing; strangely (for me), when applying here, I only have a radio mark (I have a more recent Gtk, is it that?). The two ways (buttons or radio list) are cool, the two at the same time are of course overkill. Little thing: I think the popover should close, because the window “moves” (changes width, in fact). That would be different with what gEdit does, but maybe it’s a thing they should correct the same way (they have in some case the same problem).
(In reply to comment #11) > It’s very nice here with your patch, and I really think it’s the way to go. In > your screenshot, there’s a button ‘and’ a radio thing; strangely (for me), when > applying here, I only have a radio mark (I have a more recent Gtk, is it > that?). The two ways (buttons or radio list) are cool, the two at the same time > are of course overkill. Yup, because you have a more recent GTK+ (or maybe gnome-themes-standard). I definitely prefer the new version to what's in my screenshot. I don't think we need to bother with a GtkButtonBox anymore. > Little thing: I think the popover should close, because the window “moves” > (changes width, in fact). That would be different with what gEdit does, but > maybe it’s a thing they should correct the same way (they have in some case the > same problem). I don't know; seems reasonable to me, but this should be decided by GTK+ so it works the same for all apps that use popovers.
(In reply to comment #9) > Michael, > > Great work. > In my personal opinion, it'll be better if we delay the update for the next > iteration of Calculator. There are a few advantages in waiting. Is this patch OK to push after 3.12 is branched, or do you need to review it still?
(In reply to comment #12) I upgraded gEdit in the night, and it now (logically) closes its headerbar’s popover. I don’t think GTK+ has to manage if the popover closes or not; that’s the difference with a menu: a menu is a one-action thing, a popover could be used for more complex things[1] (see the two popovers in the statusbar of gEdit, they are persistent). [1] https://wiki.gnome.org/Design/HIG/Popovers
(In reply to comment #13) > Is this patch OK to push after 3.12 is branched, or do you need to review it > still? Sure. It's good to go.. :) Thanks for the patch.
Created attachment 273565 [details] [review] patch to close the popover As I’ve done it for my personal use, here’s a patch-of-newbie-but-that-works for closing the popover, to apply on top of the one by Michael Catanzaro.
Arnaud, Thanks for the patch. :) Michael, I think it's time when we bump the GTK+ requirements to 3.12. I've already bumped up the valac requirement to 0.24. But, before we push the changes, there's one thing to be fixed. After changing the mode, the application focus doesn't return back to input box. It'll be better if we force focus the text-box just after closing the popover. Thank you.. :)
Review of attachment 273565 [details] [review]: Tried to apply this but got "fatal: invalid date format: ven., 4 avril 2014 05:02:15 +0200" , so can you please recreate using git format-patch? (You might also want to file a bug against gitg. It looks like gitg tries to make a git-style patch, but fails due to locale.) ::: src/math-window.vala @@ +225,3 @@ { + var popover = menu_button.get_popover(); + popover.hide(); Also, be sure to leave a space before the opening parentheses. This style is common in GNOME apps.
Created attachment 273678 [details] [review] patch with git format-patch Always play with a new tool before using it… Hope it will work this time. And yes, I’ll report a bug on Gitg.
(In reply to comment #17) > But, before we push the changes, there's one thing to be fixed I imagine we also need to update the Dogtail tests. Haven't looked into that yet.
Review of attachment 273678 [details] [review]: Looks good; setting to reviewed instead of accepted simply because we need to commit everything together.
Created attachment 273680 [details] [review] Focus the input field after changing the mode Otherwise, you need to click on the input field before you can enter any numbers. Spotted by Arth.
Hey Vadim, can you help us update the Dogtail tests for this? They're still able to click on the menu, but can no longer click on any of the menu items. I notice that after this patch series, all the menu items have disappeared from AT-SPI Browser: that's probably bad, right? This seems to be caused by setting the use_popover property of GtkMenuButton. Does this seem like a GTK+ bug?
(In reply to comment #23) > I notice that after this patch series, all the menu items have disappeared from > AT-SPI Browser: that's probably bad, right? This seems to be caused by setting > the use_popover property of GtkMenuButton. Does this seem like a GTK+ bug? Yes, it seems that popovers a11y in gtk still needs some improvement. I'm not that good in GTK internals, could you file a bug on that in gtk? I'm ready to provide you with a small reproducer.
Created attachment 273699 [details] [review] Update tests It seems that popovers are a11y-enabled, we were just using the wrong role name for radio button. This patch fixes it, verified locally that all tests have passed
OK, everything seems to be in order now, thanks everyone! Attachment 270413 [details] pushed as 73b139a - Depend on GTK+ 3.11.6 Attachment 270415 [details] pushed as 44495a8 - Use a popover to change modes, not a gear menu Attachment 273680 [details] pushed as d2944bf - Focus the input field after changing the mode The other two patches have been pushed as well.