Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 348582 - Shared area grows in number of part stacks after having moved views inside it
Summary: Shared area grows in number of part stacks after having moved views inside it
Status: VERIFIED FIXED
Alias: None
Product: e4
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 1.0   Edit
Hardware: All All
: P3 major (vote)
Target Milestone: 4.1 RC4   Edit
Assignee: Eric Moffatt CLA
QA Contact: Remy Suen CLA
URL:
Whiteboard:
Keywords:
: 348945 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-06-07 10:51 EDT by Remy Suen CLA
Modified: 2012-11-16 11:23 EST (History)
3 users (show)

See Also:
emoffatt: pmc_approved?
emoffatt: review+


Attachments
Patch to explicitly remove empty stacks in the MArea on reset (1.39 KB, patch)
2011-06-08 13:59 EDT, Eric Moffatt CLA
no flags Details | Diff
Screenshot depicting the problem in question. (83.69 KB, image/png)
2011-06-08 15:11 EDT, Remy Suen CLA
no flags Details
Screenshot depicting the problem in question. (76.66 KB, image/png)
2011-06-08 20:28 EDT, Remy Suen CLA
no flags Details
ModelServiceImpl patch v2 (1.60 KB, patch)
2011-06-08 21:32 EDT, Remy Suen CLA
no flags Details | Diff
EModelService patch v3 (1.41 KB, patch)
2011-06-09 15:20 EDT, Remy Suen CLA
no flags Details | Diff
Empty stack persists (431.58 KB, image/jpeg)
2012-11-16 11:23 EST, Palmer Eldritch CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Remy Suen CLA 2011-06-07 10:51:07 EDT
After you drag a view inside the shared area to create a new stack and you reset the perspective, the view's placeholder is correctly removed but its parent part stack (which is now empty) remains in the shared area.
Comment 1 Eric Moffatt CLA 2011-06-08 13:59:01 EDT
Created attachment 197625 [details]
Patch to explicitly remove empty stacks in the MArea on reset


The CleanupAddon is also correctly removing the sashes from the MArea if it can, reverting back to a single (possibly empty) MPartStack if it can.
Comment 2 Eric Moffatt CLA 2011-06-08 14:02:31 EDT
Remy, can you check the patch out ? Since the event loop has to spin I tested the state of the MArea by adding a breakpoint and examining the child list for the MArea on a separate reset...
Comment 3 Eric Moffatt CLA 2011-06-08 14:39:40 EDT
This also seems to fix bug 348625...
Comment 4 Remy Suen CLA 2011-06-08 15:11:35 EDT
Created attachment 197630 [details]
Screenshot depicting the problem in question.

I was able to get the system into this state. That stack seems to be the stack of the 'Problems' view as it still has a view menu on it (and when clicked it shows the view menu of the 'Problems' view).

