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.

Sign up for GitHub

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

Jump to bottom

Fix scrolling PopupMenu on keyboard/controller input #80271

Merged
merged 1 commit into from
Oct 9, 2023
Merged

Fix scrolling PopupMenu on keyboard/controller input #80271

merged 1 commit into from
Oct 9, 2023

Conversation

Ymanawat
Copy link
Contributor

@Ymanawat Ymanawat commented Aug 4, 2023 •
edited by YuriSizov
Loading

This PR targets the 4.x branches, has this issue also in 4.0 stable.
This resolves an issue with the popup menu's weird scrolling behavior when using the keyboard/controller, as mentioned in #80224 . It fixes the problem and also includes some code refactoring.

Production edit: Fixes #80224, fixes #42230

@Ymanawat Ymanawat requested a review from a team as a code owner August 4, 2023 17:40
@jmb462
Copy link
Contributor

jmb462 commented Aug 4, 2023

I was also working on a PR to fix this bug and I chose an alternate way to deal with scrolling position :

Instead of scrolling when reaching the bottom or the top of the scrollcontainer, it's scrolling if we reach the middle of the scrollcontainer. Here is a gif. I feel this is smoother and allow to see better what is above and under the selected item.

What do you think about it ?

bug_scroll

@Calinou Calinou added bug topic:gui cherrypick:4.0 cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Aug 4, 2023
@Ymanawat Ymanawat requested review from a team as code owners August 5, 2023 04:10
aaronfranke
aaronfranke reviewed Aug 5, 2023
View reviewed changes
.github/workflows/linux_builds.yml Outdated
@@ -59,16 +58,6 @@ jobs:
# Skip 2GiB artifact speeding up action.
artifact: false

- name: Editor with ThreadSanitizer (target=editor, tests=yes, dev_build=yes, use_tsan=yes, use_llvm=yes, linker=lld)
Copy link
Member

Choose a reason for hiding this comment

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

You broke your PR, you accidentally reverted the recent engine changes.

See this article for information of how to fix this problem: https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html (or with a GUI).

Copy link
Contributor Author

@Ymanawat Ymanawat Aug 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

While updating my fork, mistakenly committed over 50 changes automatically from the updated master branch without confirmation or something idk, So I reverted the commit. Should I keep those changes and revert back?

Copy link
Member

Choose a reason for hiding this comment

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

Neither, you should instead rebase your commits so that the merge is not in the history anymore.

@Ymanawat
Copy link
Contributor Author

Ymanawat commented Aug 5, 2023

Good work @jmb462, I thought of the same but for the intuitive usability, I kept the way I did it.

@Ymanawat Ymanawat force-pushed the optionmenu-scroll-test branch 2 times, most recently from 416a0f3 to fb2b8e8 Compare August 5, 2023 08:40
@RedworkDE RedworkDE removed request for a team August 5, 2023 08:46
@RedworkDE RedworkDE removed request for a team August 5, 2023 08:46
@dalexeev dalexeev added this to the 4.2 milestone Aug 7, 2023
@AThousandShips AThousandShips removed their request for review August 8, 2023 10:44
@Sauermann
Copy link
Contributor

This could solve also #42230

@YuriSizov YuriSizov changed the title Fixed scrolling on keyboard/controller input Fix scrolling on keyboard/controller input Aug 30, 2023
YuriSizov
YuriSizov reviewed Aug 30, 2023
View reviewed changes
scene/gui/popup_menu.cpp Outdated Show resolved Hide resolved
@rsubtil
Copy link
Contributor

rsubtil commented Aug 30, 2023

This could solve also #42230

I don't think it solves that issue completely, since it's a different type of problem: joystick inputs triggering UI actions multiple times due to being analogue.

@Ymanawat
Copy link
Contributor Author

@YuriSizov have a look.

@aaronfranke aaronfranke removed their request for review September 1, 2023 08:01
rsubtil added a commit to retrohub-org/godot that referenced this pull request Sep 6, 2023
@rsubtil
Port of godotengine#80271
40bc559
@akien-mga akien-mga merged commit c1fed53 into godotengine:master Oct 9, 2023
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Ymanawat Ymanawat deleted the optionmenu-scroll-test branch October 9, 2023 14:17
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@YuriSizov YuriSizov changed the title Fix scrolling on keyboard/controller input Fix scrolling PopupMenu on keyboard/controller input Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips AThousandShips left review comments

@YuriSizov YuriSizov YuriSizov left review comments

@aaronfranke aaronfranke aaronfranke left review comments

@KoBeWi KoBeWi KoBeWi approved these changes

Assignees
No one assigned
Labels
bug topic:gui topic:input
Projects
None yet
Milestone
4.2