From c5970f775078b68bce3866bcd9813a4a7f4ed858 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Rahir=20=28rar=29?= Date: Fri, 3 Jan 2025 09:32:00 +0000 Subject: [PATCH] [FIX] renderer: Fix grid rendering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we implemented the frozen panes, we accidently destroyed the distinction between the actual size of a cell/merge and its visual dimension in the viewport. i.e. the dimension of the part that we be rendered. However, the rest of the rendering process was relying on the box size to compute several things (button , text positions for instance) This means that all computations that relied on the actual size of a cell/merge are now false. A good example is the merge. Create a merge with some text inside of it and make sure it spans over several columns. The text is aligned at the center of the merge. Now scroll a bit and the text will stay centered in the VISIBLE part of the merge but not the merge itself. On another hand, we did not account for some text formatting, specifically text aligned on the right can overflow outside of their pane. This revision fixes the two issues: - Reintroduce a getter `getRect` to properly compute the positiontioning of text/icons of a box - Change the rendering process of the grid by splitting the rendering one pane at a time. closes odoo/o-spreadsheet#5489 Task: 4448426 X-original-commit: 44979e28bf97e1483f8d1a8fc26a474f513f9e87 Signed-off-by: Lucas Lefèvre (lul) Signed-off-by: Rémi Rahir (rar) --- src/helpers/internal_viewport.ts | 31 +++++-- src/plugins/ui_stateful/sheetview.ts | 42 ++++++++-- src/stores/grid_renderer_store.ts | 44 +++++++--- .../__snapshots__/renderer_store.test.ts.snap | 5 ++ tests/renderer_store.test.ts | 59 ++++++++++++- tests/sheet/sheetview_plugin.test.ts | 82 ++++++++++++++++++- 6 files changed, 229 insertions(+), 34 deletions(-) diff --git a/src/helpers/internal_viewport.ts b/src/helpers/internal_viewport.ts index 86467ad05..772084e9a 100644 --- a/src/helpers/internal_viewport.ts +++ b/src/helpers/internal_viewport.ts @@ -219,10 +219,10 @@ export class InternalViewport { /** * - * @param zone - * @returns Computes the absolute coordinate of a given zone inside the viewport + * Computes the visible coordinates & dimensions of a given zone inside the viewport + * */ - getRect(zone: Zone): Rect | undefined { + getVisibleRect(zone: Zone): Rect | undefined { const targetZone = intersection(zone, this); if (targetZone) { const x = @@ -239,12 +239,25 @@ export class InternalViewport { this.getters.getColRowOffset("ROW", targetZone.top, targetZone.bottom + 1), this.viewportHeight ); - return { - x, - y, - width, - height, - }; + return { x, y, width, height }; + } + return undefined; + } + + /** + * + * @returns Computes the absolute coordinates & dimensions of a given zone inside the viewport + * + */ + getFullRect(zone: Zone): Rect | undefined { + const targetZone = intersection(zone, this); + if (targetZone) { + const x = this.getters.getColRowOffset("COL", this.left, zone.left) + this.offsetCorrectionX; + const y = this.getters.getColRowOffset("ROW", this.top, zone.top) + this.offsetCorrectionY; + const width = this.getters.getColRowOffset("COL", zone.left, zone.right + 1); + + const height = this.getters.getColRowOffset("ROW", zone.top, zone.bottom + 1); + return { x, y, width, height }; } return undefined; } diff --git a/src/plugins/ui_stateful/sheetview.ts b/src/plugins/ui_stateful/sheetview.ts index 486a39118..49c9f5b96 100644 --- a/src/plugins/ui_stateful/sheetview.ts +++ b/src/plugins/ui_stateful/sheetview.ts @@ -103,6 +103,8 @@ export class SheetViewPlugin extends UIPlugin { "isPositionVisible", "getColDimensionsInViewport", "getRowDimensionsInViewport", + "getAllActiveViewportsZones", + "getRect", ] as const; readonly viewports: Record = {}; @@ -530,17 +532,30 @@ export class SheetViewPlugin extends UIPlugin { getVisibleRectWithoutHeaders(zone: Zone): Rect { const sheetId = this.getters.getActiveSheetId(); const viewportRects = this.getSubViewports(sheetId) - .map((viewport) => viewport.getRect(zone)) + .map((viewport) => viewport.getVisibleRect(zone)) .filter(isDefined); if (viewportRects.length === 0) { return { x: 0, y: 0, width: 0, height: 0 }; } - const x = Math.min(...viewportRects.map((rect) => rect.x)); - const y = Math.min(...viewportRects.map((rect) => rect.y)); - const width = Math.max(...viewportRects.map((rect) => rect.x + rect.width)) - x; - const height = Math.max(...viewportRects.map((rect) => rect.y + rect.height)) - y; - return { x, y, width, height }; + return this.recomposeRect(viewportRects); + } + + /** + * Computes the actual size and position (:Rect) of the zone on the canvas + * regardless of the viewport dimensions. + */ + getRect(zone: Zone): Rect { + const sheetId = this.getters.getActiveSheetId(); + const viewportRects = this.getSubViewports(sheetId) + .map((viewport) => viewport.getFullRect(zone)) + .filter(isDefined); + + if (viewportRects.length === 0) { + return { x: 0, y: 0, width: 0, height: 0 }; + } + const rect = this.recomposeRect(viewportRects); + return { ...rect, x: rect.x + this.gridOffsetX, y: rect.y + this.gridOffsetY }; } /** @@ -588,11 +603,16 @@ export class SheetViewPlugin extends UIPlugin { }; } + getAllActiveViewportsZones(): Zone[] { + const sheetId = this.getters.getActiveSheetId(); + return this.getSubViewports(sheetId); + } + // --------------------------------------------------------------------------- // Private // --------------------------------------------------------------------------- - private ensureMainViewportExist(sheetId) { + private ensureMainViewportExist(sheetId: UID) { if (!this.viewports[sheetId]) { this.resetViewports(sheetId); } @@ -851,4 +871,12 @@ export class SheetViewPlugin extends UIPlugin { const height = this.sheetViewHeight + this.gridOffsetY; return { xRatio: offsetCorrectionX / width, yRatio: offsetCorrectionY / height }; } + + private recomposeRect(viewportRects: Rect[]): Rect { + const x = Math.min(...viewportRects.map((rect) => rect.x)); + const y = Math.min(...viewportRects.map((rect) => rect.y)); + const width = Math.max(...viewportRects.map((rect) => rect.x + rect.width)) - x; + const height = Math.max(...viewportRects.map((rect) => rect.y + rect.height)) - y; + return { x, y, width, height }; + } } diff --git a/src/stores/grid_renderer_store.ts b/src/stores/grid_renderer_store.ts index c8a3a645b..7f746bde6 100644 --- a/src/stores/grid_renderer_store.ts +++ b/src/stores/grid_renderer_store.ts @@ -88,13 +88,23 @@ export class GridRenderer { drawLayer(renderingContext: GridRenderingContext, layer: LayerName) { switch (layer) { case "Background": - const boxes = this.getGridBoxes(); - this.drawBackground(renderingContext, boxes); - this.drawOverflowingCellBackground(renderingContext, boxes); - this.drawCellBackground(renderingContext, boxes); - this.drawBorders(renderingContext, boxes); - this.drawTexts(renderingContext, boxes); - this.drawIcon(renderingContext, boxes); + this.drawGlobalBackground(renderingContext); + for (const zone of this.getters.getAllActiveViewportsZones()) { + const { ctx } = renderingContext; + ctx.save(); + ctx.beginPath(); + const rect = this.getters.getVisibleRect(zone); + ctx.rect(rect.x, rect.y, rect.width, rect.height); + ctx.clip(); + const boxes = this.getGridBoxes(zone); + this.drawBackground(renderingContext, boxes); + this.drawOverflowingCellBackground(renderingContext, boxes); + this.drawCellBackground(renderingContext, boxes); + this.drawBorders(renderingContext, boxes); + this.drawTexts(renderingContext, boxes); + this.drawIcon(renderingContext, boxes); + ctx.restore(); + } this.drawFrozenPanes(renderingContext); break; case "Headers": @@ -106,13 +116,17 @@ export class GridRenderer { } } - private drawBackground(renderingContext: GridRenderingContext, boxes: Box[]) { - const { ctx, thinLineWidth } = renderingContext; + private drawGlobalBackground(renderingContext: GridRenderingContext) { + const { ctx } = renderingContext; const { width, height } = this.getters.getSheetViewDimensionWithHeaders(); // white background ctx.fillStyle = "#ffffff"; ctx.fillRect(0, 0, width + CANVAS_SHIFT, height + CANVAS_SHIFT); + } + + private drawBackground(renderingContext: GridRenderingContext, boxes: Box[]) { + const { ctx, thinLineWidth } = renderingContext; const areGridLinesVisible = !this.getters.isDashboard() && @@ -581,7 +595,7 @@ export class GridRenderer { const position = { sheetId, col, row }; const cell = this.getters.getEvaluatedCell(position); const showFormula = this.getters.shouldShowFormulas(); - const { x, y, width, height } = this.getters.getVisibleRect(zone); + const { x, y, width, height } = this.getters.getRect(zone); const { verticalAlign } = this.getters.getCellStyle(position); const box: Box = { @@ -713,13 +727,17 @@ export class GridRenderer { return box; } - private getGridBoxes(): Box[] { + private getGridBoxes(zone: Zone): Box[] { const boxes: Box[] = []; - const visibleCols = this.getters.getSheetViewVisibleCols(); + const visibleCols = this.getters + .getSheetViewVisibleCols() + .filter((col) => col >= zone.left && col <= zone.right); const left = visibleCols[0]; const right = visibleCols[visibleCols.length - 1]; - const visibleRows = this.getters.getSheetViewVisibleRows(); + const visibleRows = this.getters + .getSheetViewVisibleRows() + .filter((row) => row >= zone.top && row <= zone.bottom); const top = visibleRows[0]; const bottom = visibleRows[visibleRows.length - 1]; const viewport = { left, right, top, bottom }; diff --git a/tests/__snapshots__/renderer_store.test.ts.snap b/tests/__snapshots__/renderer_store.test.ts.snap index 064958d3a..5d057741f 100644 --- a/tests/__snapshots__/renderer_store.test.ts.snap +++ b/tests/__snapshots__/renderer_store.test.ts.snap @@ -5,6 +5,10 @@ exports[`renderer snapshot for a simple grid rendering 1`] = ` "context.save()", "context.fillStyle="#ffffff";", "context.fillRect(0, 0, 952.5, 974.5)", + "context.save()", + "context.beginPath()", + "context.rect(0, 0, 952, 974)", + "context.clip()", "context.strokeStyle="#E2E3E3";", "context.lineWidth=0.4;", "context.strokeRect(0.04000000000000001, 0.04000000000000001, 95.92, 22.92)", @@ -1301,6 +1305,7 @@ exports[`renderer snapshot for a simple grid rendering 1`] = ` "context.textAlign="right";", "GET:font", "context.fillText("1", 92, 7)", + "context.restore()", "context.lineWidth=2.4000000000000004;", "context.strokeStyle="#DADFE8";", "context.beginPath()", diff --git a/tests/renderer_store.test.ts b/tests/renderer_store.test.ts index b0b41e5f0..fab3b86e8 100644 --- a/tests/renderer_store.test.ts +++ b/tests/renderer_store.test.ts @@ -31,6 +31,8 @@ import { addDataValidation, copy, deleteColumns, + freezeColumns, + freezeRows, merge, paste, resizeColumns, @@ -51,7 +53,14 @@ MockCanvasRenderingContext2D.prototype.measureText = function (text: string) { jest.mock("../src/helpers/uuid", () => require("./__mocks__/uuid")); function getBoxFromText(gridRenderer: GridRenderer, text: string): Box { - return (gridRenderer["getGridBoxes"]()! as Box[]).find( + const sheetId = gridRenderer["getters"].getActiveSheetId(); + const zone = { + left: 0, + right: gridRenderer["getters"].getNumberCols(sheetId) - 1, + top: 0, + bottom: gridRenderer["getters"].getNumberRows(sheetId) - 1, + }; + return (gridRenderer["getGridBoxes"](zone)! as Box[]).find( (b) => (b.content?.textLines || []).join(" ") === text )!; } @@ -2123,8 +2132,7 @@ describe("renderer", () => { const { drawGridRenderer, gridRendererStore } = setRenderer(model); let ctx = new MockGridRenderingContext(model, 1000, 1000, {}); drawGridRenderer(ctx); - //@ts-expect-error - const boxes = gridRendererStore.getGridBoxes(); + const boxes = gridRendererStore["getGridBoxes"](toZone("A1:B2")); const boxesText = boxes.map((box) => box.content?.textLines.join("")); expect(boxesText).toEqual(["=MUNIT(2)", "", "", ""]); }); @@ -2161,4 +2169,49 @@ describe("renderer", () => { expect(renderedTexts).toContain("hello"); }); }); + + test("Each frozen pane is clipped in the grid", () => { + const { drawGridRenderer, model } = setRenderer(); + setCellContent(model, "A1", "1"); + freezeColumns(model, 2); + freezeRows(model, 1); + const spyFn = jest.fn(); + let ctx = new MockGridRenderingContext(model, 1000, 1000, { + onFunctionCall: (key, args) => { + if (["rect", "clip"].includes(key)) { + spyFn(key, args); + } + }, + }); + drawGridRenderer(ctx); + expect(spyFn).toHaveBeenCalledTimes(8); + expect(spyFn).toHaveBeenNthCalledWith(1, "rect", [ + 0, + 0, + DEFAULT_CELL_WIDTH * 2, + DEFAULT_CELL_HEIGHT, + ]); + expect(spyFn).toHaveBeenNthCalledWith(2, "clip", []); + expect(spyFn).toHaveBeenNthCalledWith(3, "rect", [ + DEFAULT_CELL_WIDTH * 2, + 0, + 760, + DEFAULT_CELL_HEIGHT, + ]); + expect(spyFn).toHaveBeenNthCalledWith(4, "clip", []); + expect(spyFn).toHaveBeenNthCalledWith(5, "rect", [ + 0, + DEFAULT_CELL_HEIGHT, + DEFAULT_CELL_WIDTH * 2, + 951, + ]); + expect(spyFn).toHaveBeenNthCalledWith(6, "clip", []); + expect(spyFn).toHaveBeenNthCalledWith(7, "rect", [ + DEFAULT_CELL_WIDTH * 2, + DEFAULT_CELL_HEIGHT, + 760, + 951, + ]); + expect(spyFn).toHaveBeenNthCalledWith(8, "clip", []); + }); }); diff --git a/tests/sheet/sheetview_plugin.test.ts b/tests/sheet/sheetview_plugin.test.ts index e0ad00a3c..b90886685 100644 --- a/tests/sheet/sheetview_plugin.test.ts +++ b/tests/sheet/sheetview_plugin.test.ts @@ -920,18 +920,96 @@ describe("Viewport of Simple sheet", () => { }); }); - test("getVisibleRect with freezed panes returns the actual visible part of a zone", () => { + test("getVisibleRect with frozen panes returns the actual visible part of a zone", () => { freezeColumns(model, 1); freezeRows(model, 1); const width = 4.5 * DEFAULT_CELL_WIDTH; const height = 5.5 * DEFAULT_CELL_HEIGHT; model.dispatch("RESIZE_SHEETVIEW", { gridOffsetX: 0, gridOffsetY: 0, width, height }); - expect(model.getters.getVisibleRect(model.getters.getActiveMainViewport())).toEqual({ + const zone = model.getters.getActiveMainViewport(); + expect(model.getters.getVisibleRect(zone)).toEqual({ x: DEFAULT_CELL_WIDTH, y: DEFAULT_CELL_HEIGHT, width: 3.5 * DEFAULT_CELL_WIDTH, height: 4.5 * DEFAULT_CELL_HEIGHT, }); + setViewportOffset(model, DEFAULT_CELL_WIDTH, DEFAULT_CELL_HEIGHT); + expect(model.getters.getVisibleRect(zone)).toEqual({ + x: DEFAULT_CELL_WIDTH, + y: DEFAULT_CELL_HEIGHT, + width: 3 * DEFAULT_CELL_WIDTH, + height: 4 * DEFAULT_CELL_HEIGHT, + }); + }); + + test("getVisibleRect takes the scroll into account", () => { + merge(model, "A1:B2"); + const zone = toZone("A1:B2"); + expect(model.getters.getVisibleRect(zone)).toEqual({ + x: 0, + y: 0, + width: DEFAULT_CELL_WIDTH * 2, + height: DEFAULT_CELL_HEIGHT * 2, + }); + setViewportOffset(model, DEFAULT_CELL_WIDTH, DEFAULT_CELL_HEIGHT); + expect(model.getters.getVisibleRect(zone)).toEqual({ + x: 0, + y: 0, + width: DEFAULT_CELL_WIDTH, + height: DEFAULT_CELL_HEIGHT, + }); + }); + + test("getRect returns the full zone dimensions regardless of the viewport size", () => { + const width = 4.5 * DEFAULT_CELL_WIDTH; + const height = 5.5 * DEFAULT_CELL_HEIGHT; + model.dispatch("RESIZE_SHEETVIEW", { gridOffsetX: 0, gridOffsetY: 0, width, height }); + expect(model.getters.getRect(model.getters.getActiveMainViewport())).toEqual({ + x: 0, + y: 0, + width: 5 * DEFAULT_CELL_WIDTH, + height: 6 * DEFAULT_CELL_HEIGHT, + }); + }); + + test("getRect with frozen panes returns the full part of a zone", () => { + freezeColumns(model, 1); + freezeRows(model, 1); + const width = 4.5 * DEFAULT_CELL_WIDTH; + const height = 5.5 * DEFAULT_CELL_HEIGHT; + model.dispatch("RESIZE_SHEETVIEW", { gridOffsetX: 0, gridOffsetY: 0, width, height }); + const zone = model.getters.getActiveMainViewport(); + expect(model.getters.getRect(zone)).toEqual({ + x: DEFAULT_CELL_WIDTH, + y: DEFAULT_CELL_HEIGHT, + width: 4 * DEFAULT_CELL_WIDTH, + height: 5 * DEFAULT_CELL_HEIGHT, + }); + setViewportOffset(model, DEFAULT_CELL_WIDTH, DEFAULT_CELL_HEIGHT); + expect(model.getters.getRect(zone)).toEqual({ + x: 0, + y: 0, + width: 4 * DEFAULT_CELL_WIDTH, + height: 5 * DEFAULT_CELL_HEIGHT, + }); + }); + + test("getRect takes the scroll into account", () => { + merge(model, "A1:B2"); + const zone = toZone("A1:B2"); + expect(model.getters.getRect(zone)).toEqual({ + x: 0, + y: 0, + width: DEFAULT_CELL_WIDTH * 2, + height: DEFAULT_CELL_HEIGHT * 2, + }); + setViewportOffset(model, DEFAULT_CELL_WIDTH, DEFAULT_CELL_HEIGHT); + expect(model.getters.getRect(zone)).toEqual({ + x: -DEFAULT_CELL_WIDTH, + y: -DEFAULT_CELL_HEIGHT, + width: DEFAULT_CELL_WIDTH * 2, + height: DEFAULT_CELL_HEIGHT * 2, + }); }); test("Loading a model with initial revisions in sheet that is deleted doesn't crash", () => {