Skip to content

Conversation

andreamah
Copy link
Contributor

@andreamah andreamah commented Feb 10, 2024

Related to #204853
Fixes #205135

cc @bpasero

@andreamah andreamah self-assigned this Feb 10, 2024
@andreamah andreamah changed the title try to ignore history on editor open try to ignore history on editor open for quicksearch Feb 13, 2024
@andreamah andreamah marked this pull request as ready for review February 13, 2024 18:27
@vscodenpa vscodenpa added this to the February 2024 milestone Feb 13, 2024
@andreamah andreamah merged commit da8f671 into main Feb 13, 2024
@andreamah andreamah deleted the grumpy-beaver branch February 13, 2024 19:22
@bpasero
Copy link
Member

bpasero commented Feb 14, 2024

@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;
Copy link
Member

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;
Copy link
Member

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({
Copy link
Member

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.

@microsoft microsoft locked and limited conversation to collaborators Jun 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quick Search should omit history
4 participants