Skip to content

Commit

Permalink
[FIX] conditional format: huge revisions on copy/paste cf
Browse files Browse the repository at this point in the history 
When copy/pasting a conditional format, the revision had one
ADD_CONDITIONAL_FORMAT command per copied cell. That lead to huge
revisions if, for example, a whole column with a CF was copied.

Now the revision only has a single ADD_CONDITIONAL_FORMAT command per
modified CF.

The exact same work was done for data validation rules.

Benchmark:
-------------------
Copy/paste a column of 100 cells with a CF:

closes #5059

Before: 46.6KB revision
After: 526B revision (divided by 88!)
Task: 4240668
Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
  • Loading branch information
@hokolomopo
hokolomopo committed Nov 22, 2024
1 parent ace25cf commit 70fe23b
Show file tree
Hide file tree
Showing 3 changed files with 103 additions and 33 deletions.
64 changes: 47 additions & 17 deletions src/clipboard_handlers/conditional_format_clipboard.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -51,6 +53,7 @@ export class ConditionalFormatClipboardHandler extends AbstractCellClipboardHand
}

paste(target: ClipboardPasteTarget, clippedContent: ClipboardContent, options: ClipboardOptions) {
this.queuedChanges = {};
if (options.pasteOption === "asValue") {
return;
}
Expand All @@ -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) {
Expand Down Expand Up @@ -114,30 +119,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.uuidv4(), 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: [] };
}
}
67 changes: 53 additions & 14 deletions src/clipboard_handlers/data_validation_clipboard.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -45,6 +49,7 @@ export class DataValidationClipboardHandler extends AbstractCellClipboardHandler
}

paste(target: ClipboardPasteTarget, clippedContent: ClipboardContent, options: ClipboardOptions) {
this.queuedChanges = {};
if (options.pasteOption) {
return;
}
Expand All @@ -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) {
Expand Down Expand Up @@ -119,17 +125,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.uuidv4() : 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: [],
}
);
}

/**
Expand All @@ -141,16 +160,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: dv,
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,
});
}
}
5 changes: 3 additions & 2 deletions src/plugins/core/conditional_format.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,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)
);
}
Expand Down

0 comments on commit 70fe23b

Please sign in to comment.