Community
Participate
Working Groups
Build Identifier: M20110909-1335 Hi, In Eclipse Indigo 3.7, selecting Help/Welcome is opening WelcomeEditor instead of Eclipse WelcomePage. This could be detected only when any of the eclipse plugins have a WelcomePage(Welcome.xml) defined in plugin properties(about.ini). In Eclipse Helios(3.6) this problem is not present. I have debugged both the eclipse code and found the probable cause. Below are my observations, I hope this helps to track the problem. * The deprecated class QuickStartAction and the new class IntroAction are sharing the same ActionDefinitionId, "org.eclipse.ui.help.quickStartAction". * As two actions share same Id, the action which is instantiated at last is getting invoked first. * In Eclipse 3.6, the order of instantiation is QuickStartAction, then IntroAction. So whenever we click Welcome from Help menu the UniversalIntro page is getting opened. * In Eclipse 3.7, Class IntroAction is removed and defined as anonyms class for the class variable ActionFactory.Intro and the order of instantiation ActionFactory.Intro followed by QuickStartAction class. So whenever Help/Welcome is selected WelcomeEditor is opened. * If no plugin have WelcomePage defined, the QuickStartAction class is skipped and the UniversalIntroPage is opened. So the problem won't be visible on eclipse Indigo unless any plugin have welcomePage defined. As one of my third party plugin is having welcomePage defined, My customized UniversalIntro page is not being displayed on Eclipse 3.7 It would be really helpful if you could provide a work around. Thanks. Reproducible: Always Steps to Reproduce: 1.Install eclipse platform 2.Add an welcome.xml to the eclipse platform plugin, "org.eclipse.platform_3.7.XX" 3.Run eclipse. Initially WelcomePage is shown. 4.Close it and reopen from Help>Welcome. Welcome Editor will be opened instead of UniversalWelcomePage. This could also be reproduced by installing any plugin with welcome.xml, For eg. Spring IDE.
Created attachment 210260 [details] Decipting Welcome page selector on clicking Help>Welcome
Created attachment 213497 [details] The patch fixes the disposed action handler usage issue IMHO, There is a bug in WorkbenchWindow.registerGlobalAction() method. If an existing action with an action handler is to be replaced by a CommandAction, old command's handler is disposed, but not removed from globalActionHandlerByCommandId map, so it still to be used every time the command is executed for the same ID (even the handler had been disposed). The patch fixes the registerGlobalAction() method by removing an old action handler from globalActionHandlerByCommandId map after it is disposed.
The patch provided (if applied) fixes the issue, as result, so no WelcomeEditor is invoked anymore instead of Eclipse Universal Intro (if exist).
Victor: please note that Eclipse 3.7 development is now done. This patch/fix might find its way into Eclipse 3.8 or 4.x, but will never be applied towards a 3.7.3 release.
(In reply to comment #4) Nick, the current patch created against trunk (I suppose that's what to be 3.8). But, as I see, the WorkbenchWindow class is equal between versions 3.7 and 3.8. Also, the patch is a one-row-like modification, so it's easily can be applied to 3.7.x (if suddenly any new versions will appear in this stream).
I willl look into this tomorrow.
(In reply to comment #6) Thanks, Daniel.
The patch looks right to me - it does not make much sense to dispose the handler and to not remove it from the map if the overwriting action is a CommandAction. What's more, no item is ever removed from globalActionHandlersByCommandId so there is no possibility that earlier registered action should become active again after removing the newly registered one. Victor, are you willing to provide an unit test, too? Paul, are you okay with releasing this patch?
Giving this bug second thought, I am not happy with relying on instantiation order due to following facts: * it is not an API and can change at any time * Bug 58886 comment 2 says: "The old welcome content will still appear in the event that your product does not have an intro." The actual problem is rather that the old welcome support is displayed while the new intro is properly defined, so I have changed the bug title. I will attach a new patch for testing in a minute - any cross-verification is welcome.
Created attachment 213548 [details] Alternative approach
Created attachment 213549 [details] mylyn/context/zip
(In reply to comment #8) Krzysztof, The problem occurs only if ActionHandler was registered for an 'old' action and then a 'new' action has to be registered with the same command ID, but the handler is not ActionHandler, but a CommandHandler. In this case the 'old' ActionHandler stays within the globalActionHandlersByCommandId map and used instead of correct CommandHandler each time the action with the same command ID is invoked. This is a bug. Also, the usage of ActionHandler which is disposed is also a bug. My patch fixes this problem. In case of both, an 'old' action and a 'new' action, have the same type, but different handlers works fine and is not affected by the patch. Probably it is not about Welcome Screen problem (it just reproduced in situation when old and new style Welcome Pages were mixed within installation), but IMHO, this should be fixed in WorkbenchWindow.registerGlobalAction() method, since it possible may rise future issues with other global actions. About unit tests... Not sure I can create the one right now - need a time to see how they're created for Eclipse Platform UI.
(In reply to comment #2) > Created attachment 213497 [details] > The patch fixes the disposed action handler usage issue > I changed it to a "final Object value = globalActionHandlersByCommandId.remove(commandId);" but I think you're right, Victor, if we state we're going to clobber the value anyway we can't have it left around. Krzysztof, I'm fine with you pursuing a proper fix for this welcome case, as it's unrelated to the lack of cleanup. PW
Created attachment 213576 [details] The patch fixes the disposed action handler usage issue (revisited) Revisited patch attached: The patch fixes the disposed action handler usage issue. Thanks to Paul for review.
(In reply to comment #14) > Created attachment 213576 [details] > The patch fixes the disposed action handler usage issue (revisited) I've released this fix to the current streams as it cleans up the leak. Thanx Victor. http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=d8f9b6bc9745b70ddb1843315d42b79badbd2072 PW
Did removing/disposing the handlers make this go away? PW
(In reply to comment #16) Removing the disposed handlers solves the problem of appearing an old Welcome Support when a new Intro Page is defined withing a product. (At least for our product it works fine). Wondered why it can't be targeted to 3.8? Krzysztof asked me about a JUnit test for the issue. Have no working test case at the moment. Probably later I will be able to provide one. BTW, Have no idea on how to register a test action withing WorkbenchWindow (the goal is to call registerGlobalAction() method due to see that globalActionHandlersByCommandId map is updated correctly) unless by using Reflection API. Is it a suitable solution here?)
(In reply to comment #17) > Removing the disposed handlers solves the problem of appearing an old Welcome > Support when a new Intro Page is defined withing a product. (At least for our > product it works fine). So I'm asking if this bug can be closed? > > Wondered why it can't be targeted to 3.8? I've released the fix into both 4.2 and 3.8, so it will be there (once 3.8 I builds start up again :-) PW
(In reply to comment #18) > > So I'm asking if this bug can be closed? > Leave it open if we need a JUnit Test for the issue, otherwise close it on your own. Thanx again for releasing the patch.
OK. That's good enough I think. PW
Created attachment 214214 [details] JUnit Test Case for the issue The patch draft containing draft of the JUnit Test Case for the issue is attached. Please verify if needed.
Verified I20120430-1800
