Updated delete action with support for overlapping ranges (#531)

* Updated delete action with support for overlapping ranges

* Remove identical resulting selections and that marks for delete action

* Spelling.se

* Cleanup

* Refactored unify ranges into separate file

* Added comment

* Added fix me comment

* Added test to cut every argument

* Updated move with unify targets

* Sort clipboard tests

* Use real clipboard instead of mock

src/actions/BringMoveSwap.ts

@@ -11,7 +11,7 @@ import update from "immutability-helper";
 import displayPendingEditDecorations from "../util/editDisplayUtils";
 import { performOutsideAdjustment } from "../util/performInsideOutsideAdjustment";
 import { flatten } from "lodash";
-import { Selection, TextEditor, Range, DecorationRangeBehavior } from "vscode";
+import { Selection, TextEditor, DecorationRangeBehavior } from "vscode";
 
 import {
   getTextWithPossibleDelimiter,
@@ -21,6 +21,7 @@ import {
   getSelectionInfo,
   performEditsAndUpdateFullSelectionInfos,
 } from "../core/updateSelections/updateSelections";
+import { unifyTargets } from "../util/unifyRanges";
 
 type ActionType = "bring" | "move" | "swap";
 
@@ -131,7 +132,7 @@ class BringMoveSwap implements Action {
         }
         // Add destination edit
         results.push({
-          range: destination.selection.selection as Range,
+          range: destination.selection.selection,
           text,
           editor: destination.selection.editor,
           originalSelection: destination,
@@ -146,32 +147,36 @@ class BringMoveSwap implements Action {
       // Prevent multiple instances of the same expanded source.
       if (!usedSources.includes(source)) {
         usedSources.push(source);
-        let text: string;
-        let range: Range;
-
         if (this.type !== "move") {
-          text = destination.selection.editor.document.getText(
-            destination.selection.selection
-          );
-          range = source.selection.selection;
-        }
-        // NB: this.type === "move"
-        else {
-          text = "";
-          source = performOutsideAdjustment(source);
-          range = source.selection.selection;
+          results.push({
+            range: source.selection.selection,
+            text: destination.selection.editor.document.getText(
+              destination.selection.selection
+            ),
+            editor: source.selection.editor,
+            originalSelection: source,
+            isSource: true,
+            isReplace: false,
+          });
         }
+      }
+    });
 
+    if (this.type === "move") {
+      let outsideSources = usedSources.map(performOutsideAdjustment);
+      // Unify overlapping targets.
+      outsideSources = unifyTargets(outsideSources);
+      outsideSources.forEach((source) => {
         results.push({
-          range,
-          text,
+          range: source.selection.selection,
+          text: "",
           editor: source.selection.editor,
           originalSelection: source,
           isSource: true,
           isReplace: false,
         });
-      }
-    });
+      });
+    }
 
     return results;
   }

src/actions/Delete.ts

@@ -9,6 +9,7 @@ import { runOnTargetsForEachEditor } from "../util/targetUtils";
 import displayPendingEditDecorations from "../util/editDisplayUtils";
 import { flatten } from "lodash";
 import { performEditsAndUpdateSelections } from "../core/updateSelections/updateSelections";
