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

Improve editor toolbar for Control nodes #63358

Merged

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Jul 23, 2022

Since #55630 is dead in waters for the time being, I've decided to extract the good parts from it, and specifically a better layout for anchor mode presets and container sizing options.

godot.windows.tools.64_2022-07-23_21-11-00.mp4

This also addresses concerns regarding changing anchor presets from the toolbar not changing grow direction and size flag options being behind that changes done in #55157.


I've added a new control node, PopupButton, to serve as a base for MenuButton and an internal control that I've added for the toolbar. I needed to do this so I can share the switch on hover behavior, and use a different kind of popup at the same time. I figured, this was the best way. MenuButton should remain as is, functionality-wise.

Closes godotengine/godot-proposals#3559

@YuriSizov YuriSizov added this to the 4.0 milestone Jul 23, 2022
@YuriSizov YuriSizov requested review from a team as code owners July 23, 2022 18:35
@YuriSizov YuriSizov force-pushed the control-simplify-enhance-toolbar branch from ee0a046 to 1ed187b Compare July 23, 2022 19:24
@YuriSizov YuriSizov requested a review from a team as a code owner July 23, 2022 19:24
doc/classes/PopupButton.xml Outdated Show resolved Hide resolved
scene/gui/menu_button.cpp Outdated Show resolved Hide resolved
@YuriSizov YuriSizov force-pushed the control-simplify-enhance-toolbar branch 2 times, most recently from 43438a4 to ec83d8f Compare July 24, 2022 15:25
@akien-mga

This comment was marked as resolved.

@YuriSizov

This comment was marked as resolved.

@akien-mga

This comment was marked as resolved.

@YuriSizov YuriSizov force-pushed the control-simplify-enhance-toolbar branch 3 times, most recently from 4b5cb40 to 42548cb Compare August 5, 2022 16:43
scene/gui/popup_button.h Outdated Show resolved Hide resolved
doc/classes/PopupButton.xml Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Aug 15, 2022

PopupButton crashes if there is no popup set.

Also, maybe the PopupButton change could be done in a separate PR? Then you could handle ColorPicker button too.
Although I guess it's not very useful by itself 🤔

@YuriSizov
Copy link
Contributor Author

PopupButton crashes if there is no popup set.

At any specific moment? Or just having it in a scene?

Also, maybe the PopupButton change could be done in a separate PR? Then you could handle ColorPicker button too. Although I guess it's not very useful by itself 🤔

Indeed, it's unlikely to be very useful. To me it was a necessity to implement the activate-on-hover behavior without doing hacks and editor-specific code injection. I'm not 100% convinced this is the best solution, but I don't have other ideas. I could forgo the on-hover behavior entirely and we can revisit it if and when users complain. The introduction of the PopupButton as base type shouldn't be compat breaking even if we need to do this after 4.0.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 15, 2022

At any specific moment?

When it's pressed.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 15, 2022

What happened to "Anchors Only" option?

@YuriSizov
Copy link
Contributor Author

What happened to "Anchors Only" option?

Ever since the original drawer's work nobody came forward and explained how it can be useful. I removed it because it was complicating the UI at this point, and now that presets also affect the grow direction, would we need more flags?..

@KoBeWi
Copy link
Member

KoBeWi commented Aug 15, 2022

Well it's useful when you position a control, but forget to use anchors beforehand. It's rather rare, but I did use it a few times 🤔
(although the workaround is just copying the position value, so it's not much of a problem)

EDIT:
I just realized now but

To me it was a necessity to implement the activate-on-hover behavior without doing hacks and editor-specific code injection.

You mean switch on hover? It's only useful when two of the buttons are visible, which is rather uncommon when working with UI.

scene/gui/popup_button.cpp Outdated Show resolved Hide resolved
@YuriSizov
Copy link
Contributor Author

YuriSizov commented Aug 16, 2022

You mean switch on hover? It's only useful when two of the buttons are visible, which is rather uncommon when working with UI.

The way switch on hover is currently implemented (and this PR doesn't change) is less than ideal. It's parent based, and so it breaks in our editor UI when we talk about toolbars. Each toolbar has its own parent hbox, so only the buttons inside of the same toolbar can be activated on hover. As a user I'd expect the entire toolbar to be able to be hover-selected like that.

So it's not just the two buttons in this toolbar, but also the buttons from other toolbars and the main toolbar that should play into this. But, as mentioned, I didn't change that here. Having a common parent class like that would allow for future improvements in this area.

But, at this point, I'm tired of dragging these toolbar improvements for almost a year already. Every time they end up entangled with something else that prevents it from moving. So I'll amend this PR today to remove anything related to the PopupButton (including having the switch-on-hover logic for these new buttons). And then maybe open a proposal for it to improve the switch on hover behavior. (Edit: Here it is, godotengine/godot-proposals#5197).

PS. Thanks for reviewing it thoroughly by the way <3

@YuriSizov YuriSizov marked this pull request as draft August 16, 2022 09:00
@YuriSizov YuriSizov force-pushed the control-simplify-enhance-toolbar branch from 42548cb to 7a60cc7 Compare August 16, 2022 14:36
@YuriSizov YuriSizov marked this pull request as ready for review August 16, 2022 14:36
@YuriSizov
Copy link
Contributor Author

Okay, removed any changes done to MenuButton and the concept of PopupButton. This resolves every related comment. I've addressed the rest of them.

For the picker popups I've created a new base class that pulled some of the shared code. But I didn't unite them entirely, because there is plenty of non-shared code in them too. But it should keep the code duplication to the minimum now.

@YuriSizov
Copy link
Contributor Author

Would appreciate an approval from @KoBeWi as he's been with me and this change through every iteration 🙃

@YuriSizov
Copy link
Contributor Author

Alright, seems very mutual :)

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

Successfully merging this pull request may close these issues.

Improve anchor panel UI
6 participants