Add support for scrollwheels #22762
Add support for scrollwheels #22762
Conversation
|
Note that the converter.dart changes look a lot bigger than they actually are because the indentation changed. |
|
I'm still trying to come up with a clean alternative to the "propagating" mechanic here. Ideally I'd like something where one node can consume part of the scroll offset, and then a later part can consume a different amount. (And indeed maybe a node in the tree could rotate the offset, e.g. if it was a 90 degree rotation in the paint coordinate system?) |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Ideally I'd like something where one node can consume part of the scroll offset, and then a later part can consume a different amount.
I thought about that while designing this, and it wasn't clear to me this would actually be a desirable behavior. If you are trying to scroll, say, an inner list in a larger scrolled UI, do you really want a single tick of the scroll wheel to both finish the scroll of the list and then also move the whole UI? I felt like the answer was probably no.
(And indeed maybe a node in the tree could rotate the offset, e.g. if it was a 90 degree rotation in the paint coordinate system?)
Interesting. Is there handling for this in the drag gesture system that would be useful for inspiration?
|
Feel free to clean up any issues you find in existing code when you run into it, for sure. Or file an issue, that's fine too. |
|
I don't think we do anything with respect to rotating the axes currently. We probably shouldn't yet here either. It's just an example of something you might want to do. You're right that eating half the offset is probably weird. Maybe that's not really a requirement. I think about this pretty much every day on my bike home, FWIW. Still trying to come up with an API that is less disruptive, is ideally backwards compatible, and supports the use cases here. Sorry it's taking so long. |
|
So here's a possible model: We use the regular dispatch model, but, for these events, instead of directly acting on them and instead of using the Gesture Arena, we have interested parties register with a "scroll event arena" of sorts, and when the the binding receives the event (as part of the hit testing), we have it tell this scroll event arena that it should close. The arena would be keyed on the scroll event object itself (not the pointer ID, since a single pointer ID can do many scrolls). By default it would just call the first handler who registered with the arena. |
|
(This would avoid any breaking changes.) |
zoechi
added
framework
|
FYI I'll be picking this up again this week (sorry for the long delay), and will try out the arena approach. |
|
@stuartmorgan are you still interested in picking this up again? |
|
I am, yes. Some other things came up before the holidays, but it's back at the top of my TODO list again. |
Updated to use this approach, PTAL (@Hixie). Since the last version I've also changed some of the callback plumbing to use a new more generic PointerGestureEvent, to future-proof this. For instance, if we want to support macOS's smart zoom (double-finger double-tap), it would need the same plumbing, and it would be nice not to have to add yet another handler for it.
For this version I'm still using the pointer ID; making arena generic would have been a fair amount of surgery, and seemed like it would be ugly to do with breaking API compat. Rather than do that or duplicate a bunch of code, I made a subclass that doesn't allow hold/release. If it can't be held open, then the fact that pointer IDs will repeat shouldn't ever be an issue. Let me know what you think of that approach. |
|
Added tests. @goderbauer @gspencergoog this should be ready for a broader review at this point. I've done a first pass at adding tests, but there may be categories of tests I missed, or more cases you'd like to see handled, so feedback welcome. @Hixie I believe I've addressed all your comments if you want to take another look. |
|
Two notes that are buried in context above:
|
|
Updated the style guide per the comments above. |
|
|
| ); | ||
| } | ||
|
|
||
| /// A base type for events that correspond to a discrete pointer signal. |
There was a problem hiding this comment.
I wouldn't call this a "base" type, especially considering that it extends another type. :-)
There was a problem hiding this comment.
Changed to just "An event that corresponds to a discrete pointer signal."
| ); | ||
| } | ||
|
|
||
| /// The pointer issued a scroll event. |
There was a problem hiding this comment.
I'd word this differently: e.g. "A pointer event object that contains information about pointer scroll events."
There was a problem hiding this comment.
While I agree in theory, that would make it inconsistent with the documentation of all of the other PointerEvent subclasses here; I wrote it this way rather than something like what you're suggesting based on the surrounding code, since consistency in API doc comments seemed important. Do you still want me to change it?
There was a problem hiding this comment.
Hrm. I'd actually like to have them all be changed, since it's kind of crappy documentation.
But, in the interest of landing this, can you just file an issue to do so? You can keep this consistent with the rest.
| class PointerScrollEvent extends PointerSignalEvent { | ||
| /// Creates a pointer scroll event. | ||
| /// | ||
| /// All of the arguments must be non-null. |
There was a problem hiding this comment.
If this is true, then there should be asserts to enforce it.
There was a problem hiding this comment.
Done. One of the asserts fired after I added it, which made me realize that pointer shouldn't actually be a field on a scroll event, since a pointer in the Flutter event system corresponds to a touch, essentially, and scrolls don't have to (and usually don't) come from a pointer that's down, nor does the scroll system need to know what pointer if any it's associated with. Yay for assertions :)
Related: all of the pointer event classes say that everything must be non-null, but none of them assert; would you like me to file a bug to track the missing asserts? (I'm not sure if that would be considered breaking, and thus potentially not worth doing.)
There was a problem hiding this comment.
Yes, please file the issue. We should be enforcing contracts about things not being null. At least until we get non-nullable types. :-)
By definition, if we break someone by adding an assert to enforce an existing contract, then we just helped them find an error in their code.
| ); | ||
| } | ||
|
|
||
| /// The pointer issued a scroll event. |
There was a problem hiding this comment.
Hrm. I'd actually like to have them all be changed, since it's kind of crappy documentation.
But, in the interest of landing this, can you just file an issue to do so? You can keep this consistent with the rest.
| class PointerScrollEvent extends PointerSignalEvent { | ||
| /// Creates a pointer scroll event. | ||
| /// | ||
| /// All of the arguments must be non-null. |
There was a problem hiding this comment.
Yes, please file the issue. We should be enforcing contracts about things not being null. At least until we get non-nullable types. :-)
By definition, if we break someone by adding an assert to enforce an existing contract, then we just helped them find an error in their code.
|
|
Adds support for pointer signals, in a way that will support both discrete events (e.g., scroll wheels, flutter/flutter#22762) and continuous gestures (e.g., trackpad scroll, flutter/flutter#21953). Also exposes these new event options to the embedder. Does not include code to send the new events from the platform shells.
Updates the strings due to the new listener widget in the scroll view hierarchy. Adds a new RenderPointerListener (signal) to each string, and re-indents everything below it.
|
@stuartmorgan I opened a new issue #35686. Essentially I am trying to have scroll function as zoom for a specific widget in my application. Would this be possible with your implementation? |
9 participants
Adds support for discrete scroll events, such as those sent by a scroll wheel.
Because they don't involve an event sequence like gestures do, they don't fit the structure used by the gesture arena, so the issue of ensuring that multiple nested scroll views don't all react to a scroll event is handled by adding a new event dispatch mode where dispatch stops as soon as a handler indicates that the event was handled, rather than walking up the entire tree as is done in the existing dispatch system.