Move `PointerDataPacketConverter` from `PlatformView` to `RuntimeController` #51952

dkwingsmt · 2024-04-05T23:39:21Z

This is a refactor that moves the PointerDataPacketConverter from PlatformView to RuntimeController.

This change is made for the following reasons:

Currently, the pointer data conversion contains no platform specific logic (because the current converter's only responsibility is to make the event sequence conform Flutter's protocol). Therefore these logics should reside in a platform-independent place.
The converter typically converts one event to many. It's better to have this conversion later than earlier.
It removes a member from PlatformView, making it closer to a pure virtual class.

The reason to choose RuntimeController as the destination is because RuntimeController manages a map for views, which is required for the converter to implement a later patch #51925.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C , Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…r-move

dkwingsmt · 2024-04-06T00:03:07Z

Update: There are some unexpected unit test failures and I'm resolving.

flutter-dashboard · 2024-04-06T00:05:24Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

…into pointer-data-converter-move

chunhtai

is the RuntimeController to be per view? or is it going to be shared by multipleview?

loic-sharma · 2024-04-08T21:33:42Z

is the RuntimeController to be per view? or is it going to be shared by multipleview?

The runtime controller is 1 per Dart VM, so it is shared by all views within a single engine instance.

loic-sharma · 2024-04-08T21:39:04Z

shell/common/input_events_unittests.cc

