Skip to content

Commit

Permalink
[FIX] Selection: Loop selection based on spreaded formula
Browse files Browse the repository at this point in the history 
Similarly to the issue addressed in PR #3214, we want `loopSelection`
to consider seemingly empty cells when considering the expansion zone.

This revision splits the behaviour of the selection processor for
`loopSelection` and the other navigation features (like cluster
jumping) as we still want to ignore cells with an empty evaluation in
those cases. The reason being that the user could be lost if, when
navigating with their keyboards, they end up with their selection on
empty cell.

This PR is the adaptation of #3573 to support cells that contain the
result of a spreaded formula.

closes #3577

Task: 3709340
Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
  • Loading branch information
@rrahir
rrahir committed Feb 9, 2024
1 parent 952543c commit 2e69e1b
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 14 deletions.
24 changes: 18 additions & 6 deletions src/selection_stream/selection_stream_processor.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -596,13 +596,12 @@ export class SelectionStreamProcessorImpl implements SelectionStreamProcessor {
* next cluster if the given cell is outside a cluster or at the border of a cluster in the given direction.
*/
private getEndOfCluster(startPosition: Position, dim: "cols" | "rows", dir: -1 | 1): HeaderIndex {
const sheet = this.getters.getActiveSheet();
let currentPosition = startPosition;

// If both the current cell and the next cell are not empty, we want to go to the end of the cluster
const nextCellPosition = this.getNextCellPosition(startPosition, dim, dir);
let mode: "endOfCluster" | "nextCluster" =
!this.isCellEmpty(currentPosition, sheet.id) && !this.isCellEmpty(nextCellPosition, sheet.id)
!this.isEvaluatedCellEmpty(currentPosition) && !this.isEvaluatedCellEmpty(nextCellPosition)
? "endOfCluster"
: "nextCluster";

Expand All @@ -615,7 +614,7 @@ export class SelectionStreamProcessorImpl implements SelectionStreamProcessor {
) {
break;
}
const isNextCellEmpty = this.isCellEmpty(nextCellPosition, sheet.id);
const isNextCellEmpty = this.isEvaluatedCellEmpty(nextCellPosition);
if (mode === "endOfCluster" && isNextCellEmpty) {
break;
} else if (mode === "nextCluster" && !isNextCellEmpty) {
Expand All @@ -629,10 +628,23 @@ export class SelectionStreamProcessorImpl implements SelectionStreamProcessor {
}

/**
* Check if a cell is empty or undefined in the model. If the cell is part of a merge,
* check if the merge containing the cell is empty.
* Checks if a cell is empty (i.e. does not have a content). If the cell is part of a merge,
* the check applies to the main cell of the merge.
*/
private isCellEmpty({ col, row }: Position, sheetId = this.getters.getActiveSheetId()): boolean {
private isCellEmpty({ col, row }: Position): boolean {
const sheetId = this.getters.getActiveSheetId();
const position = this.getters.getMainCellPosition({ sheetId, col, row });
return !(
this.getters.getCorrespondingFormulaCell(position) || this.getters.getCell(position)?.content
);
}

/**
* Checks if a cell evaluated value is empty. If the cell is part of a merge,
* the check applies to the main cell of the merge.
*/
private isEvaluatedCellEmpty({ col, row }: Position): boolean {
const sheetId = this.getters.getActiveSheetId();
const position = this.getters.getMainCellPosition({ sheetId, col, row });
const cell = this.getters.getEvaluatedCell(position);
return cell.type === CellValueType.empty;
Expand Down
21 changes: 13 additions & 8 deletions tests/plugins/selection.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -706,14 +706,15 @@ describe("Change selection to next clusters", () => {
rows: { 8: { isHidden: true } },
// prettier-ignore
cells: {
B2: { content: "content" }, E2: hiddenContent, G2: { content: "same line as merge topLeft" },
E3: hiddenContent, G3: { content: "line of merge but aligned with topLeft" },
B6: { content: "content on same line as empty merge topLeft" }, E6: hiddenContent,
B7: { content: "line of empty merge but aligned with topLeft" }, E7: hiddenContent,
A9: hiddenContent, B9: hiddenContent, C9: hiddenContent, D9: hiddenContent, E9: hiddenContent, F9: hiddenContent, G9: hiddenContent,
B2: { content: "content" }, E2: hiddenContent, G2: { content: "same line as merge topLeft" },
E3: hiddenContent, G3: { content: "line of merge but aligned with topLeft" },
B6: { content: "content on same line as empty merge topLeft" }, E6: hiddenContent,
B7: { content: "line of empty merge but aligned with topLeft" }, E7: hiddenContent,
A9: hiddenContent, B9: hiddenContent, C9: hiddenContent, D9: hiddenContent, E9: hiddenContent, F9: hiddenContent, G9: hiddenContent,
A11: { content: "A11" }, B11: { content: "B9" }, C11: { content: "C9" }, E11: hiddenContent, F11: { style: 1 }, G11: { content: "F9" }, H11: { content: "G9" },
B13: { content: "B11" }, C13: { content: "C11" }, D13: { content: "D11" },
B14: { content: "B12" }, C14: { content: "C12" },
A13: { content: '=""' }, B13: { content: "B11" }, C13: { content: "C11" }, D13: { content: "D11" },
A14: { content: '=""' }, B14: { content: "B12" }, C14: { content: "C12" },
A15: { content: '=""' }, C15: { content: "=TRANSPOSE(A13:A15)" },
},
merges: ["B2:D4", "C6:D7"],
styles: { 1: { textColor: "#fe0000" } },
Expand Down Expand Up @@ -972,7 +973,9 @@ describe("Selection loop (ctrl + a)", () => {
B2: { content: "a" }, C2: { content: "a" },
C3: { content: "merged" }, D3: { content: "merged" }, E3: { content: "a" },
C4: { content: "a"},
A6: { content : "a" }
A6: { content : "a" },
C8: { content: '=""'}, D8: { content: '=""'},
A9: { content : "=TRANSPOSE(C8:D8)" },
},
merges: ["C3:D3"],
styles: { 1: { textColor: "#fe0000" } },
Expand All @@ -988,6 +991,8 @@ describe("Selection loop (ctrl + a)", () => {
["E3", ["B2:E4", "A1:J10", "E3"]],
["A1", ["A1:J10", "A1"]],
["A6", ["A1:J10", "A6"]],
["E8", ["C8:E8", "A1:J10", "E8"]],
["A9", ["A9:A10", "A1:J10", "A9"]],
])("Selection loop with anchor %s", (anchor: string, expectedZones: string[]) => {
selectCell(model, anchor);
for (const zone of expectedZones) {
Expand Down

0 comments on commit 2e69e1b

Please sign in to comment.