GNOME Bugzilla – Bug 696468
improve GMenuModel -> GtkMenu conversion
Last modified: 2013-04-01 20:46:06 UTC
The current conversion process is bad for mostly two reasons: 1) It's inefficient: any change to the menu results in the entire menu being rebuilt (due to the difficulty of mapping a change in the GMenuModel to an index in the menu, accounting for the sizes of other sections). 2) The logic for dealing with separators has been copied to at least 4 places by now: gtk_menu_shell_bind(), gtkmodelmenu-quartz, gnome-shell and QMenuModel. The GtkMenuTracker code can now be shared by everyone (since it has nothing to do with GtkMenu) and is quite an improvement in efficiency (as measured by the testcase included). I didn't deal with action_namespace yet, but I want to get these patches uploaded so reviews can start. I will provide an update later.
Created attachment 239642 [details] [review] tests: add a test for gtk_menu_shell_bind() Borrow the RandomMenu code from the GLib testsuite and hook it up to gtk_menu_shell_bind().
Created attachment 239643 [details] [review] Introduce GtkMenuTracker GtkMenuTracker folds a nested structure of sections in a GMenuModel into a single linear menu, which it expresses to its user by means of 'insert item at position' and 'remove item at position' callbacks. The logic for where to insert separators and how to handle action namespaces is contained within the tracker, removing the need to have this logic duplicated in the 3 or 4 places that consume GMenuModel. In comparison with the previous code, the tracker no longer completely destroys and rebuilds menus every time a single change occurs. As a result, the new gtkmenu testcase now runs in approximately 3 seconds instead of ~60 before.
Created attachment 239644 [details] [review] tests: improve gtkmenu testcase Don't just create a menushell and populate it with random data -- verify that the resulting menu layout is actually correct. This is introduced in a separate commit because the old code was failing this part of the test.
Created attachment 239660 [details] [review] Introduce GtkMenuTracker GtkMenuTracker folds a nested structure of sections in a GMenuModel into a single linear menu, which it expresses to its user by means of 'insert item at position' and 'remove item at position' callbacks. The logic for where to insert separators and how to handle action namespaces is contained within the tracker, removing the need to have this logic duplicated in the 3 or 4 places that consume GMenuModel. In comparison with the previous code, the tracker no longer completely destroys and rebuilds menus every time a single change occurs. As a result, the new gtkmenu testcase now runs in approximately 3 seconds instead of ~60 before.
Review of attachment 239660 [details] [review]: Great stuff ;) ::: gtk/gtkmenushell.c @@ +525,3 @@ _gtk_key_hash_free (priv->key_hash); + if (priv->tracker) + gtk_menu_tracker_free (priv->tracker); No need to free this again here, it's already freed in dispose() ::: gtk/gtkmenutracker.c @@ +57,3 @@ +gtk_menu_tracker_section_find_model (GtkMenuTrackerSection *section, + GMenuModel *model, + gint *offset) whitespace. @@ +93,3 @@ + gboolean could_have_separator, + GMenuModel *parent_model, + gint parent_index) This function is a bit complicated. Can you add a comment saying what it does, please? @@ +112,3 @@ + + n_items += gtk_menu_tracker_section_sync_separators (subsection, tracker, offset + n_items, + could_have_separator, section->model, i); Whitespace @@ +205,3 @@ + + g_menu_model_get_item_attribute (model, position + n_items, + G_MENU_ATTRIBUTE_ACTION_NAMESPACE, "s", &action_namespace); action_namespace is not freed
Review of attachment 239644 [details] [review]: Looks good.
Also, the ABI check fails: gtk_menu_tracker_{new,free} have to be marked as internal.
Pushed with suggested changes (after an ACK from Company on IRC). Thanks for the review. Attachment 239642 [details] pushed as e250e52 - tests: add a test for gtk_menu_shell_bind() Attachment 239644 [details] pushed as 45a94cc - tests: improve gtkmenu testcase Attachment 239660 [details] pushed as 5617b58 - Introduce GtkMenuTracker