Skip to content
This repository was archived by the owner on Nov 18, 2022. It is now read-only.

accept window/progress instead of beginBuild/diagnosticEnd #231

Merged
merged 4 commits into from
Mar 3, 2018
Merged

accept window/progress instead of beginBuild/diagnosticEnd #231

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fffe938
accept window/progress for progress
algesten Jan 13, 2018
ed6896c
Less "jumpy" spinner
algesten Jan 14, 2018
a3e9198
rewrite ternary to if statements
algesten Feb 26, 2018
36c541b
use Set instead of hash
algesten Mar 2, 2018
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
44 changes: 36 additions & 8 deletions src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ function makeRlsProcess(): Promise<child_process.ChildProcess> {
});

return childProcessPromise.catch(() => {
window.setStatusBarMessage('RLS could not be started');
stopSpinner('RLS could not be started');
return Promise.reject(undefined);
});
}
Expand All @@ -151,7 +151,7 @@ function startLanguageClient(context: ExtensionContext)
}
warnOnMissingCargoToml();

window.setStatusBarMessage('RLS: starting up');
startSpinner('RLS', 'Starting');

warnOnRlsToml();
// Check for deprecated env vars.
Expand All @@ -172,7 +172,7 @@ function startLanguageClient(context: ExtensionContext)
// Create the language client and start the client.
lc = new LanguageClient('Rust Language Server', serverOptions, clientOptions);

diagnosticCounter();
progressCounter();

const disposable = lc.start();
context.subscriptions.push(disposable);
Expand Down Expand Up @@ -207,17 +207,45 @@ async function autoUpdate() {
}
}

function diagnosticCounter() {
function progressCounter() {
const runningProgress: { [index: string]: 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'm really wonky with JS, but would it be possible to replace that with Set for clarity?

Also not really sure why/how it's allowed to be const, since it's clearly modified in the callbacks, but I guess that's TypeScript shenanigans that I'm missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Set is better.

const is not typescript shenanigans but javascript. It's not actually immutable (sadly), it's just the actual variable itself that is impossible to change. I.e.

const foo = 42;
foo = 43; // FAIL

Copy link
Member

Choose a reason for hiding this comment

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

Right, makes sense. Thanks for the changes!

const asPercent = (fraction: number): string => `${Math.round(fraction * 100)}%`;
let runningDiagnostics = 0;
lc.onReady().then(() => {
lc.onNotification(new NotificationType('rustDocument/beginBuild'), function(_f: any) {

stopSpinner('RLS');

lc.onNotification(new NotificationType('window/progress'), function (progress: any) {
if (progress.done) {
delete runningProgress[progress.id];
} else {
runningProgress[progress.id] = true;
}
if (Object.keys(runningProgress).length) {
Copy link
Member

Choose a reason for hiding this comment

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

If Set is used, these can be simplified (as per above)

let status = '';
if (typeof progress.percentage === 'number') {
status = asPercent(progress.percentage);
} else if (progress.message) {
status = progress.message;
} else if (progress.title) {
status = `[${progress.title.toLowerCase()}]`;
Copy link
Member

Choose a reason for hiding this comment

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

nit: I know it's up to clients to display this on the UI side, but shouldn't we just display it how servers send it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ultimately I don't really want to end up in this part of the logic. This is a last resort fallback. We should try to send percentages or messages instead. It looked really bad to me with the caps, but this just taste and style, so I'm only going to argue my case once :)

If you think it's an important point, I'll change it.

}
startSpinner('RLS', status);
} else {
stopSpinner('RLS');
}
});

// FIXME these are legacy notifications used by RLS ca jan 2018.
// remove once we're certain we've progress on.
lc.onNotification(new NotificationType('rustDocument/beginBuild'), function (_f: any) {
runningDiagnostics++;
startSpinner('RLS: working');
startSpinner('RLS', 'working');
});
lc.onNotification(new NotificationType('rustDocument/diagnosticsEnd'), function(_f: any) {
lc.onNotification(new NotificationType('rustDocument/diagnosticsEnd'), function (_f: any) {
runningDiagnostics--;
if (runningDiagnostics <= 0) {
stopSpinner('RLS: done');
stopSpinner('RLS');
}
});
});
Expand Down
6 changes: 3 additions & 3 deletions src/rustup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export function runRlsViaRustup(env: any): Promise<child_process.ChildProcess> {
}

export async function rustupUpdate() {
startSpinner('Updating RLS...');
startSpinner('RLS', 'Updating…');

try {
const { stdout } = await execChildProcess(CONFIGURATION.rustupPath + ' update');
Expand Down Expand Up @@ -74,7 +74,7 @@ async function hasToolchain(): Promise<boolean> {
}

async function tryToInstallToolchain(): Promise<void> {
startSpinner('Installing toolchain...');
startSpinner('RLS', 'Installing toolchain…');
try {
const { stdout, stderr } = await execChildProcess(CONFIGURATION.rustupPath + ' toolchain install ' + CONFIGURATION.channel);
console.log(stdout);
Expand Down Expand Up @@ -129,7 +129,7 @@ async function hasRlsComponents(): Promise<boolean> {
}

async function installRls(): Promise<void> {
startSpinner('Installing RLS components');
startSpinner('RLS', 'Installing components');

const tryFn: (component: string) => Promise<(Error | null)> = async (component: string) => {
try {
Expand Down
15 changes: 8 additions & 7 deletions src/spinner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,15 @@

import { window } from 'vscode';

export function startSpinner(message: string) {
if (spinnerTimer == null) {
let state = 0;
spinnerTimer = setInterval(function() {
window.setStatusBarMessage(message + ' ' + spinner[state]);
state = (state + 1) % spinner.length;
}, 100);
export function startSpinner(prefix: string, postfix: string) {
if (spinnerTimer != null) {
clearInterval(spinnerTimer);
}
let state = 0;
spinnerTimer = setInterval(function() {
window.setStatusBarMessage(prefix + ' ' + spinner[state] + ' ' + postfix);
state = (state + 1) % spinner.length;
}, 100);
}

export function stopSpinner(message: string) {
Expand Down