Add dual scrollbar constructors for 2D #122349
Conversation
| // TODO(Piinks): Instead, `.dual` should be a constructor that internally | ||
| // creates two ScrollbarPainters so that they can respond to each other | ||
| // appropriately: https://github.com/flutter/flutter/issues/122345 | ||
| return RawScrollbar( |
There was a problem hiding this comment.
@TahaTesser @xu-baolin I wonder if you have any thoughts here.
There are a lot of properties in the RawScrollbar constructor that we would want to expose here for the dual constructor, but I do not want to enumerate them all twice (vertical ones and horizontal ones)
WDYT of a ScrollbarDetails or ScrollbarStyle class that can encapsulate all of the styling parameters and allowing folks to pass one for vertical and one for horizontal?
Alternatively, we could leave this for now in its simplest form and folks can override this?
Not sure. We do not have to do it all in one change either. But I am curious if you have thoughts. :)
There was a problem hiding this comment.
Another point to consider, there may be other unique ways users want to configure dual scrollbars. I filed #122351 for example as one thing I thought of.
There was a problem hiding this comment.
I think we should expose basic properties to satisfy the common usage.
Also, let it be able to do custom easily.
There was a problem hiding this comment.
I filed these for follow up, there is definitely going to be multiple changes to get to all of the features needed to do this right :)
| // TODO(Piinks): Instead, `.dual` should be a constructor that internally | ||
| // creates two ScrollbarPainters so that they can respond to each other | ||
| // appropriately: https://github.com/flutter/flutter/issues/122345 | ||
| return RawScrollbar( |
There was a problem hiding this comment.
We should override the outer RawScrollbar.notificationPredicate to let them work, right?
Maybe we can write a sample to show how to use this new feature : )
There was a problem hiding this comment.
We should also remove the default bar for desktop platforms if users choose the dual bars.
There was a problem hiding this comment.
That should not be necessary, since the new TwoDimensionalScrollable maintains the same notification depth for both axes.
Plus, it overrides the default behavior with the buildDualScrollbars method instead of the 1D buildScrollbar. :)
See this for a rough example, finishing up some of the details this week:
There was a problem hiding this comment.
Can we let this also support two nested scrollable widgets? Folks may not know this can only work for TwoDimensionalScrollable.
Just watched this, it's amazing!
There was a problem hiding this comment.
the new TwoDimensionalScrollable maintains the same notification depth for both axes.
This may have an impact on the Scrollbar implementation because the scroll notification has the same depth but a different axis direction.
There was a problem hiding this comment.
This may have an impact on the Scrollbar implementation because the scroll notification has the same depth but a different axis direction.
It actually works! I have been using it for months while I have been working on this. Just the corner space needs better handling (filed an issue linked above). And the scroll controllers must be assigned, there would be a lot of room for error if the scrollbar was just trying to respond blindly to notifications in this case. :)
There was a problem hiding this comment.
Can we let this also support two nested scrollable widgets? Folks may not know this can only work for TwoDimensionalScrollable.
I think it is reasonable to expose the notification predicate in some way, but by default this dual composition should just listen to a depth of 1 as it is intended to be used with the 2D scrolling classes I am working on. Trying to self-compose 2D scrolling has a lot of issues (which is why we're working on making it easier!), and I don't want to overcomplicate a new feature by trying to account for every possible composition folks might have created themselves. I'll make sure this is documented really well, although I cannot reference TwoDimensionalScrollable until it lands (soon!).
Of course, we could get some feedback from users once this lands and decide to change it, that's why I love this project. 😊 Right now my goal is to get a solid basic foundation in place, and build from there.
|
I'm going to go ahead and pipe in the parameters here, updating soon. |
|
Bah, I keep running into the same ugly feeling. There are so many properties on scrollbar, I don't want to enumerate them all twice on the dual constructor, so I try to wrap them all up in a details object, but then I get massive code smell from duped properties between the regular 1D scrollbar and the details object... which makes me feel like we should have the 1D constructor just take the details object for a clean API... which means we would have to deprecate everything.. and then I am sad. We probably should have just exposed the painter rather than all of the individual properties... I am not sure what the best approach is yet. Especially since the final dual scrollbar needs to have both painters working together internally. I think for now I will close this and return to it when TwoDimensionalScrollable is in. For now, it can call |
Part of the 2D scrolling proposal: flutter.dev/go/2D-Foundation
Fixes #122348
Adds a
.dualconstructor to the scrollbar classes in order to support 2D scrollingAlso adds a method to scroll behaviors for calling on this in the 2D case. Relying on ScrollBehavior again will also enable other options in 2D scrolling, like if the developer does not want one or both of the scrollbars. This is consistent with other patterns in the framework.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.