Skip to content

Commit

Permalink
[FIX] viewport: viewport maxSize should consider the client size
Browse files Browse the repository at this point in the history 
closes #2305

Task: 3204720
X-original-commit: cb964cf
Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
Signed-off-by: Rémi Rahir (rar) <[email protected]>
  • Loading branch information
@rrahir
rrahir committed Apr 7, 2023
1 parent 97297b5 commit 24ab3ed
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 55 deletions.
88 changes: 44 additions & 44 deletions src/helpers/internal_viewport.ts
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,5 @@
import { DEFAULT_CELL_HEIGHT, DEFAULT_CELL_WIDTH } from "../constants";
import {
Dimension,
Getters,
HeaderIndex,
Pixel,
Position,
Rect,
UID,
Zone,
ZoneDimension,
} from "../types";
import { Dimension, Getters, HeaderIndex, Pixel, Position, Rect, UID, Zone } from "../types";
import { intersection, isInside } from "./zones";

export class InternalViewport {
Expand All @@ -23,8 +13,8 @@ export class InternalViewport {
offsetScrollbarY: Pixel;
canScrollVertically: boolean;
canScrollHorizontally: boolean;
width: Pixel;
height: Pixel;
viewportWidth: Pixel;
viewportHeight: Pixel;
offsetCorrectionX: Pixel;
offsetCorrectionY: Pixel;

Expand All @@ -36,8 +26,8 @@ export class InternalViewport {
options: { canScrollVertically: boolean; canScrollHorizontally: boolean },
offsets: { x: Pixel; y: Pixel }
) {
this.width = sizeInGrid.width;
this.height = sizeInGrid.height;
this.viewportWidth = sizeInGrid.width;
this.viewportHeight = sizeInGrid.height;
this.offsetScrollbarX = offsets.x;
this.offsetScrollbarY = offsets.y;
this.canScrollVertically = options.canScrollVertically;
Expand All @@ -55,7 +45,11 @@ export class InternalViewport {

// PUBLIC

getMaxSize(): ZoneDimension {
/** Returns the maximum size (in Pixels) of the viewport relative to its allocated client size
* When the viewport grid size is smaller than its client width (resp. height), it will return
* the client width (resp. height).
*/
getMaxSize(): { width: Pixel; height: Pixel } {
const lastCol = this.getters.findLastVisibleColRowIndex(this.sheetId, "COL", {
first: this.boundaries.left,
last: this.boundaries.right,
Expand All @@ -72,23 +66,29 @@ export class InternalViewport {
this.sheetId,
lastRow
);
const leftColIndex = this.searchHeaderIndex("COL", lastColEnd - this.width, 0);

const leftColIndex = this.searchHeaderIndex("COL", lastColEnd - this.viewportWidth, 0);
const leftColSize = this.getters.getColSize(this.sheetId, leftColIndex);
const leftRowIndex = this.searchHeaderIndex("ROW", lastRowEnd - this.height, 0);
const leftRowIndex = this.searchHeaderIndex("ROW", lastRowEnd - this.viewportHeight, 0);
const topRowSize = this.getters.getRowSize(this.sheetId, leftRowIndex);

const width =
lastColEnd -
this.offsetCorrectionX +
(this.canScrollHorizontally
? Math.max(DEFAULT_CELL_WIDTH, Math.min(leftColSize, this.width - lastColSize))
: 0);
const height =
lastRowEnd -
this.offsetCorrectionY +
(this.canScrollVertically
? Math.max(DEFAULT_CELL_HEIGHT + 5, Math.min(topRowSize, this.height - lastRowSize))
: 0);
let width = lastColEnd - this.offsetCorrectionX;
if (this.canScrollHorizontally) {
width += Math.max(
DEFAULT_CELL_WIDTH, // leave some minimal space to let the user know they scrolled all the way
Math.min(leftColSize, this.viewportWidth - lastColSize) // Add pixels that allows the snapping at maximum horizontal scroll
);
width = Math.max(width, this.viewportWidth); // if the viewport grid size is smaller than its client width, return client width
}

let height = lastRowEnd - this.offsetCorrectionY;
if (this.canScrollVertically) {
height += Math.max(
DEFAULT_CELL_HEIGHT + 5, // leave some space to let the user know they scrolled all the way
Math.min(topRowSize, this.viewportHeight - lastRowSize) // Add pixels that allows the snapping at maximum vertical scroll
);
height = Math.max(height, this.viewportHeight); // if the viewport grid size is smaller than its client height, return client height
}

return { width, height };
}
Expand All @@ -99,7 +99,7 @@ export class InternalViewport {
* It returns -1 if no column is found.
*/
getColIndex(x: Pixel, absolute = false): HeaderIndex {
if (x < this.offsetCorrectionX || x > this.offsetCorrectionX + this.width) {
if (x < this.offsetCorrectionX || x > this.offsetCorrectionX + this.viewportWidth) {
return -1;
}
return this.searchHeaderIndex("COL", x - this.offsetCorrectionX, this.left, absolute);
Expand All @@ -111,7 +111,7 @@ export class InternalViewport {
* It returns -1 if no row is found.
*/
getRowIndex(y: Pixel, absolute = false): HeaderIndex {
if (y < this.offsetCorrectionY || y > this.offsetCorrectionY + this.height) {
if (y < this.offsetCorrectionY || y > this.offsetCorrectionY + this.viewportHeight) {
return -1;
}
return this.searchHeaderIndex("ROW", y - this.offsetCorrectionY, this.top, absolute);
Expand Down Expand Up @@ -143,7 +143,7 @@ export class InternalViewport {
const sheetId = this.sheetId;
const { start, end } = this.getters.getColDimensions(sheetId, col);
while (
end > this.offsetX + this.offsetCorrectionX + this.width &&
end > this.offsetX + this.offsetCorrectionX + this.viewportWidth &&
this.offsetX + this.offsetCorrectionX < start
) {
this.offsetX = this.getters.getColDimensions(sheetId, this.left).end - this.offsetCorrectionX;
Expand All @@ -168,7 +168,7 @@ export class InternalViewport {
const sheetId = this.sheetId;
while (
this.getters.getRowDimensions(sheetId, row).end >
this.offsetY + this.height + this.offsetCorrectionY &&
this.offsetY + this.viewportHeight + this.offsetCorrectionY &&
this.offsetY + this.offsetCorrectionY < this.getters.getRowDimensions(sheetId, row).start
) {
this.offsetY = this.getters.getRowDimensions(sheetId, this.top).end - this.offsetCorrectionY;
Expand Down Expand Up @@ -216,11 +216,11 @@ export class InternalViewport {

const width = Math.min(
this.getters.getColRowOffset("COL", targetZone.left, targetZone.right + 1),
this.width
this.viewportWidth
);
const height = Math.min(
this.getters.getColRowOffset("ROW", targetZone.top, targetZone.bottom + 1),
this.height
this.viewportHeight
);
return {
x,
Expand Down Expand Up @@ -296,12 +296,12 @@ export class InternalViewport {
private adjustViewportOffsetX() {
if (this.canScrollHorizontally) {
const { width: viewportWidth } = this.getMaxSize();
if (this.width + this.offsetScrollbarX > viewportWidth) {
this.offsetScrollbarX = Math.max(0, viewportWidth - this.width);
if (this.viewportWidth + this.offsetScrollbarX > viewportWidth) {
this.offsetScrollbarX = Math.max(0, viewportWidth - this.viewportWidth);
}
}
this.left = this.getColIndex(this.offsetScrollbarX, true);
this.right = this.getColIndex(this.offsetScrollbarX + this.width, true);
this.right = this.getColIndex(this.offsetScrollbarX + this.viewportWidth, true);
if (this.right === -1) {
this.right = this.boundaries.right;
}
Expand All @@ -314,12 +314,12 @@ export class InternalViewport {
private adjustViewportOffsetY() {
if (this.canScrollVertically) {
const { height: paneHeight } = this.getMaxSize();
if (this.height + this.offsetScrollbarY > paneHeight) {
this.offsetScrollbarY = Math.max(0, paneHeight - this.height);
if (this.viewportHeight + this.offsetScrollbarY > paneHeight) {
this.offsetScrollbarY = Math.max(0, paneHeight - this.viewportHeight);
}
}
this.top = this.getRowIndex(this.offsetScrollbarY, true);
this.bottom = this.getRowIndex(this.offsetScrollbarY + this.width, true);
this.bottom = this.getRowIndex(this.offsetScrollbarY + this.viewportWidth, true);
if (this.bottom === -1) {
this.bottom = this.boundaries.bottom;
}
Expand All @@ -333,7 +333,7 @@ export class InternalViewport {
this.left = this.searchHeaderIndex("COL", this.offsetScrollbarX, this.boundaries.left);
this.right = Math.min(
this.boundaries.right,
this.searchHeaderIndex("COL", this.width, this.left)
this.searchHeaderIndex("COL", this.viewportWidth, this.left)
);
if (this.right === -1) {
this.right = this.getters.getNumberCols(sheetId) - 1;
Expand All @@ -350,7 +350,7 @@ export class InternalViewport {
this.top = this.searchHeaderIndex("ROW", this.offsetScrollbarY, this.boundaries.top);
this.bottom = Math.min(
this.boundaries.bottom,
this.searchHeaderIndex("ROW", this.height, this.top)
this.searchHeaderIndex("ROW", this.viewportHeight, this.top)
);
if (this.bottom === -1) {
this.bottom = this.getters.getNumberRows(sheetId) - 1;
Expand Down
7 changes: 3 additions & 4 deletions src/plugins/ui_core_views/sheetview.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -331,8 +331,7 @@ export class SheetViewPlugin extends UIPlugin {
}

/**
* Return the main viewport maximum size. That is the zone dimension
* with some bottom and right padding.
* Return the main viewport maximum size relative to the client size.
*/
getMainViewportRect(): Rect {
const sheetId = this.getters.getActiveSheetId();
Expand All @@ -349,8 +348,8 @@ export class SheetViewPlugin extends UIPlugin {
const { width, height } = this.getMainViewportRect();
const viewport = this.getMainInternalViewport(sheetId);
return {
maxOffsetX: Math.max(0, width - viewport.width + 1),
maxOffsetY: Math.max(0, height - viewport.height + 1),
maxOffsetX: Math.max(0, width - viewport.viewportWidth + 1),
maxOffsetY: Math.max(0, height - viewport.viewportHeight + 1),
};
}

Expand Down
4 changes: 2 additions & 2 deletions src/types/commands.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ export interface DeleteFigureCommand extends SheetDependentCommand {
export interface CreateChartCommand extends SheetDependentCommand {
type: "CREATE_CHART";
id: UID;
position?: { x: number; y: number };
position?: { x: Pixel; y: Pixel };
size?: FigureSize;
definition: ChartDefinition;
}
Expand All @@ -440,7 +440,7 @@ export interface UpdateChartCommand extends SheetDependentCommand {
export interface CreateImageOverCommand extends SheetDependentCommand {
type: "CREATE_IMAGE";
figureId: UID;
position: { x: number; y: number };
position: { x: Pixel; y: Pixel };
size: FigureSize;
definition: Image;
}
Expand Down
38 changes: 33 additions & 5 deletions tests/plugins/grid_manipulation.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1025,9 +1025,18 @@ describe("Rows", () => {
expect(model.getters.getRowSize(sheetId, 3)).toBe(10);
expect(model.getters.getRowSize(sheetId, 4)).toBe(20);
expect(model.getters.getRowSize(sheetId, 5)).toBe(size);
const dimensions = model.getters.getMainViewportRect();
expect(dimensions).toMatchObject({ width: 192, height: 124 });
expect(model.getters.getNumberRows(sheetId)).toBe(6);
const dimensions = model.getters.getMainViewportRect();
expect(dimensions).toMatchObject({ width: 1000, height: 1000 });
model.dispatch("RESIZE_SHEETVIEW", {
width: DEFAULT_CELL_WIDTH,
height: DEFAULT_CELL_HEIGHT,
});
const newDimensions = model.getters.getMainViewportRect();
expect(newDimensions).toMatchObject({
width: 192, // col size + 1 DEFAULT_CELL_WIDTH for spacing
height: 124, // sum of row sizes + 1 DEFAULT_CELL_HEIGHT and 5px for spacing
});
});
test("On addition after", () => {
addRows(model, "after", 2, 2);
Expand All @@ -1040,7 +1049,16 @@ describe("Rows", () => {
expect(model.getters.getRowSize(sheetId, 4)).toBe(20);
expect(model.getters.getRowSize(sheetId, 5)).toBe(size);
const dimensions = model.getters.getMainViewportRect();
expect(dimensions).toMatchObject({ width: 192, height: 144 });
expect(dimensions).toMatchObject({ width: 1000, height: 1000 });
model.dispatch("RESIZE_SHEETVIEW", {
width: DEFAULT_CELL_WIDTH,
height: DEFAULT_CELL_HEIGHT,
});
const newDimensions = model.getters.getMainViewportRect();
expect(newDimensions).toMatchObject({
width: 192, // col size + 1 DEFAULT_CELL_WIDTH for spacing
height: 144, // sum of row sizes + 1 DEFAULT_CELL_HEIGHT and 5px for spacing
});
expect(model.getters.getNumberRows(sheetId)).toBe(6);
});
test("cannot delete column in invalid sheet", () => {
Expand All @@ -1052,13 +1070,23 @@ describe("Rows", () => {

test("activate Sheet: same size", () => {
addRows(model, "after", 2, 1);
model.dispatch("RESIZE_SHEETVIEW", {
width: DEFAULT_CELL_WIDTH,
height: DEFAULT_CELL_HEIGHT,
});
let dimensions = model.getters.getMainViewportRect();
expect(dimensions).toMatchObject({ width: 192, height: 124 });
expect(dimensions).toMatchObject({
width: 192, // col size + 1 DEFAULT_CELL_WIDTH for spacing
height: 124, // sum of row sizes + 1 DEFAULT_CELL_HEIGHT and 5px for spacing
});
const to = model.getters.getActiveSheetId();
createSheet(model, { activate: true, sheetId: "42" });
activateSheet(model, to);
dimensions = model.getters.getMainViewportRect();
expect(dimensions).toMatchObject({ width: 192, height: 124 });
expect(dimensions).toMatchObject({
width: 192, // col size + 1 DEFAULT_CELL_WIDTH for spacing
height: 124, // sum of row sizes + 1 DEFAULT_CELL_HEIGHT and 5px for spacing
});
});
});

Expand Down
2 changes: 2 additions & 0 deletions tests/plugins/header_visibility.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ describe("Hide Columns", () => {
},
],
});
model.dispatch("RESIZE_SHEETVIEW", { width: DEFAULT_CELL_WIDTH, height: 1000 });
const sheet = model.getters.getActiveSheet();
const dimensions = model.getters.getMainViewportRect();
hideColumns(model, ["B", "C", "D"], sheet.id);
Expand Down Expand Up @@ -204,6 +205,7 @@ describe("Hide Rows", () => {
},
],
});
model.dispatch("RESIZE_SHEETVIEW", { width: 1000, height: DEFAULT_CELL_HEIGHT });
const sheet = model.getters.getActiveSheet();
const dimensions = model.getters.getMainViewportRect();
hideRows(model, [1, 2, 3], sheet.id);
Expand Down

0 comments on commit 24ab3ed

Please sign in to comment.