Fix bug with collection item scope (#2741)

We didn't properly pop this stack of iteration states. This led to items being grouped incorrectly. ## Checklist - [x] I have added [tests](https://www.cursorless.org/docs/contributing/test-case-recorder/) - [/] I have updated the [docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and [cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet) - [/] I have not broken the cheatsheet
cursorless-dev · Jan 21, 2025 · 8874c87 · 8874c87
1 parent 4688ad8
commit 8874c87
Show file tree

Hide file tree

Showing 2 changed files with 207 additions and 31 deletions.
diff --git a/data/fixtures/scopes/textual/collectionItem.textual15.scope b/data/fixtures/scopes/textual/collectionItem.textual15.scope
@@ -0,0 +1,180 @@
+{
+    language: {},
+    choices: {
+        disabled: true,
+        items: {
+            colWidth: [2, 10]
+        }
+    },
+    versions: {}
+}
+---
+
+[#1 Content] =
+[#1 Domain] = 1:4-1:16
+      >------------<
+1|     language: {},
+
+[#1 Removal] = 1:4-2:4
+      >-------------
+1|     language: {},
+2|     choices: {
+   ----<
+
+[#1 Trailing delimiter] = 1:16-2:4
+                  >-
+1|     language: {},
+2|     choices: {
+   ----<
+
+[#1 Insertion delimiter] = ",\n"
+
+
+[#2 Content] =
+[#2 Domain] = 2:4-7:5
+      >----------
+2|     choices: {
+3|         disabled: true,
+4|         items: {
+5|             colWidth: [2, 10]
+6|         }
+7|     },
+   -----<
+
+[#2 Removal] = 2:4-8:4
+      >----------
+2|     choices: {
+3|         disabled: true,
+4|         items: {
+5|             colWidth: [2, 10]
+6|         }
+7|     },
+8|     versions: {}
+   ----<
+
+[#2 Leading delimiter] = 1:16-2:4
+                  >-
+1|     language: {},
+2|     choices: {
+   ----<
+
+[#2 Trailing delimiter] = 7:5-8:4
+       >-
+7|     },
+8|     versions: {}
+   ----<
+
+[#2 Insertion delimiter] = ",\n"
+
+
+[#3 Content] =
+[#3 Domain] = 3:8-3:22
+          >--------------<
+3|         disabled: true,
+
+[#3 Removal] = 3:8-4:8
+          >---------------
+3|         disabled: true,
+4|         items: {
+   --------<
+
+[#3 Trailing delimiter] = 3:22-4:8
+                        >-
+3|         disabled: true,
+4|         items: {
+   --------<
+
+[#3 Insertion delimiter] = ",\n"
+
+
+[#4 Content] =
+[#4 Domain] = 4:8-6:9
+          >--------
+4|         items: {
+5|             colWidth: [2, 10]
+6|         }
+   ---------<
+
+[#4 Removal] = 3:22-6:9
+                        >-
+3|         disabled: true,
+4|         items: {
+5|             colWidth: [2, 10]
+6|         }
+   ---------<
+
+[#4 Leading delimiter] = 3:22-4:8
+                        >-
+3|         disabled: true,
+4|         items: {
+   --------<
+
+[#4 Insertion delimiter] = ",\n"
+
+
+[#5 Content] =
+[#5 Domain] = 5:12-5:29
+              >-----------------<
+5|             colWidth: [2, 10]
+
+[#5 Removal] = 5:0-5:29
+  >-----------------------------<
+5|             colWidth: [2, 10]
+
+[#5 Leading delimiter] = 5:0-5:12
+  >------------<
+5|             colWidth: [2, 10]
+
+[#5 Insertion delimiter] = ",\n"
+
+
+[#6 Content] =
+[#6 Domain] = 5:23-5:24
+                         >-<
+5|             colWidth: [2, 10]
+
+[#6 Removal] = 5:23-5:26
+                         >---<
+5|             colWidth: [2, 10]
+
+[#6 Trailing delimiter] = 5:24-5:26
+                          >--<
+5|             colWidth: [2, 10]
+
+[#6 Insertion delimiter] = ", "
+
+
+[#7 Content] =
+[#7 Domain] = 5:26-5:28
+                            >--<
+5|             colWidth: [2, 10]
+
+[#7 Removal] = 5:24-5:28
+                          >----<
+5|             colWidth: [2, 10]
+
+[#7 Leading delimiter] = 5:24-5:26
+                          >--<
+5|             colWidth: [2, 10]
+
+[#7 Insertion delimiter] = ", "
+
+
+[#8 Content] =
+[#8 Domain] = 8:4-8:16
+      >------------<
+8|     versions: {}
+
+[#8 Removal] = 7:5-8:16
+       >-
+7|     },
+8|     versions: {}
+   ----------------<
+
+[#8 Leading delimiter] = 7:5-8:4
+       >-
+7|     },
+8|     versions: {}
+   ----<
+
+[#8 Insertion delimiter] = ",\n"
diff --git a/...s/modifiers/scopeHandlers/CollectionItemScopeHandler/CollectionItemTextualScopeHandler.ts b/...s/modifiers/scopeHandlers/CollectionItemScopeHandler/CollectionItemTextualScopeHandler.ts
@@ -68,51 +68,46 @@ export class CollectionItemTextualScopeHandler extends BaseScopeHandler {
         continue;
       }
 
-      const currentIterationState =
-        iterationStatesStack[iterationStatesStack.length - 1];
+      let currentIterationState: IterationState | undefined;
+
+      // Find current iteration state and pop all states not containing the separator
+      while (iterationStatesStack.length > 0) {
+        const lastState = iterationStatesStack[iterationStatesStack.length - 1];
+        if (lastState.iterationRange.contains(separator)) {
+          currentIterationState = lastState;
+          break;
+        }
+        // We are done with this iteration scope. Add all scopes from it and pop it from the stack.
+        this.addScopes(scopes, lastState);
+        iterationStatesStack.pop();
+      }
 
       // Get range for smallest containing interior
       const containingInteriorRange =
         interiorRangeFinder.getSmallestContaining(separator)?.range;
 
-      // The contain range is either the interior or the line containing the separator
+      // The containing iteration range is either the interior or the line containing the separator
       const containingIterationRange =
         containingInteriorRange ??
         editor.document.lineAt(separator.start.line).range;
 
-      if (currentIterationState != null) {
-        // The current containing iteration range is the same as the previous one. Just append delimiter.
-        if (
-          currentIterationState.iterationRange.isRangeEqual(
-            containingIterationRange,
-          )
-        ) {
-          currentIterationState.delimiters.push(separator);
-          continue;
-        }
-
-        // The current containing range does not intersect previous one. Add scopes and remove state.
-        if (!currentIterationState.iterationRange.contains(separator)) {
-          this.addScopes(scopes, currentIterationState);
-          // Remove already added state
-          iterationStatesStack.pop();
-        }
-      }
-
-      // The current containing range is the same as the previous one. Just append delimiter.
-      if (iterationStatesStack.length > 0) {
-        const lastState = iterationStatesStack[iterationStatesStack.length - 1];
-        if (lastState.iterationRange.isRangeEqual(containingIterationRange)) {
-          lastState.delimiters.push(separator);
-          continue;
-        }
+      // The current containing iteration range is the same as the previous one. Just append delimiter.
+      if (
+        currentIterationState != null &&
+        currentIterationState.iterationRange.isRangeEqual(
+          containingIterationRange,
+        )
+      ) {
+        currentIterationState.delimiters.push(separator);
+        continue;
       }
 
-      // New containing range. Add it to the list.
+      // New containing range. Add it to the set.
       if (containingInteriorRange != null) {
         usedInteriors.add(containingInteriorRange);
       }
 
+      // New containing iteration range. Push it to the stack.
       iterationStatesStack.push({
         editor,
         isEveryScope,
@@ -121,13 +116,14 @@ export class CollectionItemTextualScopeHandler extends BaseScopeHandler {
       });
     }
 
+    // Process any remaining states on the stack
     for (const state of iterationStatesStack) {
       this.addScopes(scopes, state);
     }
 
     // Add interior ranges without a delimiter in them. eg: `[foo]`
     for (const interior of interiorRanges) {
-      if (!usedInteriors.has(interior.range)) {
+      if (!usedInteriors.has(interior.range) && !interior.range.isEmpty) {
         const range = shrinkRangeToFitContent(editor, interior.range);
         if (!range.isEmpty) {
           scopes.push(