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

Implement PopupButton Control as replacement for wrongly used MenuButtons #6115

Open
Maran23 opened this issue Jan 17, 2023 · 5 comments
Open

Comments

@Maran23
Copy link

Maran23 commented Jan 17, 2023

Describe the project you are working on

A game with heavy use of UI as well as the Godot editor.

Describe the problem or limitation you are having in your project

MenuButton

A MenuButton is flat and can not be focused by default.
This makes sense, as in all applications I know the menu can NOT be focused with the keyboard (or mouse), no matter if a native menu or not. To navigate to a Menu/MenuButton, mnemonic are typically used (e.g. holding ALT + F to navigate to File)
A menu entry is also typically flat in other applications too, so that is fine as well.

The problem is that we use MenuButtons not just for Menus. We also use it when we need a Button (text or/with graphic) which shows a Popup on click.

Example in editor:

menu_button_usage_in_editor.mp4

(Some Buttons there are MenuButtons and therefore skipped by navigating with TAB (no focus border -> no focus)

Current Behaviour

The current behaviour results in some problems:

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
Example where a MenuButton fits right and where it is obvious that it will show a dropdown menu.
We expect this behaviour as the Menu is right under the the Windows title bar, which is pretty normal for Windows users and commonly used across applications.
image

Example where a MenuButton is used wrong. It is not clear from the look that this will actually show a dropdown menu. You also can not navigate to it by mouse or keyboard.
image

OptionButton

Can we use the OptionButton? -> NO.
The OptionButton is a very specialized control and acts like a typical ComboBox or DropDown.

  • It shows the current selected item (icon + text)
    -> You can not set a text/icon to the OptionButton itself
  • It can have only one selected item (radio items)
  • It is also only made for radio items (we can not show something else without many hacking into the popup)

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Goal

A new component PopupButton, which is similar to the MenuButton, but does not contain any logic related to Menus or MenuButtons. It is not flat by default and focusable by mouse and keyboard.

It contains some methods from MenuButton, which are generic and useful for PopupButton like classes.
MenuButton extends from PopupButton.

Up for discussion: OptionButton extends from PopupButton.
This will reduce more duplicated code.

Non-Goal

  • Refactor any Button/Popup/MenuButton or OptionButton related code.
    -> The idea is to just introduce a new component to fit the usecase above, shift some methods from MenuButton (maybe also OptionButton) and replace the wrongly used MenuButtons in the next iteration.
  • We do not want to mimic the OptionButton.
    ->Therefore no specialized logic will be in the PopupButton class.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

The following diagram shows my idea:
abc

PopupButton contains generic code from MenuButton and OptionButton. This can save us some duplicate code as well.

  • A MenuButton is used in Menus of any kind and contains specialized logic for Menu handling
  • A OptionButton is used when a component is required that shows the current selected option and where only one selection is allowed.
  • A PopupButton is used outside of Menus where a component is required that fits with all the other components (can be navigated by mouse and keyboard, does not look flat by default) and shows a Popup which can be modified as needed.

PopupButton UI:
image


One thing up for discussion: The current MenuButton has one property called switch_on_hover, which is enabled on some MenuButtons in the editor.
We can shift it to the PopupButton as well to support more usecases as written in this proposal: #5197

If this enhancement will not be used often, can it be worked around with a few lines of script?

All 'wrongly' used MenuButtons needs to be adjusted with at least the following two lines of code in the editor and game code:
menu_button.set_flat(false);
menu_button.set_focus_mode(FOCUS_ALL);
That does not fix the issue, that a MenuButton does not look like a control with a Popup, So a graphic may need to be set as well.

The code duplication in the editor can not be solved otherwise.

Is there a reason why this should be core and not an add-on in the asset library?

I think this should be in core as it affects the editor itself, will reduce code duplications and will help game developers if they need a Button with a customized Popup outside of a Menu.

It is also consistent with all the other Controls, which are more generic and fit into more usecases.
MenuButton fits to the Menu usecase (the default setting also indicate that). It is a much more specialized Control and may even get more specialized properties/methods in the future (e.g. mnemonics).

@Maran23 Maran23 changed the title Implement PopupButton as replacement for wrongly used MenuButtons Implement PopupButton Control as replacement for wrongly used MenuButtons Jan 17, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Jan 17, 2023

This was actually implemented once in godotengine/godot#63358, but got reverted.

@zinnschlag
Copy link

This makes sense, as in all applications I know the menu can NOT be focused with the keyboard (or mouse), no matter if a native menu or not. To navigate to a Menu/MenuButton, mnemonic are typically used (e.g. holding ALT + F to navigate to File)

Just for reference: The ALT-key navigation is not universal. It exists primarily on Windows. Not that common on Linux (though a some Linux desktops do support it). No idea about Mac. And you absolute can navigate a menu with the keyboard.

@Maran23
Copy link
Author

Maran23 commented Jan 18, 2023

This makes sense, as in all applications I know the menu can NOT be focused with the keyboard (or mouse), no matter if a native menu or not. To navigate to a Menu/MenuButton, mnemonic are typically used (e.g. holding ALT + F to navigate to File)

Just for reference: The ALT-key navigation is not universal. It exists primarily on Windows. Not that common on Linux (though a some Linux desktops do support it). No idea about Mac. And you absolute can navigate a menu with the keyboard.

Yes, with ALT + some key (in Windows). But otherwise you can not navigate into the menu. I don't mean navigating within the menu, that of course works, but that is not what my proposal is about.

@Maran23
Copy link
Author

Maran23 commented Jan 18, 2023

This was actually implemented once in godotengine/godot#63358, but got reverted.

Thank you for pointing that out. Interesting that @YuriSizov had the same idea.
Reading the comments in the PR, we may even combine this proposal with the idea from @YuriSizov in #5197.
This actually answers one question above, whether we want to have switch_on_hover in PopupButton or not.

@YuriSizov
Copy link
Contributor

Thank you for pointing that out. Interesting that @YuriSizov had the same idea.

Well, there is a noticeable lack of a generic popup button, which was a problem for me since I was making one for the anchor presets and container sizing. 🙃 While I did end up making it, the biggest missing piece was switch on hover, because of the way it was implemented. Making a base class to share that functionality seemed sensible, but turned quite hacky when trying to type the API, where extending classes need to return more specific versions of the Popup.

So that's how I ended up with my proposal, where the biggest missing piece is shared between all buttons, and a potential PopupButton doesn't need to be related to MenuButton or ColorPickerButton.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants