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

Have a good way to redirect open request into the merge editor #153110

Closed
jrieken opened this issue Jun 24, 2022 · 4 comments · Fixed by #154352
Closed

Have a good way to redirect open request into the merge editor #153110

jrieken opened this issue Jun 24, 2022 · 4 comments · Fixed by #154352
Assignees
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders merge-editor-workbench workbench-editors Managing of editor widgets in workbench window
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Jun 24, 2022

continues 15a420f and 15a420f#commitcomment-76896537

The merge editor integrates with the outside world via getControl. This make outline, breadcrumbs, breakpoint revealing/hitting (?) etc work. A hack was added to make sure open requests are redirected into the merge editor, e.g clicking on an outline entry should not open a separate editor, likewise go-to-def etc... I know that the workbench has built-in knowledge of the diff-editor for similar cases but the merge editor is a contribution and needs a more offical way of doing this...

@jrieken jrieken added debt Code quality issues workbench-editors Managing of editor widgets in workbench window merge-editor-workbench labels Jun 24, 2022
@bpasero bpasero added this to the July 2022 milestone Jun 25, 2022
@bpasero
Copy link
Member

bpasero commented Jun 25, 2022

Yeah to clarify what we do: there is a ICodeEditorService with a method openCodeEditor that somewhat serves as a replacement for IEditorService.openEditor. This service is being used from all contributions within monaco code editor as well as some workbench places. It encodes the knowledge about a diff editor to delegate the open request into the diff editor and not open a new editor.

I am not sure if we should use the editor resolver API for this or rather extend the code editor service to make it aware of text editors that have multiple sides where one is the "main" side. I think I lean towards the latter.

bpasero referenced this issue Jun 25, 2022
…orks (knows the editor/model to operate on). This opens a can of worms as other things start to work, e.g outline and breakcrumbs and that requires some "redirect magic" to ensure opening a symbol from outline/breakcrumbs stays within the merge editor...

fyi @bpasero @lramos15 I dynamically register/unregister an editor with the editor resolver service which I think is pretty bad but the only "contained/local" way I could find
@jrieken
Copy link
Member Author

jrieken commented Jul 6, 2022

I am not sure if we should use the editor resolver API for this or rather extend the code editor service to make it aware of text editors that have multiple sides where one is the "main" side. I think I lean towards the latter.

I like that. Started the work in joh/issue153110. Things look quite promising but I am hitting a dead-end with the references viewlet. It uses normal, extension contributed, resource-opening and I don't really know how to intercept that. For (symbols) outline it can use the current code editor which works nicely

@jrieken
Copy link
Member Author

jrieken commented Jul 7, 2022

Things look quite promising but I am hitting a dead-end with the references viewlet.

Same problem already exists with the normal diff editor and references view. @bpasero what do you think? Do have a nice way to tackle this? Are we cool with having the same bug with the merge editor? I guess yes

Screen.Recording.2022-07-07.at.12.00.21.mov

@bpasero
Copy link
Member

bpasero commented Jul 7, 2022

Yeah I like your approach and think behaving same as diff editor is OK, in fact I have never suffered from this issue in my day to day use and also did not get users complaining about this so far, so I also think it is OK to 🚢

@VSCodeTriageBot VSCodeTriageBot added the unreleased Patch has not yet been released in VS Code Insiders label Jul 8, 2022
@VSCodeTriageBot VSCodeTriageBot added insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Jul 11, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Aug 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders merge-editor-workbench workbench-editors Managing of editor widgets in workbench window
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants