Created attachment 229098 [details] [review]
GMenuModel menus should have a hint to hide items

Add a new GMenuModel attribute for hiding menu items for disabled
actions.

  <attribute name='when-disabled'>hide</attribute>

Will cause a menu item to be hidden instead of greyed out if the
associated action is disabled.

In order to make this work properly, be a bit more careful about calling
gtk_widget_show() on items and gtk_widget_show_all() on menu shells.

(there's another show_all too much in gtk_menu_button_set_menu_model)

This works fine for simple or toggle actions; sometimes though, what you want to do is to disable individual states of the same action.
For example, I have a "sort-by" (parameter_type: 's', state: 's') action, that is used in to build a menu like this

    <section>
      <item>
        <attribute name="action">win.sort-by</attribute>
        <attribute name="target">name</attribute>
        <attribute name="label" translatable="yes">By _Name</attribute>
      </item>
      <item>
        <attribute name="action">win.sort-by</attribute>
        <attribute name="target">size</attribute>
        <attribute name="label" translatable="yes">By _Size</attribute>
      </item>
      <item>
        <attribute name="action">win.sort-by</attribute>
        <attribute name="target">type</attribute>
        <attribute name="label" translatable="yes">By _Type</attribute>
      </item>
      <item>
        <attribute name="action">win.sort-by</attribute>
        <attribute name="target">trash-time</attribute>
        <attribute name="label" translatable="yes">By T_rash Time</attribute>
      </item>
    </section>

In this case, I would like to hide the "trash-time" menu item (but not the whole sort-by action), when the window is not in the trash directory, but the only option I have at that point is to manipulate the GMenuModel directly...not sure what's the best approach to make this easier.

Interesting.

This is what the state-hint was supposed to do: return a valid range of values for the state.

It was only ever intended to support 'debugging introspection' type purposes those (ie: tab-complete for commandline tools... that sort of thing).

Here's another desirable behavior that this approach really doesn't cover, encountered while porting Nautilus: Nautilus does searches, and allows you to save one, so let's say you have a "save-search" action on your GtkApplicationWindow. You want the action to be visible in the gear menu only in case a search directory is being displayed (so "save-search" would set when-disabled=hide), but you also want to make it insensitive while the search parameters didn't change since you last saved it.

Could be a difference between a disabled and a non-existent action?

(In reply to comment #5)
> Could be a difference between a disabled and a non-existent action?

It could, yeah - we could add the action dynamically to the action group when we load a search directory. Currently though, IIRC non-existent actions are displayed insensitive in the menu. Do you have in mind something like an additional <attribute name='when-missing'>hide</attribute>?

Sounds more like we're pushing into the direction of

  hide-when=disabled

or

  hide-when=missing

This could work - I can't think of a case right now where you would need to set both states to hide.

Did we want to move forward with something here ?

could still happen for 3.10, if a patch appears

Created attachment 265288 [details] [review]
GtkMenuTracker: cache result of hash lookup

Remove a hash lookup from the separator sync logic (which is run every
time we change a menu).  Instead, we do the lookup when creating the
section and cache the result.

This refactor will also help us in a future commit to add support for
hiding menu items based on missing actions.

Created attachment 265289 [details] [review]
GtkMenuTrackerItem: small logic tweak

Strictly speaking, can_activate should always be set back to FALSE when
the action disappears from the muxer (since we can't activate it
anymore) but we forgot to do that.

This 'bug' could never cause a problem because 'can_activate' is never
directly queried for anything at all and the item would get marked
insensitive anyway.  As soon as the action was re-added, can_activate
would be recalculated based on the new action before anything else could
happen.

All the same, this should be cleared here.

Created attachment 265290 [details] [review]
GtkMenuTracker: rework action removal a bit

Refactor the code in the action observer remove function in order to
make way for the (efficient) handling of hiding of the item in the case
that hidden-when='' is given.

Created attachment 265291 [details] [review]
GtkMenuTrackerItem: add an internal 'visible' flag

Add an internal API for checking if a GtkMenuTrackerItem is visible,
along with a signal for reporting changes in that flag.  The item will
become invisible in situations according to the new hidden-when=''
attribute, which can be set to 'action-disabled' or 'action-missing'.

This new flag doesn't actually do anything yet, and none of the
consumers of GtkMenuTracker do anything with it (nor should they).  A
followup patch will address the issue.

Created attachment 265292 [details] [review]
GtkMenuTracker: remove hidden items from the menu

Modify the tracker so that it manages the visibility of
GtkMenuTrackerItem by issuing insert and remove callbacks to the
user of the tracker.

This works by treating the GtkMenuTrackerItem as a virtual section which
contains 1 item when the item is visible and 0 items when it is hidden.

For efficiency reasons, we only employ this trick in the case that the
item has a hidden-when='' attribute set on it.

Created attachment 265293 [details] [review]
bloatpad: test hidden-when=''

Cook up some silly cases to test out the hidden-when='' attribute.

 - make sure hidden-when='action-missing' shows/hides items based on
   actions being created and destroyed

 - make sure hidden-when='action-disabled' shows/hides items based on
   actions being enabled and disabled

 - make sure hidden-when='action-missing' doesn't hide items when the
   action is merely disabled

Review of attachment 265288 [details] [review]:

ok

Review of attachment 265289 [details] [review]:

agreed

Review of attachment 265290 [details] [review]:

ok

Review of attachment 265291 [details] [review]:

ok

Review of attachment 265292 [details] [review]:

::: gtk/gtkmenutracker.c
@@ +72,3 @@
 struct _GtkMenuTrackerSection
 {
+  gpointer    model;   /* may be a GtkMenuTrackerItem or a GMenuModel */

Not sure I like where this is going... (but your extensive comment in add_items() makes this bearable ;) )

@@ +376,3 @@
+           * always have the menu item inside of a <section>, never at
+           * the toplevel.  It would be easy to add an extra boolean to
+           * check for that, but we already have a lot of those...

Please do that anyway and throw a warning to make it easy to find the reason stuff breaks when somebody adds an item without a <section>.

Review of attachment 265292 [details] [review]:

::: gtk/gtkmenutracker.c
@@ +72,3 @@
 struct _GtkMenuTrackerSection
 {
+  gpointer    model;   /* may be a GtkMenuTrackerItem or a GMenuModel */

Not sure I like where this is going... (but your extensive comment in add_items() makes this bearable ;) )

@@ +376,3 @@
+           * always have the menu item inside of a <section>, never at
+           * the toplevel.  It would be easy to add an extra boolean to
+           * check for that, but we already have a lot of those...

Please do that anyway and throw a warning to make it easy to find the reason stuff breaks when somebody adds an item without a <section>.

Review of attachment 265293 [details] [review]:

ok

(In reply to comment #22)
> +  gpointer    model;   /* may be a GtkMenuTrackerItem or a GMenuModel */
> 
> Not sure I like where this is going... (but your extensive comment in
> add_items() makes this bearable ;) )

I turned out to be less of a fan of this than I thought.  The goal here is two-fold: help to save memory (by not growing a commonly-allocated struct) and help to reduce complexity (by being able to remove the signal and unref the same thing on destroy, and use the same search function on changes).

I think maybe I'd be more comfortable with this being a GObject -- at least that way we'd get a bit more typesafety in the case that someone making changes "forgets" that it may not be a GMenuModel and tries to pass it to a GMenuModel API unconditionally..

> @@ +376,3 @@
> +           * always have the menu item inside of a <section>, never at
> +           * the toplevel.  It would be easy to add an extra boolean to
> +           * check for that, but we already have a lot of those...
> 
> Please do that anyway and throw a warning to make it easy to find the reason
> stuff breaks when somebody adds an item without a <section>.

Meh.  See the comment in the GtkMenuTrackerItem patch about invalid values for hidden-when=''.... This code will run in context of Unity and gnome-shell, and it sohuldn't spew warnings/criticals just because it got a bad menu from an application...

Comment on attachment 229098 [details] [review]
GMenuModel menus should have a hint to hide items

This was pre-menutracker (and never worked properly anyway, with respect to separators).

I guess I am doing somethign wrong but I get the items duplicated:

    def _insert_directory(self, directory, menu):
        for d in sorted(directory.subdirs, key=lambda x: x.name.lower()):
            submenu = Gio.Menu()
            item = Gio.MenuItem.new_submenu(d.name.replace('_', '__'), submenu)
            menu.append_item(item)
            section = Gio.Menu()
            item_section = Gio.MenuItem.new_section(None, section)
            submenu.append_item(item_section)

            self._insert_directory(d, section)

        for tool in sorted(directory.tools, key=lambda x: x.name.lower()):
            action_name = 'external-tool_%X_%X' % (id(tool), id(tool.name))
            self._action_tools[action_name] = tool

            action = Gio.SimpleAction(name=action_name)
            action.connect('activate', capture_menu_action, self._window, self._panel, tool)
            self._window.add_action(action)

            item = Gio.MenuItem.new(tool.name.replace('_', '__'), "win.%s" % action_name)
            item.set_attribute_value("hidden-when", GLib.Variant.new_string("action-disabled"))
            print("tool", tool.name)
            menu.append_item(item)

So I was missing a section which is now fixed. I guess that I would like to have some warning about this, but apart from that it works fine for me

The API is just fine by me. It would be nice to have a helper for it, probably in GIO where GMenu is implemented, and document the attribute there too.

Attachment 265288 [details] pushed as fb14a78 - GtkMenuTracker: cache result of hash lookup
Attachment 265289 [details] pushed as 8256b88 - GtkMenuTrackerItem: small logic tweak
Attachment 265290 [details] pushed as 8efb140 - GtkMenuTracker: rework action removal a bit
Attachment 265291 [details] pushed as 2b1aa12 - GtkMenuTrackerItem: add an internal 'visible' flag

Created attachment 265746 [details] [review]
GtkMenuTracker: fix hidden-when='' vs. separators

Ensure that adding hidden-when='' to a menu item does not produce an
extra separator item as a side effect.

Attachment 265292 [details] pushed as 2112af7 - GtkMenuTracker: remove hidden items from the menu
Attachment 265293 [details] pushed as 2bcb4fc - bloatpad: test hidden-when=''
Attachment 265746 [details] pushed as d31bf77 - GtkMenuTracker: fix hidden-when='' vs. separators

Leaving open for GIO patches.

What gio patches are those ? everything attached to this bug is committed...

(In reply to Ryan Lortie (desrt) from comment #31)
> Leaving open for GIO patches.

wat?