Enable Views to move between sidebar and panel (Part 1)

[sbatten]

Aggregating prior discussions from abandoned PRs.

• Update PanelPart with ViewContainer logic existing in ActivityBarPart with support for hiding a panel when empty Port view container logic from activitybarpart to panelpart #87444
• Abstract viewlet/panel creation away Register viewlet or panel when view container is registered #87246
• Use views service to move views between containers Enhance view service to support moving views #87461
• Support tracking view position on reload Add cache to viewdescriptorservice #88656
• Add generic move to panel / move to sidebar commands for movable views Add movable property to views and add toggle view commands #88823
• Fix issue with render being called twice for header actions. Fix Header Actions double rendering #88433
• Add movable property to views and enforce icon per view Add movable property to views and add toggle view commands #88823
• Update theming for views to support panel theming
• Move search view to new model migrate search to use new setting #89190
• Enable outline view to move Enabled movable Custom Views #89240
• Enable custom views to move Enabled movable Custom Views #89240
• Provide method for views to persist state during move
• Introduce ability to contribute a View to a Panel for extensions (should have its own item)

[sbatten]

First PR: #84642
From @sandy081

My feedback on the design:

I have a basic question on the design, why are you not using ViewDescriptor and ViewRegistry which is already an abstraction for all views (in Sidebar & Panel). There is also ViewContainer which can be used for the location of ViewDescriptor. Currently only sidebar views use view descriptors. So my suggestion would be to start moving Panels using view descriptors by creating a new View Container for each panel (similar to viewlets). This can allow moving views not only across side bar and panel but also across each viewlet. It will also give infrastructure for providing custom views in panels.

Current ViewRegistry already supports moving views from one view container to another.

vscode/src/vs/workbench/common/views.ts

Line 171 in 88c2100

moveViews(views: IViewDescriptor[], viewContainer: ViewContainer): void;

Views are updated in the Viewlets when views are added/removed/moved.

As said, currently viewlets use view descriptors and the first step would be to let panels also us it.

[sbatten]

Follow-up PR: #85096
From @sandy081

Note: We gonna use pane for split views.

Not sure if we are on right track. I see it is very confusing to have PanelViewlet which provides functionality of Viewlet and panes. This is restricting only Viewlets to extend this functionality because of the inheritance.

I would suggest to do following

1. Move panes specific functionality from PanelViewlet to a new class called PaneContainer.

2. Lets use composition instead of inheritance. Merge PanelViewlet into ViewContainerViewlet and let ViewContainerViewlet uses PaneContainer to manage views.

3. Move views specific functionality from ViewContainerViewlet into a new class called ViewContaier that extends PanelContainerWidget. This will make ViewContainerViewlet slick and contains only viewlet specific knowledge.

4. Use ViewContaier when moving Panels to view containers and views just like how ViewContainerViewlet does.

From @isidorn

After discussing with @sandy081 we drew this picture together to make things more clear for everybody.

Current state

Future state

Some notes:

• I like the idea of renaming Split View Panels to Pane.
• I agree with everything @sandy081 pointed out in the previous comment. The best place to start is on the left part of the picture - restructuring the viewlet infrastructure to make it align better with our future architecture. Changing Panels is step2
• Viewlet = Composite + Viewlet specifc knowledge
• ViewViewelet = Viewlet + ViewContainer (knowledge about registering new views)
• PaneContainer = A container around SplitView which can add panes, remove panes
• ViewContainer = PaneContainer + knowledge around view, registering views etc.
• PaneContainer and ViewContainer should probably be just merged into one class. We believe this will simplify things. This class cares about registergin views and forwarding everything to the SplitView

[sbatten]

@sandy081 @isidorn I got working the restructuring of design and ran into some problems. Below is the commit on my fork and I will summarize the steps taken so far.

https://github.com/sbatten/vscode/commit/9afe8b9e3d0d90cfd77f63764aa203a3be40c630

1. Rename PanelViewlet to ViewPaneContainer. This combines the concept of a PaneContainer and the knowledge about view registration. I just jumped to combining these concepts into one since we don't have a use to separate them currently.
2. Remove entends Viewlet from above ViewPaneContainer. This meant moving some functionality provided by Viewlet or its base classes into ViewPaneContainer but this was small and fairly simple.
3. Move all view logic from ViewContainerViewlet view/pane logic into ViewPaneContainer. This was a lot of code change and was fairly simple on the ViewPaneContainer side. However, on the ViewContainerViewlet side, we need to expose functions from ViewPaneContainer.

