[IMP] Allows sorting filter menu in readonly mode

doc/extending/command.md

@@ -42,15 +42,7 @@ coreTypes.add("MY_COMMAND_NAME");
 
 ### Read-Only Mode
 
-In read-only mode, all core commands are cancelled with the `CommandResult` `Readonly` since the spreadsheet state cannot be modified.
-However, some locale commands still need to be executed, such as updating the active sheet.
-To allow a new local command in read-only mode, add its type to `readonlyAllowedCommands`:
-
-```ts
-import { readonlyAllowedCommands } from "@odoo/o-spreadsheet";
-
-readonlyAllowedCommands.add("MY_COMMAND_NAME");
-```
+All commands are allowed in readonly mode. Because the spreadsheet’s state cannot be changed, a specific transport service (ReadonlyTransportFilter) is used to prevent revisions from being sent to the server.
 
 ## Reserved keywords in commands
 

src/components/figures/figure_carousel/figure_carousel.ts

@@ -80,6 +80,9 @@ export class CarouselFigure extends Component<Props, SpreadsheetChildEnv> {
   }
 
   onCarouselDoubleClick() {
+    if (this.env.model.getters.isReadonly()) {
+      return;
+    }
     this.env.model.dispatch("SELECT_FIGURE", { figureId: this.props.figureUI.id });
     this.env.openSidePanel("CarouselPanel", { figureId: this.props.figureUI.id });
   }

src/components/figures/figure_chart/figure_chart.ts

