Refresh on visible #1690
Refresh on visible #1690
Conversation
TylerLeonhardt
requested review from
rkeithhill,
rjmholt and
SeeminglyScience
src/features/GetCommands.ts
Outdated
| } | ||
|
|
||
| private CommandExplorerRefresh() { | ||
| if (this.languageClient === undefined) { | ||
| this.log.writeAndShowError(`<${GetCommandsFeature.name}>: ` + | ||
| this.log.writeVerbose(`<${GetCommandsFeature.name}>: ` + |
There was a problem hiding this comment.
Seems like this log message isn't super accurate Unable to instantiate, …. It probably should be more like Unable to send getCommand request, ….
|
|
||
| // Refresh the command explorer when the view is visible | ||
| this.commandsExplorerTreeView.onDidChangeVisibility( (e) => { | ||
| if (e.visible) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Do you think that that would really be that often? I would think people expect that when you click on it, it would refresh.
There was a problem hiding this comment.
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. :-)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
maybe @rjmholt can also give his thoughts?
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Also, I won't block on you, but @corbob if you wanna review this PR :) |
PR Summary
previously we were refreshing at startup every time. This change makes it so that we refresh only when the command explorer becomes visible.
I've also made an error to verbose since we should fail silently.
cc @corbob
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets.Please mark anything not applicable to this PR
NA.WIP:to the beginning of the title and remove the prefix when the PR is ready