[FIX] menu: dynamically update visible menu items

Before this commit, the menu visible menu items were given as props of the
`Menu` component. This meant that contrarily to the property `isEnabled` of
the menu items, the `isVisible` property was computed once when opening
the menu, and never updated.

This commit fixes this issue by making the `Menu` component compute the
`isVisible` property at each render, dynamically updating which menu items
are visible based on the state of the model.

Odoo task 3204730

closes #2140

Signed-off-by: Lucas Lefèvre (lul) <[email protected]>

src/components/bottom_bar/bottom_bar.ts

@@ -189,7 +189,7 @@ export class BottomBar extends Component<Props, SpreadsheetChildEnv> {
 
   openContextMenu(x: Pixel, y: Pixel, registry: MenuItemRegistry, menuId?: UID) {
     this.menuState.isOpen = true;
-    this.menuState.menuItems = registry.getAll().filter((x) => x.isVisible(this.env));
+    this.menuState.menuItems = registry.getAll();
     this.menuState.position = { x, y };
     this.menuState.menuId = menuId;
   }

src/components/figures/figure_chart/figure_chart.ts

@@ -124,7 +124,7 @@ export class ChartFigure extends Component<Props, SpreadsheetChildEnv> {
   private openContextMenu(position: DOMCoordinates) {
     const registry = this.getMenuItemRegistry();
     this.menuState.isOpen = true;
-    this.menuState.menuItems = registry.getAll().filter((x) => x.isVisible(this.env));
+    this.menuState.menuItems = registry.getAll();
     this.menuState.position = position;
   }
 

src/components/grid/grid.ts

@@ -493,9 +493,7 @@ export class Grid extends Component<Props, SpreadsheetChildEnv> {
     this.closeOpenedPopover();
     this.menuState.isOpen = true;
     this.menuState.position = { x, y };
-    this.menuState.menuItems = registries[type]
-      .getAll()
-      .filter((item) => !item.isVisible || item.isVisible(this.env));
+    this.menuState.menuItems = registries[type].getAll();
   }
 
   copy(cut: boolean, ev: ClipboardEvent) {

src/components/menu/menu.ts

@@ -111,14 +111,18 @@ export class Menu extends Component<Props, SpreadsheetChildEnv> {
     });
   }
 
+  get visibleMenuItems(): MenuItem[] {
+    return this.props.menuItems.filter((x) => x.isVisible(this.env));
+  }
+
   get subMenuPosition(): DOMCoordinates {
     const position = Object.assign({}, this.subMenu.position);
     position.y -= this.subMenu.scrollOffset || 0;
     return position;
   }
 
   get menuHeight(): Pixel {
-    return this.menuComponentHeight(this.props.menuItems);
+    return this.menuComponentHeight(this.visibleMenuItems);
   }
 
   get subMenuHeight(): Pixel {
@@ -159,7 +163,7 @@ export class Menu extends Component<Props, SpreadsheetChildEnv> {
    * and the menu item at a given index.
    */
   private subMenuVerticalPosition(position: Pixel): Pixel {
-    const menusAbove = this.props.menuItems.slice(0, position);
+    const menusAbove = this.visibleMenuItems.slice(0, position);
     return this.menuComponentHeight(menusAbove) + this.position.y;
   }
 

src/components/menu/menu.xml

@@ -1,7 +1,7 @@
 <templates>
   <t t-name="o-spreadsheet-Menu" owl="1">
     <Popover
-      t-if="props.menuItems.length"
+      t-if="visibleMenuItems.length"
       position="props.position"
       childWidth="MENU_WIDTH"
       childHeight="menuHeight"
@@ -15,7 +15,7 @@
         t-on-wheel.stop=""
         t-on-click.stop=""
         t-on-contextmenu.prevent="">
-        <t t-foreach="props.menuItems" t-as="menuItem" t-key="menuItem.id">
+        <t t-foreach="visibleMenuItems" t-as="menuItem" t-key="menuItem.id">
           <t t-set="isMenuRoot" t-value="isRoot(menuItem)"/>
           <t t-set="isMenuEnabled" t-value="isEnabled(menuItem)"/>
           <div

src/components/top_bar/top_bar.ts

@@ -374,9 +374,7 @@ export class TopBar extends Component<Props, SpreadsheetChildEnv> {
     const { left, top, height } = (ev.target as HTMLElement).getBoundingClientRect();
     this.state.menuState.isOpen = true;
     this.state.menuState.position = { x: left, y: top + height };
-    this.state.menuState.menuItems = getMenuChildren(menu, this.env).filter(
-      (item) => !item.isVisible || item.isVisible(this.env)
-    );
+    this.state.menuState.menuItems = getMenuChildren(menu, this.env);
     this.state.menuState.parentMenu = menu;
     this.isSelectingMenu = true;
     this.openedEl = ev.target as HTMLElement;
@@ -427,9 +425,7 @@ export class TopBar extends Component<Props, SpreadsheetChildEnv> {
     this.fillColor = this.style.fillColor || "#ffffff";
     this.textColor = this.style.textColor || "#000000";
 
-    this.menus = topbarMenuRegistry
-      .getAll()
-      .filter((item) => !item.isVisible || item.isVisible(this.env));
+    this.menus = topbarMenuRegistry.getAll();
   }
 
   getMenuName(menu: FullMenuItem) {

tests/components/context_menu.test.ts

@@ -18,6 +18,7 @@ import {
 
 let fixture: HTMLElement;
 let model: Model;
+let parent: Component;
 
 beforeEach(async () => {
   const clipboard = new MockClipboard();
@@ -91,7 +92,7 @@ async function renderContextMenu(
   // x, y are relative to the upper left grid corner, but the menu
   // props must take the top bar into account.
 
-  ({ fixture, model } = await mountComponent(ContextMenuParent, {
+  ({ fixture, model, parent } = await mountComponent(ContextMenuParent, {
     props: {
       x,
       y: y + TOPBAR_HEIGHT,
@@ -646,6 +647,42 @@ describe("Context Menu internal tests", () => {
     expect(fixture.querySelector(".o-menu div[data-name='visible_submenu_1']")).toBeTruthy();
     expect(fixture.querySelector(".o-menu div[data-name='invisible_submenu_1']")).toBeFalsy();
   });
+
+  test("Enabled menus are updated at each render", async () => {
+    let enabled = true;
+    const menuItems: FullMenuItem[] = [
+      createFullMenuItem("menuItem", {
+        name: "menuItem",
+        sequence: 1,
+        isEnabled: () => enabled,
+      }),
+    ];
+    await renderContextMenu(300, 300, { menuItems });
+    expect(fixture.querySelector("div[data-name='menuItem']")?.classList).not.toContain("disabled");
+
+    enabled = false;
+    parent.render(true);
+    await nextTick();
+    expect(fixture.querySelector("div[data-name='menuItem']")?.classList).toContain("disabled");
+  });
+
+  test("Visible menus are updated at each render", async () => {
+    let visible = true;
+    const menuItems: FullMenuItem[] = [
+      createFullMenuItem("menuItem", {
+        name: "menuItem",
+        sequence: 1,
+        isVisible: () => visible,
+      }),
+    ];
+    await renderContextMenu(300, 300, { menuItems });
+    expect(fixture.querySelector("div[data-name='menuItem']")).toBeTruthy();
+
+    visible = false;
+    parent.render(true);
+    await nextTick();
+    expect(fixture.querySelector("div[data-name='menuItem']")).toBeFalsy();
+  });
 });
 
 describe("Context Menu position on large screen 1000px/1000px", () => {