Moved the touch events expending to the flutter engine side#28972
Moved the touch events expending to the flutter engine side#28972chunhtai wants to merge 6 commits intoflutter:masterfrom
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
goderbauer
added
framework
There was a problem hiding this comment.
I'd keep the cases for hover and move completely separate since they don't share too much code.
There was a problem hiding this comment.
(github doesn't let me comment on the right line, this comment is meant for line 64):
Does it still make sense that this returns an iterable? Looks like it's a 1:1 mapping now?
There was a problem hiding this comment.
Do we still need _ensureStateForPointer here? Isn't it now guaranteed by the protocol, that the PointerChange.add even will have created the state?
There was a problem hiding this comment.
We should also document the legal sequence of events somewhere. Ideally, we add the diagram describing the legal sequence to https://github.com/flutter/assets-for-api-docs and link it in an appropriate space.
There was a problem hiding this comment.
An assert should also trigger if the event we receive from the engine doesn't follow the expected sequence.
There was a problem hiding this comment.
The framework here does not keep track of if the pointer is up or down since we don't need to expand the action from for move before down or hover before up. I can include it for assertion purpose, is that something we want to do?
There was a problem hiding this comment.
i haven't looked to see if it makes sense here (i'll defer to @goderbauer) but as a general rule we have many places where we have debug-only code (using asserts to make it only visible in debug mode and free in release builds) to verify invariants like this.
chunhtai
force-pushed
the
develop
branch
3 times, most recently
from
89bf457 to
a05959d
Compare
There was a problem hiding this comment.
| /// format the given packet of pointer data into a framework | |
| /// Format the given packet of pointer data into a framework |
There was a problem hiding this comment.
Is there a reason why the generator needs to be replaced by a list?
There was a problem hiding this comment.
if the result is 1 to 1 mapping, we prefer to use list
There was a problem hiding this comment.
I took that from @goderbauer 's suggestion. Should I change it back?
There was a problem hiding this comment.
| //Devices must be added before they send scroll events. | |
| // Devices must be added before they send scroll events. |
|
the build failures require new engine roll |
| /// Expand the given packet of pointer data into a sequence of framework | ||
| static String _formatErrorMessage(ui.PointerChange change, String msg) => 'Invalid event \'$change\': $msg'; | ||
|
|
||
|
|
| } | ||
|
|
||
| /// Expand the given packet of pointer data into a sequence of framework | ||
| static String _formatErrorMessage(ui.PointerChange change, String msg) => 'Invalid event \'$change\': $msg'; |
There was a problem hiding this comment.
msg -> message
(we don't use abbreviations)
| /// from physical coordinates to logical pixels. See the discussion at | ||
| /// [PointerEvent] for more details on the [PointerEvent] coordinate space. | ||
| static Iterable<PointerEvent> expand(Iterable<ui.PointerData> data, double devicePixelRatio) sync* { | ||
| static List<PointerEvent> format(List<ui.PointerData> data, double devicePixelRatio) { |
There was a problem hiding this comment.
renaming expand to format is also a breaking change. not sure that one is worth it.
There was a problem hiding this comment.
Is there a use case where developer will come in and use this function directly?
|
Closing for now until the engine side is ready. |
5 participants
Description
This decision came up when we try to deal with #20517
We decide the logic that normalize the touch events should reside in flutter/engine instead of flutter/flutter.
Engine should have platform specific code to ensure the clean touch events sent to flutter/flutter.
Related Issues
#20517
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?