This includes drawing multiple views into the shared area and stacking them with editors and then resetting.
Comment 5 Eric Moffatt CLA 2011-06-08 15:33:27 EDT
Perhaps the View Menu (which I can't see in the screen cap) is a manifestation of bug 348585 ?

I couldn't reproduce this but don't doubt that its there ,the question may be if it is worse without the patch ?
Comment 6 Remy Suen CLA 2011-06-08 20:28:12 EDT
Created attachment 197659 [details]
Screenshot depicting the problem in question.

This image will illustrate how to reproduce the problem. It's likely that the shape does not need to be this complex to induce the bug but anyway...
Comment 7 Remy Suen CLA 2011-06-08 20:29:08 EDT
Until we understand the problems described by comment 4 and comment 6, I am not comfortable with going ahead with this patch.
Comment 8 Remy Suen CLA 2011-06-08 20:59:56 EDT
(In reply to comment #6)
> It's likely that the
> shape does not need to be this complex to induce the bug but anyway...

I was able to get the problem by making the shared area have three stacks stacked horizontally going editor, 'Outline' view, second editor and then resetting the perspective.
Comment 9 Remy Suen CLA 2011-06-08 21:32:30 EDT
Created attachment 197660 [details]
ModelServiceImpl patch v2

The part stack is removed from the model but its control doesn't actually get destroyed (or reparented to the limbo shell for that matter) so it just hangs around on the workbench window. If the part stack is first unrendered and then removed from the application model then there won't be a tab folder floating around on the shell.
Comment 10 Eric Moffatt CLA 2011-06-09 09:00:43 EDT
I've opened Bug 348885 to track the fact that this pattern should be unnecessary...
Comment 11 Eric Moffatt CLA 2011-06-09 11:51:42 EDT
Remy, while it's not as bad as I feared the manifestation of bug 348625 is a door through which you can get the UI in trouble; once you're in the state defined by 348625's state then maximize the empty editor stack (seems OK) then minimize the Outline view's stack (ooops!! no EA any more and nothing in the trim...).

A reset at this point brings back the EA but it only has one button (restore) so you have to maximize / unmaximize a view in order to get everything back into what *appears* to be a normal state...

I do understand your point that we're messing with our own safety net here so we have to be even more careful but I think this is worth doing...
Comment 12 Remy Suen CLA 2011-06-09 12:12:05 EDT
Since the stacks now get unrendered properly through the rendering engine, their controls should be disposed and we should not be leaking anything (or drawing anything random on the screen). As we are only removing stacks that have zero children, this change should be safe in that we will not be inadvertently destroying any parts in between a reset request.

Eric, please work with Paul to update the readme with an entry asking the user to use 'Reset Perspective' or 'Window > New Window' to get back in a clean state without having to revert to restarting Eclipse.
Comment 13 Eric Moffatt CLA 2011-06-09 13:38:39 EDT
Committed in >20110609. Applied Remy's patch...
Comment 14 Remy Suen CLA 2011-06-09 15:17:06 EDT
*** Bug 348945 has been marked as a duplicate of this bug. ***
Comment 15 Remy Suen CLA 2011-06-09 15:20:13 EDT
Created attachment 197723 [details]
EModelService patch v3

1. Open two editors.
2. DND and split the area.
3. Ctrl+Shift+W to close them.
4. Now the area will still have a part sash container and two part stacks, one that's being rendered and one that isn't.

The attachment reverts the changes suggested by bug 197660 as the clean-up processing should now unwind the structure correctly, destroying rendered part stacks that have no children.

It is theoretically possible that the part stack in question may be the last one in the area but is not being rendered in which case it would return the wrong value even though we want it to return 'true' because it is indeed the last stack in the area. I'm not sure how realistic this scenario is but I thought I would bring it up.
Comment 16 Remy Suen CLA 2011-06-09 15:21:10 EDT
Reopening as the original fix was a workaround and the editor stack calculation code is the one that is wrong.
Comment 17 Mike Wilson CLA 2011-06-09 15:40:31 EDT
Agreed. Original fix just guaranteed we weren't in the bad state at the end. This one actually fixes the issue that got us in the bad state.
Comment 18 Eric Moffatt CLA 2011-06-09 15:57:10 EDT
This new patch is much better, the original 'empty' stacks were a manifestation of not identifying the lastEditorStack correctly.
Comment 19 Eric Moffatt CLA 2011-06-09 15:58:41 EDT
Committed in >20110609. Applied the 'v3' patch. It's been code reviewed and has had PMC (re)approval.
Comment 20 Eric Moffatt CLA 2011-06-14 15:30:54 EDT
Marking as (re)FIXED.
Comment 21 Eric Moffatt CLA 2011-06-14 15:31:14 EDT
Verified in I20110613-2055.
Comment 22 Palmer Eldritch CLA 2012-11-16 11:23:25 EST
Created attachment 223661 [details]
Empty stack persists
Comment 23 Palmer Eldritch CLA 2012-11-16 11:23:59 EST
I am still seeing this in 

Eclipse Platform

Version: 4.2.1.v20120814-120134-9JF7BHVGFyMveli1uX6aTH0q-eAap6PAgOP5mO
Build id: M20120914-1800

See the attachment I posted