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

Toggle control expand flag directly via top bar #65258

Merged
merged 1 commit into from
May 11, 2024

Conversation

RedMser
Copy link
Contributor

@RedMser RedMser commented Sep 2, 2022

Fixes #81726

  • Change the label from "Align with Expand" to just "Expand".
  • Turns "Expand" checkbox to a check buton, to make UX clearer.
  • Opening the container sizing popup loads the current value for the "Expand" check button, based on user's selection.
  • Clicking on the check button immediately toggles the flag for the selected controls.
  • Fix (unreported) bug that redo does nothing when changing shrink flags.

24-05-09_14-28-15_godot windows editor x86_64_Aldabratortoise

@RedMser
Copy link
Contributor Author

RedMser commented Sep 2, 2022

CC @YuriSizov since you created #63358 which this PR expands (hah) upon.

@YuriSizov
Copy link
Contributor

The current system was made this way on purpose, so I'm not sure if this is a good change. No other button in this or anchors' popup behaves like that and updates to match the state of the selected nodes.

@RedMser
Copy link
Contributor Author

RedMser commented Sep 2, 2022

I tried working with the current system (I'm working mainly on applications, so very UI-heavy stuff). The new top bar felt very cumbersome to use without my change in place.

While from a programming standpoint I totally understand why it was created this way, for actual users creating UIs I don't think this is a good design. Users like to get instant feedback on their changes, see what each option does and how it affects the layout.

An alternative that does not involve updating based on selection state would be to have a "Toggle Expand" button, which does not show the state, but only requires a single click to toggle the flag instead of two.

If you want, I can open a proposal for this change first and get some more opinions in.

@YuriSizov
Copy link
Contributor

YuriSizov commented Sep 2, 2022

@RedMser Yes, those are the options that we have, and the same ones I had to consider before arriving to the current state. I also work primarily with UI in Godot and understand the workflows. But none of these options are good in my opinion, including the current state. I just found it to be more consistent than others, but ultimately I don't like any of them.

So I'm all for a better option, I'm just not sure that this is better (because it's inconsistent with the rest).

Edit: And to be clear, if this is otherwise approved, I'm not going to die on that hill. 🙃 If this feels better, then it's better.

@YuriSizov YuriSizov requested a review from a team September 2, 2022 17:52
@YuriSizov YuriSizov requested a review from a team September 2, 2022 17:53
@RedMser
Copy link
Contributor Author

RedMser commented Sep 2, 2022

To address the issue of consistency, it might help to make the presets/size flags buttons also reflect the selected nodes' current values (by making them toggle buttons, part of a button group)?

And in case of differing values between selected nodes, or when one isn't using a preset, none of the options would be highlighted.

And to be clear, if this is otherwise approved, I'm not going to die on that hill. 🙃 If this feels better, then it's better.

I'll gladly help working towards a solution everyone's happy with! So far, every change to the UI system was an improvement to the previous iteration, so I'm confident we'll get there, hopefully before 4.0 releases. 🙂

@YuriSizov
Copy link
Contributor

To address the issue of consistency, it might help to make the presets/size flags buttons also reflect the selected nodes' current values (by making them toggle buttons, part of a button group)?

We can give it a try.

Would also be nice to somehow indicate that "expand" is not a standalone flag and is changing the main flag's behavior. Maybe with a highlight like I did for the movie writer button (though that may be too much).

@tokengamedev
Copy link

It is exactly what I was expecting, the buttons and checkboxes to behave. in the issue #81726. Internally it has to be managed by code.

@AThousandShips AThousandShips modified the milestones: 4.2, 4.3 Oct 26, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Apr 25, 2024

Consistency aside, IMO the main problem with this change is that the state of the button is kind of undetermined. It's obvious when you select one Control, but if you have multiple selected controls or non-Control nodes mixed in it becomes messy.

Another option to consider, not mentioned here, is making the expand flag use a modifier key. So the checkbox could be replaced with a label "Hold Shift to set with Expand" (though it's long).

If not, at least the checkbox could be changed to CheckButton, to better convey immediate action.

@ajreckof
Copy link
Member

Consistency aside, IMO the main problem with this change is that the state of the button is kind of undetermined. It's obvious when you select one Control, but if you have multiple selected controls or non-Control nodes mixed in it becomes messy.

This is not a real problem as this is already the case in the Inspector and I'm assuming this is just doing the same as the Inspector which just chooses a value. I feel like this is okay as this how most things work in those case.
Capture d’écran 2024-04-26 à 03 30 49

If not, at least the checkbox could be changed to CheckButton, to better convey immediate action.

I agree check button would be better here

@RedMser RedMser changed the title Toggle control expand flag via top bar Toggle control expand flag directly via top bar May 9, 2024
@RedMser
Copy link
Contributor Author

RedMser commented May 9, 2024

Sorry it took me a while. Updated to use a check button, since I think this UX is clearer.

@akien-mga akien-mga requested a review from KoBeWi May 10, 2024 08:16
@KoBeWi
Copy link
Member

KoBeWi commented May 10, 2024

Something is wrong with undo:

9jExvWLm42.mp4

@RedMser
Copy link
Contributor Author

RedMser commented May 10, 2024

Something is wrong with undo:

Same bug existed for changing the shrink flags in that same popup as well. I fixed both by replacing _edit_set_state with the same set flags methods.

@akien-mga akien-mga merged commit 0ffa6e2 into godotengine:master May 11, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

Control toolbar, "align with expand" checkbox does not set/unset the flag for container sizing
8 participants