Community
Participate
Working Groups
When investigating on Papyrus Performance issues in Bug 463564, we noticed that our Dynamic Menu (containing ~ 80 items in a submenu) is created several hundreds of time for each user's right click I've built a small Eclipse Plug-in contributing an empty dynamic menu (Via the org.eclipse.ui.menus extension point), showing that the fill() method is invoked way too many times (Seems random, but something like 50-100 times per user's click) This regression occurred in Eclipse Mars M6 (And still occurs in M7). Testing the same plug-in on M5 shows that the same dynamic menu is built "only" twice for a right-click (Which is probably one time too many already)
Created attachment 253384 [details] Plug-in highlighting the issue This plug-in can be used to reproduce the issue. Just right-click anywhere and see that the console message is logged 50-100 times (For each right click)
Dirk, any idea?
I see it on Linux GTK2 too. Can be side effect of the fix for bug 460556, see changes at org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerRenderer$3.handleEvent(MenuManagerRenderer.java:231) Full Backtrace: at eclipse.perf.DynamicMenuExample.fill(DynamicMenuExample.java:10) at org.eclipse.ui.internal.menus.DynamicMenuContributionItem.fill(DynamicMenuContributionItem.java:208) at org.eclipse.jface.action.MenuManager.doItemFill(MenuManager.java:724) at org.eclipse.jface.action.MenuManager.update(MenuManager.java:806) at org.eclipse.jface.action.MenuManager.update(MenuManager.java:665) at org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerRenderer$3.handleEvent(MenuManagerRenderer.java:231) at org.eclipse.e4.ui.services.internal.events.UIEventHandler$1.run(UIEventHandler.java:40) at org.eclipse.swt.widgets.Synchronizer.syncExec(Synchronizer.java:186) at org.eclipse.ui.internal.UISynchronizer.syncExec(UISynchronizer.java:145) at org.eclipse.swt.widgets.Display.syncExec(Display.java:4609) at org.eclipse.e4.ui.internal.workbench.swt.E4Application$1.syncExec(E4Application.java:211) at org.eclipse.e4.ui.services.internal.events.UIEventHandler.handleEvent(UIEventHandler.java:36) at org.eclipse.equinox.internal.event.EventHandlerWrapper.handleEvent(EventHandlerWrapper.java:197) at org.eclipse.equinox.internal.event.EventHandlerTracker.dispatchEvent(EventHandlerTracker.java:197) at org.eclipse.equinox.internal.event.EventHandlerTracker.dispatchEvent(EventHandlerTracker.java:1) at org.eclipse.osgi.framework.eventmgr.EventManager.dispatchEvent(EventManager.java:230) at org.eclipse.osgi.framework.eventmgr.ListenerQueue.dispatchEventSynchronous(ListenerQueue.java:148) at org.eclipse.equinox.internal.event.EventAdminImpl.dispatchEvent(EventAdminImpl.java:135) at org.eclipse.equinox.internal.event.EventAdminImpl.sendEvent(EventAdminImpl.java:78) at org.eclipse.equinox.internal.event.EventComponent.sendEvent(EventComponent.java:39) at org.eclipse.e4.ui.services.internal.events.EventBroker.send(EventBroker.java:85) at org.eclipse.e4.ui.internal.workbench.UIEventPublisher.notifyChanged(UIEventPublisher.java:59) at org.eclipse.emf.common.notify.impl.BasicNotifierImpl.eNotify(BasicNotifierImpl.java:374) at org.eclipse.e4.ui.model.application.ui.impl.UIElementImpl.setVisible(UIElementImpl.java:345) at org.eclipse.e4.ui.workbench.renderers.swt.ContributionRecord.updateVisibility(ContributionRecord.java:100) at org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerRendererFilter.updateElementVisibility(MenuManagerRendererFilter.java:188) at org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerShowProcessor.showMenu(MenuManagerShowProcessor.java:230) at org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerShowProcessor.menuAboutToHide(MenuManagerShowProcessor.java:110) at org.eclipse.jface.internal.MenuManagerEventHelper.showEventPostHelper(MenuManagerEventHelper.java:92) at org.eclipse.jface.action.MenuManager.handleAboutToShow(MenuManager.java:467) at org.eclipse.jface.action.MenuManager.access$1(MenuManager.java:461) at org.eclipse.jface.action.MenuManager$2.menuShown(MenuManager.java:493) at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:255) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84) at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4457) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1317) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1341) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1322) at org.eclipse.swt.widgets.Menu._setVisible(Menu.java:198) at org.eclipse.swt.widgets.Display.runPopups(Display.java:3837) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3396) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$4.run(PartRenderingEngine.java:1127) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:337) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1018) at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:156) at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:654) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:337) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:598) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:150) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:139) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:134) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:380) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:235) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:497) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:653) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:608) at org.eclipse.equinox.launcher.Main.run(Main.java:1499) at org.eclipse.equinox.launcher.Main.main(Main.java:1472)
A few of those calls are really caused by the fix for Bug 460556. But only because MenuManagerShowProcessor#menuAboutToHide() is calling showMenu() which then triggers an update of the menu item visibility. I really don't understand why. What is the reason to call showMenu() on menuAboutToHide()? -- Most of the calls are caused by the fix for Bug 391430. For every child that is added to a menu (as far as I can see any menu), the menu contents get processed again and again. Maybe we should add a check to MenuManagerRenderer#processContents() to avoid updating if nothing has changed? Not sure how to do this. I'm still investigating on this, but any help is appreciated. There is too much stuff that seems to be difficult as the MenuManagerRenderer handles both, e3 compat and plain e4. -- Also before those changes executing the dynamic menu contribution seems to be strange. As far as I can see this is the result of the PopupMenuExtender which is updating the menu although the MenuManager itself handles the contributions. That also doesn't look correct. I'm still investigating, but any further insights would be helpful.
I'm also interested in updates of this bug because I recently worked on Bug 461655, which is the follow-up of the mentioned Bug 391430. I did a good number of experiments and debug sessions, and I found that checking for (menuModel.getLabel() == null) it is determinant to see if a menu needs to be processed in "e4 way". Now, I'm not sure this could help, but if you're "debugging wildly" searching for "a determinant variable", watch also for the value of menuModel.getLabel(). I'd like to give you some more help, but my knowledge is limited for now.
I've been digging into this too, and I think we need to rework the patches for bug 391430 and bug 460556 in a different way. Unfortunately we don't have any unit tests to verify those particular bugs. (In reply to Dirk Fauth from comment #4) > Most of the calls are caused by the fix for Bug 391430. > For every child that is added to a menu (as far as I can see any menu), the > menu contents get processed again and again. Maybe we should add a check to > MenuManagerRenderer#processContents() to avoid updating if nothing has > changed? Not sure how to do this. Dirk has it right: as we process the different menu contributions, we add new MMenuItems into the menus, which triggers an series of ADD events. With the patch for bug 391430 each ADD causes processContents() to be run for item. With the patch for bug 460556, each processContents() forcibly updates each MenuManager. I put a "new Throwable().printStackTrace()" in Camille's example plugin, and here's a stack trace that I see occurring **dozens** of times (eliding the OSGI EventAdmin): java.lang.Throwable at eclipse.perf.DynamicMenuExample.fill(DynamicMenuExample.java:10) at org.eclipse.ui.internal.menus.DynamicMenuContributionItem.fill(DynamicMenuContributionItem.java:208) at org.eclipse.jface.action.MenuManager.doItemFill(MenuManager.java:724) at org.eclipse.jface.action.MenuManager.update(MenuManager.java:806) at org.eclipse.jface.action.MenuManager.update(MenuManager.java:665) at org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerRenderer.processContents(MenuManagerRenderer.java:632) at org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerRenderer.subscribeTopicChildAdded(MenuManagerRenderer.java:309) [. . . elided . . .] at org.eclipse.e4.ui.services.internal.events.EventBroker.send(EventBroker.java:85) at org.eclipse.e4.ui.internal.workbench.UIEventPublisher.notifyChanged(UIEventPublisher.java:59) at org.eclipse.emf.common.notify.impl.BasicNotifierImpl.eNotify(BasicNotifierImpl.java:374) at org.eclipse.emf.ecore.util.EcoreEList.dispatchNotification(EcoreEList.java:249) at org.eclipse.emf.common.notify.impl.NotifyingListImpl.addUnique(NotifyingListImpl.java:356) at org.eclipse.emf.common.util.AbstractEList.add(AbstractEList.java:341) at org.eclipse.e4.ui.workbench.renderers.swt.ContributionRecord.mergeIntoModel(ContributionRecord.java:250) at org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerRenderer.processAddition(MenuManagerRenderer.java:550) at org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerRenderer.generateContributions(MenuManagerRenderer.java:525) at org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerRenderer.processContributions(MenuManagerRenderer.java:490) at org.eclipse.ui.internal.PopupMenuExtender.addMenuContributions(PopupMenuExtender.java:409) at org.eclipse.ui.internal.PopupMenuExtender.menuAboutToShow(PopupMenuExtender.java:381) at org.eclipse.jface.action.MenuManager.fireAboutToShow(MenuManager.java:333) at org.eclipse.jface.action.MenuManager.handleAboutToShow(MenuManager.java:466) at org.eclipse.jface.action.MenuManager.access$1(MenuManager.java:461) at org.eclipse.jface.action.MenuManager$2.menuShown(MenuManager.java:493) I think the underlying bug is actually that the Menu VISIBLE checker was marking the menu's parent MenuManager as dirty rather than the menu itself. With the patch (attached in a second), the dozens of calls to eclipse.perf.DynamicMenuExample.fill() is reduced to just two calls All UI and E4UI tests pass with the provided patch, and I've verified the context menus on a new Papyrus Project and its contents are the same before and after this patch. I'll do some more testing tonight, but I wanted to put this up. Lars and Dirk, I believe that this is important enough to go into 4.5 and not 4.5.1. If you could take a look at this patch and do some testing too. This is a major performance regression for Papyrus, and I know it will impact some of my past clients that have Eclipse-based apps with extensive use of dynamic menus.
New Gerrit change created: https://git.eclipse.org/r/48911
Given RC3 is tomorrow, perhaps this is better for RC4?
(In reply to Brian de Alwis from comment #8) > Given RC3 is tomorrow, perhaps this is better for RC4? Changing target to RC4. Verified bug 460556 by using the Model Spy to toggle the visibility of File > New and other menu items, and verifyied that the menu items disappear and reappear. But there's a regression in other existing menus such as the Search menu where the Search… and File… items aren't updated.
(In reply to Brian de Alwis from comment #9) > Verified bug 460556 by using the Model Spy to toggle the visibility of File > > New and other menu items, and verifyied that the menu items disappear and > reappear. > > But there's a regression in other existing menus such as the Search menu > where the Search… and File… items aren't updated. You mean with your fix?
(In reply to Brian de Alwis from comment #9) > Verified bug 460556 by using the Model Spy to toggle the visibility of File > > New and other menu items, and verifyied that the menu items disappear and > reappear. It seems you misunderstand Bug 460556. It is not about menu items in menus of the main menu. It is about top level menu items of the main menu and toolbars. Did you try to set the visibility of the File menu itself? With your patch the toolbars get updated on visibility change, but the main menu stays the same. You can test this with with my simple example at here https://github.com/fipro78/e4toolbarexample Clicking on the eclipse icon in the toolbar should change the visibility of items in the main menu, the coolbar and the toolbar via changing the visible flag. It also shows changing the visibility of menu items using core expressions. But that doesn't work without the patch for Bug 400217 I contributed a while age. And that one is not yet approved. So your patch re-introduces Bug 460556. The forced update is necessary to support dynamic update of the visibility state. I was playing around with update conflation. Instead of immediately force an update, the managers to update are collected in a set and the update is performed with a delay of 100ms. In my tests this looks good and seems to not have negative effect anywhere. With this the dynamic menu contribution is also only executed once. I push this change to Gerrit so you can have a look at it. I'm not sure about it and it definitely needs to be reviewed well. But at the moment I'm out of any other matching solutions that wouldn't result in whole refactoring of the MenuManagerRenderer stuff.
New Gerrit change created: https://git.eclipse.org/r/48919
(In reply to Dirk Fauth from comment #11) > It seems you misunderstand Bug 460556. It is not about menu items in menus > of the main menu. It is about top level menu items of the main menu and > toolbars. Did you try to set the visibility of the File menu itself? Yep, I misunderstood: bug 460556 is vaguely described, has no steps to reproduce, and has no unit tests to identify regressions. > The forced update is necessary to support dynamic update of the visibility > state. I wonder why top-level menus require this change, but child menu items don't. I guess the checks must be re-done as the menu is shown. The problem here is that the calls to ContributionRecord#mergeIntoModel() links in new menu items via an #add on the corresponding Menu. Each #add triggers the change brought in for bug 391430. I also saw the dynamic fill on the forced update() calls added for bug 450556. I think your observation that toolbars don't have this problem is key. This code is thorny! > I was playing around with update conflation. Instead of immediately force an > update, the managers to update are collected in a set and the update is > performed with a delay of 100ms. In my tests this looks good and seems to > not have negative effect anywhere. With this the dynamic menu contribution > is also only executed once. Oh great, the timing loop returns :-) We may be that we have to choose the lesser of two evils here. If forcing a refresh of the top-level menus can work around bug 460556 then we may need to back that out. I have to run to a doctor's appt and won't be back for a bit. I'll take a look on my return.
(In reply to Brian de Alwis from comment #13) > Yep, I misunderstood: bug 460556 is vaguely described, has no steps to > reproduce, and has no unit tests to identify regressions. I'm sorry for that and I hope to have given you something to reproduce with the example application. I'm not sure how to provide unit tests for that without introducing screenshot interpretion since the state is not reflected correctly in the UI. > I wonder why top-level menus require this change, but child menu items > don't. I guess the checks must be re-done as the menu is shown. Because child menu items are processed on show, while the top-level menu is always visible and not rebuild. > I think your observation that toolbars don't have this problem is key. This > code is thorny! Maybe. Currently the menu rendering code tries to solve two use cases in one: initial creation and dynamic updating. And those two collide now for the update. This is why I think a complete refactoring is necessary in the long-term. > Oh great, the timing loop returns :-) Nope, no timing loop. If an update is requested, ONE conflater task is scheduled. When it is done, it is done. The next time an update is requested, a new task is scheduled. No loop! ;-) But I am also not really happy with that solution. IMHO it is the lesser evil for now, but the MenuManagerRenderer needs some rework to avoid such workarounds.
(In reply to Dirk Fauth from comment #14) > (In reply to Brian de Alwis from comment #13) > > Yep, I misunderstood: bug 460556 is vaguely described, has no steps to > > reproduce, and has no unit tests to identify regressions. > > I'm sorry for that and I hope to have given you something to reproduce with > the example application. > I'm not sure how to provide unit tests for that without introducing > screenshot interpretion since the > state is not reflected correctly in the UI. You can check the visibility state of the underlying SWT widget, right? > > I wonder why top-level menus require this change, but child menu items > > don't. I guess the checks must be re-done as the menu is shown. > > Because child menu items are processed on show, while the top-level menu is > always visible and not rebuild. Could we narrow this even further and only trigger the update if it's a top-level menu? Your fix approach looks ok; I'll put more comments on the patch.
Just to set the expectations here. This is something we have to investigate and understand in full detail in order to provide the right fix. We need steps in the UI for all affected cases and the fix must work for all of them. At the moment I'm inclined to -1 this for RC4.
I've been trying to quantify the effects with Papyrus installed and with a trivial UML model, but I'm unable to programmatically detect when the context menu is shown. So these timings are estimates. Timings are on a 2.6 GHz MacBook Pro (Core i7) — that is, pretty fast hardware. Steps to repeat: 1. Install Papyrus 2. Use File > New > Papyrus Model, select a project, and create. 3. Open the Model Explorer 4. In the Project Explorer, expand the project and double click the .di to load the model. The model should now show in the Model Explorer view. 5. Right-click on the RootElement in the Model Explorer. Situation as is: * Right-clicking on a project in the Project Explorer takes 83ms, 61, 54ms and invokes the dynamic fill 72 times. * Right-clicking on a Papyrus model from 460ms but takes up to 2s to appear (!!!) and invokes the dynamic fill 122 times. If I back out the patches for both bug 391430 and bug 460556 then I get acceptable performance. But it breaks pure E4 apps. Here's my attempt at explaining why we're seeing this change in behaviour. In E4, the menu models are backed by JFace MenuManagers rather than directly manage SWT Menus. (My guess is that this was done to simplify the compat layer?) Any menu-related model changes should update corresponding MenuManagers and then mark them as dirty. A dirty MenuManager is rebuilt via a call to MenuManger#update(), which walks the contributions managed by the MenuManager and reconciles any changes to the actual SWT menu, including processing dynamic contributions (such as used by Papyrus). Normally #update() is sent when its underlying menu is about to be shown, but because the E4 model is supposed to be kept in sync with the actual widgets, we may need to call #update() to reflect model changes. (There's only one situation that I know of where a MenuManager must be explicitly #update()'d: for the top-level menu aka Menu Bar. The Menu Bar is always visible, so there is no about-to-show event to trigger an update.) There are three primary changes to the E4 model that affects menus: 1. swizzling the visibility of a menu or menu item, either via the VISIBLE attribute or a visibleWhen condition 2. adding or removing child menu items to/from a menu 3. menu contributions Bug 460556 and bug 391430 address problems in #1 and #2. But their solutions introduces problems during the processing of #3. Bug 460556 addresses an edge case in #1 where the top-level menus must be explicitly #update()'d since it never receives an about-to-be-shown. The fix causes VISIBLE changes to explicitly call #update() on the corresponding MenuManager. I realize that it should only be done for the top-level MenuManager. Bug 391430 addresses dynamic additions of new menu items in #2. The fix listens for menu content additions (TOPIC_CHILDREN) and causes the menu contents to be re-processed via MenuManagerRenderer#processContents(). The last action performed by #processContents() is to call #update() on the corresponding MenuManager so that the widget is brought up to date. The problem is that processing menu contributions (#3), such as is performed on every context menu request, results in dynamic menu additions and visibility changes. Because of the fixes for 391430 and 460556, we now have *many* #update() calls occurring for the same menus multiple times. It's likely that we were having multiple #update()s happening prior to bug 391430 and 460556, but they were only happening once per processed contribution whereas they're now happening once per menu item added per contribution (i.e., O(n) vs O(n*m) where n = # contributions and m = # additions per contribution). Dirk has proposed a fix in https://git.eclipse.org/r/48919 that uses a Display#timerExec() to gather and delay the many MenuManager#updates() until a fixed time increment has passed. I've tried other approaches to gather and defer updates without a timerExec (such as maintaining re-entry counts) but to no avail — there are too many sequences that end up making model changes. Dirk's patch is simple and it ensures any affected MenuManager is updated eventually, and only once. Any menu that is to be shown before the timerExec executes will have been marked dirty and trigger the normal MenuManager about-to-be-shown and thus be #update()'d. The only downside to Dirk's patch is that the widget is not guaranteed to be in sync with the model immediately after any change. So I'm +1 for Dirk's patch.
Thanks Brian for the detailed analysis. For RC4 we need two component leads to +1 this change. I added Dani to get his vote.
(In reply to Lars Vogel from comment #18) > Thanks Brian for the detailed analysis. For RC4 we need two component leads > to +1 this change. I added Dani to get his vote. Even if we weren't at RC4, any change that tries to fix a problem by adding a (async) delay is a plaster and will most likely introduce new hard to detect issues. See bug 244316 for such an example.
Hi, Does that mean that this regression won't be fixed before official release? As detailled comment from Brian indicates, the menu takes around 2 seconds to appear in the Papyrus environment, which makes it bearly unusable and will be shown as a severe issue by our users. What are the solutions left? We don't ship yet our own product where we could apply patches that only apply to Papyrus. This would break E4 pure apps, but this would not be a problem in our case. Nevertheless, thank you for the attention ported to this bug by all of you.
(In reply to Remi Schnekenburger from comment #20) > Hi, > > Does that mean that this regression won't be fixed before official release? > As detailled comment from Brian indicates, the menu takes around 2 seconds > to appear in the Papyrus environment, which makes it bearly unusable and > will be shown as a severe issue by our users. > What are the solutions left? We don't ship yet our own product where we > could apply patches that only apply to Papyrus. This would break E4 pure > apps, but this would not be a problem in our case. > > Nevertheless, thank you for the attention ported to this bug by all of you. Maybe a better fix can still be found. If not, I would accept a solution where you/Papyrus could enable the -1ed fix via system property.
Dirk had to head out so I updated Dirk's patch to be conditional on the 'eclipse.workaround.bug467000' property being set to true. The name follows that used in bug 461311.
(In reply to Brian de Alwis from comment #22) > Dirk had to head out so I updated Dirk's patch to be conditional on the > 'eclipse.workaround.bug467000' property being set to true. The name follows > that used in bug 461311. +1 for the last patch - affected plugins can set flag at startup, the rest should be not affected by default. Dani?
(In reply to Andrey Loskutov from comment #23) > (In reply to Brian de Alwis from comment #22) > > Dirk had to head out so I updated Dirk's patch to be conditional on the > > 'eclipse.workaround.bug467000' property being set to true. The name follows > > that used in bug 461311. > > +1 for the last patch - affected plugins can set flag at startup, the rest > should be not affected by default. > > Dani? Looks good except that the system property value should be kept in a constant. Remi, does this plaster work for you?
(In reply to Dani Megert from comment #24) > Looks good except that the system property value should be kept in a > constant. Fixed.
> Looks good except that the system property value should be kept in a > constant. If the value is stored in a constant, then it is not possible to enable the workaround in a plugin's Activator (Because the constant at this point is already set and won't change). So for Papyrus this makes it complicated to actually deploy the workaround (We'd have to ask all our users to manually define the property) It might be useful to have a method to be able to change the value programmatically after Eclipse has started I confirm however that, once the property is correctly set, this works for Papyrus
(In reply to Camille Letavernier from comment #26) > > Looks good except that the system property value should be kept in a > > constant. > > If the value is stored in a constant, then it is not possible to enable the > workaround in a plugin's Activator (Because the constant at this point is > already set and won't change). So for Papyrus this makes it complicated to > actually deploy the workaround (We'd have to ask all our users to manually > define the property) > > It might be useful to have a method to be able to change the value > programmatically after Eclipse has started > > I confirm however that, once the property is correctly set, this works for > Papyrus Good point! I've reverted the change to patch set 5.
(In reply to Camille Letavernier from comment #26) > If the value is stored in a constant, then it is not possible to enable the > workaround in a plugin's Activator (Because the constant at this point is > already set and won't change). So for Papyrus this makes it complicated to > actually deploy the workaround (We'd have to ask all our users to manually > define the property) Please make sure you first check that the property isn't already set. Just in case this workaround causes other issues. > It might be useful to have a method to be able to change the value > programmatically after Eclipse has started I had wondered about using the new tracing preferences but we were trying to minimize the size of the patch given that it's RC4.
> Please make sure you first check that the property isn't already set. Just in case this workaround causes other issues. I've tried with and without the property; I didn't notice any undesired side effect (But I'm not sure what I should be looking for) Menus seem to be refreshed just fine in both cases I've also tried setting the property in the launch configuration (-Declipse.workaround.bug467000=true), and setting it programmatically from the Papyrus ModelExplorer's Activator. Both seem to work (And it definitely improves the performances in Papyrus context menus)
(In reply to Camille Letavernier from comment #29) > > Please make sure you first check that the property isn't already set. Just in case this workaround causes other issues. > > I've tried with and without the property; I didn't notice any undesired side > effect (But I'm not sure what I should be looking for) Bugs caused by asynchronous execution are hard to detect and reproduce, see e.g. bug 244316.
Gerrit change https://git.eclipse.org/r/48919 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=162c1d17dc731a2f47df9467bf9729d74948703c
Moving to 4.5.1 to find a proper fix.
(In reply to Camille Letavernier from comment #29) > > Please make sure you first check that the property isn't already set. Just in case this workaround causes other issues. I only meant to code your Activator like the following, just in case the user needs to override. if(System.getProperty("eclipse.workaround.bug467000") == null) { System.setProperty("eclipse.workaround.bug467000", "true"); } I apologize if you have already done this, but I thought it better to point it out explicitly.
Ah, Ok :) Good point, I will update the Papyrus contribution. Thanks!
New Gerrit change created: https://git.eclipse.org/r/59900
Gerrit change https://git.eclipse.org/r/59900 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=7a71629030b4f8cd1ee3741d6868ce3c8af815d8
New Gerrit change created: https://git.eclipse.org/r/61419
Gerrit change https://git.eclipse.org/r/61419 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=94ad5efce2045c78c92baf656732ecd6d6a16e89
(In reply to Eclipse Genie from comment #38) > Gerrit change https://git.eclipse.org/r/61419 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=94ad5efce2045c78c92baf656732ecd6d6a16e89 > Sorry Dirk, I had to reverted the change since it caused unexpected plug-in loading. PluginsNotLoadedTest.testPluginsNotLoaded fails (see official build test results). Please run this test when playing around with dynamic menu stuff. See also bug 400217 comment 16.
(In reply to Dani Megert from comment #39) > (In reply to Eclipse Genie from comment #38) > > Gerrit change https://git.eclipse.org/r/61419 was merged to [master]. > > Commit: > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=94ad5efce2045c78c92baf656732ecd6d6a16e89 > > > > Sorry Dirk, I had to reverted the change since it caused unexpected plug-in > loading. PluginsNotLoadedTest.testPluginsNotLoaded fails (see official build > test results). > > Please run this test when playing around with dynamic menu stuff. See also > bug 400217 comment 16. Are you sure this is caused by that contribution? It basically only adds squashing of menu updates by default. Why does that cause plug-in loading? I understand that I need to rethink the solution for bug 400217 because of the loading. But I don't understand why this patch should cause the plug-in loading. It does the same as before, just squashed and a little bit delayed to reduce the number of updates. It doesn't introduce a new processing of menu items as the other bug.
BTW, the test you are mentioning is part of JDT. If it is that important for platform development, should we think about moving it somehow to the platform tests?
(In reply to Dirk Fauth from comment #40) > (In reply to Dani Megert from comment #39) > > (In reply to Eclipse Genie from comment #38) > > > Gerrit change https://git.eclipse.org/r/61419 was merged to [master]. > > > Commit: > > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=94ad5efce2045c78c92baf656732ecd6d6a16e89 > > > > > > > Sorry Dirk, I had to reverted the change since it caused unexpected plug-in > > loading. PluginsNotLoadedTest.testPluginsNotLoaded fails (see official build > > test results). > > > > Please run this test when playing around with dynamic menu stuff. See also > > bug 400217 comment 16. > > Are you sure this is caused by that contribution? Yes, of course. I ran the test before and after reverting the change ;-). In reply to Dirk Fauth from comment #41) > BTW, the test you are mentioning is part of JDT. If it is that important for > platform development, should we think about moving it somehow to the > platform tests? The test covers more plug-ins when in JDT since it lives in a higher layer. We don't want to introduce a dependency to JDT, but a smaller variant that has a part with a contribution similar to the one given in bug 400217 comment 16 and opens that part should be enough to cover the dynamic menu case. Would be great if you can add such a test.
Too late for 4.5.2.
Brian, if you still plan to work on this, it should be targeted for M7.
Brian, are you still planning to work on this?
I'm going to unassign myself and add it to my try list.
Anybody please clarify this part: private void scheduleManagerUpdate(IContributionManager mgr) { < ... > synchronized (mgrToUpdate) { if (this.mgrToUpdate.isEmpty()) { < ... > this.mgrToUpdate.add(mgr); } } } why do we adding to the collection only if it is empty? This was fixed in https://git.eclipse.org/r/59900 but was reverted (unintentionally?) with https://git.eclipse.org/r/61419
New Gerrit change created: https://git.eclipse.org/r/136199
Gerrit change https://git.eclipse.org/r/136199 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=85bf65e8c7bed9026450ff2b202fad3fee8d9435
Thanks Rolf. Tested with a runtime IDE also using the model spy to dynamically update, add and remove menu items. One of the things, which still do not work with this patch is removing a menu item via the model spy, it will still be shown if I open the menu again. In the past changes in the menu renderer were highly critical, so please download the next I-Build and watch the test results. I will do the same. I committed it as we soon will have our M-test week and it will be good to have such a critical change in this test week.
AFICS no new tests are failing due to this change. Andrey, AFAIK you are watching the test very closely, please open this, if my observation is wrong.
This may have caused a regression, the preference menu entry is missing in certain workspaces for me (not all unfortunately). See Bug 546188. Rolf, please have a look.
New Gerrit change created: https://git.eclipse.org/r/140291
Gerrit change https://git.eclipse.org/r/140291 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=3679185c6b50b152227e70845828d6a527c350cc
(In reply to Eclipse Genie from comment #54) > Gerrit change https://git.eclipse.org/r/140291 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=3679185c6b50b152227e70845828d6a527c350cc > The fix caused a bad regression, see bug 467000. So, reverted it for now.
(In reply to Dani Megert from comment #55) > (In reply to Eclipse Genie from comment #54) > > Gerrit change https://git.eclipse.org/r/140291 was merged to [master]. > > Commit: > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=3679185c6b50b152227e70845828d6a527c350cc > > > The fix caused a bad regression, see bug 467000. So, reverted it for now. It's bug 546238.
The regression is caused by StackRenderer on line 1732. There it is incorrectly assumed that marking the MenuManager dirty, will force an update off it. With other refreshes disabled, this bug becomes visible. Will push an updated patch later.
(In reply to Rolf Theunissen from comment #57) > Will push an updated patch later. Please add me as reviewer as soon as the patch is ready.
(In reply to Lars Vogel from comment #58) > (In reply to Rolf Theunissen from comment #57) > > Will push an updated patch later. Any update here, Rolf?
(In reply to Lars Vogel from comment #59) > (In reply to Lars Vogel from comment #58) > > (In reply to Rolf Theunissen from comment #57) > > > Will push an updated patch later. > Any update here, Rolf? To solve the issue with the view menu, Bug 546634 must be solved. This depends on Bug 543827, which in turn depends on Bug 378495 and Bug 461655. A workaround for Bug 546634 could be applied, if this bug has higher priority.
(In reply to Rolf Theunissen from comment #60) > To solve the issue with the view menu, Bug 546634 must be solved. This > depends on Bug 543827, which in turn depends on Bug 378495 and Bug 461655. A big chain to solve. I assume you planning to work on these. Correct?
(In reply to Lars Vogel from comment #61) > A big chain to solve. I assume you planning to work on these. Correct? I plan to work on it, however I cannot give guarantees on the time-frame in which to succeed. Depends of course also on other issues that might popup while resolving the issues.
(In reply to Rolf Theunissen from comment #62) > (In reply to Lars Vogel from comment #61) > > > A big chain to solve. I assume you planning to work on these. Correct? > > I plan to work on it, however I cannot give guarantees on the time-frame in > which to succeed. Depends of course also on other issues that might popup > while resolving the issues. Sounds good. Thanks
See also https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/186087 which try to protect SWT from updating the menu image of frequently.
