Skip to content

Refresh on visible #1690

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

Merged
merged 2 commits into from
Jan 11, 2019
Merged

Refresh on visible #1690

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
64894e5
refresh on visible
TylerLeonhardt Jan 10, 2019
345dad4
Address feedback
TylerLeonhardt Jan 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions src/features/GetCommands.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,23 @@ export class GetCommandsFeature implements IFeature {
private command: vscode.Disposable;
private languageClient: LanguageClient;
private commandsExplorerProvider: CommandsExplorerProvider;
private commandsExplorerTreeView: vscode.TreeView<Command>;

constructor(private log: Logger) {
this.command = vscode.commands.registerCommand("PowerShell.RefreshCommandsExplorer",
() => this.CommandExplorerRefresh());
this.commandsExplorerProvider = new CommandsExplorerProvider();
vscode.window.registerTreeDataProvider("PowerShellCommands", this.commandsExplorerProvider);

this.commandsExplorerTreeView = vscode.window.createTreeView<Command>("PowerShellCommands",
{ treeDataProvider: this.commandsExplorerProvider });

// Refresh the command explorer when the view is visible
this.commandsExplorerTreeView.onDidChangeVisibility( (e) => {
if (e.visible) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What fires this onDidChangeVisibility callback? If it fires every time you switch away from the PS tree view and then back to it, that seems like we'd be refreshing too often. Maybe the "initial" population of the tree is just that - initial and one-time. There is a Refresh button so the user can refresh whenever they want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think that that would really be that often? I would think people expect that when you click on it, it would refresh.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess as long as the perf of getting all the commands, sorting them and removing duplicates stays fast - it's probably not a big deal to auto-refresh. But I'm a little PSES perf-paranoid these days. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it will be fine. PowerShell will cache the results after the first Get-Command.

Plus users that don't use the command explorer will not see any perf hit

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it will be fine. PowerShell will cache the results after the first Get-Command.

Will it? It takes over a full second for every invocation of Get-Command on my machine. I'm also not particularly fond of sending messages that big unnecessarily (nor the giant log messages, but that's not a great reason 😉)

I don't feel all that strongly about it though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I personally think that we should ship this since it's only run for users of the command explorer - and requires human interaction to be run (rather than just randomly running).

If it does become a problem for perf for those users, aka after feedback, we can take out the auto refreshing and rely solely on the refresh button.

Would that be alright with you guys?

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe @rjmholt can also give his thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

My main concern is a message storm DoS-ing PSES because of a callbacks from this piling up, plus creating the expectation with users that we can magically keep the command list perfectly up to date without them needing to think about it.

If a "refresh" button is too much work, we could maybe add a minimum time delay between refreshes (like 10s)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok @TylerLeonhardt has explained this to me and I didn't understand what this PR changes. Here's my understanding:

  • There is not and never will be an auto-refresh.
  • The refresh button is the best way to renew your command list.
  • This PR only adds refreshing when you reopen the Command Explorer pane (such as for the first time, or when switching back from a different pane)
  • That is, this PR only adds refreshing when the user (re)opens the command explorer pane.
  • Theoretically a user could DoS themselves by clicking into and out of the pane a lot, but they could do that now with the refresh button (and more quickly).

So, I think the current PR is the best approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

yep - I should have added that bullet list to the issue. Thanks for clarifying that, @rjmholt.

I'd like to get @rkeithhill's closure on this before merging.

this.CommandExplorerRefresh();
}
});

vscode.commands.registerCommand("PowerShell.InsertCommand", (item) => this.InsertCommand(item));
}

Expand All @@ -42,12 +53,14 @@ export class GetCommandsFeature implements IFeature {

public setLanguageClient(languageclient: LanguageClient) {
this.languageClient = languageclient;
vscode.commands.executeCommand("PowerShell.RefreshCommandsExplorer");
if (this.commandsExplorerTreeView.visible) {
vscode.commands.executeCommand("PowerShell.RefreshCommandsExplorer");
}
}

private CommandExplorerRefresh() {
if (this.languageClient === undefined) {
this.log.writeAndShowError(`<${GetCommandsFeature.name}>: ` +
this.log.writeVerbose(`<${GetCommandsFeature.name}>: ` +
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this log message isn't super accurate Unable to instantiate, …. It probably should be more like Unable to send getCommand request, ….

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed!

"Unable to instantiate; language client undefined.");
return;
}
Expand Down