Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow empty menus to activate in menu bar #729

Open
sdirix opened this issue Nov 28, 2024 · 4 comments · May be fixed by #733
Open

Allow empty menus to activate in menu bar #729

sdirix opened this issue Nov 28, 2024 · 4 comments · May be fixed by #733
Labels
enhancement New feature or request

Comments

@sdirix
Copy link

sdirix commented Nov 28, 2024

Hi! I'm working on migrating to Lumino in Theia. We’ve previously relied on PhosphorJS, and decided to transition to Lumino.

Problem

We implemented a DynamicMenuWidget whose core functionality involves dynamically calculating its menu items just before being shown. This worked in PhosphorJS

However, we’ve encountered an issue with Lumino due to a change introduced in #607. Specifically, this check in the MenuBar prevents our menus from activating:

    // An empty menu cannot be active
    if (value > -1 && this._menus[value].items.length === 0) {
      value = -1;
    }

As a result, Theia’s dynamic menus are never activated and therefore never displayed.

Workarounds

We’ve explored a few workarounds:

  • Patching Lumino locally via patch-package to remove the check, but this requires maintenance with every new Lumino release.
  • Subclassing MenuBar and overriding the activeIndex setter to bypass the check, but this requires duplicating the setter code and working around type checks on private fields.

Both approaches feel brittle and suboptimal for long-term maintenance.

Proposed Solution?

Would you be open to changes that make the MenuBar more flexible for downstream adopters like us? If so, we’d appreciate your input on the best way forward. Some potential approaches:

  • Removing the check entirely if it’s not critical for some other use cases
  • Making private fields protected in MenuBar to allow subclassing without type workarounds.
  • Moving the check into a separate method that subclasses can override.

Alternatively, if you have suggestions for a better way to implement dynamic menus in Theia that align more closely with Lumino's current design, I’d be very interested to hear your thoughts.

Thanks for all your work on Lumino—it’s a fantastic library, and we look forward to using it in Theia!

@sdirix sdirix added the enhancement New feature or request label Nov 28, 2024
@krassowski
Copy link
Member

CC @gabalafou as author of #607.

@jtpio
Copy link
Member

jtpio commented Dec 19, 2024

Thanks @sdirix for opening the issue!

Cool to see Theia updating to Lumino! Agree it would be much better if Theia could just use the Lumino packages without having to maintain a patch 👍

  • Removing the check entirely if it’s not critical for some other use cases

Looking at the PR there does not seem to be any discussion about that check. But maybe @gabalafou or @fcollonval (reviewer) will know more.

  • Making private fields protected in MenuBar to allow subclassing without type workarounds.

This is indeed an approach that has been taken before to allow for easier subclassing. Maybe one downside to this would be the protected fields increasing the API surface.

  • Moving the check into a separate method that subclasses can override.

Maybe this could be the less disruptive one, while allowing for easier customization downstream?

If you would you like to start exploring this in a draft PR, that would be great, thanks!

@fcollonval
Copy link
Member

I second @jtpio. A good way would be to create a protected validateActiveIndex method that you could override.

@sdirix
Copy link
Author

sdirix commented Dec 19, 2024

Thanks for the feedback 👍 . Sounds good, I will open such a PR

sdirix added a commit to eclipsesource/lumino that referenced this issue Dec 20, 2024
Added a protected isValidIndex method to the MenuBar class to handle
index validation before setting an index.

This enables adopters to customize the validation logic, e.g., for
supporting dynamic menus that may initially be empty.

Resolves jupyterlab#729
@sdirix sdirix linked a pull request Dec 20, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants