From 554611e73db0a01fa3ab26d9a919848878c748df Mon Sep 17 00:00:00 2001 From: Andreas Arvidsson Date: Sun, 13 Feb 2022 15:18:26 +0100 Subject: [PATCH] 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 | 43 +++++---- src/actions/Delete.ts | 9 +- .../recorded/actions/chuckArgMadeAndAir.yml | 34 +++++++ .../actions/chuckArgMadeAndAirAndJustSoon.yml | 40 ++++++++ .../recorded/actions/chuckEveryArgMade.yml | 27 ++++++ .../recorded/actions/cutEveryArgMade.yml | 27 ++++++ .../recorded/actions/moveEveryArgMade.yml | 34 +++++++ .../parseTreeParity/takePairDouble.yml | 4 - .../parseTreeParity/takePairRound.yml | 4 - .../textual/takePairDouble.yml | 4 - .../surroundingPair/textual/takePairRound.yml | 4 - src/test/suite/recorded.test.ts | 12 ++- src/util/targetUtils.ts | 31 ++++-- src/util/unifyRanges.ts | 96 ++++++++++++++++++- 14 files changed, 313 insertions(+), 56 deletions(-) create mode 100644 src/test/suite/fixtures/recorded/actions/chuckArgMadeAndAir.yml create mode 100644 src/test/suite/fixtures/recorded/actions/chuckArgMadeAndAirAndJustSoon.yml create mode 100644 src/test/suite/fixtures/recorded/actions/chuckEveryArgMade.yml create mode 100644 src/test/suite/fixtures/recorded/actions/cutEveryArgMade.yml create mode 100644 src/test/suite/fixtures/recorded/actions/moveEveryArgMade.yml diff --git a/src/actions/BringMoveSwap.ts b/src/actions/BringMoveSwap.ts index bfa91b042..72f853287 100644 --- a/src/actions/BringMoveSwap.ts +++ b/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; } diff --git a/src/actions/Delete.ts b/src/actions/Delete.ts index 47a21c716..423d1b133 100644 --- a/src/actions/Delete.ts +++ b/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 { + // 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 })); }) ); diff --git a/src/test/suite/fixtures/recorded/actions/chuckArgMadeAndAir.yml b/src/test/suite/fixtures/recorded/actions/chuckArgMadeAndAir.yml new file mode 100644 index 000000000..aa742831b --- /dev/null +++ b/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}]}] diff --git a/src/test/suite/fixtures/recorded/actions/chuckArgMadeAndAirAndJustSoon.yml b/src/test/suite/fixtures/recorded/actions/chuckArgMadeAndAirAndJustSoon.yml new file mode 100644 index 000000000..0dcddd952 --- /dev/null +++ b/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}]}] diff --git a/src/test/suite/fixtures/recorded/actions/chuckEveryArgMade.yml b/src/test/suite/fixtures/recorded/actions/chuckEveryArgMade.yml new file mode 100644 index 000000000..7c845ec8c --- /dev/null +++ b/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}] diff --git a/src/test/suite/fixtures/recorded/actions/cutEveryArgMade.yml b/src/test/suite/fixtures/recorded/actions/cutEveryArgMade.yml new file mode 100644 index 000000000..ecad8ec51 --- /dev/null +++ b/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}] diff --git a/src/test/suite/fixtures/recorded/actions/moveEveryArgMade.yml b/src/test/suite/fixtures/recorded/actions/moveEveryArgMade.yml new file mode 100644 index 000000000..1bfcc64d7 --- /dev/null +++ b/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}] diff --git a/src/test/suite/fixtures/recorded/surroundingPair/parseTreeParity/takePairDouble.yml b/src/test/suite/fixtures/recorded/surroundingPair/parseTreeParity/takePairDouble.yml index a0666f247..4512d75e4 100644 --- a/src/test/suite/fixtures/recorded/surroundingPair/parseTreeParity/takePairDouble.yml +++ b/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}}] diff --git a/src/test/suite/fixtures/recorded/surroundingPair/parseTreeParity/takePairRound.yml b/src/test/suite/fixtures/recorded/surroundingPair/parseTreeParity/takePairRound.yml index 3ca43746f..836898881 100644 --- a/src/test/suite/fixtures/recorded/surroundingPair/parseTreeParity/takePairRound.yml +++ b/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}}] diff --git a/src/test/suite/fixtures/recorded/surroundingPair/textual/takePairDouble.yml b/src/test/suite/fixtures/recorded/surroundingPair/textual/takePairDouble.yml index 8dbb91712..00ae47072 100644 --- a/src/test/suite/fixtures/recorded/surroundingPair/textual/takePairDouble.yml +++ b/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}}] diff --git a/src/test/suite/fixtures/recorded/surroundingPair/textual/takePairRound.yml b/src/test/suite/fixtures/recorded/surroundingPair/textual/takePairRound.yml index f04ac868b..61767e1f8 100644 --- a/src/test/suite/fixtures/recorded/surroundingPair/textual/takePairRound.yml +++ b/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}}] diff --git a/src/test/suite/recorded.test.ts b/src/test/suite/recorded.test.ts index 479f740ce..3ff946e06 100644 --- a/src/test/suite/recorded.test.ts +++ b/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"); } diff --git a/src/util/targetUtils.ts b/src/util/targetUtils.ts index 4a528416b..86b3b97ac 100644 --- a/src/util/targetUtils.ts +++ b/src/util/targetUtils.ts @@ -29,15 +29,10 @@ export async function runForEachEditor( getEditor: (target: T) => TextEditor, func: (editor: TextEditor, editorTargets: T[]) => Promise ): Promise { - // 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( return runForEachEditor(targets, (target) => target.selection.editor, func); } +export function groupTargetsForEachEditor(targets: TypedSelection[]) { + return groupForEachEditor(targets, (target) => target.selection.editor); +} + +export function groupForEachEditor( + 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, diff --git a/src/util/unifyRanges.ts b/src/util/unifyRanges.ts index bcd022382..3080f058e 100644 --- a/src/util/unifyRanges.ts +++ b/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 + ); +}