From c1cafae8c78ef0ff56136dc4a018bef1b5626333 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Collonval?= Date: Tue, 17 Dec 2024 17:16:37 +0100 Subject: [PATCH] Fix continuous inline completion (#17082) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add unit test * Fix wrong positioning in continuous inline completion * Fix undoManager configuration in tests --------- Co-authored-by: Frédéric Collonval Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com> --- packages/codemirror/test/editor.spec.ts | 7 ++- packages/completer/src/handler.ts | 29 ++++++---- packages/completer/src/inline.ts | 30 +++++------ packages/completer/src/testutils.ts | 5 +- packages/completer/test/inline.spec.ts | 54 ++++++++++++++++++- packages/console/test/utils.ts | 5 +- packages/debugger/test/debugger.spec.ts | 2 +- .../fileeditor/test/searchprovider.spec.ts | 5 +- packages/notebook/src/testutils.ts | 5 +- 9 files changed, 108 insertions(+), 34 deletions(-) diff --git a/packages/codemirror/test/editor.spec.ts b/packages/codemirror/test/editor.spec.ts index 08a44da9fc57..5ad15a7ee920 100644 --- a/packages/codemirror/test/editor.spec.ts +++ b/packages/codemirror/test/editor.spec.ts @@ -70,7 +70,12 @@ describe('CodeMirrorEditor', () => { model, languages, // Binding between the editor and the Yjs model - extensions: [ybinding({ ytext: sharedModel.ysource })] + extensions: [ + ybinding({ + ytext: sharedModel.ysource, + undoManager: sharedModel.undoManager ?? undefined + }) + ] }); }); diff --git a/packages/completer/src/handler.ts b/packages/completer/src/handler.ts index 5bd0326899f3..c4a716f76420 100644 --- a/packages/completer/src/handler.ts +++ b/packages/completer/src/handler.ts @@ -1,13 +1,6 @@ // Copyright (c) Jupyter Development Team. // Distributed under the terms of the Modified BSD License. -import { - CodeEditor, - COMPLETER_ACTIVE_CLASS, - COMPLETER_ENABLED_CLASS, - COMPLETER_LINE_BEGINNING_CLASS -} from '@jupyterlab/codeeditor'; -import { Text } from '@jupyterlab/coreutils'; import { CellChange, FileChange, @@ -16,12 +9,22 @@ import { ISharedText, SourceChange } from '@jupyter/ydoc'; +import { + CodeEditor, + COMPLETER_ACTIVE_CLASS, + COMPLETER_ENABLED_CLASS, + COMPLETER_LINE_BEGINNING_CLASS +} from '@jupyterlab/codeeditor'; +import { Text } from '@jupyterlab/coreutils'; import { IDataConnector } from '@jupyterlab/statedb'; import { LabIcon } from '@jupyterlab/ui-components'; import { IDisposable } from '@lumino/disposable'; import { Message, MessageLoop } from '@lumino/messaging'; import { ISignal, Signal } from '@lumino/signaling'; +import type { TransactionSpec } from '@codemirror/state'; +import type { CodeMirrorEditor } from '@jupyterlab/codemirror'; +import { InlineCompleter } from './inline'; import { CompletionTriggerKind, IInlineCompletionItem, @@ -31,7 +34,6 @@ import { IProviderReconciliator } from './tokens'; import { Completer } from './widget'; -import { InlineCompleter } from './inline'; /** * A completion handler for editors. @@ -204,11 +206,16 @@ export class CompletionHandler implements IDisposable { const { start, end, value } = patch; const cursorBeforeChange = editor.getOffsetAt(editor.getCursorPosition()); - // we need to update the shared model in a single transaction so that the undo manager works as expected - editor.model.sharedModel.updateSource(start, end, value); + // Update the document and the cursor position in the same transaction + // to ensure consistency in listeners to document changes. + // Note: it also ensures a single change is stored by the undo manager. + const transactions: TransactionSpec = { + changes: { from: start, to: end, insert: value } + }; if (cursorBeforeChange <= end && cursorBeforeChange >= start) { - editor.setCursorPosition(editor.getPositionAt(start + value.length)!); + transactions.selection = { anchor: start + value.length }; } + (editor as CodeMirrorEditor).editor.dispatch(transactions); } /** diff --git a/packages/completer/src/inline.ts b/packages/completer/src/inline.ts index caa35d49f74e..f4b1066dcb72 100644 --- a/packages/completer/src/inline.ts +++ b/packages/completer/src/inline.ts @@ -1,23 +1,23 @@ // Copyright (c) Jupyter Development Team. // Distributed under the terms of the Modified BSD License. -import { PanelLayout, Widget } from '@lumino/widgets'; -import { IDisposable } from '@lumino/disposable'; -import { ISignal, Signal } from '@lumino/signaling'; -import { Message } from '@lumino/messaging'; +import type { TransactionSpec } from '@codemirror/state'; +import { SourceChange } from '@jupyter/ydoc'; import { CodeEditor } from '@jupyterlab/codeeditor'; -import { HoverBox } from '@jupyterlab/ui-components'; import { CodeMirrorEditor } from '@jupyterlab/codemirror'; -import { SourceChange } from '@jupyter/ydoc'; -import { kernelIcon, Toolbar } from '@jupyterlab/ui-components'; import { TranslationBundle } from '@jupyterlab/translation'; +import { HoverBox, kernelIcon, Toolbar } from '@jupyterlab/ui-components'; +import { IDisposable } from '@lumino/disposable'; +import { Message } from '@lumino/messaging'; +import { ISignal, Signal } from '@lumino/signaling'; +import { PanelLayout, Widget } from '@lumino/widgets'; +import { GhostTextManager } from './ghost'; +import { CompletionHandler } from './handler'; import { IInlineCompleterFactory, IInlineCompleterSettings, IInlineCompletionList } from './tokens'; -import { CompletionHandler } from './handler'; -import { GhostTextManager } from './ghost'; const INLINE_COMPLETER_CLASS = 'jp-InlineCompleter'; const INLINE_COMPLETER_ACTIVE_CLASS = 'jp-mod-inline-completer-active'; @@ -137,15 +137,13 @@ export class InlineCompleter extends Widget { const requestPosition = editor.getOffsetAt(position); const start = requestPosition; const end = cursorBeforeChange; - // update the shared model in a single transaction so that the undo manager works as expected - editor.model.sharedModel.updateSource( - requestPosition, - cursorBeforeChange, - value - ); + const transactions: TransactionSpec = { + changes: { from: start, to: end, insert: value } + }; if (cursorBeforeChange <= end && cursorBeforeChange >= start) { - editor.setCursorPosition(editor.getPositionAt(start + value.length)!); + transactions.selection = { anchor: start + value.length }; } + (editor as CodeMirrorEditor).editor.dispatch(transactions); model.reset(); this.update(); } diff --git a/packages/completer/src/testutils.ts b/packages/completer/src/testutils.ts index f9f6b8447b40..a4ec3566c206 100644 --- a/packages/completer/src/testutils.ts +++ b/packages/completer/src/testutils.ts @@ -13,7 +13,10 @@ export function createEditorWidget(): CodeEditorWrapper { const factory = (options: CodeEditor.IOptions) => { options.extensions = [ ...(options.extensions ?? []), - ybinding({ ytext: sharedModel.ysource }) + ybinding({ + ytext: sharedModel.ysource, + undoManager: sharedModel.undoManager ?? undefined + }) ]; return new CodeMirrorEditor(options); }; diff --git a/packages/completer/test/inline.spec.ts b/packages/completer/test/inline.spec.ts index 75996e475ab5..d972b8c5775d 100644 --- a/packages/completer/test/inline.spec.ts +++ b/packages/completer/test/inline.spec.ts @@ -7,12 +7,18 @@ import { } from '@jupyterlab/completer'; import { CodeEditorWrapper } from '@jupyterlab/codeeditor'; import { nullTranslator } from '@jupyterlab/translation'; -import { framePromise, simulate } from '@jupyterlab/testing'; +import { framePromise, signalToPromise, simulate } from '@jupyterlab/testing'; import { Signal } from '@lumino/signaling'; import { createEditorWidget } from '@jupyterlab/completer/lib/testutils'; import { Widget } from '@lumino/widgets'; import { MessageLoop } from '@lumino/messaging'; import { Doc, Text } from 'yjs'; +import type { + CellChange, + FileChange, + ISharedText, + SourceChange +} from '@jupyter/ydoc'; const GHOST_TEXT_CLASS = 'jp-GhostText'; const STREAMING_INDICATOR_CLASS = 'jp-GhostText-streamingIndicator'; @@ -86,6 +92,52 @@ describe('completer/inline', () => { 'suggestion a' ); }); + + it('should set the cursor position at the same time as the completion suggestion', () => { + model.setCompletions({ + items: suggestionsAbc + }); + let editorPosition = editorWidget.editor.getCursorPosition(); + + expect(editorPosition).toEqual({ + line: 0, + column: 0 + }); + + const onContentChange = ( + str: ISharedText, + changed: SourceChange | CellChange | FileChange + ) => { + if (changed.sourceChange) { + editorPosition = editorWidget.editor.getCursorPosition(); + } + }; + editorWidget.editor.model.sharedModel.changed.connect(onContentChange); + try { + completer.accept(); + } finally { + editorWidget.editor.model.sharedModel.changed.disconnect( + onContentChange + ); + } + expect(editorPosition).toEqual({ + line: 0, + column: 12 + }); + }); + + it('should be undoable in one step', async () => { + model.setCompletions({ + items: suggestionsAbc + }); + completer.accept(); + const waitForChange = signalToPromise( + editorWidget.editor.model.sharedModel.changed + ); + editorWidget.editor.undo(); + await waitForChange; + expect(editorWidget.editor.model.sharedModel.source).toBe(''); + }); }); describe('#_setText()', () => { diff --git a/packages/console/test/utils.ts b/packages/console/test/utils.ts index cbaf36e9731d..a2d4839fd37e 100644 --- a/packages/console/test/utils.ts +++ b/packages/console/test/utils.ts @@ -20,7 +20,10 @@ const extensions = (() => { factory: ({ model }) => { const sharedModel = model.sharedModel as IYText; return EditorExtensionRegistry.createImmutableExtension([ - ybinding({ ytext: sharedModel.ysource }) + ybinding({ + ytext: sharedModel.ysource, + undoManager: sharedModel.undoManager ?? undefined + }) ]); } }); diff --git a/packages/debugger/test/debugger.spec.ts b/packages/debugger/test/debugger.spec.ts index 104b7209a171..3259b961cedb 100644 --- a/packages/debugger/test/debugger.spec.ts +++ b/packages/debugger/test/debugger.spec.ts @@ -80,7 +80,7 @@ describe('Debugger', () => { factory: ({ model }) => { const m = model.sharedModel as IYText; return EditorExtensionRegistry.createImmutableExtension( - ybinding({ ytext: m.ysource }) + ybinding({ ytext: m.ysource, undoManager: m.undoManager ?? undefined }) ); } }); diff --git a/packages/fileeditor/test/searchprovider.spec.ts b/packages/fileeditor/test/searchprovider.spec.ts index c6d9365c708c..c4c3d1557094 100644 --- a/packages/fileeditor/test/searchprovider.spec.ts +++ b/packages/fileeditor/test/searchprovider.spec.ts @@ -37,7 +37,10 @@ describe('@jupyterlab/fileeditor', () => { name: 'binding', factory: ({ model }) => { return EditorExtensionRegistry.createImmutableExtension( - ybinding({ ytext: (model.sharedModel as any).ysource }) + ybinding({ + ytext: (model.sharedModel as any).ysource, + undoManager: (model.sharedModel as any).undoManager ?? undefined + }) ); } }); diff --git a/packages/notebook/src/testutils.ts b/packages/notebook/src/testutils.ts index a913e7d4ff0f..a8bbf6b4c18f 100644 --- a/packages/notebook/src/testutils.ts +++ b/packages/notebook/src/testutils.ts @@ -107,7 +107,10 @@ export namespace NBTestUtils { name: 'binding', factory: ({ model }) => EditorExtensionRegistry.createImmutableExtension( - ybinding({ ytext: (model.sharedModel as any).ysource }) + ybinding({ + ytext: (model.sharedModel as any).ysource, + undoManager: (model.sharedModel as any).undoManager ?? undefined + }) ) }); const factoryService = new CodeMirrorEditorFactory({