+import { unifyTargets } from "../util/unifyRanges";
 
 export default class Delete implements Action {
   getTargetPreferences: () => ActionPreferences[] = () => [
@@ -23,6 +24,9 @@ export default class Delete implements Action {
     [targets]: [TypedSelection[]],
     { showDecorations = true } = {}
   ): Promise<ActionReturnValue> {
+    // Unify overlapping targets.
+    targets = unifyTargets(targets);
+
     if (showDecorations) {
       await displayPendingEditDecorations(
         targets,
@@ -44,10 +48,7 @@ export default class Delete implements Action {
           [targets.map((target) => target.selection.selection)]
         );
 
-        return updatedSelections.map((selection) => ({
-          editor,
-          selection,
-        }));
+        return updatedSelections.map((selection) => ({ editor, selection }));
       })
     );
 

src/test/suite/fixtures/recorded/actions/chuckArgMadeAndAir.yml

@@ -0,0 +1,34 @@
+languageId: typescript
+command:
+  version: 1
+  spokenForm: chuck arg made and air
+  action: remove
+  targets:
+    - type: list
+      elements:
+        - type: primitive
+          modifier: {type: containingScope, scopeType: argumentOrParameter, includeSiblings: false}
+          mark: {type: decoratedSymbol, symbolColor: default, character: m}
+        - type: primitive
+          mark: {type: decoratedSymbol, symbolColor: default, character: a}
+initialState:
+  documentContents: "function myFunk(value: string, name: string, age: number) { };"
+  selections:
+    - anchor: {line: 0, character: 0}
+      active: {line: 0, character: 0}
+  marks:
+    default.m:
+      start: {line: 0, character: 31}
+      end: {line: 0, character: 35}
+    default.a:
+      start: {line: 0, character: 45}
+      end: {line: 0, character: 48}
+finalState:
+  documentContents: "function myFunk(value: string) { };"
+  selections:
+    - anchor: {line: 0, character: 0}
+      active: {line: 0, character: 0}
+  thatMark:
+    - anchor: {line: 0, character: 29}
+      active: {line: 0, character: 29}
+fullTargets: [{type: list, elements: [{type: primitive, mark: {type: decoratedSymbol, symbolColor: default, character: m}, selectionType: token, position: contents, insideOutsideType: outside, modifier: {type: containingScope, scopeType: argumentOrParameter, includeSiblings: false}, isImplicit: false}, {type: primitive, mark: {type: decoratedSymbol, symbolColor: default, character: a}, selectionType: token, position: contents, insideOutsideType: outside, modifier: {type: containingScope, scopeType: argumentOrParameter, includeSiblings: false}, isImplicit: false}]}]

src/test/suite/fixtures/recorded/actions/chuckArgMadeAndAirAndJustSoon.yml

@@ -0,0 +1,40 @@
+languageId: typescript
+command:
+  version: 1
+  spokenForm: chuck arg made and air and just soon
+  action: remove
+  targets:
+    - type: list
+      elements:
+        - type: primitive
+          modifier: {type: containingScope, scopeType: argumentOrParameter, includeSiblings: false}
+          mark: {type: decoratedSymbol, symbolColor: default, character: m}
+        - type: primitive
+          mark: {type: decoratedSymbol, symbolColor: default, character: a}
+        - type: primitive
+          modifier: {type: toRawSelection}
+          mark: {type: decoratedSymbol, symbolColor: default, character: s}
+initialState:
+  documentContents: "function myFunk(value: string, name: string, age: number) { };"
+  selections:
+    - anchor: {line: 0, character: 0}
+      active: {line: 0, character: 0}
+  marks:
+    default.m:
+      start: {line: 0, character: 31}
+      end: {line: 0, character: 35}
+    default.a:
+      start: {line: 0, character: 45}
+      end: {line: 0, character: 48}
+    default.s:
+      start: {line: 0, character: 23}
+      end: {line: 0, character: 29}
+finalState:
+  documentContents: "function myFunk(value: ) { };"
+  selections:
+    - anchor: {line: 0, character: 0}
+      active: {line: 0, character: 0}
+  thatMark:
+    - anchor: {line: 0, character: 23}
+      active: {line: 0, character: 23}
+fullTargets: [{type: list, elements: [{type: primitive, mark: {type: decoratedSymbol, symbolColor: default, character: m}, selectionType: token, position: contents, insideOutsideType: outside, modifier: {type: containingScope, scopeType: argumentOrParameter, includeSiblings: false}, isImplicit: false}, {type: primitive, mark: {type: decoratedSymbol, symbolColor: default, character: a}, selectionType: token, position: contents, insideOutsideType: outside, modifier: {type: containingScope, scopeType: argumentOrParameter, includeSiblings: false}, isImplicit: false}, {type: primitive, mark: {type: decoratedSymbol, symbolColor: default, character: s}, selectionType: token, position: contents, insideOutsideType: outside, modifier: {type: toRawSelection}, isImplicit: false}]}]

src/test/suite/fixtures/recorded/actions/chuckEveryArgMade.yml

@@ -0,0 +1,27 @@
+languageId: typescript
+command:
+  version: 1
+  spokenForm: chuck every arg made
+  action: remove
+  targets:
+    - type: primitive
+      modifier: {type: containingScope, scopeType: argumentOrParameter, includeSiblings: true}
+      mark: {type: decoratedSymbol, symbolColor: default, character: m}
+initialState:
+  documentContents: "function myFunk(value: string, name: string, age: number) { };"
+  selections:
+    - anchor: {line: 0, character: 0}
+      active: {line: 0, character: 0}
+  marks:
+    default.m:
+      start: {line: 0, character: 31}
+      end: {line: 0, character: 35}
+finalState:
+  documentContents: function myFunk() { };
+  selections:
+    - anchor: {line: 0, character: 0}
+      active: {line: 0, character: 0}
+  thatMark:
+    - anchor: {line: 0, character: 16}
+      active: {line: 0, character: 16}
+fullTargets: [{type: primitive, mark: {type: decoratedSymbol, symbolColor: default, character: m}, selectionType: token, position: contents, insideOutsideType: outside, modifier: {type: containingScope, scopeType: argumentOrParameter, includeSiblings: true}, isImplicit: false}]

src/test/suite/fixtures/recorded/actions/cutEveryArgMade.yml

@@ -0,0 +1,27 @@
+languageId: typescript
+command:
+  version: 1
+  spokenForm: cut every arg made
+  action: cutToClipboard
+  targets:
+    - type: primitive
+      modifier: {type: containingScope, scopeType: argumentOrParameter, includeSiblings: true}
+      mark: {type: decoratedSymbol, symbolColor: default, character: m}
+initialState:
+  documentContents: "function myFunk(value: string, name: string, age: number) { };"
+  selections:
+    - anchor: {line: 0, character: 0}
+      active: {line: 0, character: 0}
+  marks:
+    default.m:
+      start: {line: 0, character: 31}
+      end: {line: 0, character: 35}
+finalState:
+  documentContents: function myFunk() { };
+  selections:
+    - anchor: {line: 0, character: 0}
+      active: {line: 0, character: 0}
+  thatMark:
+    - anchor: {line: 0, character: 16}
+      active: {line: 0, character: 16}
+fullTargets: [{type: primitive, mark: {type: decoratedSymbol, symbolColor: default, character: m}, selectionType: token, position: contents, insideOutsideType: null, modifier: {type: containingScope, scopeType: argumentOrParameter, includeSiblings: true}, isImplicit: false}]

src/test/suite/fixtures/recorded/actions/moveEveryArgMade.yml

@@ -0,0 +1,34 @@
+languageId: typescript
+command:
+  version: 1
+  spokenForm: move every arg made
+  action: moveToTarget
+  targets:
+    - type: primitive
+      modifier: {type: containingScope, scopeType: argumentOrParameter, includeSiblings: true}
+      mark: {type: decoratedSymbol, symbolColor: default, character: m}
+    - {type: primitive, isImplicit: true}
+initialState:
+  documentContents: |
+    function myFunk(value: string, name: string, age: number) { };
+  selections:
+    - anchor: {line: 1, character: 0}
+      active: {line: 1, character: 0}
+  marks:
+    default.m:
+      start: {line: 0, character: 31}
+      end: {line: 0, character: 35}
+finalState:
+  documentContents: |-
+    function myFunk() { };
+    value: string, name: string, age: number
+  selections:
+    - anchor: {line: 1, character: 40}
+      active: {line: 1, character: 40}
+  thatMark:
+    - anchor: {line: 1, character: 0}
+      active: {line: 1, character: 40}
+  sourceMark:
+    - anchor: {line: 0, character: 16}
+      active: {line: 0, character: 16}
+fullTargets: [{type: primitive, mark: {type: decoratedSymbol, symbolColor: default, character: m}, selectionType: token, position: contents, insideOutsideType: null, modifier: {type: containingScope, scopeType: argumentOrParameter, includeSiblings: true}, isImplicit: false}, {type: primitive, mark: {type: cursor}, selectionType: token, position: contents, insideOutsideType: null, modifier: {type: identity}, isImplicit: true}]

src/test/suite/fixtures/recorded/surroundingPair/parseTreeParity/takePairDouble.yml

@@ -17,11 +17,7 @@ finalState:
   selections:
     - anchor: {line: 0, character: 0}
       active: {line: 0, character: 0}
-    - anchor: {line: 0, character: 0}
-      active: {line: 0, character: 0}
   thatMark:
     - anchor: {line: 0, character: 0}
       active: {line: 0, character: 0}
-    - anchor: {line: 0, character: 0}
-      active: {line: 0, character: 0}
 fullTargets: [{type: primitive, mark: {type: decoratedSymbol, symbolColor: default, character: '"'}, selectionType: token, position: contents, insideOutsideType: inside, modifier: {type: surroundingPair, delimiter: null, delimiterInclusion: delimitersOnly}}]

src/test/suite/fixtures/recorded/surroundingPair/parseTreeParity/takePairRound.yml

@@ -16,11 +16,7 @@ finalState:
   selections:
     - anchor: {line: 0, character: 0}
       active: {line: 0, character: 0}
-    - anchor: {line: 0, character: 0}
-      active: {line: 0, character: 0}
   thatMark:
     - anchor: {line: 0, character: 0}
       active: {line: 0, character: 0}
-    - anchor: {line: 0, character: 0}
-      active: {line: 0, character: 0}
 fullTargets: [{type: primitive, mark: {type: cursor}, selectionType: token, position: contents, insideOutsideType: inside, modifier: {type: surroundingPair, delimiter: parentheses, delimiterInclusion: delimitersOnly}}]

src/test/suite/fixtures/recorded/surroundingPair/textual/takePairDouble.yml

@@ -17,11 +17,7 @@ finalState:
   selections:
     - anchor: {line: 0, character: 0}
       active: {line: 0, character: 0}
-    - anchor: {line: 0, character: 0}
-      active: {line: 0, character: 0}
   thatMark:
     - anchor: {line: 0, character: 0}
       active: {line: 0, character: 0}
-    - anchor: {line: 0, character: 0}
-      active: {line: 0, character: 0}
 fullTargets: [{type: primitive, mark: {type: decoratedSymbol, symbolColor: default, character: '"'}, selectionType: token, position: contents, insideOutsideType: inside, modifier: {type: surroundingPair, delimiter: null, delimiterInclusion: delimitersOnly}}]

src/test/suite/fixtures/recorded/surroundingPair/textual/takePairRound.yml

@@ -16,11 +16,7 @@ finalState:
   selections:
     - anchor: {line: 0, character: 0}
       active: {line: 0, character: 0}
-    - anchor: {line: 0, character: 0}
-      active: {line: 0, character: 0}
   thatMark:
     - anchor: {line: 0, character: 0}
       active: {line: 0, character: 0}
-    - anchor: {line: 0, character: 0}
-      active: {line: 0, character: 0}
 fullTargets: [{type: primitive, mark: {type: cursor}, selectionType: token, position: contents, insideOutsideType: inside, modifier: {type: surroundingPair, delimiter: parentheses, delimiterInclusion: delimitersOnly}}]

src/test/suite/recorded.test.ts

@@ -85,11 +85,13 @@ async function runTest(file: string) {
   }
 
   if (fixture.initialState.clipboard) {
-    let mockClipboard = fixture.initialState.clipboard;
-    sinon.replace(Clipboard, "readText", async () => mockClipboard);
-    sinon.replace(Clipboard, "writeText", async (value: string) => {
-      mockClipboard = value;
-    });
+    Clipboard.writeText(fixture.initialState.clipboard);
+    // FIXME https://github.com/cursorless-dev/cursorless-vscode/issues/559
+    // let mockClipboard = fixture.initialState.clipboard;
+    // sinon.replace(Clipboard, "readText", async () => mockClipboard);
+    // sinon.replace(Clipboard, "writeText", async (value: string) => {
+    //   mockClipboard = value;
+    // });
   } else {
     excludeFields.push("clipboard");
   }

src/util/targetUtils.ts

@@ -29,15 +29,10 @@ export async function runForEachEditor<T, U>(
   getEditor: (target: T) => TextEditor,
   func: (editor: TextEditor, editorTargets: T[]) => Promise<U>
 ): Promise<U[]> {
-  // Actually group by document and not editor. If the same document is open in multiple editors we want to perform all actions in one editor or an concurrency error will occur.
-  const getDocument = (target: T) => getEditor(target).document;
-  const editorMap = groupBy(targets, getDocument);
-  return await Promise.all(
-    Array.from(editorMap.values(), async (editorTargets) => {
-      // Just pick any editor with the given document open; doesn't matter which
-      const editor = getEditor(editorTargets[0]);
-      return func(editor, editorTargets);
-    })
+  return Promise.all(
+    groupForEachEditor(targets, getEditor).map(([editor, editorTargets]) =>
+      func(editor, editorTargets)
+    )
   );
 }
 
@@ -48,6 +43,24 @@ export async function runOnTargetsForEachEditor<T>(
   return runForEachEditor(targets, (target) => target.selection.editor, func);
 }
 
+export function groupTargetsForEachEditor(targets: TypedSelection[]) {
+  return groupForEachEditor(targets, (target) => target.selection.editor);
+}
+
+export function groupForEachEditor<T>(
+  targets: T[],
+  getEditor: (target: T) => TextEditor
+): [TextEditor, T[]][] {
+  // Actually group by document and not editor. If the same document is open in multiple editors we want to perform all actions in one editor or an concurrency error will occur.
+  const getDocument = (target: T) => getEditor(target).document;
+  const editorMap = groupBy(targets, getDocument);
+  return Array.from(editorMap.values(), (editorTargets) => {
+    // Just pick any editor with the given document open; doesn't matter which
+    const editor = getEditor(editorTargets[0]);
+    return [editor, editorTargets];
+  });
+}
+
 /** Get the possible leading and trailing overflow ranges of the outside target compared to the inside target */
 export function getOutsideOverflow(
   insideTarget: TypedSelection,

src/util/unifyRanges.ts

@@ -1,15 +1,24 @@
-import { Range } from "vscode";
+import { Range, Selection } from "vscode";
+import { TypedSelection } from "../typings/Types";
+import { performInsideOutsideAdjustment } from "./performInsideOutsideAdjustment";
+import { groupTargetsForEachEditor } from "./targetUtils";
 
 /** Unifies overlapping/intersecting ranges */
 export default function unifyRanges(ranges: Range[]): Range[] {
+  if (ranges.length < 2) {
+    return ranges;
+  }
   let run = true;
   while (run) {
-    [ranges, run] = onePass(ranges);
+    [ranges, run] = unifyRangesOnePass(ranges);
   }
   return ranges;
 }
 
-function onePass(ranges: Range[]): [Range[], boolean] {
+function unifyRangesOnePass(ranges: Range[]): [Range[], boolean] {
+  if (ranges.length < 2) {
+    return [ranges, false];
+  }
   const result: Range[] = [];
   let madeChanges = false;
   ranges.forEach((range) => {
@@ -26,3 +35,84 @@ function onePass(ranges: Range[]): [Range[], boolean] {
   });
   return [result, madeChanges];
 }
+
+/**
+ * Unifies overlapping/intersecting targets
+ * FIXME This code probably needs to update once we have objected oriented targets
+ * https://github.com/cursorless-dev/cursorless-vscode/issues/210
+ */
+export function unifyTargets(targets: TypedSelection[]): TypedSelection[] {
+  if (targets.length < 2) {
+    return targets;
+  }
+  return groupTargetsForEachEditor(targets).flatMap(([_editor, targets]) => {
+    if (targets.length < 2) {
+      return targets;
+    }
+    let results = [...targets];
+    results.sort((a, b) =>
+      a.selection.selection.start.compareTo(b.selection.selection.start)
+    );
+    let run = true;
+    // Merge targets untill there are no overlaps/intersections
+    while (run) {
+      [results, run] = unifyTargetsOnePass(results);
+    }
+    return results;
+  });
+}
+
+function unifyTargetsOnePass(
+  targets: TypedSelection[]
+): [TypedSelection[], boolean] {
+  if (targets.length < 2) {
+    return [targets, false];
+  }
+  const results: TypedSelection[] = [];
+  let currentGroup: TypedSelection[] = [];
+  targets.forEach((target) => {
+    // No intersection. Mark start of new group
+    if (
+      currentGroup.length &&
+      !intersects(currentGroup[currentGroup.length - 1], target)
+    ) {
+      results.push(mergeTargets(currentGroup));
+      currentGroup = [target];
+    } else {
+      currentGroup.push(target);
+    }
+  });
+  results.push(mergeTargets(currentGroup));
+  return [results, results.length !== targets.length];
+}
+
+function mergeTargets(targets: TypedSelection[]): TypedSelection {
+  if (targets.length === 1) {
+    return targets[0];
+  }
+  const first = targets[0];
+  const last = targets[targets.length - 1];
+  const typeSelection: TypedSelection = {
+    selection: {
+      editor: first.selection.editor,
+      selection: new Selection(
+        first.selection.selection.start,
+        last.selection.selection.end
+      ),
+    },
+    position: "contents",
+    selectionType: first.selectionType,
+    insideOutsideType: first.insideOutsideType,
+    selectionContext: {
+      leadingDelimiterRange: first.selectionContext.leadingDelimiterRange,
+      trailingDelimiterRange: last.selectionContext.trailingDelimiterRange,
+    },
+  };
+  return performInsideOutsideAdjustment(typeSelection);
+}
+
+function intersects(targetA: TypedSelection, targetB: TypedSelection) {
+  return !!targetA.selection.selection.intersection(
+    targetB.selection.selection
+  );
+}