GNOME Bugzilla – Bug 490483
Merging two menus with the same entries duplicates them
Last modified: 2009-06-29 17:27:24 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.
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.
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().
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) ?
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.
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.
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.
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 :-)
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.
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.
(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.
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?
Fixed your issue (we were ignoring the default values of DefaultLayout, and also didn't handle subdirs in some cases) and committed to master.