microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
163.99k stars 29.19k forks source link

Editors: revisit IEditorPane extends IPanel #91945

Closed bpasero closed 4 years ago

bpasero commented 4 years ago

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 https://github.com/microsoft/vscode/pull/91943

isidorn commented 4 years ago

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 commented 4 years ago

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

sandy081 commented 4 years ago

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

https://github.com/microsoft/vscode/blob/516d459482c33abfd44300acfe074553a718501f/src/vs/workbench/contrib/output/browser/outputView.ts#L34-L37

Currently following three instances are extending or implementing IPanel

https://github.com/microsoft/vscode/blob/516d459482c33abfd44300acfe074553a718501f/src/vs/workbench/browser/panel.ts#L17-L20

https://github.com/microsoft/vscode/blob/516d459482c33abfd44300acfe074553a718501f/src/vs/workbench/common/editor.ts#L67

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

isidorn commented 4 years ago

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

sandy081 commented 4 years ago

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

bpasero commented 4 years ago

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

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.

sbatten commented 4 years ago

@bpasero

sandy081 commented 4 years ago

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

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

sbatten commented 4 years ago

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 commented 4 years ago

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.