Multiple Menu Fixes

[Araxeus]

I'll start with saying I love the idea of this lib, but I faced some problems while using it, that I fixed in my program awhile ago
But I decided I should probably also share some fixes on the original source.

The Initial Problems:

• Submenu's not showing at all (because .menubar-menu-container CSS property overflow-y wasn't set to visible)
• Clicking on a checkbox looked like it didn't do anything (it was setting item.checked = !item.checked twice)
• Radio buttons located in a submenu wouldn't dynamically update visually
• Submenu items wouldn't close on click
• Menu Items with roles didn't work at all
• titlebar.updateMenu(null || undefined) throws an error but works (dispose of current menu)

The Fixes :

• 9dc885f
  Set menu-container internal css to overflow-y: visible;. fix Submenu appears with a scrollbar #124

• 5e17933
  Fixes checkbox's being triggered twice. fix Checkmark menu items do not work. #116 (First trigger is in electron/menu-item.ts)

• a7cfa0a / 918e8e6
  inherit closeSubmenu() in Submenu class allows submenus to close when their content is clicked
  That fix items in submenu's not closing on click

• da8999c
  Following the last change, submenu will close only if item type isn't checkbox or radio which allows them to work as before

• 76b0726
  Fixes menu roles by emulating the accelerator shortcut buttonDown when menuitem is clicked. fix menu roles not working (electron 11.2.3) #133
  abit of a hacky way but I think its way better than the ipc method that didn't even work

• e4a5df1
  Dynamically visually update radio button groups by passing menuContainer to CETMenuItem and updating all radio buttons in the menuContainer when CETMenuItem.click() is called and type === radio

• 0e61412
  if menu in titlebar.updateMenu(menu) is null || undefined , dispose of menu and exit function to avoid errors

Would love feedback on this if there is any :)

Click to view sample menu template for testing

  const template = [
    {
      label: "Options",
      submenu: [
        {
          label: "zoomIn",
          role: "zoomIn"
        },
        {
          label: "zoomOut",
          role: "zoomOut"
        },
        {
          label: "Radio1",
          type: "radio",
          checked: true
        },
        {
          label: "Radio2",
          type: "radio",
        },
        {
          label: "Checkbox1",
          type: "checkbox",
          checked: true,
          click: (item) => {
            console.log("item is checked? "+item.checked);
          }
        },
        {
          label: "Checkbox2",
          type: "checkbox",
          checked: false,
          click: (item) => {
            console.log("item is checked? "+item.checked);
          }
        },
        {
          label: "Radio Test",
          submenu: [
            {
              label: "Sample Checkbox",
              type: "checkbox",
              checked: true
            },
            {
              label: "Radio1",
              checked: true,
              type: "radio"
            },
            {
              label: "Radio2",
              type: "radio"
            },
            {
              label: "Radio3",
              type: "radio"
            }
          ]
        },
        {
          label: "Quit",
          click: ()=>{app.quit()}
        },
      ]
    }
  ];
  const menu = Menu.buildFromTemplate(template);
  Menu.setApplicationMenu(menu);

---

Also, why is this git repo not actually the same version as the npm package? (3.2.5 vs 3.2.6)
It also has an src folder instead of lib, which means that when cloning the repo you don't have access to all files
and running dev will result in an error because of lib folder missing which is abit weird 😅

[ericblade]

It also has an src folder instead of lib, which means that cloning the repo and running dev will run an error because of lib folder missing which is abit weird

source belongs in repos, build data belongs in releases, and never the two shall meet except in dev repos and build servers. :-)

(at least, that's my personal take on it)

code changes look good, although i don't know where the duplicate checkbox toggling thing is. i don't know anything about the menu stuff actually, don't use it in my own instance at all.. but it looks good at a glance to me

[Araxeus]

@ericblade
I think checkbox + radio status internally works as usual when this.item.click() is called
(but not updated visually until this.updateChecked() or this.updateRadioGroup() is called or submenu is closed and re-opened
And that's because this.item isn't rendered again unless the whole submenu gets re-rendered)

That was the duplicate checkbox
https://github.com/AlexTorresSk/custom-electron-titlebar/blob/04b3e09e0f122ebe5c11db0fcdbb36b8995cd533/src/menu/menuitem.ts#L141-L144

[timlg07]

Yes, the checkbox toggled twice would explain #116. I would love to see your changes merged, they are looking great.

[ericblade]

That was the duplicate checkbox

I see where you removed one, I was just curious where the other one is, since you said it was getting triggered twice, and that's not in the changelog, since you didn't touch it :-)

The original menuitem actions file isn't even in this git, only in release (You see the problem with repo not having the same version as release ??)

a repo might collect many changes before a release is made...

Above, I was addressing your concern about the src not being in the release, and the lib not being in the src, which is imo, something that should never occur. A repo collecting changes before a release is highly dependent on the maintainers, though. I personally advocate for an automatic release generator that will always package up master branch whenever there's a change and make a release. A lot of people haven't got to setting that sort of thing up though. I haven't even got it close to working the way I'd like it to, on even any of my bigger repos.

[ericblade]

Anyway! LGTM, also 👍

[AlexTorresDev]

Hello everybody.

Thanks guys for always being on the lookout for updates and bugs from the library. I haven't really had time to publish the 3.2.6 changes to npm.
On the other hand, when I developed this library, I did not have the complete knowledge of releases, versioning, among others; therefore there is a lib folder which is actually the build folder for the entire project 🤭.
As soon as I have time I will dedicate myself to cleaning and debugging the code.

Thanks again for your support.

[Araxeus]

@AlexTorresSk
You should know that this pull request was tested on the 3.2.6, npm release
So it should be easy to implement into npm :)

[Araxeus]

@AlexTorresSk

I ended up fixing more stuff:

• Changed radio buttons to dynamically visually update on click (by iterating over all radio buttons in the same radioGroup)

  radioGroup is a group of radio buttons in a submenu, possibly start/end with a "separator"
  -that's the way radio groups originally works in electron

• Updated and fixed a typescript compile type error that was somehow missed, sorry about that 😅
  (it has been recompiled and tested thoroughly)

• Updated example/main.js to include a menu for easy testing using yarn dev

• if menu in titlebar.updateMenu(menu) is null || undefined , dispose of menu and exit function to avoid errors

[Araxeus]

@ericblade
My last answer on why the checkbox got updated twice was wrong. this is the actual reason:

checkbox + radio checked status are updated when this.item.click() is called (behavior of electron.menuItem),
but not updated visually until this.updateChecked() OR submenu is closed and re-opened
(and that's because this.item isn't rendered again unless the whole submenu gets re-rendered)

I used this fact to enable dynamic updating of "radio" item groups

---

So CETMenuItem.onClick() was calling item.click() (first change) and then item.checked = !item.checked (second change)

Source:

https://github.com/electron/electron/blob/master/lib/browser/api/menu-item.ts#L47-L52