From 148d27f021c91683bd25290cbe3d7216516b604e Mon Sep 17 00:00:00 2001 From: Christopher Allford <6451942+ObliviousHarmony@users.noreply.github.com> Date: Mon, 24 May 2021 11:54:47 -0700 Subject: [PATCH] Clear Status When Document Closes (#30) When a diagnostic is cancelled, we were leaving behind the status entry to dangle. This adds a cancellation event to stop the status, thus removing it from the status bar when cancelled. We were also using the Uri as a key in many different places, but this led to bugs when they were not identical objects (even though identical Uris). For convenience, we've wrapped these places with a class that gets the string from the Uri as the key. --- CHANGELOG.md | 3 ++ src/__mocks__/vscode.ts | 14 +++++++-- src/common/uri-map.ts | 46 +++++++++++++++++++++++++++++ src/common/uri-set.ts | 36 ++++++++++++++++++++++ src/listeners/workspace-listener.ts | 10 ++++--- src/services/configuration.ts | 5 ++-- src/services/diagnostic-updater.ts | 5 ++++ src/services/linter-status.ts | 5 ++-- src/services/worker-service.ts | 9 ++---- src/types.ts | 5 ++-- 10 files changed, 120 insertions(+), 18 deletions(-) create mode 100644 src/common/uri-map.ts create mode 100644 src/common/uri-set.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 03e63e6..067a004 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixed +- Maps and Sets should not be keyed by the Uri instance directly. +- Linter status should be cleared when a diagnostic is cancelled. ## [1.2.0] - 2021-05-21 ### Added diff --git a/src/__mocks__/vscode.ts b/src/__mocks__/vscode.ts index 69506ca..eb564d7 100644 --- a/src/__mocks__/vscode.ts +++ b/src/__mocks__/vscode.ts @@ -1,13 +1,23 @@ const Uri: any = jest.fn().mockImplementation(() => { - return { + const created = { scheme: 'file', authority: '', path: 'test/file/path.php', query: '', fragment: '', fsPath: 'test/file/path.php', - toString: jest.fn(), + toString: (): string => { + let str = created.scheme + '://' + created.path; + if (created.query) { + str += '?' + created.query; + } + if (created.fragment) { + str += '#' + created.fragment; + } + return str; + }, }; + return created; }); Uri.joinPath = jest.fn(); diff --git a/src/common/uri-map.ts b/src/common/uri-map.ts new file mode 100644 index 0000000..e27eb7c --- /dev/null +++ b/src/common/uri-map.ts @@ -0,0 +1,46 @@ +import { Uri } from 'vscode'; + +/** + * A map for storing data keyed by a Uri. + */ +export class UriMap extends Map { + public has(key: Uri): boolean; + public has(key: string): boolean; + public has(key: Uri | string): boolean { + if (key instanceof Uri) { + key = key.toString(); + } + + return super.has(key); + } + + public set(key: Uri, value: V): this; + public set(key: string, value: V): this; + public set(key: Uri | string, value: V): this { + if (key instanceof Uri) { + key = key.toString(); + } + + return super.set(key, value); + } + + public get(key: Uri): V | undefined; + public get(key: string): V | undefined; + public get(key: Uri | string): V | undefined { + if (key instanceof Uri) { + key = key.toString(); + } + + return super.get(key); + } + + public delete(key: Uri): boolean; + public delete(key: string): boolean; + public delete(key: Uri | string): boolean { + if (key instanceof Uri) { + key = key.toString(); + } + + return super.delete(key); + } +} diff --git a/src/common/uri-set.ts b/src/common/uri-set.ts new file mode 100644 index 0000000..09b79dc --- /dev/null +++ b/src/common/uri-set.ts @@ -0,0 +1,36 @@ +import { Uri } from 'vscode'; + +/** + * A set for storing Uris. + */ +export class UriSet extends Set { + public has(key: Uri): boolean; + public has(key: string): boolean; + public has(key: Uri | string): boolean { + if (key instanceof Uri) { + key = key.toString(); + } + + return super.has(key); + } + + public add(key: Uri): this; + public add(key: string): this; + public add(key: Uri | string): this { + if (key instanceof Uri) { + key = key.toString(); + } + + return super.add(key); + } + + public delete(key: Uri): boolean; + public delete(key: string): boolean; + public delete(key: Uri | string): boolean { + if (key instanceof Uri) { + key = key.toString(); + } + + return super.delete(key); + } +} diff --git a/src/listeners/workspace-listener.ts b/src/listeners/workspace-listener.ts index d17f744..702bbab 100644 --- a/src/listeners/workspace-listener.ts +++ b/src/listeners/workspace-listener.ts @@ -9,6 +9,8 @@ import { Configuration } from '../services/configuration'; import { CodeActionEditResolver } from '../services/code-action-edit-resolver'; import { DiagnosticUpdater } from '../services/diagnostic-updater'; import { DocumentFormatter } from '../services/document-formatter'; +import { UriMap } from '../common/uri-map'; +import { UriSet } from '../common/uri-set'; /** * A class for listening to the workspace and responding to events that occur. @@ -37,12 +39,12 @@ export class WorkspaceListener implements Disposable { /** * A set containing the Uris of all the documents that we're tracking. */ - private readonly trackedDocuments: Set; + private readonly trackedDocuments: UriSet; /** * A map for applying a debounce to document updates. */ - private readonly updateDebounceMap: Map; + private readonly updateDebounceMap: UriMap; /** * The subscriptions we have to VS Code events. @@ -67,8 +69,8 @@ export class WorkspaceListener implements Disposable { this.diagnosticUpdater = diagnosticUpdater; this.codeActionEditResolver = codeActionEditResolver; this.documentFormatter = documentFormatter; - this.trackedDocuments = new Set(); - this.updateDebounceMap = new Map(); + this.trackedDocuments = new UriSet(); + this.updateDebounceMap = new UriMap(); this.subscriptions = []; } diff --git a/src/services/configuration.ts b/src/services/configuration.ts index 4b93276..dbeee5a 100644 --- a/src/services/configuration.ts +++ b/src/services/configuration.ts @@ -5,6 +5,7 @@ import { Uri, workspace as vsCodeWorkspace, } from 'vscode'; +import { UriMap } from '../common/uri-map'; /** * An enum describing the values in the `phpcsCodeSniffer.standard` configuration. @@ -83,7 +84,7 @@ export class Configuration { /** * A cache containing all of the configurations we've loaded. */ - private readonly cache: Map; + private readonly cache: UriMap; /** * The decoder for parsing file content into strings for consumption. @@ -97,7 +98,7 @@ export class Configuration { */ public constructor(workspace: typeof vsCodeWorkspace) { this.workspace = workspace; - this.cache = new Map(); + this.cache = new UriMap(); this.textDecoder = new TextDecoder(); } diff --git a/src/services/diagnostic-updater.ts b/src/services/diagnostic-updater.ts index 6cd5a89..5a138ab 100644 --- a/src/services/diagnostic-updater.ts +++ b/src/services/diagnostic-updater.ts @@ -94,6 +94,11 @@ export class DiagnosticUpdater extends WorkerService { // Record that we're going to start linting a document. this.linterStatus.start(document.uri); + // Make sure we stop linting the document the update is cancelled. + cancellationToken.onCancellationRequested(() => { + this.linterStatus.stop(document.uri); + }); + this.workerPool .waitForAvailable( 'diagnostic:' + document.fileName, diff --git a/src/services/linter-status.ts b/src/services/linter-status.ts index 8196682..e2bfaeb 100644 --- a/src/services/linter-status.ts +++ b/src/services/linter-status.ts @@ -5,6 +5,7 @@ import { Uri, window as vsCodeWindow, } from 'vscode'; +import { UriSet } from '../common/uri-set'; /** * A class for managing the linter's status. @@ -18,14 +19,14 @@ export class LinterStatus implements Disposable { /** * The documents that are actively being linted. */ - private readonly activeDocuments: Set; + private readonly activeDocuments: UriSet; /** * Constructor. */ public constructor(window: typeof vsCodeWindow) { this.statusBar = window.createStatusBarItem(StatusBarAlignment.Left); - this.activeDocuments = new Set(); + this.activeDocuments = new UriSet(); } /** diff --git a/src/services/worker-service.ts b/src/services/worker-service.ts index c8ea190..e98b8e2 100644 --- a/src/services/worker-service.ts +++ b/src/services/worker-service.ts @@ -3,11 +3,11 @@ import { CancellationTokenSource, Disposable, TextDocument, - Uri, } from 'vscode'; import { Logger } from './logger'; import { Configuration } from './configuration'; import { WorkerPool } from '../phpcs-report/worker-pool'; +import { UriMap } from '../common/uri-map'; /** * A base class for all of the updates that interact with PHPCS. @@ -31,10 +31,7 @@ export abstract class WorkerService implements Disposable { /** * A map containing all of the cancellation token sources to prevent overlapping execution. */ - private readonly cancellationTokenSourceMap: Map< - Uri, - CancellationTokenSource - >; + private readonly cancellationTokenSourceMap: UriMap; /** * Constructor. @@ -51,7 +48,7 @@ export abstract class WorkerService implements Disposable { this.logger = logger; this.configuration = configuration; this.workerPool = workerPool; - this.cancellationTokenSourceMap = new Map(); + this.cancellationTokenSourceMap = new UriMap(); } /** diff --git a/src/types.ts b/src/types.ts index 4405e92..09013a7 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1,4 +1,5 @@ -import { CodeAction as BaseCodeAction, TextDocument, Uri } from 'vscode'; +import { CodeAction as BaseCodeAction, TextDocument } from 'vscode'; +import { UriMap } from './common/uri-map'; /** * A custom code action class that adds a Uri for associating it with a document. @@ -13,4 +14,4 @@ export class CodeAction extends BaseCodeAction { /** * A collection of CodeAction instances keyed by the Uri. */ -export class CodeActionCollection extends Map {} +export class CodeActionCollection extends UriMap {}