Community
Participate
Working Groups
20060412-0010 A toolbar action that specifies 'pull down' and is not retargetable is forced to load when added to the toolbar. WWinPluginAction line 121 does a refreshSelection() and PluginAction line 277 tests isOkToCreateDelegate() which is implemented as protected boolean isOkToCreateDelegate() { if (getStyle() == IAction.AS_DROP_DOWN_MENU) { return true; } // test if the plugin has loaded String bundleId = configElement.getNamespace(); return BundleUtility.isActive(bundleId); } This seems to be related to bug 53979, bug 88534 Note that the action ins't really interested in the selection. As I see in the other bug this is also a major issue for other components.
A non-breaking fix for 3.2 could be to add a new optional attribute to the extension that e.g. states that the action is always enabled and does not need any selection changes.
Duong, this is likely more general than simply action sets but its performance related so you first...
Can you already say something if a fix / workaround can be provided for 3.2? I agree that changing the current bahviour could be a bit risky, but offering a workaround (e.g. as suggested in comment 1) could bring immediate benefit on the performance tests.
Assigning to component owner PW
The patch that did this was the fix for bug 105936 as a consequence of bug 90355. Since the work for bug 90355 has been rolled back, it doesn't look like 105936 is needed anymore. I'll release the revert into HEAD and watch for suspicious missing menu entries. PW
Released to HEAD >20070417 PW
Paul, what did you commit? Are you saying this bug is now fixed?
Sorry, I removed: if (getStyle() == IAction.AS_DROP_DOWN_MENU) { return true; } Yes, it should be fixed. I'm doing performance today and tomorrow, but I don't want to say this is fixed until I've verified some more of the delegate loading. PW
This is not fixed :-( Test Case: 1. add breakpoint to DebugUI.start() 2. start fresh target in debug mode ==> breakpoint is hit due to org.eclipse.debug.internal.ui.actions.RunHistoryMenuAction WWinPluginPulldown.MenuProxy.getMenu(Menu) needs this at the top: if (!isOkToCreateDelegate()) return null; AND: make sure *not* to add this to the getMenu(Control). HTH
Paul, due to bug 183163 the test case is slightly different: 1. start fresh target 2. exit 3. add breakpoint to DebugUI.start() 4. start target in debug mode ==> breakpoint is hit due to org.eclipse.debug.internal.ui.actions.RunHistoryMenuAction
Fixing this will improve startup performance as DebugUI and External Tools won't be loaded any longer on startup.
Created attachment 64392 [details] Fix
(In reply to comment #12) > Created an attachment (id=64392) [details] > Fix > I tried this last night, but it prevents the main menu entries from being populated. PW
Which main menu entries?
Did you add the code to the right class? Just tried and could not find missing menu entries.
(In reply to comment #15) > Did you add the code to the right class? yes :-) > Just tried and could not find missing > menu entries. I applied your patch just to be sure and launched my workspace. Go to the main Run menu. My Run As and Run History don't cascade any more. Popup on a project, Run As doesn't have a cascade. But clicking on the pull-down does get the cascade. PW
Removing the initial lines from isOkToCreateDelegate() will delay plugin loading for toolbar pulldown contributions. But it won't help with the Debug case, as the main menu has entries that need to be filled in. One possible direction would be to rework ActionContributionItem. If the action fired anothe property, say PROP_MENU, then the action contribution item could know to replace its current Menu with a new one ... this is risky work and also might involve API. I'll leave this for consideration for 3.3 and we can decide what to do with it later, but these kinds of changes won't make it in for 3.3M7. PW
Oh boy - so the WWinPluginPulldown is actually also used for the cascading main menu. Pretty unexpected from the class name and its Javadoc ;-) Anyway, good we made this iteration as I now also understand the reason why the context menu got broken in some scenarios: the code change you made from comment 8 should not have entirely removed the check as the current comment 8 "fix" also makes the CONTEXT sub-menus disappear without my fix. Here's a simple test case using plain N20070419-0010 (without my fix): 1. start 2. create a Java project 3. disable the Launching and the External Tools action sets 4. exit 5. start 6. open context menu on project ==> no Run or Debug sub-menus because they are no longer built (these are ObjectPluginAction's). We need to bring back the check from comment 8 into PluginAction but slightly modified: if (getStyle() == IAction.AS_DROP_DOWN_MENU && !WWinPluginPulldown.class.isInstance(this)) { return true; } Besides that I have a working patch ready that sets a dummy menu and then resolves it when needed but that needs some more testing and proper cleanup of the dummy menu. Stay tuned.
(In reply to comment #18) > > We need to bring back the check from comment 8 into PluginAction but slightly > modified: > if (getStyle() == IAction.AS_DROP_DOWN_MENU && > !WWinPluginPulldown.class.isInstance(this)) { > return true; > } > I've released this fix into HEAD in the meantime. PW
Created attachment 64545 [details] Fix This patch fixes the loading of plug-ins for the pulldown and the main menu: only when the corresponding menu item gets armed it will trigger the plug-in to be loaded. Unfortunately there's a flash when the loading happens as the change to the menu causes a redraw. I could not find a way to stop this. (adding Steve as SWT might have an idea here). NOTE: there is still an issue with the context menu (ObjectPluginAction) where the plug-ins get loaded when the context menu is built for the first time. This will be harder to fix as we do not have access to the owner at creation time.
Created attachment 64584 [details] Fix with different approach This second fix uses much less code and loads later but it still causes the flickering: I could not find a way to transfer the menu items of the loaded menu over to the proxy menu. Maybe SWT can help out here. If we cannot get rid of the flickering we have to discuss whether it makes sense to apply this, especially since opening the context menu will force the loading as well immediately and I doubt we can fix this for 3.3.
Also adding Silenio as Steve is away this week.
(In reply to comment #21) > Created an attachment (id=64584) [details] > Fix with different approach This causes a SEGV on GTK+, because it disposes the proxyMenu in the middle of its menuShown(*) event. I'll try putting that in an asyncExec(*) PW
(In reply to comment #23) > This causes a SEGV on GTK+, because it disposes the proxyMenu in the middle of > its menuShown(*) event. I'll try putting that in an asyncExec(*) OK, asyncExec(*) will move the dispose to later. Now if I start up and go Run>Run As I see nothing ... going to the menu a second time will work however. PW
>going to the menu a second time will work however. It should work the first time if you're not too fast with the mouse ;-) You ony moved the dispose into async, right? Does it also flash under Linux?
(In reply to comment #25) > It should work the first time if you're not too fast with the mouse ;-) You ony > moved the dispose into async, right? Right, after the proxyMenu.removeMenuListener(*): proxyMenu.getDisplay().asyncExec(new Runnable() { public void run() { proxyMenu.dispose(); } }); > Does it also flash under Linux? No, I don't get the flashing. I suspect that linux is slightly re-ordering the OS events. PW
You don't get the flashing but is the sub-menu there if you move slowly or are you required to leave the menu and come back to get the sub-menu? That would be bad.
(In reply to comment #27) > You don't get the flashing but is the sub-menu there if you move slowly or are > you required to leave the menu and come back to get the sub-menu? That would be > bad. If I start up and go to Run, and then slowly go down and stop over the highlighted Run As, I don't see the menu. I have to move my cursor off that menu at least once to get it back. PW
I've tried some other combinations, including use SWT.Show to get more event information and just trying to pop up the menu: realMenu.setLocation(event.x, event.y); realMenu.setVisible(true); But on linux it always takes 2 selections of Run As and 2 selections of Debug As, even after the plugin has been loaded. But thank you for the patch, Dani. PW
What would probably work is if we could somehow transfer the MenuItems from the menu back into the proxy but it seems that under Linux SWT doesn't like the menu to be changed while displayed and there seems to be no aboutToShow hook.
Paul, do you know why the main menus are built at all on startup and not just when needed the first time? Is this still needed? As far as I know this was done in the early days so that the key bindings (accelerators) got registered but this should no longer be needed as this should be handled by the command framework, shouldn't it?
(In reply to comment #31) > Paul, do you know why the main menus are built at all on startup and not just > when needed the first time? Is this still needed? As far as I know this was > done in the early days so that the key bindings (accelerators) got registered > but this should no longer be needed as this should be handled by the command > framework, shouldn't it? Yes, that's the current thinking. One of the optimizations we're considering for 3.4 is to have a lazy approach for the main menu (and view menus). That would at least delay the debug startup until clicking on the Run menu. But our initial investigation indicated that it would mean re-working how MenuManager recursively updates sub MenuManagers, and given how these pull down actions work it would probably involve modifying ActionContributionItem/ActionSetContributionItem as well. PW
I guess we have to shift this to 3.4. Even if we got any response from SWT on how to build that menu, any change at this point will probably be too risky.
Please make sure there is a bug for the SWT issue and follow up on us as necessary.
I'm not sure there is an issue/bug - just asking for the correct way to replace a menu while it's being opened. We then use that and if that doesn't work we can file a bug report.
But if you think it's easier to track I can file one.
When you say "Replace a menu while it's being opened", you mean replace the items, not the menu itself, right? You should be able to do this using menuShown(). Somewhere in the bug report, I'm sure there is a good reason why you can't. Let's have a bug about that.
>Let's have a bug about that. See bug 189092.
Created attachment 77164 [details] Proxy fix proof of concept v02 This is attachment #64584 [details] modified to try and copy over the real menu items. It's just a proof of concept patch. It works for Run>Run History, but it doesn't work for Run>Run As. The first time the Run As submenu shows up it will not be populated (Run As populates in its own menuShown(*) method). PW
(In reply to comment #39) > Created an attachment (id=77164) [details] > Proxy fix proof of concept v02 > > This is attachment #64584 [details] modified to try and copy over the real menu items. > It's just a proof of concept patch. Also, it would have to recursively populate submenus (or the proxy menu would dispose it). PW
Created attachment 77258 [details] Proxy fix proof of concept v03 A proxy menu with proxy items. It recursively creates other proxy submenus and will fire a menuShown(*) event for the real menu so it can pre-populate PW
Created attachment 77277 [details] MenuManager approach v01 In this approach we just remove most of the updateAll(true) calls. MenuManagers already do an update(*) call to configure their contribution items just before they're about to show. Debug (and my test plugin) are no longer activated on startup. The plugins are started when the pulldown item is rendered into a cascading menu. For example, when you pop up the main Run menu you'll start the debug plugin. PW
Created attachment 77286 [details] MenuManager + ActionContributionItem approach v03 This adds some changes to ActionContributionItem to delay loading even more. When we arm the menu item, it swaps out the proxy menu for the real menu returned from IMenuCreator (which for plugin actions instantiates their delegates). It looks promising ... but I've only tested it on Linux. PW
Created attachment 77368 [details] MenuManager + ActionContributionItem approach v04 Slight modification to the timing. PW
(In reply to comment #44) > Created an attachment (id=77368) [details] > MenuManager + ActionContributionItem approach v04 > This looks good for linux gtk and mac, but not for windows. There's either flickering or the menu disappears completely and you need to bring it back up. PW
*** Bug 53979 has been marked as a duplicate of this bug. ***
Created attachment 77999 [details] ActionContributionItem with a proxy v04
Created attachment 78000 [details] Menu Manager fix only v04 Separate the 2 potential improvements. PW
(In reply to comment #48) > Created an attachment (id=78000) [details] > Menu Manager fix only v04 Released the Menu Manager fix to HEAD >20070910 (In reply to comment #47) > Created an attachment (id=77999) [details] > ActionContributionItem with a proxy v04 This type of ActionContributionItem proxy fix will depend on what we can come up with in bug 189092 PW
(In reply to comment #49) > (In reply to comment #48) > > Created an attachment (id=78000) [details] [details] > > Menu Manager fix only v04 > > Released the Menu Manager fix to HEAD >20070910 The side effect of this fix can be seen in bug 202994. A closed editor reference will not be cleaned up until the MenuItem referencing it (via a contribution item) is disposed, and that won't happen until that particular menu manager updates. PW
Created attachment 78502 [details] ActionContributionItem clones menu v01 This creates pass through proxy items when the menu is about to show. PW
The action contribution item clone requires some more investigating. PW
Created attachment 79733 [details] PluginAction defer loading v01 Try to have PluginAction defer delegate loading as long as possible. The IMenuCreator is no longer requested just for rendering in a Menu for dropdowns, and the call to getMenuCreator() will cause load the delegate. The Menu copy v02 fix in bug 189092 allows this to work in the object contribution menu case. PW
(In reply to comment #53) > Created an attachment (id=79733) [details] > PluginAction defer loading v01 > Released to HEAD >20071018 PW
We now use the pattern suggested in bug 207263 Released to HEAD >20071025 PW
This fix has probably caused bug 207744.
In I20071030-0010 PW
Created attachment 85296 [details] Defer Plugin Loading v3.2.2 This is a 3.2.2 port that includes the MenuManager changes, ActionContributionItem changes, and PluginAction changes to defer loading of plugins with dropdown toolbars or dropdown actions placed in menus. PW
