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

Persist settings search when reloaded #59704

Merged
merged 2 commits into from Oct 3, 2018

Conversation

Projects
None yet
3 participants
@usaatyuk
Contributor

usaatyuk commented Sep 30, 2018

Implements #57289

@msftclas

This comment has been minimized.

msftclas commented Sep 30, 2018

CLA assistant check
All CLA requirements met.

@usaatyuk usaatyuk changed the title from persist settings search when reloaded to Persist settings search when reloaded Sep 30, 2018

@roblourens roblourens added this to the October 2018 milestone Oct 1, 2018

// Make sure that all extensions' settings are included in search results
const cachedState = this.editorMemento.loadState(this.group, this.input);
if (cachedState && cachedState.searchQuery) {
this.triggerSearch(cachedState.searchQuery);

This comment has been minimized.

@roblourens

roblourens Oct 1, 2018

Member

I'm not totally sure, but since this will trigger a render when it's done, can we avoid calling renderTree here?

Also, is this always safe to call before settingsTreeModel has been created?

@roblourens

This comment has been minimized.

Member

roblourens commented Oct 1, 2018

It looks like the goal is to only persist the state when the editor was closed while visible, but for some reason that's not working for me. It always persists the state. Does it work for you?

@@ -188,13 +188,6 @@ export class SettingsEditor2 extends BaseEditor {
this.updateStyles();
}
public setEditorVisible(visible: boolean, group: IEditorGroup): void {
if (!visible) {
this.editorMemento.clearState(this.input, this.group);

This comment has been minimized.

@usaatyuk

usaatyuk Oct 1, 2018

Contributor

Apparently, this.input is already cleared there, so clearState doesn't do anything

@@ -248,6 +241,7 @@ export class SettingsEditor2 extends BaseEditor {
clearInput(): void {
this.inSettingsEditorContextKey.set(false);
this.editorMemento.clearState(this.input, this.group);

This comment has been minimized.

@usaatyuk

usaatyuk Oct 1, 2018

Contributor

If we clean the state just before clearing input, it works, and has basically the same effect as if in setEditorVisible (clear the state when settings are invisible)

@@ -844,7 +858,11 @@ export class SettingsEditor2 extends BaseEditor {
if (this.settingsTreeModel) {
this.settingsTreeModel.update(resolvedSettingsRoot);
return this.renderTree(undefined, forceRefresh);

This comment has been minimized.

@usaatyuk

usaatyuk Oct 1, 2018

Contributor

Calling this before settingsTreeModel has been created doesn't seem to cause any issue, but I put it here just in case.

@usaatyuk

This comment has been minimized.

Contributor

usaatyuk commented Oct 1, 2018

Now it should work as intended.

@roblourens

Sorry for the delay, this looks great! Thanks for the PR!

@roblourens roblourens merged commit 90fc4c9 into Microsoft:master Oct 3, 2018

1 of 2 checks passed

VS Code #20181002.39 failed
Details
license/cla All CLA requirements met.
Details

@usaatyuk usaatyuk deleted the usaatyuk:persistent-settings-search branch Oct 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment