Bugzilla – Full Text Bug Listing
| Summary: | Old welcome support appears when the "new" intro is defined | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Vignesh G <gvignesh2005> |
| Component: | UI | Assignee: | Krzysztof Daniel <krzysztof.daniel> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | curtis.windatt.public, krzysztof.daniel, mistria, nboldt, pwebster, vrubezhny |
| Version: | 3.7.1 | Keywords: | helpwanted |
| Target Milestone: | 4.2 M7 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Attachments: | |||
|
Description
Vignesh G
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 |