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.

Sign up for GitHub

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

Jump to bottom

editor service cleanup #127287

Merged
merged 5 commits into from Jun 28, 2021
Merged

editor service cleanup #127287

merged 5 commits into from Jun 28, 2021

Conversation

bpasero
Copy link
Member

@bpasero bpasero commented Jun 28, 2021 •
edited

This is a follow up cleanup and some fixes for editor service, will leave comments inline to explain.

High level:

  • try to address some issues reported
  • make doResolveEditor much simpler, explicitly return EditorActivation but only use it from openEditor method as we used to

@bpasero bpasero added this to the June 2021 milestone Jun 28, 2021
@bpasero bpasero requested a review from lramos15 June 28, 2021 05:45
@bpasero bpasero self-assigned this Jun 28, 2021
lramos15
lramos15 approved these changes Jun 28, 2021
View reviewed changes
Copy link
Member

@lramos15 lramos15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me.

  1. Should we have tests for replaceEditors? I don't suspect it to be much different than openEditor as they follow the same path for the most part but it might be good to cover the three avenues.

  2. Should activation be mixed into the IEditorInputWithOptions if present and then we reduce the return signature of doResolveEditor

src/vs/workbench/services/editor/browser/editorService.ts Show resolved Hide resolved
src/vs/workbench/services/editor/test/browser/editorService.test.ts
disposables.add(accessor.editorOverrideService.registerEditor(
'*.editor-service-override-tests',
{ id: 'editorServiceOverrideTests', label: 'Label', priority: RegisteredEditorPriority.exclusive },
{ canHandleDiff: true },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super related to this PR but what yours thoughts on abandoning canHandledDiff since we don't have a canHandleUntitled as we just check for the factory function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah awesome idea, please do that 👍

@bpasero
Copy link
Member Author

bpasero commented Jun 28, 2021

Should we have tests for replaceEditors?

I opened #127305 for test coverage in openEditor, openEditors, replaceEditors.

Should activation be mixed into the IEditorInputWithOptions if present and then we reduce the return signature of doResolveEditor

We cannot, because not every call returns an editor with options.

@bpasero bpasero merged commit 2c63b5d into main Jun 28, 2021
@bpasero bpasero deleted the ben/editor-service-cleanup branch June 28, 2021 13:49
This was referenced Jun 28, 2021
Closed
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lramos15 lramos15 lramos15 approved these changes

Assignees

@bpasero bpasero

Labels
None yet
Projects
None yet
Milestone
June 2021
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@bpasero @lramos15