[FIX] conditional format: huge revisions on copy/paste cf

src/clipboard_handlers/conditional_format_clipboard.ts

@@ -26,6 +26,8 @@ export class ConditionalFormatClipboardHandler extends AbstractCellClipboardHand
   Maybe<ClipboardConditionalFormat>
 > {
   private readonly uuidGenerator = new UuidGenerator();
+  private queuedChanges: Record<UID, { toAdd: Zone[]; toRemove: Zone[]; cf: ConditionalFormat }[]> =
+    {};
 
   copy(data: ClipboardCellData): ClipboardContent | undefined {
     if (!data.zones.length) {
@@ -51,6 +53,7 @@ export class ConditionalFormatClipboardHandler extends AbstractCellClipboardHand
   }
 
   paste(target: ClipboardPasteTarget, clippedContent: ClipboardContent, options: ClipboardOptions) {
+    this.queuedChanges = {};
     if (options.pasteOption === "asValue") {
       return;
     }
@@ -62,6 +65,8 @@ export class ConditionalFormatClipboardHandler extends AbstractCellClipboardHand
     } else {
       this.pasteFromCut(sheetId, zones, clippedContent);
     }
+
+    this.executeQueuedChanges();
   }
 
   private pasteFromCut(sheetId: UID, target: Zone[], content: ClipboardContent) {
@@ -92,12 +97,13 @@ export class ConditionalFormatClipboardHandler extends AbstractCellClipboardHand
     isCutOperation?: boolean
   ) {
     if (origin?.rules && origin.rules.length > 0) {
+      const originZone = positionToZone(origin.position);
       const zone = positionToZone(target);
       for (const rule of origin.rules) {
         const toRemoveZones: Zone[] = [];
         if (isCutOperation) {
           //remove from current rule
-          toRemoveZones.push(positionToZone(origin.position));
+          toRemoveZones.push(originZone);
         }
         if (origin.position.sheetId === target.sheetId) {
           this.adaptCFRules(origin.position.sheetId, rule, [zone], toRemoveZones);
@@ -114,32 +120,55 @@ export class ConditionalFormatClipboardHandler extends AbstractCellClipboardHand
    * Add or remove cells to a given conditional formatting rule.
    */
   private adaptCFRules(sheetId: UID, cf: ConditionalFormat, toAdd: Zone[], toRemove: Zone[]) {
-    const newRangesXc = this.getters.getAdaptedCfRanges(sheetId, cf, toAdd, toRemove);
-    if (!newRangesXc) {
-      return;
+    if (!this.queuedChanges[sheetId]) {
+      this.queuedChanges[sheetId] = [];
     }
-    if (newRangesXc.length === 0) {
-      this.dispatch("REMOVE_CONDITIONAL_FORMAT", { id: cf.id, sheetId });
-      return;
+    const queuedChange = this.queuedChanges[sheetId].find((queued) => queued.cf.id === cf.id);
+    if (!queuedChange) {
+      this.queuedChanges[sheetId].push({ toAdd, toRemove, cf });
+    } else {
+      queuedChange.toAdd.push(...toAdd);
+      queuedChange.toRemove.push(...toRemove);
+    }
+  }
+
+  private executeQueuedChanges() {
+    for (const sheetId in this.queuedChanges) {
+      for (const { toAdd, toRemove, cf } of this.queuedChanges[sheetId]) {
+        const newRangesXc = this.getters.getAdaptedCfRanges(sheetId, cf, toAdd, toRemove);
+        if (!newRangesXc) {
+          continue;
+        }
+        if (newRangesXc.length === 0) {
+          this.dispatch("REMOVE_CONDITIONAL_FORMAT", { id: cf.id, sheetId });
+          continue;
+        }
+        this.dispatch("ADD_CONDITIONAL_FORMAT", {
+          cf: {
+            id: cf.id,
+            rule: cf.rule,
+            stopIfTrue: cf.stopIfTrue,
+          },
+          ranges: newRangesXc,
+          sheetId,
+        });
+      }
     }
-    this.dispatch("ADD_CONDITIONAL_FORMAT", {
-      cf: {
-        id: cf.id,
-        rule: cf.rule,
-        stopIfTrue: cf.stopIfTrue,
-      },
-      ranges: newRangesXc,
-      sheetId,
-    });
   }
 
   private getCFToCopyTo(targetSheetId: UID, originCF: ConditionalFormat): ConditionalFormat {
-    const cfInTarget = this.getters
+    let targetCF = this.getters
       .getConditionalFormats(targetSheetId)
       .find((cf) => cf.stopIfTrue === originCF.stopIfTrue && deepEquals(cf.rule, originCF.rule));
 
-    return cfInTarget
-      ? cfInTarget
-      : { ...originCF, id: this.uuidGenerator.smallUuid(), ranges: [] };
+    const queuedCfs = this.queuedChanges[targetSheetId];
+    if (!targetCF && queuedCfs) {
+      targetCF = queuedCfs.find(
+        (queued) =>
+          queued.cf.stopIfTrue === originCF.stopIfTrue && deepEquals(queued.cf.rule, originCF.rule)
+      )?.cf;
+    }
+
+    return targetCF || { ...originCF, id: this.uuidGenerator.uuidv4(), ranges: [] };
   }
 }

src/clipboard_handlers/data_validation_clipboard.ts

@@ -26,6 +26,10 @@ export class DataValidationClipboardHandler extends AbstractCellClipboardHandler
   Maybe<ClipboardDataValidationRule>
 > {
   private readonly uuidGenerator = new UuidGenerator();
+  private queuedChanges: Record<
+    UID,
+    { toAdd: Zone[]; toRemove: Zone[]; rule: DataValidationRule }[]
+  > = {};
 
   copy(data: ClipboardCellData): ClipboardContent | undefined {
     const { rowsIndexes, columnsIndexes } = data;
@@ -45,6 +49,7 @@ export class DataValidationClipboardHandler extends AbstractCellClipboardHandler
   }
 
   paste(target: ClipboardPasteTarget, clippedContent: ClipboardContent, options: ClipboardOptions) {
+    this.queuedChanges = {};
     if (options.pasteOption) {
       return;
     }
@@ -56,6 +61,7 @@ export class DataValidationClipboardHandler extends AbstractCellClipboardHandler
     } else {
       this.pasteFromCut(sheetId, zones, clippedContent);
     }
+    this.executeQueuedChanges();
   }
 
   private pasteFromCut(sheetId: UID, target: Zone[], content: ClipboardContent) {
@@ -87,6 +93,7 @@ export class DataValidationClipboardHandler extends AbstractCellClipboardHandler
   ) {
     if (origin) {
       const zone = positionToZone(target);
+      const originZone = positionToZone(origin.position);
       const rule = origin.rule;
       if (!rule) {
         const targetRule = this.getters.getValidationRuleForCell(target);
@@ -98,7 +105,7 @@ export class DataValidationClipboardHandler extends AbstractCellClipboardHandler
       }
       const toRemoveZone: Zone[] = [];
       if (isCutOperation) {
-        toRemoveZone.push(positionToZone(origin.position));
+        toRemoveZone.push(originZone);
       }
       if (origin.position.sheetId === target.sheetId) {
         const copyToRule = this.getDataValidationRuleToCopyTo(target.sheetId, rule, false);
@@ -119,17 +126,30 @@ export class DataValidationClipboardHandler extends AbstractCellClipboardHandler
     originRule: DataValidationRule,
     newId = true
   ): DataValidationRule {
-    const ruleInTargetSheet = this.getters
+    let targetRule = this.getters
       .getDataValidationRules(targetSheetId)
       .find(
         (rule) =>
           deepEquals(originRule.criterion, rule.criterion) &&
           originRule.isBlocking === rule.isBlocking
       );
 
-    return ruleInTargetSheet
-      ? ruleInTargetSheet
-      : { ...originRule, id: newId ? this.uuidGenerator.smallUuid() : originRule.id, ranges: [] };
+    const queuedRules = this.queuedChanges[targetSheetId];
+    if (!targetRule && queuedRules) {
+      targetRule = queuedRules.find(
+        (queued) =>
+          deepEquals(originRule.criterion, queued.rule.criterion) &&
+          originRule.isBlocking === queued.rule.isBlocking
+      )?.rule;
+    }
+
+    return (
+      targetRule || {
+        ...originRule,
+        id: newId ? this.uuidGenerator.uuidv4() : originRule.id,
+        ranges: [],
+      }
+    );
   }
 
   /**
@@ -141,16 +161,36 @@ export class DataValidationClipboardHandler extends AbstractCellClipboardHandler
     toAdd: Zone[],
     toRemove: Zone[]
   ) {
-    const dvZones = rule.ranges.map((range) => range.zone);
-    const newDvZones = recomputeZones([...dvZones, ...toAdd], toRemove);
-    if (newDvZones.length === 0) {
-      this.dispatch("REMOVE_DATA_VALIDATION_RULE", { sheetId, id: rule.id });
-      return;
+    if (!this.queuedChanges[sheetId]) {
+      this.queuedChanges[sheetId] = [];
+    }
+    const queuedChange = this.queuedChanges[sheetId].find((queued) => queued.rule.id === rule.id);
+    if (!queuedChange) {
+      this.queuedChanges[sheetId].push({ toAdd, toRemove, rule });
+    } else {
+      queuedChange.toAdd.push(...toAdd);
+      queuedChange.toRemove.push(...toRemove);
+    }
+  }
+
+  private executeQueuedChanges() {
+    for (const sheetId in this.queuedChanges) {
+      for (const { toAdd, toRemove, rule: dv } of this.queuedChanges[sheetId]) {
+        // Remove the zones first in case the same position is in toAdd and toRemove
+        const dvZones = dv.ranges.map((range) => range.zone);
+        const withRemovedZones = recomputeZones(dvZones, toRemove);
+        const newDvZones = recomputeZones([...withRemovedZones, ...toAdd], []);
+
+        if (newDvZones.length === 0) {
+          this.dispatch("REMOVE_DATA_VALIDATION_RULE", { sheetId, id: dv.id });
+          continue;
+        }
+        this.dispatch("ADD_DATA_VALIDATION_RULE", {
+          rule: { id: dv.id, criterion: dv.criterion, isBlocking: dv.isBlocking },
+          ranges: newDvZones.map((zone) => this.getters.getRangeDataFromZone(sheetId, zone)),
+          sheetId,
+        });
+      }
     }
-    this.dispatch("ADD_DATA_VALIDATION_RULE", {
-      rule,
-      ranges: newDvZones.map((zone) => this.getters.getRangeDataFromZone(sheetId, zone)),
-      sheetId,
-    });
   }
 }

src/plugins/core/conditional_format.ts

@@ -257,8 +257,9 @@ export class ConditionalFormatPlugin
       currentRanges = rules[replaceIndex].ranges.map(toUnboundedZone);
     }
 
-    currentRanges = currentRanges.concat(toAdd);
-    return recomputeZones(currentRanges, toRemove).map((zone) =>
+    // Remove the zones first in case the same position is in toAdd and toRemove
+    const withRemovedZones = recomputeZones(currentRanges, toRemove);
+    return recomputeZones([...toAdd, ...withRemovedZones], []).map((zone) =>
       this.getters.getRangeDataFromZone(sheetId, zone)
     );
   }

tests/clipboard/clipboard_plugin.test.ts

@@ -1,11 +1,14 @@
+import { UIPlugin } from "../../src";
 import { clipboardHandlersRegistries } from "../../src/clipboard_handlers";
 import { DEFAULT_BORDER_DESC } from "../../src/constants";
 import { toCartesian, toZone, zoneToXc } from "../../src/helpers";
 import { getClipboardDataPositions } from "../../src/helpers/clipboard/clipboard_helpers";
 import { Model } from "../../src/model";
+import { featurePluginRegistry } from "../../src/plugins";
 import {
   ClipboardMIMEType,
   ClipboardPasteTarget,
+  Command,
   CommandResult,
   DEFAULT_LOCALE,
   DEFAULT_LOCALES,
@@ -59,6 +62,7 @@ import {
   getStyle,
 } from "../test_helpers/getters_helpers";
 import {
+  addTestPlugin,
   createEqualCF,
   createModelFromGrid,
   getGrid,
@@ -1897,6 +1901,28 @@ describe("clipboard", () => {
     ]);
   });
 
+  test("copy/paste a CF zone only dispatch a singled ADD_CONDITIONAL_FORMAT", () => {
+    const commands: Command[] = [];
+    class MyUIPlugin extends UIPlugin {
+      handle = (cmd: Command) => commands.push(cmd);
+    }
+    addTestPlugin(featurePluginRegistry, MyUIPlugin);
+
+    const model = new Model({ sheets: [{ colNumber: 5, rowNumber: 5 }] });
+    const sheetId = model.getters.getActiveSheetId();
+    model.dispatch("ADD_CONDITIONAL_FORMAT", {
+      cf: createEqualCF("1", { fillColor: "#FF0000" }, "1"),
+      ranges: toRangesData(sheetId, "A1,A2"),
+      sheetId,
+    });
+
+    copy(model, "A1:A2");
+    paste(model, "B1");
+
+    expect(model.getters.getConditionalFormats(sheetId)).toMatchObject([{ ranges: ["A1:B2"] }]);
+    expect(commands.filter((c) => c.type === "ADD_CONDITIONAL_FORMAT")).toHaveLength(2);
+  });
+
   test("can copy and paste a cell which contains a cross-sheet reference", () => {
     const model = new Model();
     createSheet(model, { sheetId: "42" });

tests/data_validation/data_validation_clipboard_plugin.test.ts

@@ -1,5 +1,6 @@
-import { Model } from "../../src";
-import { DataValidationCriterion, UID } from "../../src/types";
+import { Model, UIPlugin } from "../../src";
+import { featurePluginRegistry } from "../../src/plugins";
+import { Command, DataValidationCriterion, UID } from "../../src/types";
 import {
   activateSheet,
   addDataValidation,
@@ -10,7 +11,7 @@ import {
   paste,
   removeDataValidation,
 } from "../test_helpers/commands_helpers";
-import { getDataValidationRules } from "../test_helpers/helpers";
+import { addTestPlugin, getDataValidationRules } from "../test_helpers/helpers";
 
 describe("Data validation", () => {
   let model: Model;
@@ -201,4 +202,22 @@ describe("Data validation", () => {
       { criterion: criterion2, ranges: ["G1:G5"] },
     ]);
   });
+
+  test("copy/paste a DV zone only dispatch a singled ADD_DATA_VALIDATION_RULE", () => {
+    const commands: Command[] = [];
+    class MyUIPlugin extends UIPlugin {
+      handle = (cmd: Command) => commands.push(cmd);
+    }
+    addTestPlugin(featurePluginRegistry, MyUIPlugin);
+
+    const model = new Model({ sheets: [{ colNumber: 5, rowNumber: 5 }] });
+    const sheetId = model.getters.getActiveSheetId();
+    addDataValidation(model, "A1:A2", "id", { type: "textContains", values: ["1"] });
+
+    copy(model, "A1:A2");
+    paste(model, "B1");
+
+    expect(getDataValidationRules(model, sheetId)).toMatchObject([{ ranges: ["A1:B2"] }]);
+    expect(commands.filter((c) => c.type === "ADD_DATA_VALIDATION_RULE")).toHaveLength(2);
+  });
 });