refactor: Centralize copying of drag events when handling scrollbar scrolling#181110
refactor: Centralize copying of drag events when handling scrollbar scrolling#181110luanpotter wants to merge 1 commit into
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request refactors the scrollbar drag event handling to use copyWith on event detail objects, centralizing the logic for creating modified event copies. This improves maintainability and future-proofs the code against new fields in drag events. The changes are well-implemented, and I have one suggestion to further align the code with the PR's stated goals by using copyWith in one more place where a DragUpdateDetails object is constructed manually.
7bc13c3 to
0f3b09c
Compare
0f3b09c to
b363d27
Compare
victorsanni
left a comment
There was a problem hiding this comment.
I agree with this change, but it would be nice if we don't break developers who create their own classes that extend RawScrollbar (e.g design language developers making custom scrollbars).
Is this something a data-driven fix might be able to help with?
|
@victorsanni I am not familiar with it at all, but I can take a look. or @dkwingsmt is that something you are aware/familiar with? |
Renzo-Olivares
left a comment
There was a problem hiding this comment.
Hi @luanpotter, thank you for the contribution! I left a few comments.
| // on the scrollbar thumb and then drags the scrollbar without releasing. | ||
|
|
||
| @override | ||
| void handleThumbPressStart(Offset localPosition) { |
There was a problem hiding this comment.
Another option here would be to deprecate handleThumbPressStart and add a handleThumbPressStartWithDetails or similar. That would give users some time to migrate off the old API.
There was a problem hiding this comment.
I can certainly give that idea a try - would that be instead of the "data-driven fix" ?
There was a problem hiding this comment.
I agree that it should be a new method. I didn't realize that it was a public method via RawScrollbarState.
I don't think we will need a data-driven fix (of course, good to have one if possible!). We can provide both methods at the same time and let _handleThumbDragStart call both. Just document clearly that only one should be used.
|
|
||
| /// Creates a copy of this event but replacing the provided fields, while | ||
| /// maintaining the others. | ||
| DragStartDetails copyWith({ |
There was a problem hiding this comment.
Is a copyWith necessary here? The only reason I ask is that sourceTimeStamp is nullable and when null this means that it may have come proxied events. However, copyWith here cannot return sourceTimeStamp back to null. This is potentially error-prone since a user might use details.copyWith(sourceTimeStamp: null) expecting to reset sourceTimeStamp but this will not be the case.
There was a problem hiding this comment.
it is definitely not needed - my goal here was to "centralize" in one place everything that needs to be copied.
imagine in the future someone adds yet another new field to the events - they wouldn't know to go back to the scrollbar code and add a foo = foo line when creating the copy. the attempt here was to keep this code closer to the field list.
that being said the sourceTimeStamp does pose a problem if someone wants to "clear it". however I don't think flutter can make distinction here between undefined and null (probably for the best). are there other parts of the SDK were similar "copy" methods are used / this issue is solved?
I am ok with removing it entirely if you think my rationale for it is not worth the effort.
There was a problem hiding this comment.
Yeah dart unfortunately does not provide a good solution to this problem. I think if a nullable parameter on copyWith has an equivalent non-null representation for "null" then a copyWith can be alright in that case, or if there are no nullable members at all. I understand your rationale and also like the convenience of copyWith though if the copyWith usage will be minimal i'd lean towards not including it. An apply method that requires every parameter has been done in the framework as another option but it is not too far off from just using the constructor to recreate the entire object manually.
|
Hi! @luanpotter Are you able to address the comments above so we can work together to get this landed? This is a great addition and I'm looking forward to it! |
|
Thanks for your contribution! Since I haven't seen a reply, I'm going to reluctantly close this pull request for now, but if you get time to address the comments, we'd be more than happy to take a look at a new patch. If you do send a new patch, it would be very useful to reference this one in the description in order to help reviewers see the previous context. |
|
@dkwingsmt no worries at all, this ends up being messier than I wished! I have re-created the PR here addressing @Renzo-Olivares 's insight about the copyWith footgun: |
This PR changes the signatures of
handleThumbPressStart,handleThumbPressUpdate, andhandleThumbPressEnd, related to scrollbar handling, to take in a copy of the original event with thelocalPositionadded inside, instead of separate fields.This centralizes the copying of these events by the generic platform code (while still allowing some of them to re-copy based on implementation details), ensures that additional fields are propagated (such as
kindwhen it exists,buttonswhen it is eventually added, and others) and simplifies the method signature significantly, future proofing it to future additions.This was extracted from #176968 in an attempt to simplify it, and it will make it possible to add the missing
buttonsproperty to these events more easily.This also adds a
copyWithmethod to these events in an attempt to ensure that the copies have all non-specified fields propagated by centralizing the "exhaustiveness" of the copy into the event class itself; that way if a new field is added in the future it can be trivially added to thecopyWithmethod and not require figuring out all "copiers" out there (specially if it has a default value which could go un-noticed elsewhere).Note: this is a public signature change but only if people are creating their own custom scrollbar implementations (?) without using either the material or cupertino ones; and on top of that should be very trivial to fix. But please advise if there are concerns.
Note: this has no test changes because it is a refactor which should have zero change in behaviour; the existing tests passing is proof of it working. So I am disregarding this bot, but please advise if I am mistaken.
Note: this PR assumes that the
renderBox.localToGlobal(localPosition)computation that was previously done was essentially the reverse of the global -> local computation, so essentially the flow wasglobal --(global to local)--> local --(local to global)--> global(the second step being there just because we didn't provide the original global to the method). So this is saving a computation step and should not change the behaviour. I will let the tests be the judge of this assumption but please let me know if there are concerns.Pre-launch Checklist
///).