GNOME Bugzilla – Bug 698427
Clean up remote menu code and use GtkMenuTracker
Last modified: 2013-04-23 19:58:08 UTC
Ryan convinced me today to look at GtkMenuTracker and copy/paste it into gnome-shell to use here instead of our own custom item tracking code. It seems to be a worthwhile cleanup, and helps us match GTK+ behavior, and shift the maintenance burden of the complex tracking onto Ryan. There's a bunch of other clean-ups here that might require design review but were things that made us match GTK+, and I felt were more appropriate. For instance, this patch set replaces switches with checkboxes. This is actually sort of an important cleanup so we can have a popup menu item even before it's added the action-group without removing and replacing everything, but looking at the absurdity of switches in e.g. Totem made me think this was a good idea anyway. Requires design ACK, though. Anyway, I think this is a good cleanup, and if absolutely necessary I can probably find a way to use GtkMenuTracker even if we decide to keep switch items. Note that the GtkMenuTracker has had modifications to be more introspectable. These modifications are being tracked towards putting them back upstream in bug #698425.
Created attachment 241964 [details] [review] popupMenu: Define the dot next to the menu as an "ornament" We want to remove switches in remote menus, so make way for a checkmark ornament for the popup menu item.
Created attachment 241965 [details] [review] popupMenu: Use an icon for the ornament This makes it easy to replace the dot with another icon in the future.
Created attachment 241966 [details] [review] popupMenu: Add a check ornament This will be used to replace switches in the remote menu
Created attachment 241967 [details] [review] popupMenu: Use a checkmark for boolean items This matches GTK+'s styling, and it makes most boolean switches look more natural, as a lot of booleans are not meant for hardware switches.
Created attachment 241968 [details] [review] popupMenu: Always use a PopupMenuItem By this point, we'll know we'll always have a PopupMenuItem.
Created attachment 241969 [details] [review] popupMenu: Create and insert menu items that don't have actions yet Instead of recreating the entire model, which can be expensive... but keep them insensitive for now. This also matches what GTK+ does.
Created attachment 241970 [details] [review] popupMenu: Add and use GtkMenuTracker to build the remote menu This simplifies the code required to build remote menus and put all the items in the right place, and makes us share our implementation with GTK+.
Created attachment 241971 [details] [review] popupMenu: Remove some now-unused code We don't have any sections with separators, so don't bother tracking them.
Created attachment 242015 [details] [review] popupMenu: Define the dot next to the menu as an "ornament" We want to remove switches in remote menus, so make way for a checkmark ornament for the popup menu item.
Created attachment 242016 [details] [review] popupMenu: Use an icon for the ornament This makes it easy to replace the dot with another icon in the future.
Created attachment 242018 [details] [review] popupMenu: Add a check ornament This will be used to replace switches in the remote menu
Created attachment 242019 [details] [review] popupMenu: Use a checkmark for boolean items This matches GTK+'s styling, and it makes most boolean switches look more natural, as a lot of booleans are not meant for hardware switches.
Created attachment 242020 [details] [review] popupMenu: Always use a PopupMenuItem By this point, we'll know we'll always have a PopupMenuItem.
Created attachment 242021 [details] [review] popupMenu: Create and insert menu items that don't have actions yet Instead of recreating the entire model, which can be expensive... but keep them insensitive for now. This also matches what GTK+ does.
Created attachment 242022 [details] [review] popupMenu: Add and use GtkMenuTracker to build the remote menu This simplifies the code required to build remote menus and put all the items in the right place, and makes us share our implementation with GTK+.
Created attachment 242023 [details] [review] popupMenu: Remove some now-unused code We don't have any sections with separators, so don't bother tracking them.
Review of attachment 242022 [details] [review]: ::: js/ui/popupMenu.js @@ +1902,2 @@ let item = new PopupSubMenuMenuItem(label); + this._trackMenu(submenuModel, item); This one needs to be item.menu, or it fails in _insertItem.
I didn't review the whole thing, I just applied it locally and started testing it. It's a big change, so I hope you don't mind if I take a few days.
Review of attachment 242015 [details] [review]: ::: js/ui/popupMenu.js @@ +197,1 @@ if (!this._dot) This condition can't happen now.
Review of attachment 242016 [details] [review]: Looks good.
Review of attachment 242018 [details] [review]: Yes.
Review of attachment 242019 [details] [review]: Yes.
Review of attachment 242020 [details] [review]: Yes.
Review of attachment 242021 [details] [review]: Looks good.
Review of attachment 242023 [details] [review]: Ok
Created attachment 242289 [details] [review] popupMenu: Define the dot next to the menu as an "ornament" We want to remove switches in remote menus, so make way for a checkmark ornament for the popup menu item.
Created attachment 242290 [details] [review] popupMenu: Use a unicode character for the ornament This makes it easy to replace the dot with another label in the future. Change the allocation logic, as text layout is more complicated than simple icon logic.
Created attachment 242291 [details] [review] popupMenu: Add and use GtkMenuTracker to build the remote menu This simplifies the code required to build remote menus and put all the items in the right place, and makes us share our implementation with GTK+.
Review of attachment 242289 [details] [review]: Yes.
Review of attachment 242290 [details] [review]: Yes
Review of attachment 242291 [details] [review]: Looks good.
Attachment 242018 [details] pushed as 8430353 - popupMenu: Add a check ornament Attachment 242019 [details] pushed as b03e480 - popupMenu: Use a checkmark for boolean items Attachment 242020 [details] pushed as 099c870 - popupMenu: Always use a PopupMenuItem Attachment 242021 [details] pushed as c0afe72 - popupMenu: Create and insert menu items that don't have actions yet Attachment 242023 [details] pushed as d210399 - popupMenu: Remove some now-unused code Attachment 242289 [details] pushed as 4a2f54f - popupMenu: Define the dot next to the menu as an "ornament" Attachment 242290 [details] pushed as a123ec9 - popupMenu: Use a unicode character for the ornament Attachment 242291 [details] pushed as 196fb0f - popupMenu: Add and use GtkMenuTracker to build the remote menu