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

When OptionButton is pressed, give focus to selected option #42843

Merged
merged 1 commit into from
Jan 16, 2022

Conversation

rburing
Copy link
Member

@rburing rburing commented Oct 16, 2020

A simple usability improvement: give focus to the currently selected option when an OptionButton is pressed.

godot_optionbutton_focus

The previous behavior was to focus on no option in particular (index -1), which was less clear (find the filled circle), less standard (compare e.g. web browsers) and less convenient. For users of the arrow keys, the previous behavior required (in all but two cases) at least one useless button press (to give focus to the first or last item).

It has been asked on the German Godot Community Discord and in Q&A how to achieve this functionality. As far as I could see it was impossible in plain GDScript. It was easy to implement in C++ (the commit is basically a one-liner), and I see no reason to keep the old behavior or to have any other behavior, hence this humble PR.

@Chaosus Chaosus added this to the 4.0 milestone Oct 16, 2020
@Calinou Calinou added cherrypick:3.x Considered for cherry-picking into a future 3.x release usability labels Oct 16, 2020
@rburing rburing force-pushed the optionbutton_focus branch from c2ea9e2 to 5816b74 Compare January 15, 2022 19:39
@rburing rburing requested a review from a team as a code owner January 15, 2022 19:39
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok.

We now have both setter and getter for current index, so maybe it would make sense to expose the setter and make current index a property? (but then the setter needs update() call or something)
Just wondering, the PR is fine as is.

@rburing rburing force-pushed the optionbutton_focus branch from 5816b74 to 4562106 Compare January 15, 2022 21:53
@rburing
Copy link
Member Author

rburing commented Jan 15, 2022

Thanks for the review. I updated the setter to do the visual update(), so now it should also work when called from other places. I held off on exposing a property for now. If someone has a compelling use case for it, I can still do it.

@akien-mga akien-mga merged commit 887898b into godotengine:master Jan 16, 2022
@akien-mga
Copy link
Member

Thanks!

@rburing rburing deleted the optionbutton_focus branch January 16, 2022 12:08
@akien-mga
Copy link
Member

akien-mga commented Jan 19, 2022

Cherry-picked for 3.5.
Edit: Undone, doesn't compile.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jan 19, 2022
@akien-mga
Copy link
Member

Actually undid cherry-pick as it relies on APIs which are not present in 3.x. A dedicated backport PR could be made for 3.x if needed.

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.

5 participants