Bug 222750 - [ViewMgmt] Placeholder folders are lost when multi-instance views are dragged
Summary: [ViewMgmt] Placeholder folders are lost when multi-instance views are dragged
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.3.2   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 3.4   Edit
Assignee: Eric Moffatt CLA
QA Contact:
URL:
Whiteboard:
Keywords: helpwanted
Depends on:
Blocks:
 
Reported: 2008-03-14 10:19 EDT by Marco Maccaferri CLA
Modified: 2008-04-29 14:52 EDT (History)
1 user (show)

See Also:


Attachments
Sample application (55.18 KB, application/octet-stream)
2008-03-14 10:19 EDT, Marco Maccaferri CLA
no flags Details
Proposed patch (2.01 KB, patch)
2008-04-19 15:44 EDT, Marco Maccaferri CLA
no flags Details | Diff
Slightly modified patch (2.30 KB, text/plain)
2008-04-23 12:36 EDT, Eric Moffatt CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Marco Maccaferri CLA 2008-03-14 10:19:28 EDT
Created attachment 92570 [details]
Sample application

Build ID: M20080221-1800

The attached example RCP application sets the perspective with a placeholder folder at the left of the editor area and adds all instances of the sample view in it.

 1. Select File -> Open View and the view appears in the correct position.
 2. Drag the view at the bottom of the editor area.
 3. Select File -> Open View again, the new view appears at the right of the editor area, which is wrong. I would expect that it appeared on top of the previous view.

Opening additional views makes it worse, seems that also the "default" location is lost as they appear all at the right of the editor area with reduced sizes, instead of on top of each other.

If, after starting the application, I open two views and move only one of them, a third view appears in the correct position, on top of the view that wasn't moved.

My suggestion is that the placeholders follows the location of the dragged view to some degree.
Comment 1 Marco Maccaferri CLA 2008-04-18 03:20:39 EDT
Any hint ?
Comment 2 Boris Bokowski CLA 2008-04-18 10:08:24 EDT
I agree that this should be improved, but I don't have time left to fix this in 3.4.  If you have time to investigate, patches are always welcome.
Comment 3 Marco Maccaferri CLA 2008-04-18 10:46:30 EDT
I have stepped through the code and located where the view and folder gets moved, however apparently the folder setup is moved with the view (if I understand it correctly) but any subsequent attempt to locate the target folder fails. So maybe the issue is not with the view move but with the folder location code.
Unfortunately I don't understand the views management code very well, and without a hint I think that I won't be able to build a patch.
Comment 4 Boris Bokowski CLA 2008-04-18 10:52:13 EDT
(In reply to comment #3)
> I have stepped through the code and located where the view and folder gets
> moved, however apparently the folder setup is moved with the view (if I
> understand it correctly) but any subsequent attempt to locate the target folder
> fails. So maybe the issue is not with the view move but with the folder
> location code.
> Unfortunately I don't understand the views management code very well, and
> without a hint I think that I won't be able to build a patch.

Could it be that the placeholder no longer applies to all instances of a multi-instance view once it was moved?
Comment 5 Marco Maccaferri CLA 2008-04-19 15:43:04 EDT
> Could it be that the placeholder no longer applies to all instances of a
> multi-instance view once it was moved?

I have done some more debugging, here is my understanding of the issue:

The problem seems to be located at PerspectiveHelper.derefPart where the container without visible childs is reparented and removed (lines 754-760). Seems that this code doesn't fully remove the container, because subsequent findPart calls returns the same layout part instance. With a fresh workspace, the location of the layout part is correct, after dragging the view the location becames invalid and the workbench tries to open the views in a "safe" location, with the results you can see with the sample application.

Since this operation is similar to what happens when a folder with a single view is closed, I have replaced the code at lines 754-760 with the code from removePart at lines 1249-1261 with the necessary adjustments. The result is that the placeholder is kept in the original location so all new views are opened in the same place. Views can be moved around without affecting the location where new views are opened.

I haven't done intensive tests but the patch seems to work fine.
Comment 6 Marco Maccaferri CLA 2008-04-19 15:44:26 EDT
Created attachment 96715 [details]
Proposed patch
Comment 7 Boris Bokowski CLA 2008-04-19 21:03:20 EDT
Eric, could you have a look at the patch? If anyone knows the code in PerspectiveHelper, it would be you. ;-)
Comment 8 Eric Moffatt CLA 2008-04-22 15:06:15 EDT
I'll take this one...let me look at the patch...
Comment 9 Eric Moffatt CLA 2008-04-23 12:36:36 EDT
Created attachment 97259 [details]
Slightly modified patch


I've modified the patch (but only slightly) to account for the case where there are -really- no children left. I'm only placing the actual modifications here for bookkeeping reasons...
Comment 10 Eric Moffatt CLA 2008-04-23 12:40:19 EDT
Committed in >20080423. Applied the patch...

BTW, this issue was not specific to multi-instance views. The logic to handle 'derefPart' was IMO quite bogus; dragging the only -visible- view out of a stack would cause any placeholders in that stack to be 'promoted' directly to the parent container (which is where all the new stacks were coming from).
Comment 11 Eric Moffatt CLA 2008-04-29 14:52:42 EDT
Verified in I20080429-0100.