Community
Participate
Working Groups
Build Identifier: Version: 4.2.0 Build id: I20120314-2200 I have found that when I remove menu items from a part menu the items are removed from the MMenu's children list but visually their placeholder remains. When I add the menu items back to the part menu they do not not have labels. See screen shots. Reproducible: Always Steps to Reproduce: 1.Add menu items and sub-menus to the part menu 2.Remove the items and sub-menus 3.Add the menu items and sub-menus back.
Created attachment 212819 [details] Screen shots of bug
Created attachment 212820 [details] Project to reproduce bug
Hi Josh, Could you please attach your shots as separate images (png, gif, jpg, whatever)? We can see those from the bug. PW
Created attachment 212854 [details] Original State of the Application
Created attachment 212855 [details] Remove menu items and submenu from part menu
Created attachment 212856 [details] Try to add items back
(In reply to comment #0) > Build Identifier: Version: 4.2.0 Build id: I20120314-2200 > > I have found that when I remove menu items from a part menu the items are > removed from the MMenu's children list but visually their placeholder remains. > When I add the menu items back to the part menu they do not not have labels. > See screen shots. > > Reproducible: Always > > Steps to Reproduce: > 1.Add menu items and sub-menus to the part menu > 2.Remove the items and sub-menus > 3.Add the menu items and sub-menus back. I would like to revise the steps in this comment. To the following... Steps to Reproduce: 1.Add menu items and sub-menus to the part menu 2.View the menu 3.Remove the items and sub-menus 4.View the menu 3.Add the menu items and sub-menus back. 5.View the menu. Step 2 is important because that is when the initial items are added to the MenuManager that is used behind the scenes. Step 4 and 5 will have the same results because the MenuManager is NOT updated again after the initial viewing on step 2. It doesn't seem like this logic ever expected a menu's contents to change after it has been displayed once. I am basing this on the logic in StackRenderer.showMenu(). If the menuModel doesn't have a Menu object then one is create and the menu listeners are added to the underlying menu manager. If a menu already exists for the menuModel then it is just displayed. When it is displayed the menu listeners aboutToShowMenu method is called. Unfortunately since the menu manager hasn't been updated with the changes that have been made to the menuModel, extra listeners are called that should have been removed. These listeners that are for removed items return an empty string because the text for the menu item is no longer in the localization map. So either the MenuManager needs to be created each time the part menu is displayed (which I tried this by calling renderer.removeGui(element) but that didn't seem to remove the link between the menuModel and the MenuManager in the modelToManager map in MenuManagerRenderer), or the MenuManager needs to be updated with the changes in the menuModel. This would probably involve a change to the IPresentationEngine interface. It would most likely need a updateGui(...) method. I am actually kind of curious as to why the menuManager needed to be kept around? This seems like something that is only needed when the menu is displayed, due to the MMenu menuModel being retained as the model for the menu. The only thing I could think is that that creating the MenuManager each time is expensive. I know this bug has been slated for 4.2.1 and the team is busy with getting 4.2 ready for release. I just wanted to document my research in case it could help after the release.
Josh, thanx for the update. Are you removing the IContributionItems from the view MenuManager (that you get from getViewSite().getActionBars())? Are you calling updateActionBars() afterwards? PW
(In reply to comment #8) > Josh, thanx for the update. > > Are you removing the IContributionItems from the view MenuManager (that you get > from getViewSite().getActionBars())? > > Are you calling updateActionBars() afterwards? > > PW Nope. I am not using that in my test project that is attached this bug. I am just using the part menu which is a MMenu. When I click on the part menu to display it the renderer is creating the MenuManager. I am not accessing the MenuManager at all. See MenuManagerRenderer.processContents(MElementContainer<MUIElement> container) which gets the MenuManager for the MMenu It looks like the MenuManager is created in PartRenderingEngine.createWidget(MUIElement,Object) which is called from PartRenderingEngine.safeCreateGui(MUIElement element, Object parentWidget, IEclipseContext parentContext) which is called the first time the MMenu is displayed.
(In reply to comment #9) > Nope. I am not using that in my test project that is attached this bug. Sorry, you're correct. PW
Created attachment 215508 [details] Possible fix for this bug Possible fix for this bug. The idea behind this fix is to make sure the MMenu model is in sync with the jface MenuManager. Previously items removed from the model AFTER the part was created were NOT removed from the part's MenuManager. After fixing the remove issue, I found that when I added new items to the model they would also not be added to the MenuManager, so I fixed that by handling both the ADD and REMOVE event types in the childUpdater.
I can't understand how this issue could get an importance of just normal. Changing the application model at runtime is one of the top ten highlights of e4. We're trying to add dynamically menu- and tool-items. ToolItems are added instantly using an appropriate listener mechanism. Changes to MenuItems either in MainMenu, PartMenu or PopupMenu are visible only after a workbench restart. See code fragments below. Adding ToolItems (Maintoolbar and PartToolbar) works as expected: -------snip MDirectToolItem item = MMenuFactory.INSTANCE.createDirectToolItem(); item.setContributionURI("bundleclass://" + "org.mybundle/" + "org.mybundle.TestHandler"); URL iconURL=fileLocator.find(Platform.getBundle("org.mybundle"), new Path("icons/smiley_e.png"), null); item.setIconURI(iconURL.toString()); item.setToBeRendered(true); item.setVisible(true); MApplication app = (MApplication)((EObject)m_Window).eContainer(); MToolBar mainToolbar=(MToolBar) m_ModelService.find( "toolbar:org.eclipse.ui.main.toolbar", app); mainToolbar.getChildren().add(item); --------snip Adding MenuItems does not work: -------snip MDirectMenuItem item = MMenuFactory.INSTANCE.createDirectMenuItem(); item.setContributionURI("bundleclass://" + "org.mybundle/" + "org.mybundle.TestHandler"); URL iconURL=fileLocator.find(Platform.getBundle("org.mybundle"), new Path("icons/smiley_e.png"), null); item.setIconURI(iconURL.toString()); item.setToBeRendered(true); item.setVisible(true); m_Window.getMainMenu().getChildren().add(item); --------snip
A little correction to comment #12: Dynamically added ToolItems are rendered immediately. But if you remove the ToolItem from the application model again, it is not removed from the UI, leaving Zombie ToolItems with no appropriate model element.
(In reply to comment #11) > Created attachment 215508 [details] > Possible fix for this bug Hi Josh, Thank you very much for the proposed patch. Would you be able to include some changes? 1) In your childUpdater could we treat ADD and REMOVE separately? I think for ADD calling processContents(*) is the best way to include the new MMenuItem. 2) For REMOVE, org.eclipse.e4.ui.workbench.UIEvents.EventTags.OLD_VALUE should contain the removed MMenu or MMenuItem. That can be your starting point to remove the IContributionItem for an MMenuItem (retrieved by getContribution(itemModel);) or de-process an entire sub MMenu and its MenuManager (retrieved by getManager(menuModel);), and de-link them from the internal Maps. PW
(In reply to comment #14) > (In reply to comment #11) > > Created attachment 215508 [details] > > Possible fix for this bug > > Hi Josh, Thank you very much for the proposed patch. Would you be able to > include some changes? > > 1) > > In your childUpdater could we treat ADD and REMOVE separately? I think for > ADD calling processContents(*) is the best way to include the new MMenuItem. > > 2) > > For REMOVE, org.eclipse.e4.ui.workbench.UIEvents.EventTags.OLD_VALUE should > contain the removed MMenu or MMenuItem. > > That can be your starting point to remove the IContributionItem for an > MMenuItem (retrieved by getContribution(itemModel);) or de-process an entire > sub MMenu and its MenuManager (retrieved by getManager(menuModel);), and > de-link them from the internal Maps. > > PW I am trying to apply and update this patch using 4.2.1 (tag R4_2_1) and it is not working as expected. You are correct that OLD_VALUE will give me the delete menu item or menu. When I call MenuManager parentManager = getManager((MMenu) menuModel); which is the same call in processContents(...). It always gives me null for the view menu. Before I thought it gave me a menu manager, which allowed me to remove the item or submenu from the menu manager for the view menu. Here is how I have changed the childUpdater.. Object menuModel = event.getProperty(UIEvents.EventTags.ELEMENT); if (UIEvents.isADD(event)) { processContents((MElementContainer<MUIElement>) menuModel); } else if (UIEvents.isREMOVE(event)) { MenuManager parentManager = getManager((MMenu) menuModel); Object oldValue = event.getProperty(UIEvents.EventTags.OLD_VALUE); if (oldValue instanceof MMenu) { MenuManager removedManager = getManager((MMenu) oldValue); processMenuManager(removedManager, parentManager); } else if (oldValue instanceof MMenuItem) { IContributionItem contribution = getContribution((MMenuItem) oldValue); processContributionItem(contribution, parentManager); } } I get null when calling getContribution((MMenuItem) oldValue) in the case when the oldValue is a MMenuItem. I was thinking I could get the parentManager from the ELEMENT and then the contributionItem from the OLD_VALUE. Once I had that I could remove the contributionItem from the parentManager. Apparently I am missing something. In my original patch, each time processContents was called I would go through the parentManager and check to see if the contributionItems in the manager had a MMenuElement if they didn't then I removed them from the menu manager. I guess I don't see how I can do the same here if I can't get the parentManager. I checked the modelToManager in MenuManagerRender and it doesn't have a Menu Manager for my view menu in it. It only has the menu managers for the global menus (File, Help, main.menu). It has been awhile since I did the patch I am sure I am just forgetting something. I am working on the updates. For both this bug and the toolbar bug.
I remember now what I am missing I forgot that I have view the menu once before removing items. The menu managers aren't created until the menu is viewed. Once I view the menu the code I posted works. If the menu hasn't been viewed then the menu manager is null. I added a null check that just returns if the parentManager is null. However I need to make a change to the code I put in the previous comment because the OLD_VALUE returns a list if I clear the entire menu. I will make a change for that and then re-attach the patch. (In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #11) > > > Created attachment 215508 [details] > > > Possible fix for this bug > > > > Hi Josh, Thank you very much for the proposed patch. Would you be able to > > include some changes? > > > > 1) > > > > In your childUpdater could we treat ADD and REMOVE separately? I think for > > ADD calling processContents(*) is the best way to include the new MMenuItem. > > > > 2) > > > > For REMOVE, org.eclipse.e4.ui.workbench.UIEvents.EventTags.OLD_VALUE should > > contain the removed MMenu or MMenuItem. > > > > That can be your starting point to remove the IContributionItem for an > > MMenuItem (retrieved by getContribution(itemModel);) or de-process an entire > > sub MMenu and its MenuManager (retrieved by getManager(menuModel);), and > > de-link them from the internal Maps. > > > > PW > > I am trying to apply and update this patch using 4.2.1 (tag R4_2_1) and it > is not working as expected. You are correct that OLD_VALUE will give me the > delete menu item or menu. > > When I call MenuManager parentManager = getManager((MMenu) menuModel); > which is the same call in processContents(...). It always gives me null for > the view menu. Before I thought it gave me a menu manager, which allowed me > to remove the item or submenu from the menu manager for the view menu. > > Here is how I have changed the childUpdater.. > > Object menuModel = event.getProperty(UIEvents.EventTags.ELEMENT); > if (UIEvents.isADD(event)) { > processContents((MElementContainer<MUIElement>) menuModel); > } else if (UIEvents.isREMOVE(event)) { > MenuManager parentManager = getManager((MMenu) menuModel); > Object oldValue = event.getProperty(UIEvents.EventTags.OLD_VALUE); > if (oldValue instanceof MMenu) { > MenuManager removedManager = getManager((MMenu) oldValue); > processMenuManager(removedManager, parentManager); > } else if (oldValue instanceof MMenuItem) { > IContributionItem contribution = getContribution((MMenuItem) > oldValue); > processContributionItem(contribution, parentManager); > } > } > > I get null when calling getContribution((MMenuItem) oldValue) in the case > when the oldValue is a MMenuItem. I was thinking I could get the > parentManager from the ELEMENT and then the contributionItem from the > OLD_VALUE. Once I had that I could remove the contributionItem from the > parentManager. Apparently I am missing something. > > In my original patch, each time processContents was called I would go > through the parentManager and check to see if the contributionItems in the > manager had a MMenuElement if they didn't then I removed them from the menu > manager. > > I guess I don't see how I can do the same here if I can't get the > parentManager. > > I checked the modelToManager in MenuManagerRender and it doesn't have a Menu > Manager for my view menu in it. It only has the menu managers for the global > menus (File, Help, main.menu). It has been awhile since I did the patch I am > sure I am just forgetting something. I am working on the updates. For both > this bug and the toolbar bug.
Created attachment 223365 [details] Updated patch with suggestions from PW Updated this patch with suggestions from Comment 14
Created attachment 223414 [details] Additional Modification To Patch I updated the patch to call parentManager.update(false) after removing items or menus from the parentManager. This is being done in the processContents after changes are made and I think it needs to be done also when items are removed.
Released as http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=8929a2049341288baf2a349dbf7bb97151bb96cc Thanx Josh. PW
(In reply to comment #19) > Released as > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=8929a2049341288baf2a349dbf7bb97151bb96cc > > Thanx Josh. > > PW Sorry Paul and Josh, I had to revert this as it caused the following severe issues: - Navigator > context menu > New is empty after the first invocation - Package Explorer > context menu > New is empty after the first invocation (see bug 399305). - Project Explorer > context menu > New has its items duplicated after the first invocation We never disposed the menus so far, also not in 3.x. Clients who cache and reuse the menus are now no longer working or do strange stuff.
(In reply to comment #19) > Released as > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=8929a2049341288baf2a349dbf7bb97151bb96cc OK, Josh, could you please revisit the fix with an eye to the New menu (it might effect Show In as well) PW
Looking at this in https://git.eclipse.org/r/#/c/10860/ PW
Josh, I've pushed up Patch Set #5 to https://git.eclipse.org/r/#/c/10860 Could you please confirm that this still fixes your problem? Then I'll review it again, I think we need to change the ADD section. PW
(In reply to comment #23) > Josh, I've pushed up Patch Set #5 to https://git.eclipse.org/r/#/c/10860 > > Could you please confirm that this still fixes your problem? Then I'll > review it again, I think we need to change the ADD section. > > PW What version of the SDK do I need to get the patch to work? I changed my Eclipse to run on Version: 4.3.0 Build id: I20130314-1330 Which found several missing imports when I applied your patch I am still having one issue Line 687-688 has this code ici = (IContributionItem) ((IContextFunction) obj) .compute(lclContext); The compiler tells me that compute takes two parameters.
(In reply to comment #24) > > What version of the SDK do I need to get the patch to work? > > I changed my Eclipse to run on > Version: 4.3.0 > Build id: I20130314-1330 > > Which found several missing imports when I applied your patch > > I am still having one issue > Line 687-688 has this code > ici = (IContributionItem) ((IContextFunction) obj) .compute(lclContext); > > The compiler tells me that compute takes two parameters. It'll have to be amended, as the IContextFunction interface changed in M6 (you have the right build) to have 2 parameters instead of 1. Passing in null is acceptable. PW
(In reply to comment #25) > (In reply to comment #24) > > > > What version of the SDK do I need to get the patch to work? > > > > I changed my Eclipse to run on > > Version: 4.3.0 > > Build id: I20130314-1330 > > > > Which found several missing imports when I applied your patch > > > > I am still having one issue > > Line 687-688 has this code > > ici = (IContributionItem) ((IContextFunction) obj) .compute(lclContext); > > > > The compiler tells me that compute takes two parameters. > > It'll have to be amended, as the IContextFunction interface changed in M6 > (you have the right build) to have 2 parameters instead of 1. Passing in > null is acceptable. > > PW I tried passing in null, which allowed me to run the test program. In my workflow I get a NPE 1. Open app 2. View the part menu and submenu 3. Press the button to remove the menus 4. View the menu (the items in the view menu have been removed. There is an empty item in the submenu) 5. NPE occurs when the menu disappears I will attach a screenshot of the empty menu item and the NPE stack trace. I haven't debugged yet to see why we are getting the NPE
Created attachment 228502 [details] Broken Submenu with Patch 5 This is a screenshot of the broken submenu occurs when the user views the part menu after the pressing the button to remove the menu items.
Created attachment 228503 [details] NPE Stack trace I get a NPE when the menu closes after viewing it. This occurs after the menu items have been removed.
(In reply to comment #28) > Created attachment 228503 [details] > NPE Stack trace OK, tracked down that problem. I've updated the change and merged it in. It works for your example project and doesn't interfere with SDK menus (New, Show In, etc). Josh, could you also test to make sure it does what you want? Merged as http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=d0d966c9226abaada1a9b6bc98e4c486ceb4f640 PW
(In reply to comment #29) > (In reply to comment #28) > > Created attachment 228503 [details] > > NPE Stack trace > > OK, tracked down that problem. I've updated the change and merged it in. > It works for your example project and doesn't interfere with SDK menus (New, > Show In, etc). > > Josh, could you also test to make sure it does what you want? > > Merged as > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=d0d966c9226abaada1a9b6bc98e4c486ceb4f640 > > PW I had to revert that change since it randomly reorders the menus, see bug 405259.
Sorry Dani, I think I'll defer this to Luna. PW
*** Bug 498320 has been marked as a duplicate of this bug. ***
*** Bug 461655 has been marked as a duplicate of this bug. ***
(In reply to Dani Megert from comment #30) > I had to revert that change since it randomly reorders the menus, see bug > 405259. I tracked down that this was caused by Bug 558279
See the Gerrit, still work in progress. There are still many corner cases that do not work correctly. And many tests need to be added. Especially the reconciling of legacy items has some major issues. The current implementation assumes a one-to-one relation between Opaque Menu Items and ContributionItems, however this is not true. The same ContributionItem can occur multiple times at different places. Also SubMenuManager/SubContribution adds to this problems. In turn, this messes with the Customize Perspective Dialog, i.e. ActionSets and ordering is broken quickly. It might be worth while to investigate to replace ActionSetMenuManager/ActionSetContributionItem by an E4 model implementation.
Too late for 4.16. Removing target milestone.
