Bug 522450 - [CleanupAddon] sash container removal after DnD from [DetachedWindow] is too aggressive. Minimized views lost permanently.
Summary: [CleanupAddon] sash container removal after DnD from [DetachedWindow] is too ...
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.6   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: 4.8 M7   Edit
Assignee: darrel karisch CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-09-18 15:57 EDT by darrel karisch CLA
Modified: 2018-04-23 18:14 EDT (History)
3 users (show)

See Also:


Attachments
Defect Demo (1021.77 KB, video/mp4)
2018-02-06 09:48 EST, darrel karisch CLA
no flags Details
Bug Fix similar to embedded suggestion (1.89 KB, patch)
2018-02-06 09:48 EST, darrel karisch CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description darrel karisch CLA 2017-09-18 15:57:26 EDT
1) Detach a part stack containing multiple views from main window
2) DnD one view in detached window stack into new part stack within same detached window
3) Minimize the original part stack in the detached window
4) DnD the view of the remaining part stack into main window
   a) CleanupAddon removes sash from detached window
   b) Detached window is set invisible (CleanupAddon response to sash toBeRendered set to false)
5) All views in detached window are lost permanently in that perspective

A solution is to avoid removing the sash of a detached window when the sash's last stack is minimized...

CleanupAddon...
@Inject
@Optional
private void subscribeTopicChildren(@UIEventTopic(UIEvents.ElementContainer.TOPIC_CHILDREN) Event event) {
...

	// if a sash container has only one element then remove
	// it and move
	// its child up to where it used to be...
	MUIElement theChild = container.getChildren().get(0);
	MElementContainer<MUIElement> parentContainer = container.getParent();
	// ...unless its parent is a detached window
	// and its last remaining part stack is minimized
	if (parentContainer != null && parentContainer.getParent() != null
	   && !theChild.getTags().contains(IPresentationEngine.MINIMIZED)) {
		int index = parentContainer.getChildren().indexOf(container);
...
}
Comment 1 Andrey Loskutov CLA 2017-09-18 16:14:58 EDT
Darrel, can you please provide a Gerrit patch? Thanks!
Comment 2 darrel karisch CLA 2017-10-06 13:05:25 EDT
(In reply to darrel karisch from comment #0)
> 1) Detach a part stack containing multiple views from main window
> 2) DnD one view in detached window stack into new part stack within same
> detached window
> 3) Minimize the original part stack in the detached window
> 4) DnD the view of the remaining part stack into main window
>    a) CleanupAddon removes sash from detached window
>    b) Detached window is set invisible (CleanupAddon response to sash
> toBeRendered set to false)
> 5) All views in detached window are lost permanently in that perspective
> 
> A solution is to avoid removing the sash of a detached window when the
> sash's last stack is minimized...
> 
> CleanupAddon...
> @Inject
> @Optional
> private void
> subscribeTopicChildren(@UIEventTopic(UIEvents.ElementContainer.
> TOPIC_CHILDREN) Event event) {
> ...
> 
> 	// if a sash container has only one element then remove
> 	// it and move
> 	// its child up to where it used to be...
> 	MUIElement theChild = container.getChildren().get(0);
> 	MElementContainer<MUIElement> parentContainer = container.getParent();
> 	// ...unless its parent is a detached window
> 	// and its last remaining part stack is minimized
> 	if (parentContainer != null && parentContainer.getParent() != null
> 	   && !theChild.getTags().contains(IPresentationEngine.MINIMIZED)) {
> 		int index = parentContainer.getChildren().indexOf(container);
> ...
> }

I erred a bit with the logic to identify a detached window.
replace...
    parentContainer != null && parentContainer.getParent() != null
with...
    !(modelService.getContainer(parentContainer) instanceof MPerspective)

thus the updated modification... 
	// if a sash container has only one element then remove
	// it and move
	// its child up to where it used to be...
	MUIElement theChild = container.getChildren().get(0);
	MElementContainer<MUIElement> parentContainer = container.getParent();
	// ...unless its remaining part stack element is minimized
	// and its parent is a detached window
	if (!theChild.getTags().contains(IPresentationEngine.MINIMIZED)
 	   && !(modelService.getContainer(parentContainer) instanceof MPerspective)) {
		int index = parentContainer.getChildren().indexOf(container);
Comment 3 Andrey Loskutov CLA 2017-10-06 13:07:03 EDT
(In reply to Andrey Loskutov from comment #1)
> Darrel, can you please provide a Gerrit patch? Thanks!

Darrel, can you provide a *Gerrit* patch *please*?
Comment 4 darrel karisch CLA 2018-02-06 09:48:36 EST
Created attachment 272556 [details]
Defect Demo

Task List view lost permanently
Comment 5 darrel karisch CLA 2018-02-06 09:48:54 EST
Created attachment 272557 [details]
Bug Fix similar to embedded suggestion

The situation is a bit more complex than the test case.

Consider three tiled stacks (e.g. vertically) in a detached window:
detached  window
    sash1
   /     \
stack1    sash2
          /   \
     *stack2  *stack3

*where stack2 and stack3 are minimized

DnD stack1 back to the main window.
sash1 is unrendered and removed in CleaunAddon.subscribeTopicChildren.
unrendering sash1 leads to CleaunAddon.subscribeRenderingChanged
which sets sash1's parent, the detached window, invisible since the window has no immediate visible and rendered children.
Tthis leaves the parts in stack2 and stack3 unreachable.
Hence, the additional check for detached window in subscribeRenderingChanged.
Comment 6 Eclipse Genie CLA 2018-02-06 10:01:22 EST
New Gerrit change created: https://git.eclipse.org/r/116796
Comment 7 darrel karisch CLA 2018-02-06 11:13:55 EST
My testing with the proposed fix does not reveal any unintentional side-effects or defects with stickyFolderRight or any other container in the main window.
Comment 8 darrel karisch CLA 2018-02-07 09:46:19 EST
Changing subscribeTopicChildren code is unnecessary.

The change to subscribeRenderingChanged in the diff attachment is sufficient to fix the problem.
Comment 9 Lars Vogel CLA 2018-04-23 18:14:10 EDT
Thanks, Darrel.