Skip to content

Add status bar notification when PowerShell is running #1227

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

Merged

Conversation

rjmholt
Copy link
Contributor

@rjmholt rjmholt commented Mar 13, 2018

Coupled with PowerShellEditorServices #632, this adds a status bar item for when PowerShell is executing, indicating to the user that something is running.

Demo:
Execution status update in PowerShell extension

break;

// If the execution has stopped, destroy the previous notification
case ExecutionStatus.Completed:
Copy link
Member

Choose a reason for hiding this comment

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

Does tslint not yell at you for missing the break statement in cases? That's surprising to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't think so. It's set to error on any linting warnings, so it shouldn't have compiled if it did. I'll add it in though -- I recall thinking about it (and wondering why tslint didn't care), but I must have forgotten to put it in.

Copy link
Contributor Author

@rjmholt rjmholt Mar 15, 2018

Choose a reason for hiding this comment

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

Oh I see, you mean tslint shouldn't allow fallthrough? I would have put default but I want to ignore ExecutionStatus.Pending. So fallthrough seemed to match my needs best. Do we want to avoid that? I can change if so 😄

Copy link
Member

Choose a reason for hiding this comment

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

eh. I'm fine with fallthrough but I know it's controversial so I was surprised :)

@TylerLeonhardt
Copy link
Member

VSCode added a progress api not too long ago. I was messing with it to fix Write-Progress not showing up (never finished that work though).

It seemed like it works pretty well so we probably don't need to use the custom class of ours, we can use the new API (supported in 1.12 and higher which isn't a problem at all).

Eventually, for anything "progress" related, we can stick to using this API.

Thoughts?

Oh and the text in the progress API goes exactly where @rjmholt's text is in that gif.

@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Mar 15, 2018

Here's an example of the progress API:

vscode.window.withProgress({ location: vscode.ProgressLocation.Window, title: 'hello'}, p => {
            return new Promise((resolve, reject) => {
                p.report({message: 'Start working...' });
                let count= 0;
                let handle = setInterval(() => {
                    count++;
                    p.report({message: 'Worked ' + count + ' steps' });
                    if (count >= 10) {
                        clearInterval(handle);
                        resolve();
                    }
                }, 1000);
            });
        });

@rjmholt
Copy link
Contributor Author

rjmholt commented Mar 15, 2018

The text is the part I'm least happy with -- just not familiar with VSCode APIs. But if we could get a richer indicator or a progress bar or a colour change (something you would notice in the corner of your eye), that would make me happier.

@rjmholt
Copy link
Contributor Author

rjmholt commented Mar 19, 2018

Might possibly be worth refactoring in light of this SO answer.

Currently I export the resolve out of the Promise to be called later, but unsure if that is bad. We don't really have a case to use reject here yet.

Thoughts?

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

Just a quick question and a few nitpicks :)

import { LanguageClient, NotificationType, RequestType } from "vscode-languageclient";
import { ICheckboxQuickPickItem, showCheckboxQuickPick } from "../controls/checkboxQuickPick";
import { IFeature } from "../feature";

import * as AnimatedStatusBar from "../controls/animatedStatusBar";
Copy link
Member

Choose a reason for hiding this comment

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

nit: we don't need this anymore, right?

@@ -3,12 +3,17 @@
*--------------------------------------------------------*/

import vscode = require("vscode");
import { Disposable } from "vscode-jsonrpc";
Copy link
Member

Choose a reason for hiding this comment

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

nit: we don't need this anymore, right?

case ExecutionStatus.Completed:
case ExecutionStatus.Aborted:
case ExecutionStatus.Failed:
this.resolveStatusBarPromise();
Copy link
Member

Choose a reason for hiding this comment

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

if for whatever reason... vscode powershell doesn't get the ExecutionStatus.Running message but it gets the ExecutionStatus.Completed message... will this line throw an error if this.resolveStatusBarPromise takes the default value?

@TylerLeonhardt
Copy link
Member

So it looks like you could change the color if we could get the StatusBarItem object of the progress:
https://code.visualstudio.com/docs/extensionAPI/vscode-api#_a-namestatusbaritemaspan-classcodeitem-id1139statusbaritemspan

but I'm not sure if that's possible.

Add a notification listener for execution status events from the
Language Server (Running, Completed, etc) and alter the status bar to
inform the user when PowerShell is executing a script.
@rjmholt rjmholt force-pushed the execution-status-notifications branch from f407709 to 13ef63b Compare March 19, 2018 22:47
Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I'll leave it open for a day or so to see if anyone one else wants to take a look.

@rkeithhill
Copy link
Contributor

My only thought on this was that perhaps this execution status should be displayed as part of the current PowerShell status bar info (version / session menu). That said, that status disappears when you select a file that isn't a PowerShell script file.

@TylerLeonhardt
Copy link
Member

@rkeithhill that, and I think we wouldn't be able to use the Progress API because the Progress API adds a little spinner. That'd get really annoying if that was there constantly.

@TylerLeonhardt
Copy link
Member

Long live the spinner! 🔁

@TylerLeonhardt TylerLeonhardt merged commit 945b126 into PowerShell:master Mar 20, 2018
@rjmholt rjmholt deleted the execution-status-notifications branch December 11, 2019 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants