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

Investigate the impact of grid editor on extensions and API #51001

Closed
bpasero opened this issue Jun 2, 2018 · 9 comments
Closed

Investigate the impact of grid editor on extensions and API #51001

bpasero opened this issue Jun 2, 2018 · 9 comments
Assignees
Labels
api on-testplan workbench-editor-grid Grid layout issues in the editor area
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Jun 2, 2018

Some random notes now that grid editor layout has landed on master:

  • should we deprecate ViewColumn in favour of new API that exposes the grid system to extension authors?
  • if we decide to keep ViewColumn, how should it map to the grid system
    • should the order be by creation time (current solution)
    • should the order be by visual appearance (this would probably map best to our previous behaviour)
  • should we just introduce an OR-type for ViewColumn that is number so that the column is never undefined when > 3? in the current state viewcolumn will be undefined for any editor that opens in a position > 3 which is bad because extensions might have used this as a check to find out if the editor is in the editor area or not (e.g. panel, embedded editor)
  • we probably want to introduce a new dynamic ViewColumn that allows to open an editor to the side of the active one (ViewColumn.SIDE_BY_SIDE?) so that an extension can simply use that instead of having to make the math with absolute view columns
@bpasero bpasero added api workbench-editor-grid Grid layout issues in the editor area labels Jun 2, 2018
@bpasero bpasero added this to the June 2018 milestone Jun 2, 2018
@bpasero bpasero self-assigned this Jun 2, 2018
@attilah
Copy link

attilah commented Jun 6, 2018

I am not sure if it already exposed, but accessing the View configuration from code/extensions code AND support programmatic opening of either individual or a set of views with a preconfigured setup (think of 3D editors where you open up X, Y, Z, Camera views altogether as a "workspace".

Is this supported/considered as part of this feature?

Predefined constants for 1/1, 1/2, 1/3, 1/4 view sizes which would dynamically be evaluated when someone refers to it AND would stick to the view, so in case of a window resize each view's size would be recalculated based on this parameter.

@jpoon
Copy link

jpoon commented Jun 14, 2018

For vscodevim, each editor has it's own state (undo stack, history, cursor position, etc), and every time the active editor changes, we load the appropriate state. We maintain a map of all the states with the key being fileName + viewColumn such that if the same file is open across two different panels, it will have separate state.

Lately, we've been running into an assortment of issues with the extension loading the wrong state because: (1) file name has changed, (2) user moved the editor to a different panel, etc. With the introduction of grids, I think this will exacerbate the problem. (ref: VSCodeVim/Vim#2688)

Is there a better alternative to generating a unique ID for the editor? If we continue using the filename + position in the editor as the unique identifier, we'll need better events for when a user moves the window to a different viewColumn/gridPosition as the current API (onDidChangeTextEditorViewColumn) only fires on certain view column changes (#35602).

@bpasero
Copy link
Member Author

bpasero commented Jun 14, 2018

@jpoon we will still maintain compatibility and have a unique ViewColumn per editor (by just counting from left to right and up to down) even in grid world. When the user moves the editor group to another location, you will still get an event as before.

We never supported this event when moving individual editors though and I don't think grid will make it any different. For us, moving an editor is closing it at the originating editor group and opening it at the target. it is not even the same editor widget underneath.

So it seems to me you do not have a requirement for a row/column specific view column? rather as before, a unique position for each editor?

bpasero added a commit that referenced this issue Jun 14, 2018
@bpasero
Copy link
Member Author

bpasero commented Jun 14, 2018

Landed via #51876

@jpoon
Copy link

jpoon commented Jun 14, 2018

So it seems to me you do not have a requirement for a row/column specific view column? rather as before, a unique position for each editor?

That is correct. We need a unique ID for each editor and we are generating that based off it's position. Hence, with the introduction of the grid, we'd need something similar or... if there was some other way to garner uniqueness that would be optimal.

@bpasero
Copy link
Member Author

bpasero commented Jun 15, 2018

@jpoon you can use textEditor.viewColumn to get a unique position in the grid.

@jpoon
Copy link

jpoon commented Jun 15, 2018

If we continue to generate a unique ID based off it's position, we'd need to update that ID if the textEditor position in the grid changes. Will there be an event that an extension can register to be notified of such changes? The behaviour of the existing onDidChangeTextEditorViewColumn event is suboptimal for this use case as it only seems to fire on very specific view column changes as you noted in #35602.

@bpasero
Copy link
Member Author

bpasero commented Jun 15, 2018

@jpoon the event will fire if:

  • you move the group (containing all editors) to another position in the grid
  • you close a group and as a result the position of other groups changes (e.g. close a group to the left of the current one)

We do not fire an event if you drag a single editor tab out of a group into another group. But this behaviour is totally unrelated to us moving to a grid model, it has always been like that for the reasons I mentioned earlier.

@jpoon
Copy link

jpoon commented Jun 15, 2018

We do not fire an event if you drag a single editor tab out of a group into another group.

Maybe this belongs in a separate issue, but can I request we get an event for just this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api on-testplan workbench-editor-grid Grid layout issues in the editor area
Projects
None yet
Development

No branches or pull requests

3 participants