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

Scrolling refactoring and Bug fixes. Add Axis enum arg to Edge::Position in graph. #662

Merged

Conversation

mitchmindtree
Copy link
Contributor

WIP - Do not merge yet

Closes #659.
Fixes #660.
Closes #661.

Ok, refactoring complete!

I'm much happier with these changes, they're much clearer and I can confidently say that I understand the behaviour of each distinct step of the process - not something I can say for the old scrolling setup, which was a little hack-ish and resulted in some hard-to-track-down bugs.

This also fixes bugs in the graph logic related to the relative positioning of widgets. Previously, the graph only had a single edge to represent relative positioning across both axes. I've added an Axis enum to the Position variant, which allows us to track relative positioning uniquely across each axis. This allows for saying cool stuff like .align_left_of(X).down_from(Y), and to know that relative positioning logic will be handled properly when evaluating scroll offsets, etc.

Finally, scrolling will now also properly consider the Padding of the KidArea of the widget that has scroll_kids enabled. I.e., you can now scroll past the edge of the last widget by the padding amount that is specified for that edge. This is just like how there's a little bit of white padding when you scroll past the widgets at the bottom of this github page.

… so far - yet to make it generic over axes).
…g on both X and Y axes. Closes PistonDevelopers#660. Also update graph modules to latest scrolling simplification changes. Simplify scroll_offset function in algo.rs. Properly consider currently updated widgets during calculation of bounding_box.
…dges for tracking relative positioning uniquely across each axis.
…lling methods to scroll_kids for clarity of behaviour. Change pre_update_cache args to consider relative positioning across both axes in accordance with PistonDevelopers#660 changes.
…ted sets of widgets. Update call to bounding_box with latest changes, using consts for args rather than magic values.
…ssary arguments and using an internal recursive function rather than trying to recurse on the public, user-facing function. The new bounding_box algorithm now returns the absolute bounding box, rather than one relative to the parent rect.
…or the kid_widgets is in sync with the position of the kid_area.
…to internal scroll::Axis trait. Update scroll::State::update to simplification of bounding_box algo, providing simpler calculation of offset_bounds.
… a channel to update matrix elements instead of borrowing the matrix directly in the inner closure.
@mitchmindtree
Copy link
Contributor Author

I've udpated the example to avoid the nightly regression here (the regression was considered a bug fix), however it's hard to tell whether or not this is now fixed, as it looks like travis is now failing due to another regression on nightly. It looks like there's a fix for it which should land soon, but there's no point in having it block this as we're still building fine on stable.

@mitchmindtree
Copy link
Contributor Author

Just updated the main comment with more thorough details of this refactoring.

Travis is passing on stable and I've successfully tested the changes with the all_widgets.rs and canvas.rs examples, as well as on two personal projects with much more intricate, nested scrolling behaviour.

I've published these changes as 0.29.0 - this also includes the changes mentioned in #661 and closes that issue.

mitchmindtree added a commit that referenced this pull request Jan 9, 2016
Scrolling refactoring and Bug fixes. Add `Axis` enum arg to `Edge::Position` in graph.
@mitchmindtree mitchmindtree merged commit 062385c into PistonDevelopers:master Jan 9, 2016
@mitchmindtree mitchmindtree deleted the scrolling_refactor branch February 16, 2016 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant