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

Horizontal scroll for Tree #62744

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

AThousandShips
Copy link
Member

Adds horizontal scroll WHEEL_LEFT/WHEEL_RIGHT support to Tree

Fixes proposals 4816

@AThousandShips AThousandShips requested a review from a team as a code owner July 5, 2022 17:34
@Calinou Calinou added this to the 4.0 milestone Jul 5, 2022
@YuriSizov
Copy link
Contributor

Maybe these 4 cases for up/down and left/right scrolling can be simplified by taking the core of the logic and putting it in a helper method that takes a scroll node and a direction (as 1 or -1)? I think that would reduce the chances to make a mistake if you need to modify this code in future.

@AThousandShips AThousandShips marked this pull request as draft July 5, 2022 17:55
@AThousandShips
Copy link
Member Author

@YuriSizov That's a great idea! I'll take a look at that tomorrow!

@AThousandShips
Copy link
Member Author

AThousandShips commented Jul 5, 2022

My immediate idea is:
bool scroll_combined(ScrollBar *h_scroll_bar, ScrollBar *v_scroll_bar, double p_h_factor, double p_v_factor) taking the factor in pages for scroll, as that seems to be the general approach for the UI and returns true if any scroll changed

Will test it out tomorrow, as I'm now having dinner

@YuriSizov
Copy link
Contributor

The existing code doesn't handle several directions at the same time, so that wouldn't be useful without other changes. But if you do implement it like that, then you don't need to pass the scroll nodes. You already have access to them from the class. I only suggested passing a scroll node reference so that both vertical and horizontal could be handled by the same helper.

@AThousandShips
Copy link
Member Author

AThousandShips commented Jul 5, 2022

Thank you

Though there is the pan gesture that scrolls both directions at once, though I don't have any input device for that currently so wouldn't be able to test it

I was thinking of putting this function in ScrollBar so it could be used by Scroll Container and other classes as well

But I'll see about adding that as a separate thing for convenience later now taht I thought about it

@AThousandShips
Copy link
Member Author

AThousandShips commented Jul 5, 2022

There, unified scrolling into a helper function, and horizontal scrolling should now work on Tree as well

I'll see if the uses of pan in scrolling like this is widespread enough to add a helper function possibly in Scroll Bar to unify this and make it easy to tweak, but unrelated to this so leaving it for later

(Shouldn't write code late at night)

@AThousandShips AThousandShips marked this pull request as ready for review July 5, 2022 20:14
@AThousandShips AThousandShips force-pushed the tree_h_scroll branch 15 times, most recently from d85456c to bf755aa Compare July 6, 2022 14:38
@AThousandShips AThousandShips force-pushed the tree_h_scroll branch 5 times, most recently from 3109099 to 56d6536 Compare July 7, 2022 10:27
@akien-mga
Copy link
Member

@AThousandShips Please try not to push updates to your PR so much (over 20 times in 20 hours). Since you're still a first-time contributor the CI doesn't run automatically but otherwise it would and this would really hammer the CI runners unnecessarily.

You can rebase when there are conflicts, or if a lot of time has passed and you suspect that some merged PRs might impact the behavior of your PR.

@AThousandShips
Copy link
Member Author

AThousandShips commented Jul 7, 2022

@akien-mga Got it! Thank you for informing me, wasn't aware how the CI worked in that regard

@akien-mga akien-mga merged commit ca18a02 into godotengine:master Jul 8, 2022
@akien-mga
Copy link
Member

akien-mga commented Jul 8, 2022

Thanks! And congrats for your first merged Godot contribution 🎉

@AThousandShips AThousandShips deleted the tree_h_scroll branch July 8, 2022 09:53
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.

Add support for horizontal scrolling in Tree using mouse buttons (WHEEL_LEFT/WHEEL_RIGHT)
5 participants