From ae0e8d9ed1d5def3cd1c3365cda8ecf0134e9223 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Rahir?= Date: Mon, 31 Oct 2022 08:32:40 +0000 Subject: [PATCH] [FIX] selection: Altering selection should scroll the viewport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit b437f446b introduced an error. while fixing the viewport scrolling while in autofill (it shouldn't happen) it also prevent an change of selection (not an extension) to properly scroll the viewport. The problem ultimately lies in the fact that the autofill was using the selectionProcessor to override its selection instead of extending it. In order to fix this, it was necessary to alter the API of the selection processor to allow more granularity in the steps to extend a zone selection: i.e. instead of a step of 1 or jump to the end of a zone, we need to support all POSITIVE values of steps. Task 3050101 closes odoo/o-spreadsheet#1824 X-original-commit: b167b3e6d9f1eb5421f6fe5d3df5d83e50e008c5 Signed-off-by: Lucas Lefèvre (lul) Signed-off-by: Rémi Rahir (rar) --- src/components/composer/composer/composer.ts | 4 +-- src/components/grid/grid.ts | 4 +-- src/components/helpers/selection_helpers.ts | 4 +-- src/helpers/zones.ts | 8 +++--- src/plugins/ui_core_views/sheetview.ts | 19 +++++++++---- src/plugins/ui_feature/autofill.ts | 13 ++++++--- .../selection_stream_processor.ts | 28 +++++++++++-------- src/types/commands.ts | 1 + src/types/misc.ts | 8 +++--- src/types/selection.ts | 2 +- tests/plugins/navigation.test.ts | 21 ++++++++++++++ tests/plugins/sheetview.test.ts | 20 +++++++++++++ tests/test_helpers/commands_helpers.ts | 4 +-- 13 files changed, 98 insertions(+), 38 deletions(-) diff --git a/src/components/composer/composer/composer.ts b/src/components/composer/composer/composer.ts index a2ecb0f55..89f44268f 100644 --- a/src/components/composer/composer/composer.ts +++ b/src/components/composer/composer/composer.ts @@ -267,7 +267,7 @@ export class Composer extends Component { const direction = ev.shiftKey ? "left" : "right"; this.env.model.dispatch("STOP_EDITION"); - this.env.model.selection.moveAnchorCell(direction, "one"); + this.env.model.selection.moveAnchorCell(direction, 1); } private processEnterKey(ev: KeyboardEvent) { @@ -284,7 +284,7 @@ export class Composer extends Component { } this.env.model.dispatch("STOP_EDITION"); const direction = ev.shiftKey ? "up" : "down"; - this.env.model.selection.moveAnchorCell(direction, "one"); + this.env.model.selection.moveAnchorCell(direction, 1); } private processEscapeKey() { diff --git a/src/components/grid/grid.ts b/src/components/grid/grid.ts index 031aeaa62..dc8ea0bfe 100644 --- a/src/components/grid/grid.ts +++ b/src/components/grid/grid.ts @@ -156,8 +156,8 @@ export class Grid extends Component { ? this.props.onGridComposerCellFocused() : this.props.onComposerContentFocused(); }, - TAB: () => this.env.model.selection.moveAnchorCell("right", "one"), - "SHIFT+TAB": () => this.env.model.selection.moveAnchorCell("left", "one"), + TAB: () => this.env.model.selection.moveAnchorCell("right", 1), + "SHIFT+TAB": () => this.env.model.selection.moveAnchorCell("left", 1), F2: () => { const cell = this.env.model.getters.getActiveCell(); cell.type === CellValueType.empty diff --git a/src/components/helpers/selection_helpers.ts b/src/components/helpers/selection_helpers.ts index 8c7fd09da..04e2dc2d5 100644 --- a/src/components/helpers/selection_helpers.ts +++ b/src/components/helpers/selection_helpers.ts @@ -13,8 +13,8 @@ export function updateSelectionWithArrowKeys( ) { const direction = arrowMap[ev.key]; if (ev.shiftKey) { - selection.resizeAnchorZone(direction, ev.ctrlKey ? "end" : "one"); + selection.resizeAnchorZone(direction, ev.ctrlKey ? "end" : 1); } else { - selection.moveAnchorCell(direction, ev.ctrlKey ? "end" : "one"); + selection.moveAnchorCell(direction, ev.ctrlKey ? "end" : 1); } } diff --git a/src/helpers/zones.ts b/src/helpers/zones.ts index 5af142500..6f7f5e6cb 100644 --- a/src/helpers/zones.ts +++ b/src/helpers/zones.ts @@ -1,5 +1,5 @@ import { _lt } from "../translation"; -import { Position, UnboundedZone, Viewport, Zone, ZoneDimension } from "../types"; +import { Position, UnboundedZone, Zone, ZoneDimension } from "../types"; import { lettersToNumber, numberToLetters, toCartesian, toXC } from "./coordinates"; import { range } from "./misc"; import { isColReference, isRowReference } from "./references"; @@ -599,7 +599,7 @@ export function mergeOverlappingZones(zones: Zone[]) { * This function will compare the modifications of selection to determine * a cell that is part of the new zone and not the previous one. */ -export function findCellInNewZone(oldZone: Zone, currentZone: Zone, viewport: Viewport): Position { +export function findCellInNewZone(oldZone: Zone, currentZone: Zone): Position { let col: number, row: number; const { left: oldLeft, right: oldRight, top: oldTop, bottom: oldBottom } = oldZone!; const { left, right, top, bottom } = currentZone; @@ -609,7 +609,7 @@ export function findCellInNewZone(oldZone: Zone, currentZone: Zone, viewport: Vi col = right; } else { // left and right don't change - col = viewport.left > left || left > viewport.right ? viewport.left : left; + col = left; } if (top != oldTop) { row = top; @@ -617,7 +617,7 @@ export function findCellInNewZone(oldZone: Zone, currentZone: Zone, viewport: Vi row = bottom; } else { // top and bottom don't change - row = viewport.top > top || top > viewport.bottom ? viewport.top : top; + row = top; } return { col, row }; } diff --git a/src/plugins/ui_core_views/sheetview.ts b/src/plugins/ui_core_views/sheetview.ts index 1762cc59b..2c0ef1a5e 100644 --- a/src/plugins/ui_core_views/sheetview.ts +++ b/src/plugins/ui_core_views/sheetview.ts @@ -152,13 +152,20 @@ export class SheetViewPlugin extends UIPlugin { case "AlterZoneCorner": break; case "ZonesSelected": - // altering a zone should not move the viewport + let { col, row } = findCellInNewZone(event.previousAnchor.zone, event.anchor.zone); + if (event.mode === "updateAnchor") { + const oldZone = event.previousAnchor.zone; + const newZone = event.anchor.zone; + // altering a zone should not move the viewport in a dimension that wasn't changed + const { top, bottom, left, right } = this.getters.getActiveMainViewport(); + if (oldZone.left === newZone.left && oldZone.right === newZone.right) { + col = left > col || col > right ? left : col; + } + if (oldZone.top === newZone.top && oldZone.bottom === newZone.bottom) { + row = top > row || row > bottom ? top : row; + } + } const sheetId = this.getters.getActiveSheetId(); - let { col, row } = findCellInNewZone( - event.previousAnchor.zone, - event.anchor.zone, - this.getters.getActiveMainViewport() - ); col = Math.min(col, this.getters.getNumberCols(sheetId) - 1); row = Math.min(row, this.getters.getNumberRows(sheetId) - 1); this.refreshViewport(this.getters.getActiveSheetId(), { col, row }); diff --git a/src/plugins/ui_feature/autofill.ts b/src/plugins/ui_feature/autofill.ts index 9b880ddea..83e750f28 100644 --- a/src/plugins/ui_feature/autofill.ts +++ b/src/plugins/ui_feature/autofill.ts @@ -1,4 +1,4 @@ -import { clip, isInside, toCartesian, toXC, union } from "../../helpers/index"; +import { clip, isInside, toCartesian, toXC } from "../../helpers/index"; import { autofillModifiersRegistry, autofillRulesRegistry } from "../../registries/index"; import { AutofillData, @@ -85,6 +85,7 @@ export class AutofillPlugin extends UIPlugin { static getters = ["getAutofillTooltip"] as const; private autofillZone: Zone | undefined; + private steps: number | undefined; private lastCellSelected: { col?: number; row?: number } = {}; private direction: DIRECTION | undefined; private tooltip: Tooltip | undefined; @@ -167,7 +168,7 @@ export class AutofillPlugin extends UIPlugin { * useful to set it to false when we need to fill the tooltip */ private autofill(apply: boolean) { - if (!this.autofillZone || this.direction === undefined) { + if (!this.autofillZone || !this.steps || this.direction === undefined) { this.tooltip = undefined; return; } @@ -226,12 +227,12 @@ export class AutofillPlugin extends UIPlugin { } if (apply) { - const zone = union(this.getters.getSelectedZone(), this.autofillZone); this.autofillZone = undefined; + this.selection.resizeAnchorZone(this.direction, this.steps); this.lastCellSelected = {}; this.direction = undefined; + this.steps = 0; this.tooltip = undefined; - this.selection.selectZone({ cell: { col: zone.left, row: zone.top }, zone }); } } @@ -248,15 +249,19 @@ export class AutofillPlugin extends UIPlugin { switch (this.direction) { case DIRECTION.UP: this.saveZone(row, source.top - 1, source.left, source.right); + this.steps = source.top - row; break; case DIRECTION.DOWN: this.saveZone(source.bottom + 1, row, source.left, source.right); + this.steps = row - source.bottom; break; case DIRECTION.LEFT: this.saveZone(source.top, source.bottom, col, source.left - 1); + this.steps = source.left - col; break; case DIRECTION.RIGHT: this.saveZone(source.top, source.bottom, source.right + 1, col); + this.steps = col - source.right; break; } this.autofill(false); diff --git a/src/selection_stream/selection_stream_processor.ts b/src/selection_stream/selection_stream_processor.ts index aba5e925f..cbe14b63c 100644 --- a/src/selection_stream/selection_stream_processor.ts +++ b/src/selection_stream/selection_stream_processor.ts @@ -147,7 +147,10 @@ export class SelectionStreamProcessor /** * Set the selection to one of the cells adjacent to the current anchor cell. */ - moveAnchorCell(direction: SelectionDirection, step: SelectionStep = "one"): DispatchResult { + moveAnchorCell(direction: SelectionDirection, step: SelectionStep = 1): DispatchResult { + if (step !== "end" && step <= 0) { + return new DispatchResult(CommandResult.InvalidSelectionStep); + } const { col, row } = this.getNextAvailablePosition(direction, step); return this.selectCell(col, row); } @@ -193,7 +196,10 @@ export class SelectionStreamProcessor * The anchor cell remains where it is. It's the opposite side * of the anchor zone which moves. */ - resizeAnchorZone(direction: SelectionDirection, step: SelectionStep = "one"): DispatchResult { + resizeAnchorZone(direction: SelectionDirection, step: SelectionStep = 1): DispatchResult { + if (step !== "end" && step <= 0) { + return new DispatchResult(CommandResult.InvalidSelectionStep); + } const sheetId = this.getters.getActiveSheetId(); const anchor = this.anchor; const { col: anchorCol, row: anchorRow } = anchor.cell; @@ -437,7 +443,7 @@ export class SelectionStreamProcessor */ private getNextAvailablePosition( direction: SelectionDirection, - step: SelectionStep = "one" + step: SelectionStep = 1 ): Position { const { col, row } = this.anchor.cell; const delta = this.deltaToTarget({ col, row }, direction, step); @@ -536,20 +542,20 @@ export class SelectionStreamProcessor ): Delta { switch (direction) { case "up": - return step === "one" - ? [0, -1] + return step !== "end" + ? [0, -step] : [0, this.getEndOfCluster(position, "rows", -1) - position.row]; case "down": - return step === "one" - ? [0, 1] + return step !== "end" + ? [0, step] : [0, this.getEndOfCluster(position, "rows", 1) - position.row]; case "left": - return step === "one" - ? [-1, 0] + return step !== "end" + ? [-step, 0] : [this.getEndOfCluster(position, "cols", -1) - position.col, 0]; case "right": - return step === "one" - ? [1, 0] + return step !== "end" + ? [step, 0] : [this.getEndOfCluster(position, "cols", 1) - position.col, 0]; } } diff --git a/src/types/commands.ts b/src/types/commands.ts index cb0bbd5b9..9fac25e33 100644 --- a/src/types/commands.ts +++ b/src/types/commands.ts @@ -1113,6 +1113,7 @@ export const enum CommandResult { MergeInFilter, NonContinuousTargets, DuplicatedFigureId, + InvalidSelectionStep, } export interface CommandHandler { diff --git a/src/types/misc.ts b/src/types/misc.ts index 36ffdfa22..97af2e746 100644 --- a/src/types/misc.ts +++ b/src/types/misc.ts @@ -232,10 +232,10 @@ export type BorderCommand = export type BorderDescription = { vertical?: BorderDescr; horizontal?: BorderDescr } | undefined; export const enum DIRECTION { - UP, - DOWN, - LEFT, - RIGHT, + UP = "up", + DOWN = "down", + LEFT = "left", + RIGHT = "right", } export type ChangeType = "REMOVE" | "RESIZE" | "MOVE" | "CHANGE" | "NONE"; diff --git a/src/types/selection.ts b/src/types/selection.ts index 9ba67c324..66bef5fde 100644 --- a/src/types/selection.ts +++ b/src/types/selection.ts @@ -1,3 +1,3 @@ export type SelectionDirection = "up" | "down" | "left" | "right"; -export type SelectionStep = "one" | "end"; +export type SelectionStep = number | "end"; diff --git a/tests/plugins/navigation.test.ts b/tests/plugins/navigation.test.ts index 8bd3da2c7..5d3737453 100644 --- a/tests/plugins/navigation.test.ts +++ b/tests/plugins/navigation.test.ts @@ -398,4 +398,25 @@ describe("Navigation starting from hidden cells", () => { expect(model.getters.getPosition()).toEqual(toCartesian(endPosition)); } ); + + test("Cannot from zero or negative steps does nothing", () => { + const model = new Model({ + sheets: [ + { + colNumber: 5, + rowNumber: 2, + }, + ], + }); + selectCell(model, "C1"); + hideColumns(model, ["C"]); + const move1 = moveAnchorCell(model, "down", 0); + expect(move1).toBeCancelledBecause(CommandResult.InvalidSelectionStep); + const move2 = moveAnchorCell(model, "up", -1); + expect(move2).toBeCancelledBecause(CommandResult.InvalidSelectionStep); + const move3 = moveAnchorCell(model, "right", -2); + expect(move3).toBeCancelledBecause(CommandResult.InvalidSelectionStep); + const move4 = moveAnchorCell(model, "left", -3); + expect(move4).toBeCancelledBecause(CommandResult.InvalidSelectionStep); + }); }); diff --git a/tests/plugins/sheetview.test.ts b/tests/plugins/sheetview.test.ts index 2bc8b18fb..fa757c437 100644 --- a/tests/plugins/sheetview.test.ts +++ b/tests/plugins/sheetview.test.ts @@ -667,6 +667,26 @@ describe("Viewport of Simple sheet", () => { }); }); + describe("Cross Move Position with selection outside the viewport affects offset", () => { + test("Move horizontally a cell which row is outside the viewport", () => { + const { bottom } = model.getters.getActiveMainViewport(); + selectCell(model, toXC(0, bottom + 3)); + const viewport = { ...model.getters.getActiveMainViewport() }; + model.dispatch("SET_VIEWPORT_OFFSET", { offsetX: 0, offsetY: 0 }); + moveAnchorCell(model, "right"); + expect(model.getters.getActiveMainViewport()).toMatchObject(viewport); + }); + + test("Move vertically a cell which col is outside the viewport", () => { + const { right } = model.getters.getActiveMainViewport(); + selectCell(model, toXC(right + 3, 0)); + const viewport = { ...model.getters.getActiveMainViewport() }; + model.dispatch("SET_VIEWPORT_OFFSET", { offsetX: 0, offsetY: 0 }); + moveAnchorCell(model, "down"); + expect(model.getters.getActiveMainViewport()).toMatchObject(viewport); + }); + }); + test("Move position on cells that are taller than the client's height", () => { const { height } = model.getters.getSheetViewDimensionWithHeaders(); resizeRows(model, [0], height + 50); diff --git a/tests/test_helpers/commands_helpers.ts b/tests/test_helpers/commands_helpers.ts index ac58f74fb..114d3ca1b 100644 --- a/tests/test_helpers/commands_helpers.ts +++ b/tests/test_helpers/commands_helpers.ts @@ -479,7 +479,7 @@ export function selectCell(model: Model, xc: string): DispatchResult { export function moveAnchorCell( model: Model, direction: SelectionDirection, - step: SelectionStep = "one" + step: SelectionStep = 1 ): DispatchResult { return model.selection.moveAnchorCell(direction, step); } @@ -487,7 +487,7 @@ export function moveAnchorCell( export function resizeAnchorZone( model: Model, direction: SelectionDirection, - step: SelectionStep = "one" + step: SelectionStep = 1 ): DispatchResult { return model.selection.resizeAnchorZone(direction, step); }