After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 696468 - improve GMenuModel -> GtkMenu conversion
improve GMenuModel -> GtkMenu conversion
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Class: GtkApplication
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2013-03-23 20:27 UTC by Allison Karlitskaya (desrt)
Modified: 2013-04-01 20:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tests: add a test for gtk_menu_shell_bind() (7.16 KB, patch)
2013-03-23 20:27 UTC, Allison Karlitskaya (desrt)
committed Details | Review
Introduce GtkMenuTracker (33.40 KB, patch)
2013-03-23 20:27 UTC, Allison Karlitskaya (desrt)
none Details | Review
tests: improve gtkmenu testcase (7.65 KB, patch)
2013-03-23 20:27 UTC, Allison Karlitskaya (desrt)
committed Details | Review
Introduce GtkMenuTracker (34.45 KB, patch)
2013-03-24 03:37 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Allison Karlitskaya (desrt) 2013-03-23 20:27:28 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.
Comment 1 Allison Karlitskaya (desrt) 2013-03-23 20:27:31 UTC
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().
Comment 2 Allison Karlitskaya (desrt) 2013-03-23 20:27:34 UTC
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.
Comment 3 Allison Karlitskaya (desrt) 2013-03-23 20:27:37 UTC
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.
Comment 4 Allison Karlitskaya (desrt) 2013-03-24 03:37:38 UTC
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.
Comment 5 Lars Karlitski 2013-04-01 19:18:27 UTC
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
Comment 6 Lars Karlitski 2013-04-01 19:35:13 UTC
Review of attachment 239644 [details] [review]:

Looks good.
Comment 7 Lars Karlitski 2013-04-01 19:52:37 UTC
Also, the ABI check fails: gtk_menu_tracker_{new,free} have to be marked as internal.
Comment 8 Allison Karlitskaya (desrt) 2013-04-01 20:45:58 UTC
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