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

Editors: revisit IEditorPane extends IPanel #91945

Closed
bpasero opened this issue Mar 3, 2020 · 10 comments
Closed

Editors: revisit IEditorPane extends IPanel #91945

bpasero opened this issue Mar 3, 2020 · 10 comments
Assignees
Labels
debt Code quality issues layout General VS Code workbench layout issues
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Mar 3, 2020

For a long time now, the workbench editors have extended IPanel because they can live both in the editor part as well as the panel (e.g. outline uses an editor).

With the advent of view panes that can flow between side bar, panel and probably other places in the future I think we need to revisit what the workbench editor should extend from after all.

I can imagine that we will allow to e.g. have a output panel and some other view to coexist in the same container, so I think extending from IPanel does not make a lot of sense anymore for editors. It should probably be the same thing that views extend from?

//cc @isidorn @sandy081

PS: I just pushed a change that renamed IEditor to IEditorPane via #91943

@bpasero bpasero added debt Code quality issues under-discussion Issue is under discussion for relevance, priority, approach layout General VS Code workbench layout issues labels Mar 3, 2020
@isidorn
Copy link
Contributor

isidorn commented Mar 3, 2020

Yeah, we should drop that since Output is the only one using it. Output could use compoisiton and just have an instance of an Editor which makes more sense imho.

@bpasero
Copy link
Member Author

bpasero commented Mar 3, 2020

In am fine either way if it turns out we do not gain a lot from this inheritance.

@sandy081
Copy link
Member

sandy081 commented Mar 3, 2020

I moved Output view and adopted to ViewPane last milestone. It is now using composition

export class OutputViewPane extends ViewPane {
private readonly editor: OutputEditor;
private channelId: string | undefined;

Currently following three instances are extending or implementing IPanel

export abstract class Panel extends Composite implements IPanel { }
export abstract class PaneCompositePanel extends PaneComposite implements IPanel { }

export interface IEditorPane extends IPanel {

I am aware of first two and not sure whey editor extends panel and it is still needed or not.

@isidorn
Copy link
Contributor

isidorn commented Mar 3, 2020

Since the output now uses composition I think the inheritence is no longer needed and we can remove it.

@sandy081
Copy link
Member

sandy081 commented Mar 3, 2020

@bpasero Moving this back to you. You can remove this hierarchy from IEditorPane.

@sandy081 sandy081 removed the under-discussion Issue is under discussion for relevance, priority, approach label Mar 3, 2020
@bpasero
Copy link
Member Author

bpasero commented Mar 3, 2020

@sandy081 @sbatten unclear to me though what editors should extend from, I am not up to speed with latest workbench layout development. I see the following types but wonder about when to use which:

  • IPanel & Panel
  • IView & View
  • IComposite & Composite
  • PaneComposite (?)
  • Pane

Is there some wiki that explains the relationship?

My current thinking is that IEditorPane should probably extend IComposite to keep those methods available. My understanding is that IComposite is the entire container of views either in the sidebar or panel area.

@bpasero bpasero added this to the March 2020 milestone Mar 3, 2020
@sbatten
Copy link
Member

sbatten commented Mar 3, 2020

@bpasero

  • Panel and Viewlet are roughly equivalent since IPanel adds nothing to IComposite.
  • IComposite is just a view area in the sidebar or panel. In the old world, viewlets and panels diverged after this point where panels only allowed a single view and viewlets inherited from a view container class.
  • IPaneComposite is the generalized version of a multi-pane/view capable composite. It uses composition to hold a ViewPaneContainer which does all the split-view functionality needed for multiple views in the sidebar. As of this release, all panels are using the same thing but they are limited to one view in their split view.
  • Pane is just a type of IView that is used in the sidebar (and soon the panel) to have a header and body with actions.

@sandy081
Copy link
Member

sandy081 commented Mar 3, 2020

We had a design picture in the beginning representing viewlets, panels and view panes but I do not think we wrote it down any where. But @sbatten summarised it well. Some other notes I have is

  • Composite is the base that all workbench parts shall extend (Viewlet, Panel, Editor)

  • Pane is a component with an header and body and features like collapsible, resizable, draggable etc. All those panes (views, editor group) with these features extend Pane. Pane implements IView interface in SplitView. To reduce the confusion, we shall rename IView to IPane and remove the view terminology completely in split views (may be call splitpanes )?

  • Our end goal is to remove Viewlet and Panel and replace them with PaneComposite (shall we call it ViewPaneComposite?). Not sure if this is feasible but ideal to have.

To summarise, composites and panes are base classes that everybody shall extend according to their requirements.

So in your case, editor shall extend Composite for now.

Here is my thinking going forward - Every workbench part shall extend PaneComposite with unique ViewContainerLocation. This will enable following

  • All workbench parts are composition of view panes
  • View panes can float around composites easily. (Say Terminal can be moved from Panel location to Editor location).
  • Extensions can contribute web views to sideBar or Panel location and allow users to move them to editor location.
  • Makes workbench layout flexible. Easy to define and create more locations say - another panel on the right.

@sbatten
Copy link
Member

sbatten commented Mar 3, 2020

To reduce the confusion, we shall rename IView to IPane and remove the view terminology completely in split views (may be call splitpanes )?

I don't think this is right since GridView is also using split view and workbench parts are not Panes.

@bpasero bpasero closed this as completed in 3124d49 Mar 4, 2020
@bpasero
Copy link
Member Author

bpasero commented Mar 4, 2020

So in your case, editor shall extend Composite for now.

I pushed that.

Should there be a follow up debt issue to clean this up as suggested? At this point I find there should probably not be any difference between a viewlet and a panel, both are the same in my mind, just at different locations. With the support for panels having multiple views, this is quite obvious.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues layout General VS Code workbench layout issues
Projects
None yet
Development

No branches or pull requests

4 participants