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 490483 - Merging two menus with the same entries duplicates them
Merging two menus with the same entries duplicates them
Status: RESOLVED FIXED
Product: gnome-menus
Classification: Core
Component: general
2.20.x
Other Linux
: Normal minor
: ---
Assigned To: gnome-menus dummy account
gnome-menus dummy account
Depends on:
Blocks: 543078
 
 
Reported: 2007-10-26 11:34 UTC by Josselin Mouette
Modified: 2009-06-29 17:27 UTC
See Also:
GNOME target: ---
GNOME version: 2.19/2.20


Attachments
Sort again the entries after a merge (2.88 KB, patch)
2008-05-13 11:20 UTC, Josselin Mouette
needs-work Details | Review
[libmenu] Sort inlined items unless inline_header is used (21.59 KB, patch)
2009-06-27 09:15 UTC, Vincent Untz
reviewed Details | Review

Description Josselin Mouette 2007-10-26 11:34:11 UTC
I have a setup in which some submenus are merged with the following layout (permitted by the freedesktop specification):
    <DefaultLayout inline="true" inline_limit="8" inline_header="false">
      <Merge type="menus"/>
      <Merge type="files"/>
    </DefaultLayout>

This works fine, but with two caveats:
 - the entries are not sorted alphabetically; this is normal behavior with inline_header="true", but with inline_header="false" the sorting should be done after merging them;
 - when the same entry matches two of the merged submenus, it displays twice.
Comment 1 Josselin Mouette 2008-05-13 11:20:00 UTC
Created attachment 110846 [details] [review]
Sort again the entries after a merge

This is an attempt that seems to fix both issues. It does it by sorting again the entries after a merge, but only if the merge is done without inline_alias nor inline_header.

I guess that it can break some custom layouts (where the whole layout is done by hand) under some circumstances, but I haven’t even seen one so I can’t test.
Comment 2 Vincent Untz 2008-05-17 14:11:06 UTC
Took a quick look at the patch. One issue is that your sort works fine for the default layout you pasted here (menus and then files) but it will break if I want to have files before menus or something which is mixed.

It's probably better to fix the first issue by doing the step to process inline stuff at the beginning of process_layout_info(). The second issue could then be fixed in merge_entries() and merge_subdirs_and_entries().
Comment 3 Josselin Mouette 2008-05-18 13:15:58 UTC
I’m afraid I don’t understand why moving this to the beginning of process_layout_info would help. The sort is done much earlier than that anyway, in gmenu_tree_strip_duplicate_children, which is executed at the end of gmenu_tree_load_layout. To work correctly in all cases, wouldn’t the inling have to be done right before stripping duplicates (for inline_header=false) and right after it (for inline_header=true) ?
Comment 4 Vincent Untz 2008-05-18 13:41:21 UTC
The sort which is done in gmenu_tree_strip_duplicate_children() is not to get the order in the menu, it's just used so that it's possible to actually remove the duplicates.

Now, I'm not 100% sure that process_layout_info() is the best place -- I'd need to check to be sure -- but it's relatively reasonable. Stripping duplicates can be moved from earlier to there if required.
Comment 5 Vincent Untz 2008-05-19 12:39:59 UTC
FWIW, the current behavior does what the spec specifies: "If the inline attribute is "true" the menu that is referenced may be copied into the current menu at the current point instead of being inserted as sub-menu of the current menu." I agree that it's not really the best thing, though. Will try to get the spec fixed.
Comment 6 Vincent Untz 2009-06-27 09:15:31 UTC
Created attachment 137448 [details] [review]
[libmenu] Sort inlined items unless inline_header is used

We also strip duplicate entries in a menu when we do such a sort.

This is actually not a trivial task since it means we have to correctly
preprocess all inline data before using the layout -- else, we'd try to
sort items while a layout has been used, which is too late (since you
can specify that you want mixed entries and directories, eg; or even
entries or directories in a specified order).

We make sure to evaluate the inline at the deepest first, so we can
correctly propagate as much inlines as possible to the top.
Comment 7 Vincent Untz 2009-06-27 09:16:45 UTC
Josselin: this is what I'm considering to commit. If you have some time to test it works fine for you, that'd be amazing :-)
Comment 8 Josselin Mouette 2009-06-27 11:12:14 UTC
Thanks for your interest in this bug, Vincent. I understand it is quite complicated to fix.

However with the patch, the inline and inline_limit settings appear to be entirely ignored. With our <DefaultLayout inline="true" inline_limit="6" inline_header="false"> for the games submenus, all submenus appear, including those which are empty.
Comment 9 Vincent Untz 2009-06-27 11:39:52 UTC
Ah, could be the bug I fixed in git (just pushed two commits I did locally yesterday). The inline attributes of DefaultLayout were not correctly inherited by children.

The empty menus shouldn't appear, though; that'd be a regression. I'll try to take a look tonight.
Comment 10 Vincent Untz 2009-06-27 15:03:45 UTC
(In reply to comment #9)
> The empty menus shouldn't appear, though; that'd be a regression. I'll try to
> take a look tonight.

I'm not playing with the same applications.menu as you do, but here, unless I put show_empty="true", I don't see empty submenus. Weird.
Comment 11 Josselin Mouette 2009-06-27 15:32:50 UTC
Maybe there are other changes that affect this behavior. I tried the two you committed this morning without success, but should I try with git master?
Comment 12 Vincent Untz 2009-06-29 17:27:24 UTC
Fixed your issue (we were ignoring the default values of DefaultLayout, and also didn't handle subdirs in some cases) and committed to master.