Fix #9203: Drag and drop sections between views #9644

EstherPerelman · 2021-06-27T13:35:18Z

Signed-off-by: Esther Perelman [email protected]

This PR enable to group multiple views at the same container without the need to navigate between the containers

What it does

Fix #9203

How to test

Install Docker extension & Gitlens extension (available in theia Extensions view).
Run the command (ctrl + shift + p): View: Reset Workbench Layout
Try Drag & Drop views between containers:
Attentions:
- d&d for Extensions view is disabled (as vscode).
- dragging is allowed only inside the container for: File explorer view part and debug view parts (you still able to drop views from other containers on those parts)
- restore last containers views after f5 should work.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

colin-grant-work · 2021-07-01T21:55:19Z

@EstherPerelman, are you still having trouble with the Search-in-Workspace widget? When I pull your code, I'm able to search and get results. However, I'm not able to drop anything in that widget. Have you removed the code that placed it in a ViewContainer?

EstherPerelman · 2021-07-04T08:08:47Z

@EstherPerelman, are you still having trouble with the Search-in-Workspace widget? When I pull your code, I'm able to search and get results. However, I'm not able to drop anything in that widget. Have you removed the code that placed it in a ViewContainer?

@colin-grant-work Thank for trying to help! I'm still having this trouble, It's very strange I haven't removed any code - for me it still enables dropping into it but not showing the search results, Do the other containers enable d&d for you?

EstherPerelman · 2021-07-11T10:25:41Z

Hi all, If there's no solution for fixing the problem with search container - is it possible to push it without the ability to use the dnd inside search container?
cc @colin-grant-work

colin-grant-work

With minor exceptions (one immediately below, and one in the comments), both of them apparently related to the special status of the debug session widget, this is working pretty well for me. The logic for identifying and moving parts is pretty involved, but I don't see any easy simplifications. I will see if I can work out what's going on with the search-in-workspace widget - I suspect that it's another special case - but I have no objection in principle to merging this and leaving the SIW as a later to-do.

I got somewhat inconsistent behavior with the highlighting for drop. In a plugin view container (GitLens, in my case), and the internal SCM widget, I got highlighting when dropping parts from other containers. However, in the debug view container, after collapsing all of the parts, there was no highlighting when I dragged parts from other containers. The drop functionality still worked fine, though.

packages/core/src/browser/style/tabs.css

packages/core/src/browser/view-container.ts

colin-grant-work · 2021-07-13T16:06:33Z

@EstherPerelman, I think the Search-in-Workspace widget is actually fine and the problem is only styling - the tree is ending up with a height of 0px. Going through the element tree and adding height: 100% to the tree container and its parents until they showed up seemed to fix the problem - fiddle around with the CSS to find the most parsimonious solution.

EstherPerelman · 2021-07-26T11:51:48Z

@colin-grant-work Thank you so much for the help and review! setting the height of search-in-workspace to 100% did solve the problem!
I added more fixes and solved according to your comments...
Thanks👍
(Responds late because I haven't been at work for a week...)

packages/core/src/browser/view-container.ts

colin-grant-work · 2021-07-29T19:12:49Z

This is working very nicely for me. Now only the comments about the shifting icons and references to debug in core remain. 👍

EstherPerelman · 2021-07-29T19:31:10Z

This is working very nicely for me. Now only the comments about the shifting icons and references to debug in core remain. 👍

Great! I'm still working on this because the restore after refresh(f5) not working well, I still didn't succeed to see it working perfect, Maybe adding a ViewContainerManager will solve the problem...

EstherPerelman · 2021-08-11T16:08:56Z

@colin-grant-work

A:

If possible, we should avoid referring to anything related to debug in core. Perhaps define an interface here that the debug widget could implement that would get around these problems?

I have started to implement as you wrote but saw that it doesn't fit all places (where I have only the ViewContainer widget without information about the parent parent ...), Anyway I have found a solution by creating draggingPartManager which make things easier...

B:

I think the problem still exists. I think we probably need to set the box-sizing property for these tabs to be border-box (hopefully that will fix it - it may be more complex)

I have spent time on this but box-sizing setting and others just didn't solve it (while setting outline instead of border the icon didn't move but the appearance was not uniform)