@@ -27,6 +27,9 @@ export class ChartFigure extends Component<Props, SpreadsheetChildEnv> {
   static components = { ChartDashboardMenu };
 
   onDoubleClick() {
+    if (this.env.model.getters.isReadonly()) {
+      return;
+    }
     this.env.model.dispatch("SELECT_FIGURE", { figureId: this.props.figureUI.id });
     this.env.openSidePanel("ChartPanel");
   }

src/components/filters/filter_menu/filter_menu.ts

@@ -53,7 +53,7 @@ export class FilterMenu extends Component<Props, SpreadsheetChildEnv> {
       this.table.range.sheetId,
       this.table.range.zone
     );
-    return !this.env.model.getters.isReadonly() && coreTable?.type !== "dynamic";
+    return coreTable?.type !== "dynamic";
   }
 
   get table() {

src/components/grid/grid.ts

@@ -446,6 +446,44 @@ export class Grid extends Component<Props, SpreadsheetChildEnv> {
     "Alt+Shift+ArrowDown": () => this.processHeaderGroupingKey("down"),
   };
 
+  private readonlyAllowedShortcuts = new Set([
+    "Tab",
+    "Shift+Tab",
+    "Escape",
+    "Ctrl+A",
+    "Ctrl+Z",
+    "Ctrl+Y",
+    "F4",
+    "Alt+Enter",
+    "Ctrl+Home",
+    "Ctrl+End",
+    "Shift+ ",
+    "Ctrl+ ",
+    "Ctrl+H",
+    "Ctrl+F",
+    "Ctrl+Shift+ ",
+    "ArrowRight",
+    "ArrowLeft",
+    "ArrowUp",
+    "ArrowDown",
+    "Alt+ArrowRight",
+    "Alt+ArrowLeft",
+    "Alt+ArrowUp",
+    "Alt+ArrowDown",
+    "Ctrl+ArrowRight",
+    "Ctrl+ArrowLeft",
+    "Ctrl+ArrowUp",
+    "Ctrl+ArrowDown",
+    "Shift+ArrowRight",
+    "Shift+ArrowLeft",
+    "Shift+ArrowUp",
+    "Shift+ArrowDown",
+    "Ctrl+Shift+ArrowRight",
+    "Ctrl+Shift+ArrowLeft",
+    "Ctrl+Shift+ArrowUp",
+    "Ctrl+Shift+ArrowDown",
+  ]);
+
   private focusComposerFromActiveCell() {
     const cell = this.env.model.getters.getActiveCell();
     cell.type === CellValueType.empty
@@ -647,6 +685,9 @@ export class Grid extends Component<Props, SpreadsheetChildEnv> {
 
   onKeydown(ev: KeyboardEvent) {
     const keyDownString = keyboardEventToShortcutString(ev);
+    if (this.env.model.getters.isReadonly() && !this.readonlyAllowedShortcuts.has(keyDownString)) {
+      return;
+    }
     const handler = this.keyDownMapping[keyDownString];
     if (handler) {
       ev.preventDefault();

src/helpers/ui/sheet_interactive.ts

@@ -10,6 +10,9 @@ export function interactiveRenameSheet(
   name: string,
   errorCallback: () => void
 ) {
+  if (env.model.getters.isReadonly()) {
+    return;
+  }
   const result = env.model.dispatch("RENAME_SHEET", {
     sheetId,
     newName: name,

src/index.ts

@@ -248,7 +248,6 @@ export type {
   TransportService,
 } from "./types/collaborative/transport_service";
 export {
-  canExecuteInReadonly,
   CommandResult,
   coreTypes,
   DispatchResult,
@@ -259,7 +258,6 @@ export {
   isCoreCommand,
   isSheetDependent,
   lockedSheetAllowedCommands,
-  readonlyAllowedCommands,
 } from "./types/commands";
 export type { CancelledReason } from "./types/commands";
 export { CellErrorType, EvaluationError } from "./types/errors";

src/model.ts

@@ -29,7 +29,6 @@ import { StateObserver } from "./state_observer";
 import { _t, setDefaultTranslationMethod } from "./translation";
 import { StateUpdateMessage } from "./types/collaborative/transport_service";
 import {
-  canExecuteInReadonly,
   Command,
   CommandDispatcher,
   CommandHandler,
@@ -512,9 +511,6 @@ export class Model extends EventBus<any> implements CommandDispatcher {
   dispatch: CommandDispatcher["dispatch"] = (type: CommandTypes, payload?: any) => {
     const command: Command = createCommand(type, payload);
     const status: Status = this.status;
-    if (this.getters.isReadonly() && !canExecuteInReadonly(command)) {
-      return new DispatchResult(CommandResult.Readonly);
-    }
     if (!this.session.canApplyOptimisticUpdate()) {
       return new DispatchResult(CommandResult.WaitingSessionConfirmation);
     }

src/types/commands.ts

@@ -197,29 +197,6 @@ export const invalidSubtotalFormulasCommands = new Set<CommandTypes>([
   "UPDATE_FILTER",
 ]);
 
-export const readonlyAllowedCommands = new Set<CommandTypes>([
-  "START",
-  "ACTIVATE_SHEET",
-
-  "COPY",
-
-  "RESIZE_SHEETVIEW",
-  "SET_VIEWPORT_OFFSET",
-  "SET_ZOOM",
-
-  "EVALUATE_CELLS",
-  "EVALUATE_CHARTS",
-
-  "SET_FORMULA_VISIBILITY",
-
-  "UPDATE_FILTER",
-  "UPDATE_CHART",
-  "UPDATE_CHART_REGION",
-  "UPDATE_CAROUSEL_ACTIVE_ITEM",
-
-  "UPDATE_PIVOT",
-]);
-
 export const lockedSheetAllowedCommands = new Set<Command["type"]>([
   // core commands
   "LOCK_SHEET",
@@ -357,10 +334,6 @@ export function isCoreCommand(cmd: Command): cmd is CoreCommand {
   return coreTypes.has(cmd.type as any);
 }
 
-export function canExecuteInReadonly(cmd: Command): boolean {
-  return readonlyAllowedCommands.has(cmd.type);
-}
-
 //#region Core Commands
 // ------------------------------------------------
 

tests/collaborative/collaborative.test.ts

@@ -603,11 +603,6 @@ describe("Multi users synchronisation", () => {
       (user) => getCellContent(user, "A1"),
       "hello"
     );
-    setCellContent(david, "A1", "I'm David and I want access !");
-    expect([alice, bob, charlie, david]).toHaveSynchronizedValue(
-      (user) => getCellContent(user, "A1"),
-      "hello"
-    );
   });
 
   test("autofill overwrite style and format", () => {

tests/figures/chart/chart_animations.test.ts

@@ -1,5 +1,5 @@
 import { Chart } from "chart.js";
-import { Model, readonlyAllowedCommands } from "../../../src";
+import { Model } from "../../../src";
 import { ChartAnimationStore } from "../../../src/components/figures/chart/chartJs/chartjs_animation_store";
 import { toChartDataSource } from "../../test_helpers/chart_helpers";
 import {
@@ -60,8 +60,6 @@ describe("Chart animations in dashboard", () => {
   });
 
   test("Animations are replayed only when chart data changes", async () => {
-    readonlyAllowedCommands.add("UPDATE_CELL");
-
     const model = new Model();
     createChart(model, {
       type: "bar",
@@ -81,13 +79,9 @@ describe("Chart animations in dashboard", () => {
     setCellContent(model, "A2", "6");
     await nextTick();
     expect(mockedChart.config.options.animation).toEqual({ animateRotate: true });
-
-    readonlyAllowedCommands.delete("UPDATE_CELL");
   });
 
   test("Treemap animation are not replayed when data does not change but runtime is re-created", async () => {
-    readonlyAllowedCommands.add("UPDATE_CELL");
-
     const model = new Model();
     createChart(model, {
       type: "treemap",
@@ -102,8 +96,6 @@ describe("Chart animations in dashboard", () => {
     setCellContent(model, "B1", "6");
     await nextTick();
     expect(mockedChart.config.options.animation).toBe(false);
-
-    readonlyAllowedCommands.delete("UPDATE_CELL");
   });
 
   test("Charts are animated when chart type changes", async () => {

tests/figures/chart/gauge/gauge_rendering.test.ts

@@ -1,4 +1,4 @@
-import { Model, readonlyAllowedCommands, Rect } from "../../../../src";
+import { Model, Rect } from "../../../../src";
 import { GaugeChartComponent } from "../../../../src/components/figures/chart/gauge/gauge_chart_component";
 import { CHART_PADDING, CHART_TITLE_FONT_SIZE } from "../../../../src/constants";
 import { chartMutedFontColor } from "../../../../src/helpers/figures/charts/chart_common";
@@ -271,8 +271,6 @@ describe("Gauge chart component animation", () => {
   });
 
   test("Animations are replayed only when chart data changes", async () => {
-    readonlyAllowedCommands.add("UPDATE_CELL");
-
     const model = new Model();
     createGaugeChart(model, { dataRange: "A1" });
     model.updateMode("dashboard");
@@ -289,7 +287,5 @@ describe("Gauge chart component animation", () => {
     setCellContent(model, "A1", "6");
     await nextTick();
     expect(gaugeAnimationSpy).toHaveBeenCalledTimes(2);
-
-    readonlyAllowedCommands.delete("UPDATE_CELL");
   });
 });

tests/lock_sheet/lock_sheet_plugin.test.ts

@@ -6,15 +6,13 @@ import {
   isCoreCommand,
   isSheetDependent,
   lockedSheetAllowedCommands,
-  readonlyAllowedCommands,
 } from "../../src";
 import { createChart, createSheet, lockSheet } from "../test_helpers/commands_helpers";
 import { TEST_COMMANDS } from "../test_helpers/constants";
 import { addPivot } from "../test_helpers/pivot_helpers";
 
 const allowedCommands: Command["type"][] = [];
 const rejectedCommands: Command["type"][] = [];
-const readonlyCommands: Command["type"][] = [];
 
 (Object.keys(TEST_COMMANDS) as CoreCommand["type"][]).forEach((cmdType, cmd) => {
   if (
@@ -25,9 +23,6 @@ const readonlyCommands: Command["type"][] = [];
   } else {
     rejectedCommands.push(cmdType);
   }
-  if (readonlyAllowedCommands.has(cmdType)) {
-    readonlyCommands.push(cmdType);
-  }
 });
 
 describe("Lock Sheet plugin", () => {
@@ -51,8 +46,9 @@ describe("Lock Sheet plugin", () => {
     }
   });
 
-  test("read only commands bypass lock in dashboard mode", () => {
-    for (const cmdType of readonlyCommands) {
+  test.each(["UPDATE_CHART", "UPDATE_PIVOT"])(
+    "%s command bypass lock in dashboard mode",
+    (cmdType: Command["type"]) => {
       const model = new Model();
       createSheet(model, { name: "Another sheet", position: 0 });
       createChart(model, { type: "bar" }, "chartId");
@@ -62,5 +58,5 @@ describe("Lock Sheet plugin", () => {
       const result = model.dispatch(cmdType, TEST_COMMANDS[cmdType]);
       expect(result).toBeSuccessfullyDispatched();
     }
-  });
+  );
 });

tests/model/model.test.ts

@@ -182,11 +182,6 @@ describe("Model", () => {
     expect(model.getters.isReadonly()).toBe(true);
   });
 
-  test("Some commands are not dispatched in readonly mode", () => {
-    const model = new Model({}, { mode: "readonly" });
-    expect(setCellContent(model, "A1", "hello")).toBeCancelledBecause(CommandResult.Readonly);
-  });
-
   test("Moving the selection is allowed in readonly mode", () => {
     const model = new Model({}, { mode: "readonly" });
     expect(selectCell(model, "A15")).toBeSuccessfullyDispatched();

tests/table/filter_menu_component.test.ts

@@ -5,6 +5,7 @@ import {
   Model,
   UID,
 } from "../../src";
+import { getCellContent } from "../test_helpers";
 import {
   createDynamicTable,
   createTableWithFilter,
@@ -393,18 +394,23 @@ describe("Filter menu component", () => {
     });
   });
 
-  test("cannot sort filter table in readonly mode", async () => {
-    createTableWithFilter(model, "A10:B15");
+  test("can sort filter table in readonly mode", async () => {
+    createTableWithFilter(model, "A1:A3");
+    setCellContent(model, "A2", "1");
+    setCellContent(model, "A3", "-2");
     await nextTick();
-    await openFilterMenu("A10");
+    await openFilterMenu("A1");
     expect(
       [...fixture.querySelectorAll(".o-filter-menu-item")].map((el) => el.textContent?.trim())
-    ).toEqual(["Sort ascending (A ⟶ Z)", "Sort descending (Z ⟶ A)", "(Blanks)"]);
+    ).toEqual(["Sort ascending (A ⟶ Z)", "Sort descending (Z ⟶ A)", "-2", "1"]);
     model.updateMode("readonly");
     await nextTick();
     expect(
       [...fixture.querySelectorAll(".o-filter-menu-item")].map((el) => el.textContent?.trim())
-    ).toEqual(["(Blanks)"]);
+    ).toEqual(["Sort ascending (A ⟶ Z)", "Sort descending (Z ⟶ A)", "-2", "1"]);
+    await simulateClick(".o-filter-menu-item:nth-of-type(1)");
+    expect(getCellContent(model, "A2")).toBe("-2");
+    expect(getCellContent(model, "A3")).toBe("1");
   });
 
   test("cannot sort dynamic table", async () => {