From a6a7a279b9a296007691f3b136af5dbf9bf00ec8 Mon Sep 17 00:00:00 2001 From: Rich Chiodo false Date: Mon, 22 Apr 2024 10:26:06 -0700 Subject: [PATCH 1/5] Create new events for sending notebook changes --- client/src/common/diagnostic.ts | 57 ++++++++++++++++++++++++++- client/src/common/features.ts | 3 +- client/src/common/notebook.ts | 68 ++++++++++++++++++++++++++++++++- 3 files changed, 124 insertions(+), 4 deletions(-) diff --git a/client/src/common/diagnostic.ts b/client/src/common/diagnostic.ts index e2e05c7d9..ff3c8605e 100644 --- a/client/src/common/diagnostic.ts +++ b/client/src/common/diagnostic.ts @@ -15,7 +15,9 @@ import { ClientCapabilities, ServerCapabilities, DocumentSelector, DidOpenTextDocumentNotification, DidChangeTextDocumentNotification, DidSaveTextDocumentNotification, DidCloseTextDocumentNotification, LinkedMap, Touch, RAL, TextDocumentFilter, PreviousResultId, DiagnosticRegistrationOptions, DiagnosticServerCancellationData, DocumentDiagnosticParams, DocumentDiagnosticRequest, DocumentDiagnosticReportKind, - WorkspaceDocumentDiagnosticReport, WorkspaceDiagnosticRequest, WorkspaceDiagnosticParams, DiagnosticOptions, DiagnosticRefreshRequest, DiagnosticTag + WorkspaceDocumentDiagnosticReport, WorkspaceDiagnosticRequest, WorkspaceDiagnosticParams, DiagnosticOptions, DiagnosticRefreshRequest, DiagnosticTag, + DidChangeNotebookDocumentNotification, + NotebookDocumentSyncRegistrationType } from 'vscode-languageserver-protocol'; import { generateUuid } from './utils/uuid'; @@ -970,6 +972,15 @@ class DiagnosticFeatureProviderImpl implements DiagnosticProviderShape { this.diagnosticRequestor.pull(textDocument, () => { addToBackgroundIfNeeded(textDocument); }); } })); + const notebookFeature = client.getFeature(NotebookDocumentSyncRegistrationType.method); + disposables.push(notebookFeature.onOpenNotificationSent((event) => { + // Send a pull for all opened cells in the notebook. + for (const cell of event.getCells()) { + if (matches(cell.document)) { + this.diagnosticRequestor.pull(cell.document, () => { addToBackgroundIfNeeded(cell.document); }); + } + } + })); disposables.push(tabs.onOpen((opened) => { for (const resource of opened) { @@ -1007,6 +1018,15 @@ class DiagnosticFeatureProviderImpl implements DiagnosticProviderShape { pulledTextDocuments.add(textDocument.uri.toString()); } } + // Do the same for already open notebook cells. + for (const notebookDocument of Workspace.notebookDocuments) { + for (const cell of notebookDocument.getCells()) { + if (matches(cell.document)) { + this.diagnosticRequestor.pull(cell.document, () => { addToBackgroundIfNeeded(cell.document); }); + pulledTextDocuments.add(cell.document.uri.toString()); + } + } + } // Pull all tabs if not already pulled as text document if (diagnosticPullOptions.onTabs === true) { @@ -1029,6 +1049,29 @@ class DiagnosticFeatureProviderImpl implements DiagnosticProviderShape { this.diagnosticRequestor.pull(textDocument, () => { this.backgroundScheduler.trigger(); }); } })); + disposables.push(notebookFeature.onChangeNotificationSent(async (event) => { + // Send a pull for all changed cells in the notebook. + const changedCells = event.cells?.textContent || []; + for (const cell of changedCells) { + if (matches(cell.document)) { + this.diagnosticRequestor.pull(cell.document, () => { this.backgroundScheduler.trigger(); }); + } + } + + // Clear out any closed cells. + const closedCells = event.cells?.structure?.didClose || []; + for (const cell of closedCells) { + this.diagnosticRequestor.forgetDocument(cell.document); + } + + // Send a pull for any new opened cells. + const openedCells = event.cells?.structure?.didOpen || []; + for (const cell of openedCells) { + if (matches(cell.document)) { + this.diagnosticRequestor.pull(cell.document, () => { this.backgroundScheduler.trigger(); }); + } + } + })); } if (diagnosticPullOptions.onSave === true) { @@ -1039,6 +1082,13 @@ class DiagnosticFeatureProviderImpl implements DiagnosticProviderShape { this.diagnosticRequestor.pull(event.textDocument); } })); + disposables.push(notebookFeature.onSaveNotificationSent((event) => { + for (const cell of event.getCells()) { + if (matches(cell.document)) { + this.diagnosticRequestor.pull(cell.document); + } + } + })); } // When the document closes clear things up @@ -1046,6 +1096,11 @@ class DiagnosticFeatureProviderImpl implements DiagnosticProviderShape { disposables.push(closeFeature.onNotificationSent((event) => { this.cleanUpDocument(event.textDocument); })); + disposables.push(notebookFeature.onCloseNotificationSent((event) => { + for (const cell of event.getCells()) { + this.cleanUpDocument(cell.document); + } + })); // Same when a tabs closes. tabs.onClose((closed) => { diff --git a/client/src/common/features.ts b/client/src/common/features.ts index 14970d784..90122c1c9 100644 --- a/client/src/common/features.ts +++ b/client/src/common/features.ts @@ -15,7 +15,8 @@ import { import { CallHierarchyPrepareRequest, ClientCapabilities, CodeActionRequest, CodeLensRequest, CompletionRequest, DeclarationRequest, DefinitionRequest, - DidChangeTextDocumentNotification, DidCloseTextDocumentNotification, DidCreateFilesNotification, DidDeleteFilesNotification, DidOpenTextDocumentNotification, + DidChangeNotebookDocumentNotification, + DidChangeTextDocumentNotification, DidCloseTextDocumentNotification, DidCreateFilesNotification, DidDeleteFilesNotification, DidOpenNotebookDocumentNotification, DidOpenTextDocumentNotification, DidRenameFilesNotification, DidSaveTextDocumentNotification, DocumentColorRequest, DocumentDiagnosticRequest, DocumentFormattingRequest, DocumentHighlightRequest, DocumentLinkRequest, DocumentOnTypeFormattingRequest, DocumentRangeFormattingRequest, DocumentSelector, DocumentSymbolRequest, ExecuteCommandOptions, ExecuteCommandRequest, FileOperationRegistrationOptions, FoldingRangeRequest, GenericNotificationHandler, GenericRequestHandler, HoverRequest, ImplementationRequest, InitializeParams, InlayHintRequest, InlineCompletionRegistrationOptions, InlineCompletionRequest, InlineValueRequest, diff --git a/client/src/common/notebook.ts b/client/src/common/notebook.ts index f9fdfa1b3..45f083b08 100644 --- a/client/src/common/notebook.ts +++ b/client/src/common/notebook.ts @@ -16,7 +16,7 @@ import * as Is from './utils/is'; import * as _c2p from './codeConverter'; import * as _p2c from './protocolConverter'; -import { DynamicFeature, FeatureClient, RegistrationData, FeatureState } from './features'; +import { DynamicFeature, FeatureClient, RegistrationData, FeatureState, NotifyingFeature, NotificationSendEvent } from './features'; function ensure(target: T, key: K): T[K] { @@ -413,12 +413,26 @@ export type NotebookDocumentMiddleware = { }; export interface NotebookDocumentSyncFeatureShape { + onOpenNotificationSent: vscode.Event; + onChangeNotificationSent: vscode.Event; + onCloseNotificationSent: vscode.Event; + onSaveNotificationSent: vscode.Event; sendDidOpenNotebookDocument(notebookDocument: vscode.NotebookDocument): Promise; sendDidSaveNotebookDocument(notebookDocument: vscode.NotebookDocument): Promise; sendDidChangeNotebookDocument(event: VNotebookDocumentChangeEvent): Promise; sendDidCloseNotebookDocument(notebookDocument: vscode.NotebookDocument): Promise; } +export interface NotificationNotebookSendEvent

{ + type: proto.ProtocolNotificationType; + params: P; +} + +export interface NotifyingNotebookFeature

{ + onNotificationSent: vscode.Event>; +} + + class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeatureShape { private readonly client: FeatureClient; @@ -427,6 +441,10 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature private readonly notebookDidOpen: Set; private readonly disposables: vscode.Disposable[]; private readonly selector: vscode.DocumentSelector; + private readonly _onChangeNotificationSent: vscode.EventEmitter; + private readonly _onOpenNotificationSent: vscode.EventEmitter; + private readonly _onCloseNotificationSent: vscode.EventEmitter; + private readonly _onSaveNotificationSent: vscode.EventEmitter; constructor(client: FeatureClient, options: proto.NotebookDocumentSyncOptions) { this.client = client; @@ -435,6 +453,10 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature this.notebookDidOpen = new Set(); this.disposables = []; this.selector = client.protocol2CodeConverter.asDocumentSelector($NotebookDocumentSyncOptions.asDocumentSelector(options)); + this._onChangeNotificationSent = new vscode.EventEmitter(); + this._onOpenNotificationSent = new vscode.EventEmitter(); + this._onCloseNotificationSent = new vscode.EventEmitter(); + this._onSaveNotificationSent = new vscode.EventEmitter(); // open vscode.workspace.onDidOpenNotebookDocument((notebookDocument) => { @@ -475,6 +497,22 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature return 'notebook'; } + public get onChangeNotificationSent(): vscode.Event { + return this._onChangeNotificationSent.event; + } + + public get onOpenNotificationSent(): vscode.Event { + return this._onOpenNotificationSent.event; + } + + public get onCloseNotificationSent(): vscode.Event { + return this._onCloseNotificationSent.event; + } + + public get onSaveNotificationSent(): vscode.Event { + return this._onSaveNotificationSent.event; + } + public handles(textDocument: vscode.TextDocument): boolean { if (vscode.languages.match(this.selector, textDocument) > 0) { return true; @@ -665,6 +703,7 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature } private async doSendOpen(notebookDocument: vscode.NotebookDocument, cells: vscode.NotebookCell[]): Promise { + this._onOpenNotificationSent.fire(notebookDocument); const send = async (notebookDocument: vscode.NotebookDocument, cells: vscode.NotebookCell[]): Promise => { const nb = Converter.c2p.asNotebookDocument(notebookDocument, cells, this.client.code2ProtocolConverter); const cellDocuments: TextDocumentItem[] = cells.map(cell => this.client.code2ProtocolConverter.asTextDocumentItem(cell.document)); @@ -688,6 +727,7 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature } private async doSendChange(event: VNotebookDocumentChangeEvent, cells: vscode.NotebookCell[] | undefined = this.getMatchingCells(event.notebook)): Promise { + this._onChangeNotificationSent.fire(event); const send = async (event: VNotebookDocumentChangeEvent): Promise => { try { await this.client.sendNotification(proto.DidChangeNotebookDocumentNotification.type, { @@ -711,6 +751,7 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature } private async doSendSave(notebookDocument: vscode.NotebookDocument): Promise { + this._onSaveNotificationSent.fire(notebookDocument); const send = async (notebookDocument: vscode.NotebookDocument): Promise => { try { await this.client.sendNotification(proto.DidSaveNotebookDocumentNotification.type, { @@ -730,6 +771,7 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature } private async doSendClose(notebookDocument: vscode.NotebookDocument, cells: vscode.NotebookCell[]): Promise { + this._onCloseNotificationSent.fire(notebookDocument); const send = async (notebookDocument: vscode.NotebookDocument, cells: vscode.NotebookCell[]): Promise => { try { await this.client.sendNotification(proto.DidCloseNotebookDocumentNotification.type, { @@ -856,6 +898,10 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature export type $NotebookCellTextDocumentFilter = NotebookCellTextDocumentFilter & { sync: true }; export type NotebookDocumentProviderShape = { + onOpenNotificationSent: vscode.Event; + onChangeNotificationSent: vscode.Event; + onCloseNotificationSent: vscode.Event; + onSaveNotificationSent: vscode.Event; getProvider(notebookCell: vscode.NotebookCell): NotebookDocumentSyncFeatureShape |undefined; }; @@ -871,6 +917,11 @@ export class NotebookDocumentSyncFeature implements DynamicFeature().event; + this.onOpenNotificationSent = new vscode.EventEmitter().event; + this.onCloseNotificationSent = new vscode.EventEmitter().event; + this.onSaveNotificationSent = new vscode.EventEmitter().event; + // We don't receive an event for cells where the document changes its language mode // Since we allow servers to filter on the language mode we fire such an event ourselves. vscode.workspace.onDidOpenTextDocument((textDocument) => { @@ -941,6 +992,14 @@ export class NotebookDocumentSyncFeature implements DynamicFeature; + public onOpenNotificationSent: vscode.Event; + + public onChangeNotificationSent: vscode.Event; + + public onCloseNotificationSent: vscode.Event; + + public onSaveNotificationSent: vscode.Event; + public fillClientCapabilities(capabilities: proto.ClientCapabilities): void { const synchronization = ensure(ensure(capabilities, 'notebookDocument')!, 'synchronization')!; synchronization.dynamicRegistration = true; @@ -966,6 +1025,10 @@ export class NotebookDocumentSyncFeature implements DynamicFeature): void { const provider = new NotebookDocumentSyncFeatureProvider(this.client, data.registerOptions); + this.onChangeNotificationSent = provider.onChangeNotificationSent; + this.onOpenNotificationSent = provider.onOpenNotificationSent; + this.onCloseNotificationSent = provider.onCloseNotificationSent; + this.onSaveNotificationSent = provider.onSaveNotificationSent; this.registrations.set(data.id, provider); } @@ -1019,4 +1082,5 @@ export class NotebookDocumentSyncFeature implements DynamicFeature Date: Mon, 22 Apr 2024 16:30:05 -0700 Subject: [PATCH 2/5] Get test working --- client-node-tests/src/integration.test.ts | 47 ++++++++++++++++ .../src/servers/fullNotebookServer.ts | 27 ++++++++- client/src/common/diagnostic.ts | 17 +++++- client/src/common/features.ts | 3 +- client/src/common/notebook.ts | 55 ++++++++++++------- 5 files changed, 125 insertions(+), 24 deletions(-) diff --git a/client-node-tests/src/integration.test.ts b/client-node-tests/src/integration.test.ts index 0dd3eeaff..c662a37cb 100644 --- a/client-node-tests/src/integration.test.ts +++ b/client-node-tests/src/integration.test.ts @@ -8,6 +8,7 @@ import * as assert from 'assert'; import * as path from 'path'; import * as vscode from 'vscode'; import * as lsclient from 'vscode-languageclient/node'; +import * as proto from 'vscode-languageserver-protocol'; import { MemoryFileSystemProvider } from './memoryFileSystemProvider'; import { vsdiag, DiagnosticProviderMiddleware } from 'vscode-languageclient/lib/common/diagnostic'; @@ -16,6 +17,11 @@ namespace GotNotifiedRequest { export const type = new lsclient.RequestType(method); } +namespace SetDiagnosticsNotification { + export const method: 'testing/setDiagnostics' = 'testing/setDiagnostics'; + export const type = new lsclient.NotificationType(method); +} + async function revertAllDirty(): Promise { return vscode.commands.executeCommand('_workbench.revertAllDirty'); } @@ -1688,6 +1694,47 @@ suite('Full notebook tests', () => { assert.strictEqual(notified, true); await revertAllDirty(); }); + + test('Notebook document: pull diagnostics', async(): Promise => { + const notebook = await vscode.workspace.openNotebookDocument('jupyter-notebook', createNotebookData()); + + // Send the diagnostics for the first cell. + const report: proto.DocumentDiagnosticReport = { + kind: 'full', + items: [ + { message: 'notebook-error', range: { start: { line: 0, character: 0 }, end: { line: 0, character: 0 }} } + ] + }; + await client.sendNotification(SetDiagnosticsNotification.method, { uri: notebook.cellAt(0).document.uri.toString(), report }); + + // notebook has to be visible for diagnostics to be published. + await vscode.window.showNotebookDocument(notebook); + + const promise = new Promise((resolve) => { + client.middleware.provideDiagnostics = async (doc, p, token, next) => { + const result = await next(doc, p, token); + if (result?.kind === 'full' && result?.items.length > 0) { + // Need to be async so that the diagnostics are published. + setTimeout(() => resolve(), 10); + } + return result; + }; + }); + + // Change the notebook cell, this should cause the diagnostics to be published. + const edit = new vscode.WorkspaceEdit(); + edit.insert(notebook.cellAt(0).document.uri, new vscode.Position(0, 0), '# a comment\n'); + await vscode.workspace.applyEdit(edit); + + // Wait for the diagnostics to be published. + await promise; + const diagnostics = vscode.languages.getDiagnostics(notebook.cellAt(0).document.uri); + assert.strictEqual(diagnostics.length, 1); + const diagnostic = diagnostics[0]; + assert.strictEqual(diagnostic.message, 'notebook-error'); + + await revertAllDirty(); + }); }); suite('Simple notebook tests', () => { diff --git a/client-node-tests/src/servers/fullNotebookServer.ts b/client-node-tests/src/servers/fullNotebookServer.ts index 892dfe868..282c5437e 100644 --- a/client-node-tests/src/servers/fullNotebookServer.ts +++ b/client-node-tests/src/servers/fullNotebookServer.ts @@ -6,7 +6,10 @@ import { createConnection, InitializeParams, ServerCapabilities, TextDocumentSyncKind, RequestType, DidOpenNotebookDocumentNotification, DidChangeNotebookDocumentNotification, DidSaveNotebookDocumentNotification, - DidCloseNotebookDocumentNotification + DidCloseNotebookDocumentNotification, + PublishDiagnosticsNotification, + DocumentDiagnosticReport, + NotificationType } from 'vscode-languageserver/node'; const connection = createConnection(); @@ -20,6 +23,13 @@ namespace GotNotifiedRequest { export const type = new RequestType(method); } +namespace SetDiagnosticsNotification { + export const method: 'testing/setDiagnostics' = 'testing/setDiagnostics'; + export const type = new NotificationType(method); +} + +const diagnostics = new Map(); + connection.onInitialize((_params: InitializeParams): any => { const capabilities: ServerCapabilities = { textDocumentSync: TextDocumentSyncKind.Full, @@ -28,6 +38,12 @@ connection.onInitialize((_params: InitializeParams): any => { notebook: { notebookType: 'jupyter-notebook' }, cells: [{ language: 'python' }] }] + }, + diagnosticProvider: { + identifier: 'diagnostic-provider', + documentSelector: null, + interFileDependencies: false, + workspaceDiagnostics: false } }; return { capabilities }; @@ -45,6 +61,11 @@ connection.notebooks.synchronization.onDidSaveNotebookDocument(() => { connection.notebooks.synchronization.onDidCloseNotebookDocument(() => { receivedNotifications.add(DidCloseNotebookDocumentNotification.method); }); +connection.languages.diagnostics.on((params) => { + receivedNotifications.add(PublishDiagnosticsNotification.method); + const result = diagnostics.get(params.textDocument.uri) || { kind: 'unchanged', resultId: params.previousResultId || '' }; + return result; +}); connection.onRequest(GotNotifiedRequest.type, (method: string) => { const result = receivedNotifications.has(method); @@ -54,5 +75,9 @@ connection.onRequest(GotNotifiedRequest.type, (method: string) => { return result; }); +connection.onNotification(SetDiagnosticsNotification.method, (params) => { + diagnostics.set(params.uri, params.report); +}); + // Listen on the connection connection.listen(); diff --git a/client/src/common/diagnostic.ts b/client/src/common/diagnostic.ts index ff3c8605e..e20751316 100644 --- a/client/src/common/diagnostic.ts +++ b/client/src/common/diagnostic.ts @@ -8,7 +8,8 @@ import * as minimatch from 'minimatch'; import { Disposable, languages as Languages, window as Window, workspace as Workspace, CancellationToken, ProviderResult, Diagnostic as VDiagnostic, CancellationTokenSource, TextDocument, CancellationError, Event as VEvent, EventEmitter, DiagnosticCollection, Uri, TabInputText, TabInputTextDiff, - TabChangeEvent, Event, TabInputCustom, workspace + TabChangeEvent, Event, TabInputCustom, workspace, + TabInputNotebook } from 'vscode'; import { @@ -16,7 +17,6 @@ import { DidSaveTextDocumentNotification, DidCloseTextDocumentNotification, LinkedMap, Touch, RAL, TextDocumentFilter, PreviousResultId, DiagnosticRegistrationOptions, DiagnosticServerCancellationData, DocumentDiagnosticParams, DocumentDiagnosticRequest, DocumentDiagnosticReportKind, WorkspaceDocumentDiagnosticReport, WorkspaceDiagnosticRequest, WorkspaceDiagnosticParams, DiagnosticOptions, DiagnosticRefreshRequest, DiagnosticTag, - DidChangeNotebookDocumentNotification, NotebookDocumentSyncRegistrationType } from 'vscode-languageserver-protocol'; @@ -24,6 +24,7 @@ import { generateUuid } from './utils/uuid'; import { TextDocumentLanguageFeature, FeatureClient, LSPCancellationError } from './features'; +import { NotebookDocumentSyncFeature } from './notebook'; function ensure(target: T, key: K): T[K] { if (target[key] === void 0) { @@ -270,6 +271,16 @@ class Tabs { public isVisible(document: TextDocument | Uri): boolean { const uri = document instanceof Uri ? document : document.uri; + if (uri.scheme === NotebookDocumentSyncFeature.CellScheme) { + // Notebook cells aren't in the list of tabs, but the notebook should be. + return Workspace.notebookDocuments.some(notebook => { + if (this.open.has(notebook.uri.toString())) { + const cell = notebook.getCells().find(cell => cell.document.uri.toString() === uri.toString()); + return cell !== undefined; + } + return false; + }); + } return this.open.has(uri.toString()); } @@ -291,6 +302,8 @@ class Tabs { uri = input.modified; } else if (input instanceof TabInputCustom) { uri = input.uri; + } else if (input instanceof TabInputNotebook) { + uri = input.uri; } if (uri !== undefined && !seen.has(uri.toString())) { seen.add(uri.toString()); diff --git a/client/src/common/features.ts b/client/src/common/features.ts index 90122c1c9..14970d784 100644 --- a/client/src/common/features.ts +++ b/client/src/common/features.ts @@ -15,8 +15,7 @@ import { import { CallHierarchyPrepareRequest, ClientCapabilities, CodeActionRequest, CodeLensRequest, CompletionRequest, DeclarationRequest, DefinitionRequest, - DidChangeNotebookDocumentNotification, - DidChangeTextDocumentNotification, DidCloseTextDocumentNotification, DidCreateFilesNotification, DidDeleteFilesNotification, DidOpenNotebookDocumentNotification, DidOpenTextDocumentNotification, + DidChangeTextDocumentNotification, DidCloseTextDocumentNotification, DidCreateFilesNotification, DidDeleteFilesNotification, DidOpenTextDocumentNotification, DidRenameFilesNotification, DidSaveTextDocumentNotification, DocumentColorRequest, DocumentDiagnosticRequest, DocumentFormattingRequest, DocumentHighlightRequest, DocumentLinkRequest, DocumentOnTypeFormattingRequest, DocumentRangeFormattingRequest, DocumentSelector, DocumentSymbolRequest, ExecuteCommandOptions, ExecuteCommandRequest, FileOperationRegistrationOptions, FoldingRangeRequest, GenericNotificationHandler, GenericRequestHandler, HoverRequest, ImplementationRequest, InitializeParams, InlayHintRequest, InlineCompletionRegistrationOptions, InlineCompletionRequest, InlineValueRequest, diff --git a/client/src/common/notebook.ts b/client/src/common/notebook.ts index 45f083b08..e26d181a3 100644 --- a/client/src/common/notebook.ts +++ b/client/src/common/notebook.ts @@ -16,7 +16,7 @@ import * as Is from './utils/is'; import * as _c2p from './codeConverter'; import * as _p2c from './protocolConverter'; -import { DynamicFeature, FeatureClient, RegistrationData, FeatureState, NotifyingFeature, NotificationSendEvent } from './features'; +import { DynamicFeature, FeatureClient, RegistrationData, FeatureState } from './features'; function ensure(target: T, key: K): T[K] { @@ -513,6 +513,10 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature return this._onSaveNotificationSent.event; } + public addDisposables(disposables: vscode.Disposable[]): void { + this.disposables.push(...disposables); + } + public handles(textDocument: vscode.TextDocument): boolean { if (vscode.languages.match(this.selector, textDocument) > 0) { return true; @@ -703,13 +707,12 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature } private async doSendOpen(notebookDocument: vscode.NotebookDocument, cells: vscode.NotebookCell[]): Promise { - this._onOpenNotificationSent.fire(notebookDocument); const send = async (notebookDocument: vscode.NotebookDocument, cells: vscode.NotebookCell[]): Promise => { - const nb = Converter.c2p.asNotebookDocument(notebookDocument, cells, this.client.code2ProtocolConverter); const cellDocuments: TextDocumentItem[] = cells.map(cell => this.client.code2ProtocolConverter.asTextDocumentItem(cell.document)); try { + this._onOpenNotificationSent.fire(notebookDocument); await this.client.sendNotification(proto.DidOpenNotebookDocumentNotification.type, { - notebookDocument: nb, + notebookDocument: Converter.c2p.asNotebookDocument(notebookDocument, cells, this.client.code2ProtocolConverter), cellTextDocuments: cellDocuments }); } catch (error) { @@ -727,9 +730,9 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature } private async doSendChange(event: VNotebookDocumentChangeEvent, cells: vscode.NotebookCell[] | undefined = this.getMatchingCells(event.notebook)): Promise { - this._onChangeNotificationSent.fire(event); const send = async (event: VNotebookDocumentChangeEvent): Promise => { try { + this._onChangeNotificationSent.fire(event); await this.client.sendNotification(proto.DidChangeNotebookDocumentNotification.type, { notebookDocument: Converter.c2p.asVersionedNotebookDocumentIdentifier(event.notebook, this.client.code2ProtocolConverter), change: Converter.c2p.asNotebookDocumentChangeEvent(event, this.client.code2ProtocolConverter) @@ -751,9 +754,9 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature } private async doSendSave(notebookDocument: vscode.NotebookDocument): Promise { - this._onSaveNotificationSent.fire(notebookDocument); const send = async (notebookDocument: vscode.NotebookDocument): Promise => { try { + this._onSaveNotificationSent.fire(notebookDocument); await this.client.sendNotification(proto.DidSaveNotebookDocumentNotification.type, { notebookDocument: { uri: this.client.code2ProtocolConverter.asUri(notebookDocument.uri) } }); @@ -771,9 +774,9 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature } private async doSendClose(notebookDocument: vscode.NotebookDocument, cells: vscode.NotebookCell[]): Promise { - this._onCloseNotificationSent.fire(notebookDocument); const send = async (notebookDocument: vscode.NotebookDocument, cells: vscode.NotebookCell[]): Promise => { try { + this._onCloseNotificationSent.fire(notebookDocument); await this.client.sendNotification(proto.DidCloseNotebookDocumentNotification.type, { notebookDocument: { uri: this.client.code2ProtocolConverter.asUri(notebookDocument.uri) }, cellTextDocuments: cells.map(cell => this.client.code2ProtocolConverter.asTextDocumentIdentifier(cell.document)) @@ -912,15 +915,19 @@ export class NotebookDocumentSyncFeature implements DynamicFeature; private readonly registrations: Map; private dedicatedChannel: vscode.DocumentSelector | undefined; + private readonly _onChangeNotificationSent: vscode.EventEmitter; + private readonly _onOpenNotificationSent: vscode.EventEmitter; + private readonly _onCloseNotificationSent: vscode.EventEmitter; + private readonly _onSaveNotificationSent: vscode.EventEmitter; constructor(client: FeatureClient) { this.client = client; this.registrations = new Map(); this.registrationType = proto.NotebookDocumentSyncRegistrationType.type; - this.onChangeNotificationSent = new vscode.EventEmitter().event; - this.onOpenNotificationSent = new vscode.EventEmitter().event; - this.onCloseNotificationSent = new vscode.EventEmitter().event; - this.onSaveNotificationSent = new vscode.EventEmitter().event; + this._onChangeNotificationSent = new vscode.EventEmitter(); + this._onOpenNotificationSent = new vscode.EventEmitter(); + this._onCloseNotificationSent = new vscode.EventEmitter(); + this._onSaveNotificationSent = new vscode.EventEmitter(); // We don't receive an event for cells where the document changes its language mode // Since we allow servers to filter on the language mode we fire such an event ourselves. @@ -992,13 +999,21 @@ export class NotebookDocumentSyncFeature implements DynamicFeature; - public onOpenNotificationSent: vscode.Event; + public get onOpenNotificationSent(): vscode.Event { + return this._onOpenNotificationSent.event; + } - public onChangeNotificationSent: vscode.Event; + public get onChangeNotificationSent(): vscode.Event { + return this._onChangeNotificationSent.event; + } - public onCloseNotificationSent: vscode.Event; + public get onCloseNotificationSent(): vscode.Event { + return this._onCloseNotificationSent.event; + } - public onSaveNotificationSent: vscode.Event; + public get onSaveNotificationSent(): vscode.Event { + return this._onSaveNotificationSent.event; + } public fillClientCapabilities(capabilities: proto.ClientCapabilities): void { const synchronization = ensure(ensure(capabilities, 'notebookDocument')!, 'synchronization')!; @@ -1025,10 +1040,12 @@ export class NotebookDocumentSyncFeature implements DynamicFeature): void { const provider = new NotebookDocumentSyncFeatureProvider(this.client, data.registerOptions); - this.onChangeNotificationSent = provider.onChangeNotificationSent; - this.onOpenNotificationSent = provider.onOpenNotificationSent; - this.onCloseNotificationSent = provider.onCloseNotificationSent; - this.onSaveNotificationSent = provider.onSaveNotificationSent; + const disposables: vscode.Disposable[] = []; + disposables.push(provider.onChangeNotificationSent((p) => this._onChangeNotificationSent.fire(p))); + disposables.push(provider.onOpenNotificationSent((p) => this._onOpenNotificationSent.fire(p))); + disposables.push(provider.onCloseNotificationSent((p) => this._onCloseNotificationSent.fire(p))); + disposables.push(provider.onSaveNotificationSent((p) => this._onSaveNotificationSent.fire(p))); + provider.addDisposables(disposables); this.registrations.set(data.id, provider); } From a2ea72aabcd7d4a8d473ce2635bb251033c788a3 Mon Sep 17 00:00:00 2001 From: Rich Chiodo false Date: Mon, 29 Apr 2024 10:23:43 -0700 Subject: [PATCH 3/5] Review feedback --- client-node-tests/src/index.ts | 4 +- client/src/common/diagnostic.ts | 22 ++++++---- client/src/common/notebook.ts | 72 +++++++++------------------------ 3 files changed, 36 insertions(+), 62 deletions(-) diff --git a/client-node-tests/src/index.ts b/client-node-tests/src/index.ts index 5bbfa290d..a62994998 100644 --- a/client-node-tests/src/index.ts +++ b/client-node-tests/src/index.ts @@ -6,7 +6,9 @@ export function run(testsRoot: string, cb: (error: any, failures?: number) => vo // Create the mocha test const mocha = new Mocha({ ui: 'tdd', - color: true + color: true, + timeout: 60000, + grep: 'pull diagnostics' }); diff --git a/client/src/common/diagnostic.ts b/client/src/common/diagnostic.ts index e20751316..71d79bd85 100644 --- a/client/src/common/diagnostic.ts +++ b/client/src/common/diagnostic.ts @@ -8,8 +8,7 @@ import * as minimatch from 'minimatch'; import { Disposable, languages as Languages, window as Window, workspace as Workspace, CancellationToken, ProviderResult, Diagnostic as VDiagnostic, CancellationTokenSource, TextDocument, CancellationError, Event as VEvent, EventEmitter, DiagnosticCollection, Uri, TabInputText, TabInputTextDiff, - TabChangeEvent, Event, TabInputCustom, workspace, - TabInputNotebook + TabChangeEvent, Event, TabInputCustom, workspace, TabInputNotebook, NotebookCell } from 'vscode'; import { @@ -931,6 +930,11 @@ class DiagnosticFeatureProviderImpl implements DiagnosticProviderShape { : Languages.match(documentSelector, document) > 0 && tabs.isVisible(document); }; + const matchesCell = (cell: NotebookCell): boolean => { + // Cells match if the language is allowed and if the containing notebook is visible. + return Languages.match(documentSelector, cell.document) > 0 && tabs.isVisible(cell.notebook.uri); + }; + const isActiveDocument = (document: TextDocument | Uri): boolean => { return document instanceof Uri ? this.activeTextDocument?.uri.toString() === document.toString() @@ -989,7 +993,7 @@ class DiagnosticFeatureProviderImpl implements DiagnosticProviderShape { disposables.push(notebookFeature.onOpenNotificationSent((event) => { // Send a pull for all opened cells in the notebook. for (const cell of event.getCells()) { - if (matches(cell.document)) { + if (matchesCell(cell)) { this.diagnosticRequestor.pull(cell.document, () => { addToBackgroundIfNeeded(cell.document); }); } } @@ -1034,7 +1038,7 @@ class DiagnosticFeatureProviderImpl implements DiagnosticProviderShape { // Do the same for already open notebook cells. for (const notebookDocument of Workspace.notebookDocuments) { for (const cell of notebookDocument.getCells()) { - if (matches(cell.document)) { + if (matchesCell(cell)) { this.diagnosticRequestor.pull(cell.document, () => { addToBackgroundIfNeeded(cell.document); }); pulledTextDocuments.add(cell.document.uri.toString()); } @@ -1064,9 +1068,11 @@ class DiagnosticFeatureProviderImpl implements DiagnosticProviderShape { })); disposables.push(notebookFeature.onChangeNotificationSent(async (event) => { // Send a pull for all changed cells in the notebook. - const changedCells = event.cells?.textContent || []; + const textEvents = event.cells?.textContent || []; + const changedCells = textEvents.map( + (c) => event.notebook.getCells().find(cell => cell.document.uri.toString() === c.document.uri.toString())); for (const cell of changedCells) { - if (matches(cell.document)) { + if (cell && matchesCell(cell)) { this.diagnosticRequestor.pull(cell.document, () => { this.backgroundScheduler.trigger(); }); } } @@ -1080,7 +1086,7 @@ class DiagnosticFeatureProviderImpl implements DiagnosticProviderShape { // Send a pull for any new opened cells. const openedCells = event.cells?.structure?.didOpen || []; for (const cell of openedCells) { - if (matches(cell.document)) { + if (matchesCell(cell)) { this.diagnosticRequestor.pull(cell.document, () => { this.backgroundScheduler.trigger(); }); } } @@ -1097,7 +1103,7 @@ class DiagnosticFeatureProviderImpl implements DiagnosticProviderShape { })); disposables.push(notebookFeature.onSaveNotificationSent((event) => { for (const cell of event.getCells()) { - if (matches(cell.document)) { + if (matchesCell(cell)) { this.diagnosticRequestor.pull(cell.document); } } diff --git a/client/src/common/notebook.ts b/client/src/common/notebook.ts index e26d181a3..7d2eaec8c 100644 --- a/client/src/common/notebook.ts +++ b/client/src/common/notebook.ts @@ -413,25 +413,12 @@ export type NotebookDocumentMiddleware = { }; export interface NotebookDocumentSyncFeatureShape { - onOpenNotificationSent: vscode.Event; - onChangeNotificationSent: vscode.Event; - onCloseNotificationSent: vscode.Event; - onSaveNotificationSent: vscode.Event; sendDidOpenNotebookDocument(notebookDocument: vscode.NotebookDocument): Promise; sendDidSaveNotebookDocument(notebookDocument: vscode.NotebookDocument): Promise; sendDidChangeNotebookDocument(event: VNotebookDocumentChangeEvent): Promise; sendDidCloseNotebookDocument(notebookDocument: vscode.NotebookDocument): Promise; } -export interface NotificationNotebookSendEvent

{ - type: proto.ProtocolNotificationType; - params: P; -} - -export interface NotifyingNotebookFeature

{ - onNotificationSent: vscode.Event>; -} - class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeatureShape { @@ -441,22 +428,21 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature private readonly notebookDidOpen: Set; private readonly disposables: vscode.Disposable[]; private readonly selector: vscode.DocumentSelector; - private readonly _onChangeNotificationSent: vscode.EventEmitter; - private readonly _onOpenNotificationSent: vscode.EventEmitter; - private readonly _onCloseNotificationSent: vscode.EventEmitter; - private readonly _onSaveNotificationSent: vscode.EventEmitter; - constructor(client: FeatureClient, options: proto.NotebookDocumentSyncOptions) { + constructor( + client: FeatureClient, + options: proto.NotebookDocumentSyncOptions, + private readonly _onChangeNotificationSent: vscode.EventEmitter, + private readonly _onOpenNotificationSent: vscode.EventEmitter, + private readonly _onCloseNotificationSent: vscode.EventEmitter, + private readonly _onSaveNotificationSent: vscode.EventEmitter + ) { this.client = client; this.options = options; this.notebookSyncInfo = new Map(); this.notebookDidOpen = new Set(); this.disposables = []; this.selector = client.protocol2CodeConverter.asDocumentSelector($NotebookDocumentSyncOptions.asDocumentSelector(options)); - this._onChangeNotificationSent = new vscode.EventEmitter(); - this._onOpenNotificationSent = new vscode.EventEmitter(); - this._onCloseNotificationSent = new vscode.EventEmitter(); - this._onSaveNotificationSent = new vscode.EventEmitter(); // open vscode.workspace.onDidOpenNotebookDocument((notebookDocument) => { @@ -497,26 +483,6 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature return 'notebook'; } - public get onChangeNotificationSent(): vscode.Event { - return this._onChangeNotificationSent.event; - } - - public get onOpenNotificationSent(): vscode.Event { - return this._onOpenNotificationSent.event; - } - - public get onCloseNotificationSent(): vscode.Event { - return this._onCloseNotificationSent.event; - } - - public get onSaveNotificationSent(): vscode.Event { - return this._onSaveNotificationSent.event; - } - - public addDisposables(disposables: vscode.Disposable[]): void { - this.disposables.push(...disposables); - } - public handles(textDocument: vscode.TextDocument): boolean { if (vscode.languages.match(this.selector, textDocument) > 0) { return true; @@ -710,11 +676,11 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature const send = async (notebookDocument: vscode.NotebookDocument, cells: vscode.NotebookCell[]): Promise => { const cellDocuments: TextDocumentItem[] = cells.map(cell => this.client.code2ProtocolConverter.asTextDocumentItem(cell.document)); try { - this._onOpenNotificationSent.fire(notebookDocument); await this.client.sendNotification(proto.DidOpenNotebookDocumentNotification.type, { notebookDocument: Converter.c2p.asNotebookDocument(notebookDocument, cells, this.client.code2ProtocolConverter), cellTextDocuments: cellDocuments }); + this._onOpenNotificationSent.fire(notebookDocument); } catch (error) { this.client.error('Sending DidOpenNotebookDocumentNotification failed', error); throw error; @@ -732,11 +698,11 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature private async doSendChange(event: VNotebookDocumentChangeEvent, cells: vscode.NotebookCell[] | undefined = this.getMatchingCells(event.notebook)): Promise { const send = async (event: VNotebookDocumentChangeEvent): Promise => { try { - this._onChangeNotificationSent.fire(event); await this.client.sendNotification(proto.DidChangeNotebookDocumentNotification.type, { notebookDocument: Converter.c2p.asVersionedNotebookDocumentIdentifier(event.notebook, this.client.code2ProtocolConverter), change: Converter.c2p.asNotebookDocumentChangeEvent(event, this.client.code2ProtocolConverter) }); + this._onChangeNotificationSent.fire(event); } catch (error) { this.client.error('Sending DidChangeNotebookDocumentNotification failed', error); throw error; @@ -756,10 +722,10 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature private async doSendSave(notebookDocument: vscode.NotebookDocument): Promise { const send = async (notebookDocument: vscode.NotebookDocument): Promise => { try { - this._onSaveNotificationSent.fire(notebookDocument); await this.client.sendNotification(proto.DidSaveNotebookDocumentNotification.type, { notebookDocument: { uri: this.client.code2ProtocolConverter.asUri(notebookDocument.uri) } }); + this._onSaveNotificationSent.fire(notebookDocument); } catch (error) { this.client.error('Sending DidSaveNotebookDocumentNotification failed', error); throw error; @@ -776,11 +742,11 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature private async doSendClose(notebookDocument: vscode.NotebookDocument, cells: vscode.NotebookCell[]): Promise { const send = async (notebookDocument: vscode.NotebookDocument, cells: vscode.NotebookCell[]): Promise => { try { - this._onCloseNotificationSent.fire(notebookDocument); await this.client.sendNotification(proto.DidCloseNotebookDocumentNotification.type, { notebookDocument: { uri: this.client.code2ProtocolConverter.asUri(notebookDocument.uri) }, cellTextDocuments: cells.map(cell => this.client.code2ProtocolConverter.asTextDocumentIdentifier(cell.document)) }); + this._onCloseNotificationSent.fire(notebookDocument); } catch (error) { this.client.error('Sending DidCloseNotebookDocumentNotification failed', error); throw error; @@ -1039,13 +1005,13 @@ export class NotebookDocumentSyncFeature implements DynamicFeature): void { - const provider = new NotebookDocumentSyncFeatureProvider(this.client, data.registerOptions); - const disposables: vscode.Disposable[] = []; - disposables.push(provider.onChangeNotificationSent((p) => this._onChangeNotificationSent.fire(p))); - disposables.push(provider.onOpenNotificationSent((p) => this._onOpenNotificationSent.fire(p))); - disposables.push(provider.onCloseNotificationSent((p) => this._onCloseNotificationSent.fire(p))); - disposables.push(provider.onSaveNotificationSent((p) => this._onSaveNotificationSent.fire(p))); - provider.addDisposables(disposables); + const provider = new NotebookDocumentSyncFeatureProvider( + this.client, + data.registerOptions, + this._onChangeNotificationSent, + this._onOpenNotificationSent, + this._onCloseNotificationSent, + this._onSaveNotificationSent); this.registrations.set(data.id, provider); } From 0e2cf1681851d356fcf06e6025bc8feeef705a9d Mon Sep 17 00:00:00 2001 From: Rich Chiodo false Date: Mon, 29 Apr 2024 10:26:58 -0700 Subject: [PATCH 4/5] Remove temp test change --- client-node-tests/src/index.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/client-node-tests/src/index.ts b/client-node-tests/src/index.ts index a62994998..5bbfa290d 100644 --- a/client-node-tests/src/index.ts +++ b/client-node-tests/src/index.ts @@ -6,9 +6,7 @@ export function run(testsRoot: string, cb: (error: any, failures?: number) => vo // Create the mocha test const mocha = new Mocha({ ui: 'tdd', - color: true, - timeout: 60000, - grep: 'pull diagnostics' + color: true }); From 3c26f3691ddb7cdb66e25119cbd90185b8691f24 Mon Sep 17 00:00:00 2001 From: Rich Chiodo false Date: Wed, 8 May 2024 09:05:32 -0700 Subject: [PATCH 5/5] Review feedback --- client/src/common/notebook.ts | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/client/src/common/notebook.ts b/client/src/common/notebook.ts index 7d2eaec8c..f162e8484 100644 --- a/client/src/common/notebook.ts +++ b/client/src/common/notebook.ts @@ -428,14 +428,18 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature private readonly notebookDidOpen: Set; private readonly disposables: vscode.Disposable[]; private readonly selector: vscode.DocumentSelector; + private readonly onChangeNotificationSent: vscode.EventEmitter; + private readonly onOpenNotificationSent: vscode.EventEmitter; + private readonly onCloseNotificationSent: vscode.EventEmitter; + private readonly onSaveNotificationSent: vscode.EventEmitter; constructor( client: FeatureClient, options: proto.NotebookDocumentSyncOptions, - private readonly _onChangeNotificationSent: vscode.EventEmitter, - private readonly _onOpenNotificationSent: vscode.EventEmitter, - private readonly _onCloseNotificationSent: vscode.EventEmitter, - private readonly _onSaveNotificationSent: vscode.EventEmitter + onChangeNotificationSent: vscode.EventEmitter, + onOpenNotificationSent: vscode.EventEmitter, + onCloseNotificationSent: vscode.EventEmitter, + onSaveNotificationSent: vscode.EventEmitter ) { this.client = client; this.options = options; @@ -443,6 +447,10 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature this.notebookDidOpen = new Set(); this.disposables = []; this.selector = client.protocol2CodeConverter.asDocumentSelector($NotebookDocumentSyncOptions.asDocumentSelector(options)); + this.onChangeNotificationSent = onChangeNotificationSent; + this.onOpenNotificationSent = onOpenNotificationSent; + this.onCloseNotificationSent = onCloseNotificationSent; + this.onSaveNotificationSent = onSaveNotificationSent; // open vscode.workspace.onDidOpenNotebookDocument((notebookDocument) => { @@ -680,7 +688,7 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature notebookDocument: Converter.c2p.asNotebookDocument(notebookDocument, cells, this.client.code2ProtocolConverter), cellTextDocuments: cellDocuments }); - this._onOpenNotificationSent.fire(notebookDocument); + this.onOpenNotificationSent.fire(notebookDocument); } catch (error) { this.client.error('Sending DidOpenNotebookDocumentNotification failed', error); throw error; @@ -702,7 +710,7 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature notebookDocument: Converter.c2p.asVersionedNotebookDocumentIdentifier(event.notebook, this.client.code2ProtocolConverter), change: Converter.c2p.asNotebookDocumentChangeEvent(event, this.client.code2ProtocolConverter) }); - this._onChangeNotificationSent.fire(event); + this.onChangeNotificationSent.fire(event); } catch (error) { this.client.error('Sending DidChangeNotebookDocumentNotification failed', error); throw error; @@ -725,7 +733,7 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature await this.client.sendNotification(proto.DidSaveNotebookDocumentNotification.type, { notebookDocument: { uri: this.client.code2ProtocolConverter.asUri(notebookDocument.uri) } }); - this._onSaveNotificationSent.fire(notebookDocument); + this.onSaveNotificationSent.fire(notebookDocument); } catch (error) { this.client.error('Sending DidSaveNotebookDocumentNotification failed', error); throw error; @@ -746,7 +754,7 @@ class NotebookDocumentSyncFeatureProvider implements NotebookDocumentSyncFeature notebookDocument: { uri: this.client.code2ProtocolConverter.asUri(notebookDocument.uri) }, cellTextDocuments: cells.map(cell => this.client.code2ProtocolConverter.asTextDocumentIdentifier(cell.document)) }); - this._onCloseNotificationSent.fire(notebookDocument); + this.onCloseNotificationSent.fire(notebookDocument); } catch (error) { this.client.error('Sending DidCloseNotebookDocumentNotification failed', error); throw error;