C:

Restore after refresh suppose to work well now...

Finally
Only one issue remains: the border style that cause the icons to move...

EstherPerelman · 2021-08-12T10:59:56Z

@colin-grant-work
I found a solution for the issue with the border
So - the PR is full ready now for another review
Thanks for all advices and comments

msujew

Great work. I almost wanted to approve, until I noticed an issue with reloading the browser application. This happens both on firefox and chrome:

dnd.mp4

As you can see, after reloading and moving a section the view becomes kind of unresponsive and multiple sections try to move into the same space. This issue persists after reloading and can only be resolved by resetting the layout.

packages/core/src/browser/shell/tab-bars.ts

packages/core/src/browser/style/tabs.css

packages/core/src/browser/view-container.ts

packages/debug/src/browser/view/debug-session-widget.ts

packages/navigator/src/browser/navigator-widget-factory.ts

colin-grant-work · 2021-08-12T17:17:53Z

Another VSCode feature that doesn't necessarily need to be implemented here, but that you may want to consider is the draggability of parts that are the sole child of their view container. E.g. in VSCode, if the Search-in-Workspace widget is the sole child of the search container, you can start a drag on the ViewContainer title that behaves the same as dragging the ViewContainerPart's title.

EstherPerelman · 2021-08-12T20:15:52Z

Another VSCode feature that doesn't necessarily need to be implemented here, but that you may want to consider is the draggability of parts that are the sole child of their view container. E.g. in VSCode, if the Search-in-Workspace widget is the sole child of the search container, you can start a drag on the ViewContainer title that behaves the same as dragging the ViewContainerPart's title.

@colin-grant-work I know about this, it also available in vscode to drop the part header on the sidebar and a new tab will be created,
I'm afraid of the mess with creating new viewContainers, I'll need to create a new id to a duplicate container - then to get it by this id in other places... don't know how it can be managed easily

colin-grant-work · 2021-08-12T20:19:40Z

@colin-grant-work I know about this, it also available vscode to drop the part header on the sidebar and a new tab will be created,
I'm afraid of the mess with creating new viewContainers, I'll need to create a new id to a duplicate container - then to get it by this id in other places... don't know how it can be managed...

It sounds reasonable to omit this for now. I only noticed its absence because I was having trouble finding anything to drag, without the debug widgets being draggable :-).

EstherPerelman · 2021-08-12T21:34:07Z

Great work. I almost wanted to approve, until I noticed an issue with reloading the browser application. This happens both on firefox and chrome:

@msujew thanks for review & almost approving;) hopping to resolve this soon, I find it little difficult to fix (I'm not familiar with the containerLayout handles and sizes behavior) if you know about something that might help I'll be happy to see...

packages/core/src/browser/view-container.ts

EstherPerelman · 2021-08-17T08:15:47Z

Actually if there will be no solution we are able to completely disable drag & drop for the debugger container (which is the only one who makes these troubles) as done for Extensions, WDYS?
(I have already fixed all other issues)

@msujew @colin-grant-work

EstherPerelman · 2021-09-12T10:34:19Z

@colin-grant-work I have addressed the issue with search icon and the next issues:

I'm seeing one odd behavior. It looks like the git decorations for modified files remain attached to the Source Control View Container even if you move the source control widget out of that container:

I have pushed a commit that supports the decorations moves, But It doesn't work for the widgets that move into the debug container.

Another minor oddity:

Move another view into the Search container

Collapse the Search-in-Workspace widget

Use ctrl+shift+f to activate the Search-in-Workspace widget.

Observe that its title bar is highlighted, but the widget remains collapsed.

I have added a code that opens the part on activating the widget by ctrl+shift+f / ctrl+shift+g / etc...

colin-grant-work · 2021-09-13T15:39:13Z

Both of the new behaviors are working for me 👍. @msujew, @vince-fugnitto, would you mind confirming that the bad behaviors have been addressed and there are no new regressions?

One thing I realized recently is that the highlight behavior in VSCode (adding a white bar above or below the icon) isn't actually connected to the dragging between containers, but to general drag-drop repositioning behavior, since in VSCode you can drop a ViewContainerPart on the main tabbar, and it will create a new ViewContainer. In Phosphor, the repositioning drag-drop is handled by shifting the icons around to open space for a repositioning. In this context, that's an undesirable behavior, because we want to drop in a container, not between container icons. We may need to rethink one or the other of these drag behaviors if we implement VSCode's full set of interactive possibilities.

packages/core/src/browser/view-container.ts

vince-fugnitto · 2021-09-13T16:48:12Z

packages/search-in-workspace/src/browser/search-in-workspace-factory.ts

+@injectable()
+export class SearchInWorkspaceFactory implements WidgetFactory {
+
+    static ID = SEARCH_VIEW_CONTAINER_ID;


@EstherPerelman the static ID can be removed it is not used, and we already export the constant SEARCH_VIEW_CONTAINER_ID. We should either use the constant, or the static ID but not both.

@vince-fugnitto I fixed it, It just the same as done in NavigatorWidgetFactory...

packages/search-in-workspace/src/browser/search-in-workspace-factory.ts

packages/core/src/browser/shell/tab-bars.ts

packages/core/src/browser/shell/tab-bar-decorator.ts

EstherPerelman · 2021-09-14T08:27:30Z

One thing I realized recently is that the highlight behavior in VSCode (adding a white bar above or below the icon) isn't actually connected to the dragging between containers, but to general drag-drop repositioning behavior, since in VSCode you can drop a ViewContainerPart on the main tabbar, and it will create a new ViewContainer. In Phosphor, the repositioning drag-drop is handled by shifting the icons around to open space for a repositioning. In this context, that's an undesirable behavior, because we want to drop in a container, not between container icons. We may need to rethink one or the other of these drag behaviors if we implement VSCode's full set of interactive possibilities.

@vince-fugnitto absolutely right! I still think these bars gives the user more indication on the dragging but I can remove it if you want to...

colin-grant-work · 2021-09-14T14:00:03Z

@vince-fugnitto absolutely right! I still think these bars gives the user more indication on the dragging but I can remove it if you want to...

No, I think the bars are the more future-proof version of the drag-drop marker. Eventually, we will want to be able to drop both inside of and next to, and only the bars - not the shifting behavior - will allow that. Just something for the person who implements the next round to think about 🙂.

packages/core/src/browser/shell/tab-bar-decorator.ts

packages/core/src/browser/shell/application-shell.ts

msujew

It seems to work well in Chrome now, but there's some strange behavior in Firefox (92.0):

dnd-firefox.mp4

As soon as I let the mouse button go, it starts dragging correctly. But before letting go, the dragged container stays at the same place.

EstherPerelman · 2021-09-29T10:15:58Z

It seems to work well in Chrome now, but there's some strange behavior in Firefox (92.0):

@msujew thanks for finding that, I called the event preventDefault which seems to solve it. (sorry for the late answer, from now on I am available)

Can you merge it for tomorrow release?
cc @colin-grant-work @vince-fugnitto

msujew

Alright, looks good to me 👍

vince-fugnitto · 2021-09-29T12:30:51Z

Can you merge it for tomorrow release? cc @colin-grant-work @vince-fugnitto

@EstherPerelman last release we had a couple of major regressions after merging big pull-requests late in the month (resulted in two patch releases). Should we instead wait till right after the release so we have the month to discover any possible regressions? In any case I'll still have to take the time to re-review the code and functionality before approving.

EstherPerelman · 2021-09-29T13:04:24Z

Can you merge it for tomorrow release? cc @colin-grant-work @vince-fugnitto

@EstherPerelman last release we had a couple of major regressions after merging big pull-requests late in the month (resulted in two patch releases). Should we instead wait till right after the release so we have the month to discover any possible regressions? In any case I'll still have to take the time to re-review the code and functionality before approving.

@vince-fugnitto you are right, I also think it's better to wait after the release in case of regressions, Thank you!

EstherPerelman · 2021-10-04T07:15:32Z

@colin-grant-work / @vince-fugnitto, Please notify me before merging so I'll resolve conflicts..

EstherPerelman · 2021-10-06T09:41:24Z

@vince-fugnitto why don't you merge it? I don't want to wait till the end of the month...

Signed-off-by: Esther Perelman <[email protected]>

This reverts commit 924f67f.

vince-fugnitto · 2021-10-06T12:22:06Z

@vince-fugnitto why don't you merge it? I don't want to wait till the end of the month...

@EstherPerelman I set time aside to be able to test the pr again first (I had never approved) #9644 (comment).

In any case I'll still have to take the time to re-review the code and functionality before approving.

Stale Review

vince-fugnitto

I'm fine with the changes, we can fix any possible regressions during the month 👍
@colin-grant-work please merge if you are comfortable with the updates.

EstherPerelman force-pushed the drag-and-drop branch 2 times, most recently from 4c457c5 to 2787877 Compare June 28, 2021 08:28

colin-grant-work self-requested a review July 13, 2021 12:52

colin-grant-work reviewed Jul 13, 2021

View reviewed changes

vince-fugnitto added tree issues related to the tree (ex: tree widget) vscode issues related to VSCode compatibility labels Jul 16, 2021

EstherPerelman force-pushed the drag-and-drop branch from 8c0efe1 to e9a2fbe Compare July 26, 2021 12:01

EstherPerelman marked this pull request as draft July 28, 2021 07:22

colin-grant-work reviewed Jul 29, 2021

View reviewed changes

packages/core/src/browser/view-container.ts Outdated Show resolved Hide resolved

EstherPerelman marked this pull request as ready for review August 11, 2021 15:12

EstherPerelman force-pushed the drag-and-drop branch 2 times, most recently from 4212b43 to cb3f559 Compare August 11, 2021 16:19

msujew reviewed Aug 12, 2021

View reviewed changes

colin-grant-work reviewed Aug 12, 2021

View reviewed changes

msujew reviewed Aug 13, 2021

View reviewed changes

packages/core/src/browser/view-container.ts Outdated Show resolved Hide resolved

EstherPerelman force-pushed the drag-and-drop branch from cbeebd3 to bd37cd1 Compare August 15, 2021 17:06

EstherPerelman force-pushed the drag-and-drop branch from 94f37b3 to 5426df7 Compare September 9, 2021 18:59

vince-fugnitto reviewed Sep 13, 2021

View reviewed changes

vince-fugnitto previously requested changes Sep 16, 2021

View reviewed changes

packages/core/src/browser/shell/tab-bar-decorator.ts Outdated Show resolved Hide resolved

packages/core/src/browser/shell/application-shell.ts Outdated Show resolved Hide resolved

msujew reviewed Sep 21, 2021

View reviewed changes

EstherPerelman force-pushed the drag-and-drop branch from d38e7b4 to 5770021 Compare September 29, 2021 10:08

msujew approved these changes Sep 29, 2021

View reviewed changes

EstherPerelman added 7 commits October 6, 2021 13:58

Fix eclipse-theia#9203: Drag and drop sections between views

b2034e3

Signed-off-by: Esther Perelman <[email protected]>

expand part on wrapped activation

625522a

Signed-off-by: Esther Perelman <[email protected]>

set decorations to follow the widgets on moving

8ffc606

Signed-off-by: Esther Perelman <[email protected]>

Review fixes

cc031bc

Signed-off-by: Esther Perelman <[email protected]>

better code for the onDidChangeTrackableWidgets event

511a3ac

Revert "better code for the onDidChangeTrackableWidgets event"

c964462

This reverts commit 924f67f.

fix firefox strange behavior

85464e8

EstherPerelman force-pushed the drag-and-drop branch from 5770021 to 85464e8 Compare October 6, 2021 11:00

vince-fugnitto approved these changes Oct 6, 2021

View reviewed changes

colin-grant-work merged commit 6825c60 into eclipse-theia:master Oct 6, 2021

vince-fugnitto mentioned this pull request Oct 14, 2021

tree-view: support 'reset location' #10272

Open

vince-fugnitto added this to the 1.19.0 milestone Oct 28, 2021

vince-fugnitto mentioned this pull request Nov 18, 2021

search-in-workspace: correct scrollbar display when no results #10410

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #9203: Drag and drop sections between views #9644

Fix #9203: Drag and drop sections between views #9644

EstherPerelman commented Jun 27, 2021 •

edited

Loading

colin-grant-work commented Jul 1, 2021

EstherPerelman commented Jul 4, 2021

EstherPerelman commented Jul 11, 2021

colin-grant-work left a comment

colin-grant-work commented Jul 13, 2021 •

edited

Loading

EstherPerelman commented Jul 26, 2021 •

edited

Loading

colin-grant-work commented Jul 29, 2021

EstherPerelman commented Jul 29, 2021

EstherPerelman commented Aug 11, 2021 •

edited

Loading

EstherPerelman commented Aug 12, 2021

msujew left a comment

colin-grant-work commented Aug 12, 2021

EstherPerelman commented Aug 12, 2021 •

edited

Loading

colin-grant-work commented Aug 12, 2021

EstherPerelman commented Aug 12, 2021 •

edited

Loading

EstherPerelman commented Aug 17, 2021

EstherPerelman commented Sep 12, 2021 •

edited

Loading

colin-grant-work commented Sep 13, 2021 •

edited

Loading

vince-fugnitto Sep 13, 2021

EstherPerelman Sep 14, 2021

EstherPerelman commented Sep 14, 2021

colin-grant-work commented Sep 14, 2021

msujew left a comment

EstherPerelman commented Sep 29, 2021 •

edited

Loading

msujew left a comment

vince-fugnitto commented Sep 29, 2021

EstherPerelman commented Sep 29, 2021

EstherPerelman commented Oct 4, 2021

EstherPerelman commented Oct 6, 2021

vince-fugnitto commented Oct 6, 2021

vince-fugnitto left a comment

Fix #9203: Drag and drop sections between views #9644

Fix #9203: Drag and drop sections between views #9644

Conversation

EstherPerelman commented Jun 27, 2021 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

colin-grant-work commented Jul 1, 2021

EstherPerelman commented Jul 4, 2021

EstherPerelman commented Jul 11, 2021

colin-grant-work left a comment

Choose a reason for hiding this comment

colin-grant-work commented Jul 13, 2021 • edited Loading

EstherPerelman commented Jul 26, 2021 • edited Loading

colin-grant-work commented Jul 29, 2021

EstherPerelman commented Jul 29, 2021

EstherPerelman commented Aug 11, 2021 • edited Loading

EstherPerelman commented Aug 12, 2021

msujew left a comment

Choose a reason for hiding this comment

colin-grant-work commented Aug 12, 2021

EstherPerelman commented Aug 12, 2021 • edited Loading

colin-grant-work commented Aug 12, 2021

EstherPerelman commented Aug 12, 2021 • edited Loading

EstherPerelman commented Aug 17, 2021

EstherPerelman commented Sep 12, 2021 • edited Loading

colin-grant-work commented Sep 13, 2021 • edited Loading

vince-fugnitto Sep 13, 2021

Choose a reason for hiding this comment

EstherPerelman Sep 14, 2021

Choose a reason for hiding this comment

EstherPerelman commented Sep 14, 2021

colin-grant-work commented Sep 14, 2021

msujew left a comment

Choose a reason for hiding this comment

EstherPerelman commented Sep 29, 2021 • edited Loading

msujew left a comment

Choose a reason for hiding this comment

vince-fugnitto commented Sep 29, 2021

EstherPerelman commented Sep 29, 2021

EstherPerelman commented Oct 4, 2021

EstherPerelman commented Oct 6, 2021

vince-fugnitto commented Oct 6, 2021

vince-fugnitto left a comment

Choose a reason for hiding this comment

EstherPerelman commented Jun 27, 2021 •

edited

Loading

colin-grant-work commented Jul 13, 2021 •

edited

Loading

EstherPerelman commented Jul 26, 2021 •

edited

Loading

EstherPerelman commented Aug 11, 2021 •

edited

Loading

EstherPerelman commented Aug 12, 2021 •

edited

Loading

EstherPerelman commented Aug 12, 2021 •

edited

Loading

EstherPerelman commented Sep 12, 2021 •

edited

Loading

colin-grant-work commented Sep 13, 2021 •

edited

Loading

EstherPerelman commented Sep 29, 2021 •

edited

Loading