Problems

1. ViewContainerViewlet frequently needs to expose functionality from ViewPaneContainer that it used to have direct access to but doesn't anymore due to composition.

2. Derived classes of ViewContainerViewlet(e.g. SCMViewlet) assume they have access to the ViewPanes through inheritance. My feeling is that the majority of these implementations rely heavily on inheritance of the pane manipulation. In order to fix this, I feel I have to write a bunch of methods on ViewPaneContainer to expose this access.

3. The previous interface for ViewContainerViewlet used its access to create a standard of working with the ViewPanes. After exposing everything above in ViewPaneContainer, I will have to mirror this code in ViewContainerViewlet.

4. Lastly, everything I do for in the Viewlet class ends up being not Viewlet-specific but necessary to prevent every derived class from having to duplicate it. This means Panel will eventually have to do this.

Proposal

Perhaps I am overthinking it, so I thought I would share my progress but additionally share another proposal for design structure. Here is a diagram I drew.

1. FancyComposite (name not permanent) is a class that composes the ViewPaneContainer. It would take a ViewPaneContainer in its constructor. This allows Panel and Viewlet to benefit from the composition aspect without any duplication of code.

2. Panel and Viewlet are thin layers above FancyComposite to do anything specific.

3. ViewPaneContainer functions almost identically as it would in the previous proposal.

4. ExplorerViewPaneContainer and other view containers would extend ViewPaneContainer instead of a Viewlet or Panel. As stated above, I feel these view containers are doing more logic relevant to the ViewPaneContainer than the Viewlet itself. Additionally, ViewPaneContainer is just a collection of views which is the end-goal for these contributions.

Let me know what you think of this proposal or if you have ideas on addressing my issues with the original proposal.

[isidorn]

@sbatten thanks for the nice picture. Having the FancyComposite makes sense to me. And if the new architecture works better for you I fully support it.
However one thing which is not clear to me is why would the Explorer and other views extened the ViewPaneContainer. Why don't we make the ViewPaneContainer protected in the FancyComposite so that the Explorer and other viewlets / panels can access it once they inherit from Panel / Viewlet.
I am just worried if making the Explorer and all other views no longer a Viewet / Panel would prove to be much more work (for example in the ViewletService etc...) and would change something that a lot of us are used to: Explorer being a Viewlet.

Let me know what you think.

[sandy081]

Discussed with @isidorn and here is what we think:

I also agree if the new architecture works for you, go for it. I also liked the direction where it is going that there will be no viewlets and panels in future and its all views and view containers.

I think renaming current viewlets (Explorer, SCM and others) to view containers and extend ViewPaneContainer is less work, because these viewlets extend and override ViewPaneContainer behaviour more than providing viewlet behaviour. I am not much worried about what we are used to if we get a nice and clean design.

Also, this means that we will have single generic Viewlet implementation and all current viewlets register this Viewlet in viewlet descriptor registry with their respective ids. Once we reach this, I would recommend to move registering viewlet inside View container registry and make it an implementation detail. So it means, if I want to create a view container say explorer in activity bar, I would just register a view container providing my ExplorerViewContainer implementation. Eg:

Registry.as<IViewContainersRegistry>(ViewContainerExtensions.ViewContainersRegistry).registerViewContainer(
ExplorerViewContainer
'workbench.view.explorer', 
nls.localize('explore', "Explorer"),
ViewContainerLocation.Activitybar,
'codicon-files',
0)`

This shall register explorer ViewContainer and also internally a viewlet for explorer should be registered.

Note: As @isidorn said there is some code that is accessing these viewlets through viewlet service. So please make sure you take care of this by providing access to ViewContainer from Viewlet. Eg:

vscode/src/vs/workbench/contrib/files/browser/fileActions.ts

Lines 655 to 656 in a153842

	const explorerViewlet = await this.viewletService.openViewlet(VIEWLET_ID) as ExplorerViewlet;
	const explorerView = explorerViewlet.getExplorerView();

[sbatten]

Added support for ViewPaneContainer in a new Panel #87191