Community
Participate
Working Groups
Created attachment 280499 [details] Screenshot of problem Steps to reproduce: When contributing a MenuManager via extension point org.eclipse.ui.menus / menuContribution + org.eclipse.ui.menus.ExtensionContributionFactory the created MenuManager is shown as disabled. For details see attached screenshot + sample project. Reason: This was introduced with the fix for bug 547050. Analysis: I think disabling the MDirectMenuItem if no object is assigned must not be done in the legacy case, i.e. if MDirectMenuItem wraps an opaque legacy IMenuManager.
Created attachment 280501 [details] Sample project for problem
Created attachment 280502 [details] Sample project for problem
New Gerrit change created: https://git.eclipse.org/r/151949
Sebastian, could you please check if bug 552413 is related or not and if your patch fixes that too?
(In reply to Andrey Loskutov from comment #4) > Sebastian, could you please check if bug 552413 is related or not and if > your patch fixes that too? Yes, this is the same issue. I did however do some more thorough analysis now. Bug 547050 is not really the problem and is not doing anything wrong. It just triggered an unfortunate side-effect. Example: Even if this line: https://github.com/eclipse/eclipse.platform.ui/blob/517d5cc7ff75f37a3769e8c945b4e36d2a9f3316/bundles/org.eclipse.e4.ui.workbench.renderers.swt/src/org/eclipse/e4/ui/workbench/renderers/swt/MenuManagerRendererFilter.java#L238 is replaced with contrib.setEnabled(false); contrib.setEnabled(true); the problem remains. Instead, the actual problem is the following: - When contrib.setEnabled(); is called it triggeres a UI event UIEvents.Item.TOPIC_ENABLED - This is handled by MenuManagerRenderer.subscribeItemEnabledUpdate(Event) which does IContributionItem.update(). Note that this does *not* pass in the actual enablement value in anways. - Instead, IContributionItem.update() runs some heuristic to figure out the enablement based on the number of visible widgets. - At that point the menu hasn't even shown up yet, so there are no widgets, yet -> Menu gets disabled We are just unlucky that this UI event is being broadcast, or rather, we were lucky before that it didn't... The method in question org.eclipse.jface.action.MenuManager.updateMenuItem() has been modified over and over again, due to similar problems: Bug 34969, bug 90355, ... Suggestion: Never change enablement in MenuManager.updateMenuItem().
New Gerrit change created: https://git.eclipse.org/r/151986
(In reply to Sebastian Ratz from comment #5) > Suggestion: > Never change enablement in MenuManager.updateMenuItem(). The MenuManager 'isEnabled' always returns true, so that would be in line again. However, the updateMenuItem is changed that the menu will be disabled when there are no visible children, one of the mentioned bugs must be about that. I think that similar logic of the 'isVisible' must be used to determine the 'isEnabled' state. The menu must be enabled when there are visible children. Instead of looking at the widget, the contribution items must be queried for this. I was also wondering if the updateMenuItem() is still needed if the 'isEnabled' logic would be updated to include this, but this changes the semantics of the default implementation.
(In reply to Rolf Theunissen from comment #7) > (In reply to Sebastian Ratz from comment #5) > > Suggestion: > > Never change enablement in MenuManager.updateMenuItem(). > > The MenuManager 'isEnabled' always returns true, so that would be in line > again. However, the updateMenuItem is changed that the menu will be disabled > when there are no visible children, one of the mentioned bugs must be about > that. > > I think that similar logic of the 'isVisible' must be used to determine the > 'isEnabled' state. The menu must be enabled when there are visible children. > Instead of looking at the widget, the contribution items must be queried for > this. > I was also wondering if the updateMenuItem() is still needed if the > 'isEnabled' logic would be updated to include this, but this changes the > semantics of the default implementation. If the menu contains no visible children, isVisible() of the menu itself already returns false. The implementation of isEnabled() would then be exactly the same, but useless, as the menu is invisible anyways. I don't think an intermediate state "visible + not enabled" even should exist for a submenu.
The dynamic visibility calculation was implemented in bug 47098. This is *after* all those changes to the somehow broken updateMenuItem() method. As per my last comment, can't we just get rid of updateMenuItem() now?
(In reply to Sebastian Ratz from comment #9) > The dynamic visibility calculation was implemented in bug 47098. > > This is *after* all those changes to the somehow broken updateMenuItem() > method. > > As per my last comment, can't we just get rid of updateMenuItem() now? When an item is forced to be visible, it would make sense to disable. I don't know if this can occur. Getting rid of updateMenuItem() would be even better, though I wonder what would happen if only JFace code is used in an application. Some test cases for all these situations would be highly appreciated.
(In reply to Rolf Theunissen from comment #10) > (In reply to Sebastian Ratz from comment #9) > > The dynamic visibility calculation was implemented in bug 47098. > > > > This is *after* all those changes to the somehow broken updateMenuItem() > > method. > > > > As per my last comment, can't we just get rid of updateMenuItem() now? > > When an item is forced to be visible, it would make sense to disable. I > don't know if this can occur. > Getting rid of updateMenuItem() would be even better, though I wonder what > would happen if only JFace code is used in an application. Some test cases > for all these situations would be highly appreciated. The visible + disabled constellation cannot occur for MenuManager. I think I covered all constellations in the test cases in https://git.eclipse.org/r/#/c/151986/ @Rolf Any objections to merge?
*** Bug 552413 has been marked as a duplicate of this bug. ***
Gerrit change https://git.eclipse.org/r/151986 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=26d37803217bce02868dcc04a75877221d4e6a5f
