Merge pull request #772 from rust-lang/progress-tweaks

Progress tweaks

Author: Xanewok

CHANGELOG.md

@@ -1,5 +1,9 @@
 ### Unreleased
 
+* (!) Don't immediately start server instances for every already opened file
+* (!) Don't immediately start server instances for newly added workspace folders
+* Dynamically show progress only for the active client workspace
+* Correctly run tasks based on active text editor rather than last opened Rust file
 * Use smooth, universally supported spinner in the status bar ⚙️
 
 ### 0.7.3 - 2020-04-21

src/extension.ts

@@ -9,7 +9,7 @@ import {
   IndentAction,
   languages,
   RelativePattern,
-  TextDocument,
+  TextEditor,
   Uri,
   window,
   workspace,
@@ -29,10 +29,8 @@ import { checkForRls, ensureToolchain, rustupUpdate } from './rustup';
 import { startSpinner, stopSpinner } from './spinner';
 import { activateTaskProvider, Execution, runRlsCommand } from './tasks';
 import { withWsl } from './utils/child_process';
-import {
-  getOuterMostWorkspaceFolder,
-  nearestParentWorkspace,
-} from './utils/workspace';
+import { Observable, Observer } from './utils/observable';
+import { nearestParentWorkspace } from './utils/workspace';
 import { uriWindowsToWsl, uriWslToWindows } from './utils/wslpath';
 
 /**
@@ -48,17 +46,17 @@ interface ProgressParams {
 }
 
 export async function activate(context: ExtensionContext) {
-  context.subscriptions.push(configureLanguage());
-  context.subscriptions.push(...registerCommands());
-
-  workspace.onDidOpenTextDocument(doc => whenOpeningTextDocument(doc));
-  workspace.onDidChangeWorkspaceFolders(e => whenChangingWorkspaceFolders(e));
-  window.onDidChangeActiveTextEditor(
-    ed => ed && whenOpeningTextDocument(ed.document),
+  context.subscriptions.push(
+    ...[
+      configureLanguage(),
+      ...registerCommands(),
+      workspace.onDidChangeWorkspaceFolders(whenChangingWorkspaceFolders),
+      window.onDidChangeActiveTextEditor(onDidChangeActiveTextEditor),
+    ],
   );
-  // Installed listeners don't fire immediately for already opened files, so
-  // trigger an open event manually to fire up RLS instances where needed
-  workspace.textDocuments.forEach(whenOpeningTextDocument);
+  // Manually trigger the first event to start up server instance if necessary,
+  // since VSCode doesn't do that on startup by itself.
+  onDidChangeActiveTextEditor(window.activeTextEditor);
 
   // Migrate the users of multi-project setup for RLS to disable the setting
   // entirely (it's always on now)
@@ -92,55 +90,38 @@ export async function deactivate() {
   return Promise.all([...workspaces.values()].map(ws => ws.stop()));
 }
 
-// Taken from https://github.com/Microsoft/vscode-extension-samples/blob/master/lsp-multi-server-sample/client/src/extension.ts
-function whenOpeningTextDocument(document: TextDocument) {
-  if (document.languageId !== 'rust' && document.languageId !== 'toml') {
-    return;
-  }
+/** Tracks dynamically updated progress for the active client workspace for UI purposes. */
+const progressObserver: Observer<{ message: string } | null> = new Observer();
 
-  let folder = workspace.getWorkspaceFolder(document.uri);
-  if (!folder) {
+function onDidChangeActiveTextEditor(editor: TextEditor | undefined) {
+  if (!editor || !editor.document) {
     return;
   }
+  const { languageId, uri } = editor.document;
 
-  folder = nearestParentWorkspace(folder, document.uri.fsPath);
-
-  if (!folder) {
-    stopSpinner(`RLS: Cargo.toml missing`);
+  const workspace = clientWorkspaceForUri(uri, {
+    startIfMissing: languageId === 'rust' || languageId === 'toml',
+  });
+  if (!workspace) {
     return;
   }
 
-  const folderPath = folder.uri.toString();
+  activeWorkspace = workspace;
 
-  if (!workspaces.has(folderPath)) {
-    const workspace = new ClientWorkspace(folder);
-    activeWorkspace = workspace;
-    workspaces.set(folderPath, workspace);
-    workspace.start();
-  } else {
-    const ws = workspaces.get(folderPath);
-    activeWorkspace = typeof ws === 'undefined' ? null : ws;
-  }
+  const updateProgress = (progress: { message: string } | null) => {
+    if (progress) {
+      startSpinner(`[${workspace.folder.name}] ${progress.message}`);
+    } else {
+      stopSpinner(`[${workspace.folder.name}]`);
+    }
+  };
+
+  progressObserver.bind(activeWorkspace.progress, updateProgress);
+  // Update UI ourselves immediately and don't wait for value update callbacks
+  updateProgress(activeWorkspace.progress.value);
 }
 
 function whenChangingWorkspaceFolders(e: WorkspaceFoldersChangeEvent) {
-  // If a VSCode workspace has been added, check to see if it is part of an existing one, and
-  // if not, and it is a Rust project (i.e., has a Cargo.toml), then create a new client.
-  for (let folder of e.added) {
-    folder = getOuterMostWorkspaceFolder(folder);
-    if (workspaces.has(folder.uri.toString())) {
-      continue;
-    }
-    for (const f of fs.readdirSync(folder.uri.fsPath)) {
-      if (f === 'Cargo.toml') {
-        const workspace = new ClientWorkspace(folder);
-        workspaces.set(folder.uri.toString(), workspace);
-        workspace.start();
-        break;
-      }
-    }
-  }
-
   // If a workspace is removed which is a Rust workspace, kill the client.
   for (const folder of e.removed) {
     const ws = workspaces.get(folder.uri.toString());
@@ -154,25 +135,58 @@ function whenChangingWorkspaceFolders(e: WorkspaceFoldersChangeEvent) {
 // Don't use URI as it's unreliable the same path might not become the same URI.
 const workspaces: Map<string, ClientWorkspace> = new Map();
 
+/**
+ * Fetches a `ClientWorkspace` for a given URI. If missing and `startIfMissing`
+ * option was provided, it is additionally initialized beforehand, if applicable.
+ */
+function clientWorkspaceForUri(
+  uri: Uri,
+  options?: { startIfMissing: boolean },
+): ClientWorkspace | undefined {
+  const rootFolder = workspace.getWorkspaceFolder(uri);
+  if (!rootFolder) {
+    return;
+  }
+
+  const folder = nearestParentWorkspace(rootFolder, uri.fsPath);
+  if (!folder) {
+    return undefined;
+  }
+
+  const existing = workspaces.get(folder.uri.toString());
+  if (!existing && options && options.startIfMissing) {
+    const workspace = new ClientWorkspace(folder);
+    workspaces.set(folder.uri.toString(), workspace);
+    workspace.start();
+  }
+
+  return workspaces.get(folder.uri.toString());
+}
+
 // We run one RLS and one corresponding language client per workspace folder
 // (VSCode workspace, not Cargo workspace). This class contains all the per-client
 // and per-workspace stuff.
 class ClientWorkspace {
+  public readonly folder: WorkspaceFolder;
   // FIXME(#233): Don't only rely on lazily initializing it once on startup,
   // handle possible `rust-client.*` value changes while extension is running
   private readonly config: RLSConfiguration;
   private lc: LanguageClient | null = null;
-  private readonly folder: WorkspaceFolder;
   private disposables: Disposable[];
+  private _progress: Observable<{ message: string } | null>;
+  get progress() {
+    return this._progress;
+  }
 
   constructor(folder: WorkspaceFolder) {
     this.config = RLSConfiguration.loadFromWorkspace(folder.uri.fsPath);
     this.folder = folder;
     this.disposables = [];
+    this._progress = new Observable<{ message: string } | null>(null);
   }
 
   public async start() {
-    startSpinner('RLS', 'Starting');
+    this._progress.value = { message: 'Starting' };
 
     const serverOptions: ServerOptions = async () => {
       await this.autoUpdate();
@@ -273,7 +287,6 @@ class ClientWorkspace {
 
     const runningProgress: Set<string> = new Set();
     await this.lc.onReady();
-    stopSpinner('RLS');
 
     this.lc.onNotification(
       new NotificationType<ProgressParams, void>('window/progress'),
@@ -292,9 +305,9 @@ class ClientWorkspace {
           } else if (progress.title) {
             status = `[${progress.title.toLowerCase()}]`;
           }
-          startSpinner('RLS', status);
+          this._progress.value = { message: status };
         } else {
-          stopSpinner('RLS');
+          this._progress.value = null;
         }
       },
     );

src/rustup.ts

@@ -21,7 +21,7 @@ export interface RustupConfig {
 }
 
 export async function rustupUpdate(config: RustupConfig) {
-  startSpinner('RLS', 'Updating…');
+  startSpinner('Updating…');
 
   try {
     const { stdout } = await withWsl(config.useWSL).exec(
@@ -103,7 +103,7 @@ async function hasToolchain(config: RustupConfig): Promise<boolean> {
 }
 
 async function tryToInstallToolchain(config: RustupConfig) {
-  startSpinner('RLS', 'Installing toolchain…');
+  startSpinner('Installing toolchain…');
   try {
     const { command, args } = withWsl(config.useWSL).modifyArgs(config.path, [
       'toolchain',
@@ -154,7 +154,7 @@ async function hasRlsComponents(config: RustupConfig): Promise<boolean> {
 }
 
 async function installRlsComponents(config: RustupConfig) {
-  startSpinner('RLS', 'Installing components…');
+  startSpinner('Installing components…');
 
   for (const component of REQUIRED_COMPONENTS) {
     try {

src/spinner.ts

@@ -1,9 +1,9 @@
 import { window } from 'vscode';
 
-export function startSpinner(prefix: string, postfix: string) {
-  window.setStatusBarMessage(`${prefix} $(settings-gear~spin) ${postfix}`);
+export function startSpinner(message: string) {
+  window.setStatusBarMessage(`RLS $(settings-gear~spin) ${message}`);
 }
 
-export function stopSpinner(message: string) {
-  window.setStatusBarMessage(message);
+export function stopSpinner(message?: string) {
+  window.setStatusBarMessage(`RLS ${message || ''}`);
 }

src/utils/observable.ts

@@ -0,0 +1,69 @@
+/**
+ * A wrapper around a value of type `T` that can be subscribed to whenever the
+ * underlying value changes.
+ */
+export class Observable<T> {
+  private _listeners: Set<(arg: T) => void> = new Set();
+  private _value: T;
+  /** Returns the current value. */
+  get value() {
+    return this._value;
+  }
+  /** Every change to the value triggers all the registered callbacks. */
+  set value(value: T) {
+    this._value = value;
+    this._listeners.forEach(fn => fn(value));
+  }
+
+  constructor(value: T) {
+    this._value = value;
+  }
+
+  /**
+   * Registers a listener function that's called whenever the underlying value
+   * changes.
+   * @returns a function that unregisters the listener when called.
+   */
+  public observe(fn: (arg: T) => void): () => void {
+    this._listeners.add(fn);
+
+    return () => this._listeners.delete(fn);
+  }
+}
+
+/**
+ * Capable of observing an `Observable<T>` type.
+ *
+ * Convenient when using a single observer that potentially binds multiple times
+ * to different observables, where it automatically unregisters from previous
+ * observables.
+ */
+// tslint:disable-next-line: max-classes-per-file
+export class Observer<T> {
+  private _observable?: Observable<T>;
+  private _stopObserving?: () => void;
+  /** Returns the current value of a bound observable, if there is one. */
+  get value() {
+    return this._observable && this._observable.value;
+  }
+  /**
+   * Binds to an observable value, along with the provided listener that's
+   * called whenever the underlying value changes.
+   */
+  public bind(observable: Observable<T>, handler: (arg: T) => void) {
+    this.stop();
+
+    this._observable = observable;
+    this._stopObserving = observable.observe(handler);
+  }
+  /** Unbinds from the observable, deregistering the previously bound callback. */
+  public stop() {
+    if (this._stopObserving) {
+      this._stopObserving();
+      delete this._stopObserving;
+    }
+    if (this._observable) {
+      delete this._observable;
+    }
+  }
+}