Skip to content

Commit

Permalink
[REM] merge: remove useless topLeft attribute from Merge interface
Browse files Browse the repository at this point in the history 
The "topLeft" attribute in the Merge interface was used as a shortcut to get the top left position of the merge zone. However, this was creating redundant code.

closes #4171

Task: 3912050
Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
  • Loading branch information
@Rachico
Rachico committed May 15, 2024
1 parent 9c01c36 commit a0aa4ca
Show file tree
Hide file tree
Showing 8 changed files with 43 additions and 52 deletions.
14 changes: 8 additions & 6 deletions src/plugins/core/merge.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,11 +254,14 @@ export class MergePlugin extends CorePlugin<MergeState> implements MergeState {
}

getMainCellPosition(position: CellPosition): CellPosition {
if (!this.isInMerge(position)) {
return position;
}
const mergeTopLeftPos = this.getMerge(position)!.topLeft;
return { sheetId: position.sheetId, col: mergeTopLeftPos.col, row: mergeTopLeftPos.row };
const mergeZone = this.getMerge(position);
return mergeZone
? {
sheetId: position.sheetId,
col: mergeZone.left,
row: mergeZone.top,
}
: position;
}

isMergeHidden(sheetId: UID, merge: Merge): boolean {
Expand Down Expand Up @@ -539,7 +542,6 @@ function exportMerges(merges: Record<number, Range | undefined>): string[] {
function rangeToMerge(mergeId: number, range: Range): Merge {
return {
...range.zone,
topLeft: { col: range.zone.left, row: range.zone.top },
id: mergeId,
};
}
2 changes: 1 addition & 1 deletion src/plugins/ui_feature/autofill.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ export class AutofillPlugin extends UIPlugin {
}
}
const originMerge = this.getters.getMerge(originPosition);
if (originMerge?.topLeft.col === originCol && originMerge?.topLeft.row === originRow) {
if (originMerge?.left === originCol && originMerge?.top === originRow) {
this.dispatch("ADD_MERGE", {
sheetId,
target: [
Expand Down
1 change: 0 additions & 1 deletion src/types/misc.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,6 @@ export interface PixelPosition {

export interface Merge extends Zone {
id: number;
topLeft: Position;
}

export interface Highlight {
Expand Down
10 changes: 5 additions & 5 deletions tests/autofill/autofill_plugin.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -657,9 +657,9 @@ describe("Autofill", () => {
XCToMergeCellMap(model, ["A1", "A2", "A4", "A5", "A7", "A8"])
);
expect(getMerges(model)).toEqual({
"1": { bottom: 1, id: 1, left: 0, right: 0, top: 0, topLeft: toCartesian("A1") },
"2": { bottom: 4, id: 2, left: 0, right: 0, top: 3, topLeft: toCartesian("A4") },
"3": { bottom: 7, id: 3, left: 0, right: 0, top: 6, topLeft: toCartesian("A7") },
"1": { bottom: 1, id: 1, left: 0, right: 0, top: 0 },
"2": { bottom: 4, id: 2, left: 0, right: 0, top: 3 },
"3": { bottom: 7, id: 3, left: 0, right: 0, top: 6 },
});
expect(getCellContent(model, "A1")).toBe("1");
expect(getCellContent(model, "A4")).toBe("2");
Expand All @@ -672,8 +672,8 @@ describe("Autofill", () => {
autofill("A1:A2", "A5");
expect(getMergeCellMap(model)).toEqual(XCToMergeCellMap(model, ["A1", "A2", "A3", "A4"]));
expect(getMerges(model)).toEqual({
"1": { bottom: 1, id: 1, left: 0, right: 0, top: 0, topLeft: toCartesian("A1") },
"2": { bottom: 3, id: 2, left: 0, right: 0, top: 2, topLeft: toCartesian("A3") },
"1": { bottom: 1, id: 1, left: 0, right: 0, top: 0 },
"2": { bottom: 3, id: 2, left: 0, right: 0, top: 2 },
});
});

Expand Down
22 changes: 9 additions & 13 deletions tests/cells/merges_plugin.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ describe("merges", () => {
expect(getCellContent(model, "B2", sheet1)).toBe("b2");
expect(getMergeCellMap(model)).toEqual(XCToMergeCellMap(model, ["B2", "B3"]));
expect(getMerges(model)).toEqual({
"1": { bottom: 2, id: 1, left: 1, right: 1, top: 1, topLeft: toCartesian("B2") },
"1": { bottom: 2, id: 1, left: 1, right: 1, top: 1 },
});
});

Expand All @@ -69,7 +69,7 @@ describe("merges", () => {
});
expect(getMergeCellMap(model)).toEqual(XCToMergeCellMap(model, ["B2", "B3"]));
expect(getMerges(model)).toEqual({
"1": { bottom: 2, id: 1, left: 1, right: 1, top: 1, topLeft: toCartesian("B2") },
"1": { bottom: 2, id: 1, left: 1, right: 1, top: 1 },
});

selectCell(model, "B2");
Expand All @@ -90,8 +90,8 @@ describe("merges", () => {
});
merge(model, "B2:B3", secondSheetId);
expect(model.getters.getMerges(secondSheetId)).toEqual([
{ ...toZone("C2:C3"), id: 2, topLeft: toCartesian("C2") },
{ ...toZone("B2:B3"), id: 3, topLeft: toCartesian("B2") },
{ ...toZone("C2:C3"), id: 2 },
{ ...toZone("B2:B3"), id: 3 },
]);
expect(model.getters.getMerge({ sheetId: secondSheetId, col: 2, row: 1 })?.id).toBe(2);
expect(model.getters.getMerge({ sheetId: secondSheetId, col: 1, row: 1 })?.id).toBe(3);
Expand Down Expand Up @@ -451,7 +451,7 @@ describe("merges", () => {
});
});

test("setting border to topleft => setting style => merging => unmerging", () => {
test("setting border to => setting style => merging => unmerging", () => {
const model = new Model();
setZoneBorders(model, { position: "external" }, ["A1"]);
setStyle(model, "A1", { fillColor: "red" });
Expand Down Expand Up @@ -507,7 +507,7 @@ describe("merges", () => {

expect(getMergeCellMap(model)).toEqual(XCToMergeCellMap(model, ["B2", "B3"]));
expect(getMerges(model)).toEqual({
"1": { bottom: 2, id: 1, left: 1, right: 1, top: 1, topLeft: toCartesian("B2") },
"1": { bottom: 2, id: 1, left: 1, right: 1, top: 1 },
});

// undo
Expand All @@ -519,7 +519,7 @@ describe("merges", () => {
redo(model);
expect(getMergeCellMap(model)).toEqual(XCToMergeCellMap(model, ["B2", "B3"]));
expect(getMerges(model)).toEqual({
"2": { bottom: 2, id: 2, left: 1, right: 1, top: 1, topLeft: toCartesian("B2") },
"2": { bottom: 2, id: 2, left: 1, right: 1, top: 1 },
});
});

Expand Down Expand Up @@ -612,12 +612,8 @@ describe("merges", () => {
sheetIdTo: secondSheetId,
});
addColumns(model, "before", "A", 1, "42");
expect(model.getters.getMerges(firstSheetId)).toEqual([
{ ...toZone("C1:C2"), id: 1, topLeft: toCartesian("C1") },
]);
expect(model.getters.getMerges(secondSheetId)).toEqual([
{ ...toZone("D1:D2"), id: 2, topLeft: toCartesian("D1") },
]);
expect(model.getters.getMerges(firstSheetId)).toEqual([{ ...toZone("C1:C2"), id: 1 }]);
expect(model.getters.getMerges(secondSheetId)).toEqual([{ ...toZone("D1:D2"), id: 2 }]);
});

describe("isSingleCellOrMerge getter", () => {
Expand Down
12 changes: 4 additions & 8 deletions tests/collaborative/collaborative.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Model, UIPlugin } from "../../src";
import { DEFAULT_REVISION_ID, MESSAGE_VERSION } from "../../src/constants";
import { functionRegistry } from "../../src/functions";
import { getDefaultCellHeight, range, toCartesian, toZone } from "../../src/helpers";
import { getDefaultCellHeight, range, toZone } from "../../src/helpers";
import { DEFAULT_TABLE_CONFIG } from "../../src/helpers/table_presets";
import { featurePluginRegistry } from "../../src/plugins";
import { Command, CommandResult, CoreCommand, DataValidationCriterion } from "../../src/types";
Expand Down Expand Up @@ -284,13 +284,13 @@ describe("Multi users synchronisation", () => {

expect([alice, bob, charlie]).toHaveSynchronizedValue((user) => getCell(user, "B3"), undefined);
expect(alice.getters.getMerges(sheetId)).toMatchObject([
{ bottom: 2, left: 0, top: 0, right: 1, topLeft: toCartesian("A1") },
{ bottom: 2, left: 0, top: 0, right: 1 },
]);
expect(bob.getters.getMerges(sheetId)).toMatchObject([
{ bottom: 2, left: 0, top: 0, right: 1, topLeft: toCartesian("A1") },
{ bottom: 2, left: 0, top: 0, right: 1 },
]);
expect(charlie.getters.getMerges(sheetId)).toMatchObject([
{ bottom: 2, left: 0, top: 0, right: 1, topLeft: toCartesian("A1") },
{ bottom: 2, left: 0, top: 0, right: 1 },
]);
undo(bob);
expect([alice, bob, charlie]).toHaveSynchronizedValue((user) => getCell(user, "B3"), undefined);
Expand Down Expand Up @@ -364,10 +364,6 @@ describe("Multi users synchronisation", () => {
{
...toZone("A80:A98"),
id: 1,
topLeft: {
col: 0,
row: 79,
},
},
]
);
Expand Down
2 changes: 1 addition & 1 deletion tests/model/model_import_export.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ describe("Import", () => {
expect(Object.keys(getMerges(model))).toHaveLength(0);
activateSheet(model, sheet1);
expect(Object.keys(getMerges(model))).toHaveLength(1);
expect(Object.values(getMerges(model))[0].topLeft).toEqual(toCartesian("A2"));
expect(getMerges(model)[1]).toMatchObject(toZone("A2:B2"));
});

test("can import cell without content", () => {
Expand Down
32 changes: 15 additions & 17 deletions tests/sheet/sheet_manipulation_plugin.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { DEFAULT_BORDER_DESC, DEFAULT_CELL_HEIGHT, DEFAULT_CELL_WIDTH } from "../../src/constants";
import { lettersToNumber, toCartesian, toXC, toZone } from "../../src/helpers";
import { lettersToNumber, toXC, toZone } from "../../src/helpers";
import { Model } from "../../src/model";
import { Border, CommandResult } from "../../src/types";
import { CellErrorType } from "../../src/types/errors";
Expand Down Expand Up @@ -267,9 +267,9 @@ describe("Columns", () => {
test("On deletion", () => {
deleteColumns(model, ["B", "D"]);
expect(getMerges(model)).toEqual({
1: { id: 1, topLeft: toCartesian("A1"), top: 0, bottom: 0, left: 0, right: 2 },
2: { id: 2, topLeft: toCartesian("B2"), top: 1, bottom: 1, left: 1, right: 2 },
3: { id: 3, topLeft: toCartesian("B3"), top: 2, bottom: 2, left: 1, right: 2 },
1: { id: 1, top: 0, bottom: 0, left: 0, right: 2 },
2: { id: 2, top: 1, bottom: 1, left: 1, right: 2 },
3: { id: 3, top: 2, bottom: 2, left: 1, right: 2 },
});
expect(getMergeCellMap(model)).toEqual(
XCToMergeCellMap(model, ["A1", "B1", "C1", "B2", "C2", "B3", "C3"])
Expand All @@ -280,10 +280,10 @@ describe("Columns", () => {
addColumns(model, "before", "B", 1);
addColumns(model, "after", "A", 1);
expect(getMerges(model)).toEqual({
1: { id: 1, topLeft: toCartesian("A1"), top: 0, bottom: 0, left: 0, right: 6 },
2: { id: 2, topLeft: toCartesian("D2"), top: 1, bottom: 1, left: 3, right: 6 },
3: { id: 3, topLeft: toCartesian("E3"), top: 2, bottom: 2, left: 4, right: 6 },
4: { id: 4, topLeft: toCartesian("D4"), top: 3, bottom: 3, left: 3, right: 5 },
1: { id: 1, top: 0, bottom: 0, left: 0, right: 6 },
2: { id: 2, top: 1, bottom: 1, left: 3, right: 6 },
3: { id: 3, top: 2, bottom: 2, left: 4, right: 6 },
4: { id: 4, top: 3, bottom: 3, left: 3, right: 5 },
});
// prettier-ignore
expect(getMergeCellMap(model)).toEqual(
Expand Down Expand Up @@ -512,7 +512,6 @@ describe("Columns", () => {
expect(Object.values(getMerges(model))[0]).toMatchObject({
left: 2,
right: 5,
topLeft: toCartesian("C4"),
});
});
});
Expand Down Expand Up @@ -997,9 +996,9 @@ describe("Rows", () => {
test("On deletion", () => {
deleteRows(model, [1, 3]);
expect(getMerges(model)).toEqual({
1: { id: 1, topLeft: toCartesian("A1"), top: 0, bottom: 2, left: 0, right: 0 },
2: { id: 2, topLeft: toCartesian("B2"), top: 1, bottom: 2, left: 1, right: 1 },
3: { id: 3, topLeft: toCartesian("C2"), top: 1, bottom: 2, left: 2, right: 2 },
1: { id: 1, top: 0, bottom: 2, left: 0, right: 0 },
2: { id: 2, top: 1, bottom: 2, left: 1, right: 1 },
3: { id: 3, top: 1, bottom: 2, left: 2, right: 2 },
});
expect(getMergeCellMap(model)).toEqual(
XCToMergeCellMap(model, ["A1", "A2", "A3", "B2", "B3", "C2", "C3"])
Expand All @@ -1009,10 +1008,10 @@ describe("Rows", () => {
addRows(model, "before", 1, 1);
addRows(model, "after", 0, 1);
expect(getMerges(model)).toEqual({
1: { id: 1, topLeft: toCartesian("A1"), top: 0, bottom: 6, left: 0, right: 0 },
2: { id: 2, topLeft: toCartesian("B4"), top: 3, bottom: 6, left: 1, right: 1 },
3: { id: 3, topLeft: toCartesian("C5"), top: 4, bottom: 6, left: 2, right: 2 },
4: { id: 4, topLeft: toCartesian("D4"), top: 3, bottom: 5, left: 3, right: 3 },
1: { id: 1, top: 0, bottom: 6, left: 0, right: 0 },
2: { id: 2, top: 3, bottom: 6, left: 1, right: 1 },
3: { id: 3, top: 4, bottom: 6, left: 2, right: 2 },
4: { id: 4, top: 3, bottom: 5, left: 3, right: 3 },
});
// prettier-ignore
expect(getMergeCellMap(model)).toEqual(
Expand Down Expand Up @@ -1124,7 +1123,6 @@ describe("Rows", () => {
expect(Object.values(getMerges(model))[0]).toMatchObject({
top: 2,
bottom: 5,
topLeft: toCartesian("D3"),
});
});

Expand Down

0 comments on commit a0aa4ca

Please sign in to comment.