@@ -124,7  124,9 @@ static void TestSimulatedInputEvents(
    for (int i = 0, j = 0; i < num_events; j  = 1) {
      double t = j * frame_time;
      while (i < num_events && delivery_time(i) <= t) {
-        ShellTest::DispatchFakePointerData(shell.get());
+        // Use a different x every time for the pointer data converter to
+        // generate non-empty events.


Should we move the "use a different x" trick to DispatchFakePointerData's implementation? I don't feel strongly about this, feel free to keep as is.

loic-sharma · 2024-04-08T21:40:17Z

runtime/runtime_controller.cc

-    platform_configuration->DispatchPointerDataPacket(packet);
+    std::unique_ptr<PointerDataPacket> converted_packet =
+        pointer_data_packet_converter_.Convert(packet);
+    if (converted_packet->GetLength() != 0) {


I see PlatformView::DispatchPointerDataPacket didn't have this length check before. Could you explain why it was added?

You're right. It was a legacy design that proves to be no longer necessary. I've removed the length check and all code that was created for it.

chunhtai · 2024-04-08T21:45:06Z

The runtime controller is 1 per Dart VM, so it is shared by all views within a single engine instance.

If that is the case, should we still to put it below this level? The converter has state to keep track of pointer down or up, and each converter state should not be mixed between different view. If we put in runtime controller, we would have to maintain one converter per view.

dkwingsmt · 2024-04-08T23:11:57Z

The runtime controller is 1 per engine, and putting the converter under RuntimeController means that there's one converter per engine. The converter currently doesn't distinguish states between views. It keeps states by pointers, and this scheme works for multiple views (IIRC add/remove pointer events are dispatched when a pointer moves in/out of a view).

I don't think it needs to keep states by views, but even if we need to, we can add a private class under it. And even in that case, the converter needs to be aware of views, which PlatformView doesn't provide.

…r-move

chunhtai · 2024-04-08T23:30:54Z

I don't think it needs to keep states by views

If you have a gesture across multiple view, like a drag and drop, should they be treated as one gesture, or 2 gesture one for each view? sharing the same state or not will change how pointer event is synthesized

the converter needs to be aware of views, which PlatformView doesn't provide.

Can you explain a bit more? what information from the view does converter need?

…into pointer-data-converter-move

dkwingsmt · 2024-04-08T23:53:13Z

Can you explain a bit more? what information from the view does converter need?

The converter simply needs to know when a view is added or removed. However, currently we managed to implement the entire multi-view rendering without adding such API to PlatformView.

Well, now that I think about it, it might have been a better idea to add Add/RemoveView to PlatformView instead of to Shell. It feels like Shell is just a glue between the engine's components, not something to operate on. And in that case, PlatformView will have view information and this change is not needed.

This goes with the current convention on how a message is dispatched to the shell by platforms that do not use the embedder API. For example, iOS dispatches a system pointer event to the shell by:

engine/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

Lines 342 to 348 in 932c550

    
           - (void)dispatchPointerDataPacket:(std::unique_ptr<flutter::PointerDataPacket>)packet { 
        
             if (!self.platformView) { 
        
               return; 
        
             } 
        
             self.platformView->DispatchPointerDataPacket(std::move(packet)); 
        
           }

The iOS platform does not operate the shell, but operate the platform view.

@loic-sharma What do you think?

dkwingsmt · 2024-04-08T23:56:10Z

If you have a gesture across multiple view, like a drag and drop, should they be treated as one gesture, or 2 gesture one for each view? sharing the same state or not will change how pointer event is synthesized

The idea is that, a pointer's state is indexed by its pointer ID. The converter tracks whether this pointer travels across the view boundary and synthesize add/remove events accordingly.

An easier way to think about it is what Flutter "remembers" about pointers of an app.

Flutter should think, "I have 2 pointers, pointer 1 is currently on view 10, pointer 2 is currently not on any views."
Flutter should not think, "I have 3 views, view 10 currently has 1 pointer on it, view 11 and view 12 don't have any pointers on it".

loic-sharma · 2024-04-09T15:59:40Z

Well, now that I think about it, it might have been a better idea to add Add/RemoveView to PlatformView instead of to Shell. It feels like Shell is just a glue between the engine's components, not something to operate on. And in that case, PlatformView will have view information and this change is not needed.

I think the Shell setup we have today fits this paradigm: Shell::AddView just switches to the UI thread and then calls Engine::AddView. It's still just glue :)

Also, the PlatformView is operated on the platform thread, right? Would the approach you suggested require us to track views' states in the engine on the platform thread as well? Personally I'd rather have as much pointer logic on the UI thread as possible.

chunhtai · 2024-04-09T19:20:20Z

The idea is that, a pointer's state is indexed by its pointer ID. The converter tracks whether this pointer travels across the view boundary and synthesize add/remove events accordingly.

yes that can work, but then the converter will be more complex to not only memorize the pointer is down/up, it also need to know which view the pointer has been downed on.

I am not too familiar with the shell/platformview/view relationship, if it makes more sense to handle it in the shell level, then I think this is fine.

dkwingsmt · 2024-04-09T22:00:34Z

then the converter will be more complex to not only memorize the pointer is down/up, it also need to know which view the pointer has been downed on.

But this is unavoidable. The native platforms typically dispatch pointer-add/remove-events when a pointer enters/leaves a view. The app would like to know it as well. So this behavior is intended. The only problem is how to organize our data to compute the logic conveniently.

If we don't dispatch pointer-add/remove-events when a pointer enters/leaves a view, what's the behavior then? When the pointer hovers out of the Flutter window, Flutter probably won't receive any pointer events, and therefore the app will stop seeing pointer events for a long time before it sees another pointer event faraway when the pointer enters a Flutter view again.

chinmaygarde · 2024-04-26T20:07:15Z

Are we making progress on this?

chunhtai · 2024-04-26T21:01:50Z

If we don't dispatch pointer-add/remove-events when a pointer enters/leaves a view, what's the behavior then? When the pointer hovers out of the Flutter window, Flutter probably won't receive any pointer events, and therefore the app will stop seeing pointer events for a long time before it sees another pointer event faraway when the pointer enters a Flutter view again.

We definitely need a pointer add when pointer enters, not sure about leaves. I think most native app don't send pointer up when pointer leaves a window.

If you want to mimic the behavior with a single pointerpacketconverter, it need to constantly checking whether a move/hover has entered/leaves a new view. If we keep separate converter for each view, it should mostly work out of the box.

However, as I state before, if this is the place that makes most sense for pointerpacketconverter, then i guess we have no choice.

chunhtai

Talked with @dkwingsmt offline. It makes sense to pull these kind of logic out of platform view. LGTM

…untimeController` (flutter/engine#51952)

…147444) flutter/engine@8af10eb...cecf5aa 2024-04-26 [email protected] Move `PointerDataPacketConverter` from `PlatformView` to `RuntimeController` (flutter/engine#51952) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/ doc/main/autoroll/README.md

Impl

516e48b

dkwingsmt requested review from loic-sharma, cbracken and chunhtai April 5, 2024 23:40

dkwingsmt and others added 2 commits April 5, 2024 16:59

Merge branch 'main' into pointer-data-converter-move

f5efe7a

Merge remote-tracking branch 'origin/main' into pointer-data-converte…

4d113e2

…r-move

dkwingsmt marked this pull request as draft April 6, 2024 00:02

dkwingsmt added 2 commits April 5, 2024 17:36

Fix tests

ec0fd5b

Merge remote-tracking branch 'dkwingsmt/pointer-data-converter-move' …

aaf739c

…into pointer-data-converter-move

dkwingsmt marked this pull request as ready for review April 8, 2024 18:38

Merge branch 'main' into pointer-data-converter-move

bc91f91

chunhtai reviewed Apr 8, 2024

View reviewed changes

loic-sharma reviewed Apr 8, 2024

View reviewed changes

dkwingsmt added 2 commits April 8, 2024 16:20

Merge remote-tracking branch 'origin/main' into pointer-data-converte…

6c75417

…r-move

Remove the length check

6717f85

Merge remote-tracking branch 'dkwingsmt/pointer-data-converter-move' …

750b31c

…into pointer-data-converter-move

dkwingsmt mentioned this pull request Apr 9, 2024

Move Shell::Add/RemoveView to PlatformView and refine embedder API doc #52003

Merged

8 tasks

chunhtai approved these changes Apr 26, 2024

View reviewed changes

Merge branch 'main' into pointer-data-converter-move

8959ca3

dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 26, 2024

auto-submit bot merged commit cecf5aa into flutter:main Apr 26, 2024
31 checks passed

engine-flutter-autoroll mentioned this pull request Apr 27, 2024

Roll Flutter Engine from 8af10eba3ef3 to cecf5aa8a778 (1 revision) flutter/flutter#147444

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 27, 2024

cecf5aa8a Move PointerDataPacketConverter from PlatformView to `R…

3918e1e

…untimeController` (flutter/engine#51952)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `PointerDataPacketConverter` from `PlatformView` to `RuntimeController` #51952

Move `PointerDataPacketConverter` from `PlatformView` to `RuntimeController` #51952

dkwingsmt commented Apr 5, 2024 •

edited

Loading

dkwingsmt commented Apr 6, 2024

flutter-dashboard bot commented Apr 6, 2024

chunhtai left a comment

loic-sharma commented Apr 8, 2024

loic-sharma Apr 8, 2024

loic-sharma Apr 8, 2024

dkwingsmt Apr 9, 2024

chunhtai commented Apr 8, 2024 •

edited

Loading

dkwingsmt commented Apr 8, 2024 •

edited

Loading

chunhtai commented Apr 8, 2024

dkwingsmt commented Apr 8, 2024 •

edited

Loading

dkwingsmt commented Apr 8, 2024

loic-sharma commented Apr 9, 2024

chunhtai commented Apr 9, 2024

dkwingsmt commented Apr 9, 2024 •

edited

Loading

chinmaygarde commented Apr 26, 2024

chunhtai commented Apr 26, 2024

chunhtai left a comment

Move PointerDataPacketConverter from PlatformView to RuntimeController #51952

Move PointerDataPacketConverter from PlatformView to RuntimeController #51952

Conversation

dkwingsmt commented Apr 5, 2024 • edited Loading

Pre-launch Checklist

dkwingsmt commented Apr 6, 2024

flutter-dashboard bot commented Apr 6, 2024

chunhtai left a comment

Choose a reason for hiding this comment

loic-sharma commented Apr 8, 2024

loic-sharma Apr 8, 2024

Choose a reason for hiding this comment

loic-sharma Apr 8, 2024

Choose a reason for hiding this comment

dkwingsmt Apr 9, 2024

Choose a reason for hiding this comment

chunhtai commented Apr 8, 2024 • edited Loading

dkwingsmt commented Apr 8, 2024 • edited Loading

chunhtai commented Apr 8, 2024

dkwingsmt commented Apr 8, 2024 • edited Loading

dkwingsmt commented Apr 8, 2024

loic-sharma commented Apr 9, 2024

chunhtai commented Apr 9, 2024

dkwingsmt commented Apr 9, 2024 • edited Loading

chinmaygarde commented Apr 26, 2024

chunhtai commented Apr 26, 2024

chunhtai left a comment

Choose a reason for hiding this comment

Move `PointerDataPacketConverter` from `PlatformView` to `RuntimeController` #51952

Move `PointerDataPacketConverter` from `PlatformView` to `RuntimeController` #51952

dkwingsmt commented Apr 5, 2024 •

edited

Loading

chunhtai commented Apr 8, 2024 •

edited

Loading

dkwingsmt commented Apr 8, 2024 •

edited

Loading

dkwingsmt commented Apr 8, 2024 •

edited

Loading

dkwingsmt commented Apr 9, 2024 •

edited

Loading