Add optional parameter to showOptions called newEditorGroup#107555
Add optional parameter to showOptions called newEditorGroup#107555tomerstav wants to merge 4 commits intomicrosoft:masterfrom
Conversation
|
|
|
I suspect this is not the correct fix.
|
jrieken
left a comment
jrieken
left a comment
There was a problem hiding this comment.
This seems wrong, please explain those changes
I misunderstood how the code was working. I initially thought that the implementation for adding a new view col was expecting non zero based indexing as explained in the docs. https://code.visualstudio.com/api/references/vscode-api#ViewColumn My changes fixed the issue but I can see from the failed integration testing and comments that this isn't the right solution. I will convert this pr into a draft and attempt this again. |
tomerstav
force-pushed
the
view-col-assignment
branch
from
5ff2599 to
cd69b98
Compare
tomerstav
force-pushed
the
view-col-assignment
branch
from
cd69b98 to
9b35b0e
Compare
tomerstav
changed the title
bpasero
left a comment
bpasero
left a comment
There was a problem hiding this comment.
Overall I think this can break existing extensions because we used to return SIDE_GROUP which is defined as opening to the side of the currently active group and NEW_GROUP is entirely different, opening a new group after the last group by appearance.
|
|
||
| private newSideRightGroup(): IEditorGroup { | ||
| const lastGroupIndex = this.editorGroupService.groups.length - 1; | ||
| return this.editorGroupService.addGroup(this.editorGroupService.groups[lastGroupIndex], GroupDirection.RIGHT); |
There was a problem hiding this comment.
The group direction should not be hardcoded but taken from user settings via:
const direction = preferredSideBySideGroupDirection(this.configurationService);| } | ||
|
|
||
| private newSideRightGroup(): IEditorGroup { | ||
| const lastGroupIndex = this.editorGroupService.groups.length - 1; |
There was a problem hiding this comment.
That is not a safe way to find the last group, I think you want:
this.editorGroupService.getGroups(GroupsOrder.GRID_APPEARANCE)| return neighbourGroup; | ||
| } | ||
|
|
||
| private newSideRightGroup(): IEditorGroup { |
There was a problem hiding this comment.
| private newSideRightGroup(): IEditorGroup { | |
| private newSideGroup(): IEditorGroup { |
There was a problem hiding this comment.
Not applicable anymore. New approach.
|
|
||
| // Group: New group | ||
| else if (group === NEW_GROUP) { | ||
| targetGroup = this.newSideRightGroup(); |
There was a problem hiding this comment.
| targetGroup = this.newSideRightGroup(); | |
| targetGroup = this.newSideGroup(); |
|
|
||
| type CachedEditorInput = ResourceEditorInput | IFileEditorInput | UntitledTextEditorInput; | ||
| type OpenInEditorGroup = IEditorGroup | GroupIdentifier | SIDE_GROUP_TYPE | ACTIVE_GROUP_TYPE; | ||
| type OpenInEditorGroup = IEditorGroup | GroupIdentifier | SIDE_GROUP_TYPE | ACTIVE_GROUP_TYPE | NEW_GROUP_TYPE; |
There was a problem hiding this comment.
I find NEW_GROUP_TYPE is not fully expressing what it does. Based on the implementation it seems to create a new group after the last group by appearance. If so I think the name should reflect that, because as a user of the API one might think that it opens a new group from the currently active group and not the last group.
There was a problem hiding this comment.
I will remove this with new approach.
tomerstav
changed the title
bpasero
left a comment
bpasero
left a comment
There was a problem hiding this comment.
Why is newEditorGroup a boolean and not another type similar to SIDE_GROUP_TYPE? I would rather introduce something like NEW_GROUP_TYPE.
We need to keep track of the view column number. Groups internally was keeping track of that number. I have created a NEW_GROUP_TYPE and converted newEditorGroup internally to keep track of the view column number instead. |
|
It looks like #106973 is now closed with a workaround and I am not happy with the change in this PR to be honest. I would suggest to not push this forward. |
bpasero
left a comment
bpasero
left a comment
There was a problem hiding this comment.
Rejected as per #107555 (comment)
Could you explain why you are not happy with this PR? I would like to learn from this. |
|
I actually started to review this PR but then ended up changing a lot of the code myself, almost rewriting it. In principle I am not opposed to the idea of having a new group type in editor land, but it touched too many areas in the end and I am not convinced we actually still need this? |
I think the work around is not a good long term solution. But fair enough I will close the PR. |
4 participants
The issue was that when a position for a view column that does not exist was converted into editor group it would be marked as side group. Side groups create a new column based on the neighbour of the active group you are on. If it does not exist it will create a new group. So for example if you had two view columns open and you clicked the first one then the first view column is the active group. The second view column is the neighbour of the active group. And the new column would open inside the second view column. But if you clicked the second view column. The active group is the second view column. The neighbour doesn't exist. So the new view column will be a new third group.
My implementation adds an optional parameter to showOptions called newEditorGroup. When set to true, a new view column is created in a new editor group at the position specified.
Examples:
const panel = vscode.window.createWebviewPanel("", "", {viewColumn:1, newEditorGroup: true});const panel = vscode.window.createWebviewPanel("", "", {viewColumn:3, newEditorGroup: true});This PR fixes #106973