Community
Participate
Working Groups
Bug 549818 caused bug 396796 to re-appear. A similar problem with MultiPageEditorPart was addressed in bug 550986, but that is not sufficient in general. Example: - Try to contribute to a context menu entry to org.eclipse.debug.ui.DebugView, e.g.: <extension point="org.eclipse.ui.menus"> <menuContribution allPopups="true" locationURI="popup:org.eclipse.debug.ui.DebugView"> <command commandId="some.custom.command" label="Foo" style="push"> </command> </menuContribution> </extension> - Right click in the Debug view and notice that the command is not there - In the debug view, go to the view menu -> Layout -> Breadcrumb - Right click now and notice that the custom command shows up now. Reason: - The debug view creates *two* MenuManager instances for the *same* menu ID org.eclipse.debug.ui.DebugView, one in org.eclipse.debug.ui.AbstractDebugView.createContextMenu(Control) and once in org.eclipse.debug.internal.ui.views.launch.LaunchViewBreadcrumb.createMenuManager() - For MenuManagers a PopupMenuExtender is created - Since the change in 549818, the MPopupMenu menu model is *re-used* for the 2nd instance. - This sounds reasonable since the models are supposed to be identical. - However, later in org.eclipse.ui.internal.PopupMenuExtender.createModelFor(String), org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerRenderer.linkModelToManager(MMenu, MenuManager) is called. And that one builds and expects a bi-directional 1:1 relationship between MMenu and MenuManager. - Result: When the breadcrumb MenuManager is created and the *same* MMenu is re-used, that 1:1 relationship is broken, we have now 2 MenuManager instances but only 1 MMenu: mmenu1 -> menumanager2 menumanager1 -> mmenu1 menumanager2 -> mmenu1 - Now, when the context menu is actually updated, org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerRenderer.getManager(MMenu) comes into play, returning the 2nd MenuManager for the breadcrumb, and only that once receives the custom command contribution.
I propose to revert the change to PopupMenuExtender ( https://git.eclipse.org/r/#/c/147881/ ): Being unable to contribute to certain menus is kind of a blocker. The persistence of duplicate menu entries described in bug 549818 should actually never happen, since the menu models created by PopupMenuExtender are never persisted anyways from what I can tell. (The duplicate menu entries mentioned by 549818 were created by code). Any objections?
New Gerrit change created: https://git.eclipse.org/r/151712
(In reply to Sebastian Ratz from comment #1) > I propose to revert the change to PopupMenuExtender ( > https://git.eclipse.org/r/#/c/147881/ ): > > Being unable to contribute to certain menus is kind of a blocker. > > The persistence of duplicate menu entries described in bug 549818 should > actually never happen, since the menu models created by PopupMenuExtender > are never persisted anyways from what I can tell. (The duplicate menu > entries mentioned by 549818 were created by code). > > Any objections? +1. Bug 549818 must be reopened once the patch is reverted.
(In reply to Sebastian Ratz from comment #1) > I propose to revert the change to PopupMenuExtender ( > https://git.eclipse.org/r/#/c/147881/ ): > > Being unable to contribute to certain menus is kind of a blocker. > > The persistence of duplicate menu entries described in bug 549818 should > actually never happen, since the menu models created by PopupMenuExtender > are never persisted anyways from what I can tell. (The duplicate menu > entries mentioned by 549818 were created by code). > > Any objections? No objection to the revert, though the original code is functionally incorrect (it checks for conditions that never can be true). W.r.t. the duplication and persistence, even when model entries are created in code they must be cleaned actively. That cleaning is not done, but adding a non-persist tag will do the trick for now. That workaround can be pushed for Bug 549818. The issue with the one-to-one relation between M(Popup)Menu and MenuManager must be investigated further, my suspicion is that EMenuService suffers a similar issue.
The persistence flag is already set, by the fixes for Bug 509371, that fixes Bug 549818 too.
Gerrit change https://git.eclipse.org/r/151712 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=7efdf6755aefe96cd46a578e0b0a65b9ea44a2b2
Verified that example works again with build I20191030-0010.
(In reply to Andrey Loskutov from comment #3) > Bug 549818 must be reopened once the patch is reverted. Any comment on this?
*** Bug 550986 has been marked as a duplicate of this bug. ***
*** Bug 553108 has been marked as a duplicate of this bug. ***
