Merge pull request jupyterlab#14475 from afshin/completer-bug

Fix completer bug with cycling through options

packages/codemirror/src/editor.ts

@@ -492,16 +492,14 @@ export class CodeMirrorEditor implements CodeEditor.IEditor {
     const tree = ensureSyntaxTree(this.state, this.doc.length);
     if (tree) {
       tree.iterate({
-        enter: (node: SyntaxNodeRef) => {
-          // If it has a child, it is not a leaf, but we still want to enter
-          if (node.node.firstChild !== null) {
-            return true;
+        enter: (ref: SyntaxNodeRef) => {
+          if (ref.node.firstChild === null) {
+            tokens.push({
+              value: this.state.sliceDoc(ref.from, ref.to),
+              offset: ref.from,
+              type: ref.name
+            });
           }
-          tokens.push({
-            value: this.state.sliceDoc(node.from, node.to),
-            offset: node.from,
-            type: node.name
-          });
           return true;
         }
       });
@@ -517,32 +515,29 @@ export class CodeMirrorEditor implements CodeEditor.IEditor {
     let token: CodeEditor.IToken | null = null;
     if (tree) {
       tree.iterate({
-        enter: (node: SyntaxNodeRef) => {
-          // If it has a child, it is not a leaf, but we still want to enter
-          if (node.node.firstChild !== null) {
+        enter: (ref: SyntaxNodeRef) => {
+          // If a token has already been discovered, stop iterating.
+          if (token) {
+            return false;
+          }
+          // If it is not a leaf, keep iterating.
+          if (ref.node.firstChild) {
             return true;
           }
-          if (offset >= node.from && offset <= node.to) {
+          // If the relevant leaf token has been found, stop iterating.
+          if (offset >= ref.from && offset <= ref.to) {
             token = {
-              value: this.state.sliceDoc(node.from, node.to),
-              offset: node.from,
-              type: node.name
+              value: this.state.sliceDoc(ref.from, ref.to),
+              offset: ref.from,
+              type: ref.name
             };
-            // We have just found the relevant leaf token, no need to iterate further
             return false;
           }
           return true;
         }
       });
     }
-    if (token !== null) {
-      return token;
-    } else {
-      return {
-        value: '',
-        offset: offset
-      };
-    }
+    return token || { offset, value: '' };
   }
 
   /**

packages/codemirror/test/editor.spec.ts

@@ -457,6 +457,17 @@ describe('CodeMirrorEditor', () => {
         value: 'bar'
       });
     });
+    it('should return preceeding token when it is the last token', async () => {
+      model.mimeType = 'text/x-python';
+      model.sharedModel.setSource('import');
+      // Needed to have the sharedModel content transferred to the editor document
+      await sleep(0.01);
+      expect(editor.getTokenAt(6)).toStrictEqual({
+        type: 'import',
+        offset: 0,
+        value: 'import'
+      });
+    });
   });
 
   describe('#getTokens()', () => {

packages/completer/src/model.ts

@@ -368,13 +368,13 @@ export class CompleterModel implements Completer.IModel {
     let results: Private.ICompletionMatch[] = [];
     for (let item of items) {
       // See if label matches query string
-      // With ICompletionItems, the label may include parameters, so we exclude them from the matcher.
+      // With ICompletionItems, the label may include parameters,
+      // so we exclude them from the matcher.
       // e.g. Given label `foo(b, a, r)` and query `bar`,
       // don't count parameters, `b`, `a`, and `r` as matches.
       const index = item.label.indexOf('(');
-      let prefix = index > -1 ? item.label.substring(0, index) : item.label;
-      prefix = escapeHTML(prefix);
-      let match = StringExt.matchSumOfSquares(prefix, query);
+      const text = index > -1 ? item.label.substring(0, index) : item.label;
+      const match = StringExt.matchSumOfSquares(escapeHTML(text), query);
       // Filter non-matching items.
       if (match) {
         // Highlight label text if there's a match

packages/completer/src/widget.ts

@@ -522,30 +522,27 @@ export class Completer extends Widget {
   private _cycle(direction: Private.scrollType): void {
     const items = this.node.querySelectorAll(`.${ITEM_CLASS}`);
     const index = this._activeIndex;
+    const last = items.length - 1;
     let active = this.node.querySelector(`.${ACTIVE_CLASS}`) as HTMLElement;
     active.classList.remove(ACTIVE_CLASS);
 
-    if (direction === 'up') {
-      this._activeIndex = index === 0 ? items.length - 1 : index - 1;
-    } else if (direction === 'down') {
-      this._activeIndex = index < items.length - 1 ? index + 1 : 0;
-    } else {
-      // Measure the number of items on a page.
-      const boxHeight = this.node.getBoundingClientRect().height;
-      const itemHeight = active.getBoundingClientRect().height;
-      const pageLength = Math.floor(boxHeight / itemHeight);
-
-      // Update the index
-      if (direction === 'pageUp') {
-        this._activeIndex = index - pageLength;
-      } else {
-        this._activeIndex = index + pageLength;
+    switch (direction) {
+      case 'up':
+        this._activeIndex = index === 0 ? last : index - 1;
+        break;
+      case 'down':
+        this._activeIndex = index < last ? index + 1 : 0;
+        break;
+      case 'pageUp':
+      case 'pageDown': {
+        // Measure the number of items on a page and clamp to the list length.
+        const container = this.node.getBoundingClientRect();
+        const current = active.getBoundingClientRect();
+        const page = Math.floor(container.height / current.height);
+        const sign = direction === 'pageUp' ? -1 : 1;
+        this._activeIndex = Math.min(Math.max(0, index + sign * page), last);
+        break;
       }
-      // Clamp to the length of the list.
-      this._activeIndex = Math.min(
-        Math.max(0, this._activeIndex),
-        items.length - 1
-      );
     }
 
     active = items[this._activeIndex] as HTMLElement;
@@ -593,12 +590,13 @@ export class Completer extends Widget {
         // then emit a completion signal with that `query` (=subset match),
         // but only if the query has actually changed.
         // See: https://github.com/jupyterlab/jupyterlab/issues/10439#issuecomment-875189540
-        if (model.query && model.query != this._lastSubsetMatch) {
+        if (model.query && model.query !== this._lastSubsetMatch) {
           model.subsetMatch = true;
           this._selected.emit(model.query);
           model.subsetMatch = false;
           this._lastSubsetMatch = model.query;
         }
+
         // If the query changed, update rendering of the options.
         if (populated) {
           this.update();