Skip to content

Commit

Permalink
[IMP] sheetview: remove useless Object.values()
Browse files Browse the repository at this point in the history
The SheetView plugin was doing Object.values() on arrays, which was
both useless and allocated some memory for new arrays.

This commit removes the useless Object.values() calls.

closes #3978

Task: 3846620
Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
  • Loading branch information
hokolomopo committed May 7, 2024
1 parent c8953c8 commit 3662b14
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 12 deletions.
13 changes: 6 additions & 7 deletions src/plugins/ui_stateful/sheetview.ts
Expand Up @@ -131,7 +131,7 @@ export class SheetViewPlugin extends UIPlugin {
case "SET_VIEWPORT_OFFSET":
return this.chainValidations(
this.checkScrollingDirection,
this.checkViewportsWillChange
this.checkIfViewportsWillChange
)(cmd);
case "RESIZE_SHEETVIEW":
return this.chainValidations(
Expand Down Expand Up @@ -263,8 +263,7 @@ export class SheetViewPlugin extends UIPlugin {
this.resetViewports(sheetId);
if (this.shouldAdjustViewports) {
const position = this.getters.getSheetPosition(sheetId);
const viewports = this.getSubViewports(sheetId);
Object.values(viewports).forEach((viewport) => {
this.getSubViewports(sheetId).forEach((viewport) => {
viewport.adjustPosition(position);
});
}
Expand Down Expand Up @@ -640,7 +639,7 @@ export class SheetViewPlugin extends UIPlugin {
return CommandResult.Success;
}

private checkViewportsWillChange({ offsetX, offsetY }: SetViewportOffsetCommand) {
private checkIfViewportsWillChange({ offsetX, offsetY }: SetViewportOffsetCommand) {
const sheetId = this.getters.getActiveSheetId();
const { maxOffsetX, maxOffsetY } = this.getMaximumSheetOffset();
const willScroll = this.getSubViewports(sheetId).some((viewport) =>
Expand All @@ -649,7 +648,7 @@ export class SheetViewPlugin extends UIPlugin {
clip(offsetY, 0, maxOffsetY)
)
);
return willScroll ? CommandResult.Success : CommandResult.NoViewportScroll;
return willScroll ? CommandResult.Success : CommandResult.ViewportScrollLimitsReached;
}

private getMainViewport(sheetId: UID): Viewport {
Expand Down Expand Up @@ -699,7 +698,7 @@ export class SheetViewPlugin extends UIPlugin {
private setSheetViewOffset(offsetX: Pixel, offsetY: Pixel) {
const sheetId = this.getters.getActiveSheetId();
const { maxOffsetX, maxOffsetY } = this.getMaximumSheetOffset();
Object.values(this.getSubViewports(sheetId)).forEach((viewport) =>
this.getSubViewports(sheetId).forEach((viewport) =>
viewport.setViewportOffset(clip(offsetX, 0, maxOffsetX), clip(offsetY, 0, maxOffsetY))
);
}
Expand Down Expand Up @@ -782,7 +781,7 @@ export class SheetViewPlugin extends UIPlugin {
* Adjust the viewport such that the anchor position is visible
*/
private refreshViewport(sheetId: UID, anchorPosition: Position) {
Object.values(this.getSubViewports(sheetId)).forEach((viewport) => {
this.getSubViewports(sheetId).forEach((viewport) => {
viewport.adjustViewportZone();
viewport.adjustPosition(anchorPosition);
});
Expand Down
2 changes: 1 addition & 1 deletion src/types/commands.ts
Expand Up @@ -1202,7 +1202,7 @@ export const enum CommandResult {
Readonly = "Readonly",
InvalidViewportSize = "InvalidViewportSize",
InvalidScrollingDirection = "InvalidScrollingDirection",
NoViewportScroll = "NoViewportScroll",
ViewportScrollLimitsReached = "ViewportScrollLimitsReached",
FigureDoesNotExist = "FigureDoesNotExist",
InvalidConditionalFormatId = "InvalidConditionalFormatId",
InvalidCellPopover = "InvalidCellPopover",
Expand Down
16 changes: 12 additions & 4 deletions tests/sheet/sheetview_plugin.test.ts
Expand Up @@ -74,10 +74,14 @@ describe("Viewport of Simple sheet", () => {
});

test("SET_VIEWPORT_OFFSET is refused if it won't scroll any viewport", () => {
expect(setViewportOffset(model, 0, 0)).toBeCancelledBecause(CommandResult.NoViewportScroll);
expect(setViewportOffset(model, 0, 0)).toBeCancelledBecause(
CommandResult.ViewportScrollLimitsReached
);

expect(setViewportOffset(model, 10, 10)).toBeSuccessfullyDispatched();
expect(setViewportOffset(model, 10, 10)).toBeCancelledBecause(CommandResult.NoViewportScroll);
expect(setViewportOffset(model, 10, 10)).toBeCancelledBecause(
CommandResult.ViewportScrollLimitsReached
);
});

test("Select cell correctly affects offset", () => {
Expand Down Expand Up @@ -947,10 +951,14 @@ describe("Multi Panes viewport", () => {
test("SET_VIEWPORT_OFFSET is refused if it won't scroll any viewport", () => {
freezeColumns(model, 4);
freezeRows(model, 5);
expect(setViewportOffset(model, 0, 0)).toBeCancelledBecause(CommandResult.NoViewportScroll);
expect(setViewportOffset(model, 0, 0)).toBeCancelledBecause(
CommandResult.ViewportScrollLimitsReached
);

expect(setViewportOffset(model, 10, 10)).toBeSuccessfullyDispatched();
expect(setViewportOffset(model, 10, 10)).toBeCancelledBecause(CommandResult.NoViewportScroll);
expect(setViewportOffset(model, 10, 10)).toBeCancelledBecause(
CommandResult.ViewportScrollLimitsReached
);
});

test("Freezing row generates 2 panes", () => {
Expand Down

0 comments on commit 3662b14

Please sign in to comment.