[FIX] clipboard: cross-sheet cut/paste is broken for tables/cfs

Preface: the handling of `MOVE_RANGES` is broken in multiple plugins.
When calling `adaptRanges`, we don't check that the resulting range
is in the same sheet as the original range.

Fixing that in stable is probably not a good idea. This would mean that
suddenly tables/cfs/merges could appear where they were previously
not, and could break existing sheets.

With the clipboard refactoring, the `MOVE_RANGES` command is now
dispatched at the start of the paste handling (in `CellClipboardHandler`).
At this dispatch, the CFs/Tables/Merges are moved to the wrong sheet.
So in a cut, when the time comes to delete the original Table, the
table is not found at the copy position, and the delete fails.

This commit fixes that by dispatching the `MOVE_RANGES` command at the
very end of the paste handling, after the tables/cfs/merges have been
moved/deleted by their respective handlers.

Note: the cut/paste of filter values does not work anymore. It
somewhat was working before because of the `MOVE_RANGES`, but doesn't
work anymore since `UPDATE_FILTER` is not working before the next
evaluation because of dynamic tables.

closes #5951

Task: 3905618
X-original-commit: 43d5192
Signed-off-by: Vincent Schippefilt (vsc) <[email protected]>
Signed-off-by: Adrien Minne (adrm) <[email protected]>

Author: hokolomopo

src/clipboard_handlers/cell_clipboard.ts

@@ -189,13 +189,6 @@ export class CellClipboardHandler extends AbstractCellClipboardHandler<
     this.clearClippedZones(content);
     const selection = target[0];
     this.pasteZone(sheetId, selection.left, selection.top, content.cells, options);
-    this.dispatch("MOVE_RANGES", {
-      target: content.zones,
-      sheetId: content.sheetId,
-      targetSheetId: sheetId,
-      col: selection.left,
-      row: selection.top,
-    });
   }
 
   /**

src/clipboard_handlers/index.ts

@@ -8,6 +8,7 @@ import { ConditionalFormatClipboardHandler } from "./conditional_format_clipboar
 import { DataValidationClipboardHandler } from "./data_validation_clipboard";
 import { ImageClipboardHandler } from "./image_clipboard";
 import { MergeClipboardHandler } from "./merge_clipboard";
+import { ReferenceClipboardHandler } from "./references_clipboard";
 import { SheetClipboardHandler } from "./sheet_clipboard";
 import { TableClipboardHandler } from "./tables_clipboard";
 
@@ -27,4 +28,5 @@ clipboardHandlersRegistries.cellHandlers
   .add("merge", MergeClipboardHandler)
   .add("border", BorderClipboardHandler)
   .add("table", TableClipboardHandler)
-  .add("conditionalFormat", ConditionalFormatClipboardHandler);
+  .add("conditionalFormat", ConditionalFormatClipboardHandler)
+  .add("references", ReferenceClipboardHandler);

src/clipboard_handlers/merge_clipboard.ts

@@ -1,3 +1,4 @@
+import { isDefined } from "../helpers";
 import {
   CellPosition,
   ClipboardCellData,
@@ -11,6 +12,7 @@ import {
 import { AbstractCellClipboardHandler } from "./abstract_cell_clipboard_handler";
 
 interface ClipboardContent {
+  sheetId: UID;
   merges: Maybe<Merge>[][];
 }
 
@@ -31,15 +33,16 @@ export class MergeClipboardHandler extends AbstractCellClipboardHandler<
       }
       merges.push(mergesInRow);
     }
-    return { merges };
+    return { merges, sheetId };
   }
 
   /**
    * Paste the clipboard content in the given target
    */
   paste(target: ClipboardPasteTarget, content: ClipboardContent, options: ClipboardOptions) {
     if (options.isCutOperation) {
-      return;
+      const copiedMerges = content.merges.flat().filter(isDefined);
+      this.dispatch("REMOVE_MERGE", { sheetId: content.sheetId, target: copiedMerges });
     }
     this.pasteFromCopy(target.sheetId, target.zones, content.merges, options);
   }

src/clipboard_handlers/references_clipboard.ts

@@ -0,0 +1,29 @@
+import { ClipboardCellData, ClipboardOptions, ClipboardPasteTarget, UID, Zone } from "../types";
+import { AbstractCellClipboardHandler } from "./abstract_cell_clipboard_handler";
+
+interface ClipboardContent {
+  zones: Zone[];
+  sheetId: UID;
+}
+
+export class ReferenceClipboardHandler extends AbstractCellClipboardHandler<ClipboardContent, {}> {
+  copy(data: ClipboardCellData): ClipboardContent | undefined {
+    return {
+      zones: data.clippedZones,
+      sheetId: data.sheetId,
+    };
+  }
+
+  paste(target: ClipboardPasteTarget, content: ClipboardContent, options: ClipboardOptions) {
+    if (options.isCutOperation) {
+      const selection = target.zones[0];
+      this.dispatch("MOVE_RANGES", {
+        target: content.zones,
+        sheetId: content.sheetId,
+        targetSheetId: target.sheetId,
+        col: selection.left,
+        row: selection.top,
+      });
+    }
+  }
+}

tests/clipboard/clipboard_plugin.test.ts

@@ -348,11 +348,7 @@ describe("clipboard", () => {
     });
     copy(model, "B1");
     paste(model, "B4");
