-
Notifications
You must be signed in to change notification settings - Fork 29.8k
fix: SelectableRegion should only finalize selection after changing #159698
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
fix: SelectableRegion should only finalize selection after changing #159698
Conversation
dfdfe23 to
270846f
Compare
…ever changed the selection, for example a single tap + drag on mobile platforms does not change the selection and should not dispatch a finalized notification
270846f to
7a20c65
Compare
justinmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
| void _finalizeSelectableRegionStatus() { | ||
| if (_selectionStatusNotifier.value != SelectableRegionSelectionStatus.changing) { | ||
| // Don't finalize the selection again if it is not currently changing. | ||
| return; | ||
| } | ||
| _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is still a risk that someone will edit this class in the future and directly set the status to finalized without realizing that they should have used this method instead. However, I can't think of a better way to do it right now 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take that back, you have that covered with the assert in the setter below, nice!
There are some cases where selection behavior varies on a given platform by the pointer device, for example dragging to select changes on mobile platforms depending on whether a mouse or a touch is used. When a mouse is used a user can drag to select normally, when a touch is used the selection does not change on mobile platforms when dragging.
Before this change at the end of a touch drag users would still be notified the selection was finalized even though nothing changed and they had not previously received a
changingnotification. After this change the selection is no longer finalized unless theSelectableRegionSelectionStatuswas previously in achangingstate.Pre-launch Checklist
///).