Fix continuous inline completion (jupyterlab#17082)

* Add unit test * Fix wrong positioning in continuous inline completion * Fix undoManager configuration in tests --------- Co-authored-by: Frédéric Collonval <[email protected]> Co-authored-by: Michał Krassowski <[email protected]>
krassowski · Dec 17, 2024 · c1cafae · c1cafae
1 parent f7a203d
commit c1cafae
Show file tree

Hide file tree

Showing 9 changed files with 108 additions and 34 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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({