-
Notifications
You must be signed in to change notification settings - Fork 34.9k
try to ignore history on editor open for quicksearch #204880
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
Conversation
@andreamah sorry for the late response, I should have been the reviewer here, can you add me for future PRs in this area? I will post a follow up PR with some cleanup and thoughts. Thanks! |
@@ -545,6 +545,7 @@ export class TestMenuService implements IMenuService { | |||
export class TestHistoryService implements IHistoryService { | |||
|
|||
declare readonly _serviceBrand: undefined; | |||
declare shouldIgnoreActiveEditorChange: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not very safe, this would not actually put the property on the class as far as I know but just declare it as type to be there.
@@ -149,6 +149,7 @@ export class TestStorageService extends InMemoryStorageService { | |||
export class TestHistoryService implements IHistoryService { | |||
|
|||
declare readonly _serviceBrand: undefined; | |||
declare shouldIgnoreActiveEditorChange: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...same here
this.editorSequencer.queue(async () => { | ||
// disable and re-enable history service so that we can ignore this history entry | ||
this._historyService.shouldIgnoreActiveEditorChange = true; | ||
await this._editorService.openEditor({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opening an editor can fail with an error thrown and so this would leave the history in ignore-mode, without ever setting it back. At least a try/finally
is needed.
Related to #204853
Fixes #205135
cc @bpasero