-    const sheetId = model.getters.getActiveSheetId();
-    expect(model.getters.isInMerge({ sheetId, ...toCartesian("B4") })).toBe(true);
-    expect(model.getters.isInMerge({ sheetId, ...toCartesian("B5") })).toBe(true);
-    expect(model.getters.isInMerge({ sheetId, ...toCartesian("C4") })).toBe(true);
-    expect(model.getters.isInMerge({ sheetId, ...toCartesian("B5") })).toBe(true);
+    expect(model.getters.getMerges("s1")).toMatchObject([toZone("B1:C2"), toZone("B4:C5")]);
   });
 
   test("can cut and paste merged content", () => {
@@ -361,14 +357,19 @@ describe("clipboard", () => {
     });
     cut(model, "B1:C2");
     paste(model, "B4");
-    expect(model.getters.isInMerge({ sheetId: "s2", ...toCartesian("B1") })).toBe(false);
-    expect(model.getters.isInMerge({ sheetId: "s2", ...toCartesian("B2") })).toBe(false);
-    expect(model.getters.isInMerge({ sheetId: "s2", ...toCartesian("C1") })).toBe(false);
-    expect(model.getters.isInMerge({ sheetId: "s2", ...toCartesian("C2") })).toBe(false);
-    expect(model.getters.isInMerge({ sheetId: "s2", ...toCartesian("B4") })).toBe(true);
-    expect(model.getters.isInMerge({ sheetId: "s2", ...toCartesian("B5") })).toBe(true);
-    expect(model.getters.isInMerge({ sheetId: "s2", ...toCartesian("C4") })).toBe(true);
-    expect(model.getters.isInMerge({ sheetId: "s2", ...toCartesian("C5") })).toBe(true);
+    expect(model.getters.getMerges("s2")).toHaveLength(1);
+    expect(model.getters.getMerges("s2")).toMatchObject([toZone("B4:C5")]);
+  });
+
+  test("can cut and paste merged content in another sheet", () => {
+    const model = new Model({
+      sheets: [{ id: "s1", colNumber: 5, rowNumber: 5, merges: ["B1:C2"] }, { id: "s2" }],
+    });
+    cut(model, "B1:C2");
+    activateSheet(model, "s2");
+    paste(model, "B4");
+    expect(model.getters.getMerges("s1")).toEqual([]);
+    expect(model.getters.getMerges("s2")).toMatchObject([toZone("B4:C5")]);
   });
 
   test("Pasting merge on content will remove the content", () => {
@@ -1685,6 +1686,25 @@ describe("clipboard", () => {
     expect(getStyle(model, "C2")).toEqual({});
   });
 
+  test("can cut and paste a conditional format in another sheet", () => {
+    const model = new Model();
+    const sheet1Id = model.getters.getActiveSheetId();
+    createSheet(model, { sheetId: "sheet2Id" });
+    const sheetId = model.getters.getActiveSheetId();
+    model.dispatch("ADD_CONDITIONAL_FORMAT", {
+      cf: createEqualCF("1", { fillColor: "#FF0000" }, "1"),
+      ranges: toRangesData(sheetId, "A1:A2"),
+      sheetId,
+    });
+    cut(model, "A1:A2");
+    activateSheet(model, "sheet2Id");
+    paste(model, "C1");
+    expect(model.getters.getConditionalFormats(sheet1Id)).toEqual([]);
+    expect(model.getters.getConditionalFormats("sheet2Id")).toMatchObject([
+      { ranges: ["C1:C2"], rule: { type: "CellIsRule", style: { fillColor: "#FF0000" } } },
+    ]);
+  });
+
   test("copy cells with CF => remove origin CF => paste => it should paste with original CF", () => {
     const model = new Model();
     const sheetId = model.getters.getActiveSheetId();

tests/table/tables_plugin.test.ts

@@ -2,6 +2,7 @@ import { CommandResult, Model } from "../../src";
 import { toUnboundedZone, toZone, zoneToXc } from "../../src/helpers";
 import { UID } from "../../src/types";
 import {
+  activateSheet,
   addColumns,
   addRows,
   copy,
@@ -721,25 +722,29 @@ describe("Table plugin", () => {
 
     test("Can cut and paste a whole table", () => {
       createTable(model, "A1:B4");
-      updateFilter(model, "A1", ["thisIsAValue"]);
 
       cut(model, "A1:B4");
       paste(model, "A5");
       expect(getTable(model, "A1")).toBeFalsy();
       const copiedTable = getTable(model, "A5");
       expect(copiedTable).toBeTruthy();
-      expect(
-        model.getters.getFilterHiddenValues({
-          sheetId,
-          col: copiedTable!.range.zone.left,
-          row: copiedTable!.range.zone.top,
-        })
-      ).toEqual(["thisIsAValue"]);
+    });
+
+    test("Can cut and paste a whole table in another sheet", () => {
+      const sheet1Id = model.getters.getActiveSheetId();
+      createTable(model, "A1:B4");
+      createSheet(model, { sheetId: "sheet2Id" });
+
+      cut(model, "A1:B4");
+      activateSheet(model, "sheet2Id");
+      paste(model, "A5");
+      expect(model.getters.getTables(sheet1Id)).toHaveLength(0);
+      const copiedTable = getTable(model, "A5", "sheet2Id");
+      expect(copiedTable).toMatchObject({ range: { _zone: toZone("A5:B8") } });
     });
 
     test("Can cut and paste multiple tables", () => {
       createTable(model, "A1:B4");
-      updateFilter(model, "A1", ["thisIsAValue"]);
       createTable(model, "D5:D7");
 
       cut(model, "A1:D7");
@@ -749,13 +754,6 @@ describe("Table plugin", () => {
 
       const copiedTable = getTable(model, "A5");
       expect(copiedTable).toBeTruthy();
-      expect(
-        model.getters.getFilterHiddenValues({
-          sheetId,
-          col: copiedTable!.range.zone.left,
-          row: copiedTable!.range.zone.top,
-        })
-      ).toEqual(["thisIsAValue"]);
       expect(getTable(model, "D9")).toBeTruthy();
     });