Bugzilla – Full Text Bug Listing
| Summary: | Closing an additional window of an Eclipse 4 application doesn't unrender it | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Remy Suen <remy.suen> | ||||||||
| Component: | UI | Assignee: | Remy Suen <remy.suen> | ||||||||
| Status: | VERIFIED FIXED | QA Contact: | Eric Moffatt <emoffatt> | ||||||||
| Severity: | critical | ||||||||||
| Priority: | P3 | CC: | emoffatt, m.stier, Mike_Wilson, pwebster, tom.schindl | ||||||||
| Version: | 4.2 | Flags: | Mike_Wilson:
pmc_approved+
tom.schindl: review+ emoffatt: review+ |
||||||||
| Target Milestone: | 4.2 M6 | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 371087 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Remy Suen
Created attachment 197237 [details]
Window unrendering patch v1
We change the renderer to always override the SWT request to close the window and then internally have the rendering engine unrender the window if the user confirms that this action should go through.
I don't feel comfortable removing the window from its parent unless it is the application because detached windows, certainly from a perspective point of view, should stick around.
The rendering engine's stop() method previous call to shell's stop() method ultimately meant that the windows would all be removed on shutdown, this is obviously not what we want to happen.
(In reply to comment #1) > Created attachment 197237 [details] > Window unrendering patch v1 Eric, can you look at this patch? This looks fine to me as long as you're sure that it won't affect the current 4.1 code flow...(or unless it is fixing a leak in 4.1). Created attachment 197251 [details] Window unrendering patch v2 (In reply to comment #3) > This looks fine to me as long as you're sure that it won't affect the current > 4.1 code flow...(or unless it is fixing a leak in 4.1). Looking back at the original attachment, there is some amount of risk involved here so I'm attaching a newer one. This new patch only changes the default IWindowCloseHandler implementation in the WBWRenderer. In WorkbenchWindow's setup() method, we set our own compatibility implementation into the context so the default handler is overridden so this does not affect the code path of the SDK at all. Because windows are removed from the application now, a CME may occur in the rendering engine's stop() method (it gets thrown in the tests). While swapping it out for removeGui(MUIElement) is the proper way to close it, since the stop() method is called by the workbench on restart and other scenarios, I think it's best to leave the method as is and just wrap the returned list in an array list to prevent the CME and let it continue to do what it has always done. (In reply to comment #4) > Created attachment 197251 [details] > Window unrendering patch v2 Eric or Paul, could you review this new patch? Looks safer. PW (In reply to comment #6) > Looks safer. Thanks, patch released to CVS HEAD. Verified with I20110604-2201 on Windows XP. The new test is also in the build's test results page. This changes causes an e4 application to not restore after the first shutdown. I think the removal and setting of rendering to false should only happen when a window is closed manually and not as part of the shutdown of the application. I me thinks we should rollback the change and live with the none removal of windows and fix it in 4.2 and/or 4.1.2 (In reply to comment #9) > This changes causes an e4 application to not restore after the first shutdown. > I think the removal and setting of rendering to false should only happen when a > window is closed manually and not as part of the shutdown of the application. How embarrassing. We should actually be calling removeGui(*) on the windows instead of just closing the shells (or at least dispose the shells so that an SWT.Close event is not sent). I guess my first patch was better. > I me thinks we should rollback the change and live with the none removal of > windows and fix it in 4.2 and/or 4.1.2 Not automatically removing the windows is a minor nuisance to me but leaking the entire structure of a window is rather disturbing to me. Created attachment 197773 [details] Window unrendering patch v3 This patch reverts the changes from attachment 197251 [details] in WBWRenderer and PartRenderingEngine. It also removes the three tests that were added in that patch. (In reply to comment #11) > Created attachment 197773 [details] > Window unrendering patch v3 Eric and Tom, please review this patch revert, thanks. Confirm that the application can be started from a retored state. Remy, the reversion looks fine. I think I see where the original issue arose... Rather than calling window.setToBeRendered(false) we should be calling removeGui. Also, the test to remove the window should only affect all but the last one (sort of like 'isLastEditorStack)... Go to go from me +1 +1 for reverting the change. Need to provide a real fix for 4.1.1. (In reply to comment #11) > Created attachment 197773 [details] > Window unrendering patch v3 Patch released to HEAD. Thanks everyone. Just because it came up in the newsgroup today. Can we revisit this bug to properly handle @PD (In reply to comment #17) > Just because it came up in the newsgroup today. Can we revisit this bug to > properly handle @PD Tom, when the user closes a top-level window, should it be removed from the application's list of children? And if it was removed from a window's or a perspective's list of child windows, should it be removed from that list? Tom, what do you think about comment 18? (In reply to comment #18) > Tom, when the user closes a top-level window, should it be removed from the > application's list of children? And if it was removed from a window's or a > perspective's list of child windows, should it be removed from that list? I would say that the window should be removed from the list in both cases. What do you think, Tom? Top-level windows that are closed manually (i.e. hitting the red 'x') should be set to TBR false (to get them unrendered) and then removed from the app's list. DW's are slightly different...it's ok for one to become TBR false if, for example, all its views are closed. In this case the choice of removing from its perspective's list should be based on whether it's 'empty' (i.e. contains no views or placeholders). If it becomes empty (the user drags the last view out of it) then it should be removed. Otherwise it has to be left in the list so that a subsequent open of a view will re-expose it... I guess I agree with Eric and his conclusion - I'd like to get a fix for this in M6 to fill the EAP gap in bug #371087 Rereading this I'm not sure I still agree because removing it from the app-list would put us in the same situation we've run into 4.1 where on restart there's no window anymore in the application I missed a condition in my previous comment... ...Except where a lop-level window closed through 'x' is the *last* top-level window. In this case it should be left as TBR true (but still have removeGui called on it. That's sounds correct - this would be in alignment of the patch in bug 371087, right? Just one more note on why this is important beside - this will lead to memory leaks because the clean up of instance objects in only done in the PREngine and if this code is not run the memory will be leaked. I don't know if this also the case for the Compat Layer but for EAP-Apps this is certainly the case. Fix pushed to master. http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=81e764b3b98e971702e955b15c6ac7f3997e071b Thank you everyone for their patience. Marking as VERIFIED. Tom told me previously that it's "working like a charm". |