Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
11 of 13 tasks
sbatten opened this issue Nov 20, 2019 · 10 comments
Closed
11 of 13 tasks

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

sbatten opened this issue Nov 20, 2019 · 10 comments
Assignees
Labels
feature-request Request for new features or functionality layout General VS Code workbench layout issues on-testplan
Milestone

Comments

@sbatten
Copy link
Member

sbatten commented Nov 20, 2019

Aggregating prior discussions from abandoned PRs.

@sbatten sbatten self-assigned this Nov 20, 2019
@sbatten
Copy link
Member Author

sbatten commented Nov 20, 2019

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.

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
Copy link
Member Author

sbatten commented Nov 20, 2019

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.
  1. Lets use composition instead of inheritance. Merge PanelViewlet into ViewContainerViewlet and let ViewContainerViewlet uses PaneContainer to manage views.
  1. 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.
  1. 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
WhatsApp Image 2019-11-19 at 11 21 33 (1)

Future state
WhatsApp Image 2019-11-19 at 11 21 33

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 sbatten added layout General VS Code workbench layout issues feature-request Request for new features or functionality labels Nov 20, 2019
@sbatten sbatten added this to the November 2019 milestone Nov 20, 2019
@sbatten
Copy link
Member Author

sbatten commented Nov 22, 2019

@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.

sbatten@9afe8b9

  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.

image

  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
Copy link
Contributor

isidorn commented Nov 22, 2019

@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
Copy link
Member

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:

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

@sbatten
Copy link
Member Author

sbatten commented Dec 17, 2019

Added support for ViewPaneContainer in a new Panel #87191

@sandy081
Copy link
Member

Introduce ability to contribute a View to a Panel

@sbatten Is not this supported by adding support -> for ViewPaneContainer in a new Panel

@sandy081
Copy link
Member

@sbatten Tracking adopting problems panel using views and view containers here - #87243

@lukaszpolowczyk
Copy link

lukaszpolowczyk commented Jan 13, 2020

Will it be possible to move views between sidebars?
For example, moving Breakpoints from "Debugg and Run" to "Explorer"?

Can you include it in your plans?

@sbatten sbatten changed the title Enable Views to move between sidebar and panel Enable Views to move between sidebar and panel (Part 1) Jan 30, 2020
@sbatten sbatten closed this as completed Jan 30, 2020
@sbatten
Copy link
Member Author

sbatten commented Jan 30, 2020

Work continued in #89729

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality layout General VS Code workbench layout issues on-testplan
Projects
None yet
Development

No branches or pull requests

4 participants