Created attachment 205895 [details] [review]
libmenu: Start with the full pool of entries to exclude from

If we want to exclude entries at all, we need to start with things to exclude!
It makes logical sense that if exclusion is the inverse of inclusion against
an empty set, we need to start with the inverse of the empty set, that is,
the full pool of entries.

We need excluded entries in gnome-shell so that we can still do application
tracking, even when the .desktop file isn't exposed by any heirarchies.

Err, unless I'm mistaken, this breaks our implementation of the spec: "(Thus an <Exclude> element that appears before any <Include> elements will have no effect, for example, as no desktop entries have been included yet.)"

Can you give me an example of what you're trying to fix?

Sure. Maybe I'm confused on what the INCLUDE_EXCLUDED flag does, maybe it should require a new flag.

In gnome-shell, we want application tracking to make sure that windows are bound back to their respective .desktop file, recognized by WM_CLASS.

gnome-menus only exposes .desktop files which are exposed by some hierarchy - there are some categories, like "Core", that aren't exposed through any hierarchy, but we still need to supply tracking for them.

Right, but are you saying that gmenu_tree_entry_get_is_excluded() doesn't work?

Here's a small test I did:

 - change applications.menu to have this for Multimedia:

  <Menu>
    <Name>Multimedia</Name>
    <Directory>AudioVideo.directory</Directory>
    <Include>
      <And>
        <Category>AudioVideo</Category>
      </And>
    </Include>
    <Exclude> <Filename>totem.desktop</Filename> </Exclude>
  </Menu>

(Note the <Exclude> tag)

 - change util/gnome-menus-ls.js to use GMenu.TreeFlags.INCLUDE_EXCLUDED and call entry.get_is_excluded() for all entries.

And I get the right thing when running it:

/Sound & Video/ Totem   /usr/share/applications/totem.desktop   true

(while all other entries display "false" for get_is_excluded())

So... I'm not sure what's wrong :-)

Can you show me a .menu file and a test program that exhibits the issue you want to fix?

Using the same modificatino to gnome-menus-ls.js, this .menu file doesn't work:

  <Menu>
    <Name>Multimedia</Name>
    <Directory>AudioVideo.directory</Directory>
    <Include>
      <And>
        <Category>AudioVideo</Category>
        <Not><Filename>totem.desktop</Filename></Not>
      </And>
    </Include>
  </Menu>

Right, but in that case, you do not exclude totem: you just don't include it. "not <Include>" doesn't mean "<Exclude>" :-)

In your specific case, totem will appear in the "Other" category. It won't appear at all if you use <Exclude>.

From my system:

  <!-- Other -->
  <Menu>
    <Name>Other</Name>
    <Directory>X-GNOME-Other.directory</Directory>
    <OnlyUnallocated/>
    <Include>
      <And>
        <Not><Category>Core</Category></Not>
        <Not><Category>Settings</Category></Not>
        <Not><Category>SystemSetup</Category></Not>
        <Not><Category>X-Red-Hat-ServerConfig</Category></Not>
        <Not><Category>Screensaver</Category></Not>
        <Not><Category>Documentation</Category></Not>
      </And>
    </Include>
  </Menu> <!-- End Other -->

Items in "Core" won't appear in "Other" either.

Ah, I see. It's a weird case. But INCLUDE_EXCLUDED cannot be used for that.

We could potentially add something like INCLUDE_UNALLOCATED, though.

Comment on attachment 205895 [details] [review]
libmenu: Start with the full pool of entries to exclude from

Based on our discussion, I don't think this patch is the right approach.

Created attachment 206606 [details] [review]
libmenu: Add GMENU_TREE_FLAGS_INCLUDE_UNALLOCATED flag

Add a new GMENU_TREE_FLAGS_INCLUDE_UNALLOCATED flag to add in the root
directory entries that are not allocated anywhere else. This is useful
if the user really wants to get absolutely all entries (in addition to
using INCLUDE_EXCLUDED, which is a bit different, and
INCLUDE_NODISPLAY).

Add gmenu_tree_entry_get_is_unallocated() API matching this flag.

Created attachment 206607 [details] [review]
util: Add --include-unallocated option to gnome-menu-spec-test

We want easy testing for GMENU_TREE_FLAGS_INCLUDE_UNALLOCATED.

Jasper: this implements what I have in mind. Does it more or less do what you'd like?

However, if I look at the diff I get in the menu when running this, it's .desktop files for gnome-control-center panels and gnome-shell.desktop (if I also include NoDisplay=true entries).

So, hrm, which .desktop file did you want to get this way? :-)

(In reply to comment #12)
> Jasper: this implements what I have in mind. Does it more or less do what you'd
> like?
> 
> However, if I look at the diff I get in the menu when running this, it's
> .desktop files for gnome-control-center panels and gnome-shell.desktop (if I
> also include NoDisplay=true entries).
> 
> So, hrm, which .desktop file did you want to get this way? :-)

A .desktop file of my own creation that I marked as "Core" for symmetry with gnome-shell.desktop, and then decided that yak shaving this entire thing when app tracking wasn't working was a better idea than removing "Core".

For reference, I do think that we should have this patch and that gnome-shell should use this anyway: it could happen that a .menu file doesn't use <OnlyUnallocated>, in which case that would matter a lot.

Was this pushed? (I'm not sure this is absolutely necessary, I'm just cleaning up my bugs)

Now yes :-)