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 2 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
40 changes: 32 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,41 @@ 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)

const status =
typeof progress.percentage === 'number' ? asPercent(progress.percentage) :
Copy link
Member

Choose a reason for hiding this comment

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

Can you use an if statment rather than these nested ternary operators, it's pretty hard to read as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

progress.message ? progress.message :
Copy link
Member

Choose a reason for hiding this comment

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

Title is ignored in favour of message, if it's specified - is that intended?
This leads to RLS * [build] followed by RLS * crate_name when building, where we lose information that we're building that crate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Xanewok This is indeed intended. I wanted to keep the information flow at the bottom terse, and it's mostly obviously what it's doing. The [<title>] is to get something rather than RLS *, but could maybe dropped.

It's kinda interesting to see how long it spends doing [diagnostics] though. Already now I can see we should look into performance here.

progress.title ? `[${progress.title.toLowerCase()}]` : '';
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