[Linux] Add Multi-Touch Support for Linux #54214

RBT22 · 2024-07-30T11:15:06Z

This draft PR aims to address the lack of multi-touch support under Linux, leveraging the existing implementation used for Windows. As I am not an expert in this domain, I would greatly appreciate feedback on the implementation.

flutter/flutter#133239
flutter/flutter#52202

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.

flutter-dashboard · 2024-07-30T11:15:09Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

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.

robert-ancell · 2024-08-13T23:42:14Z

shell/platform/linux/fl_engine_private.h

+                   int32_t device_id,
+                   int modifiers_state);
+
+void SendPointerEventWithData(FlEngine* engine,


This should be called fl_engine_send_pointer_event

robert-ancell · 2024-08-13T23:44:22Z

shell/platform/linux/fl_engine_private.h

                                        double scroll_delta_y,
                                        int64_t buttons);

+void OnPointerDown(FlEngine* self,


FlEngine shouldn't contain handlers like this, instead look at fl_scrolling_manager.cc as an example of how to handle this in a separate class.

robert-ancell · 2024-08-13T23:46:20Z

shell/platform/linux/fl_renderer.h

+// Struct holding the state of an individual pointer. The engine doesn't keep
+// track of which buttons have been pressed, so it's the embedding's
+// responsibility.
+struct PointerState {


This struct seems to be in the wrong place, as it relates to the engine, not the renderer. To match existing code like fl_engine_send_mouse_pointer_event this should probably be an internal struct and not in a header.

I moved it to the engine, but not 100% sure what you meant.. Can you help me with this, please?

This structure is only used in fl_engine.cc, therefore it should be defined in there. It's currently in a header, which suggests it's part of external API for a module.

Yes, thanks for the clarification!

robert-ancell · 2024-08-13T23:47:10Z

shell/platform/linux/fl_engine.cc

 static constexpr int32_t kPointerPanZoomDeviceId = 1;

+// Keeps track of pointer states in relation to the window.
+std::unordered_map<int32_t, std::unique_ptr<PointerState>> pointer_states_;


This should be inside a class and use GHashTable (it gets too confusing mixing C++ and GObject).

robert-ancell · 2024-08-13T23:49:04Z

Thanks for working on this! I did a quick first review, the main issues seem to be moving this logic into it's own class and using GObject style instead of C++ style.

RBT22 · 2024-08-21T09:47:25Z

Thank you for the feedback! I’ll try to allocate some time for the restructuring next week.

I’ve been testing the code since the PR was opened, and I found an issue with the current implementation on Ubuntu 24.04. When using a three-finger gesture to switch between applications, the touch_event_cb's GDK_TOUCH_END part is not triggered when focus is lost. This results in various unexpected behaviors because the pointers are not removed. Do you have a suggestion for which signal handler could be used to handle this? Thanks in advance!

robert-ancell · 2024-08-22T04:38:48Z

Based on the behaviour of the other GTK input events I think you just have to handle the missing events and synthesize any that are missing. This is because Flutter expects a perfect input stream and GTK doesn't seem to guarantee this. I expect the event was intercepted by the shell or another layer in GTK and will not be reported to the application.

If you do think it's a mistake in GTK though please make a test case and report at https://gitlab.gnome.org/GNOME/gtk/ - that should clarify if we need to handle this ourselves.

chrisDexfo · 2024-09-06T18:18:36Z

@RBT22 Are you planning on resuming the work for this soon ?

The lack of multi-touch handling on linux is currently a blocking issue here...

RBT22 · 2024-09-06T18:26:27Z

Sorry for the inconvenience, everyone. Something urgent has come up, and I likely won’t be able to finish this in the next 2-3 weeks.

chrisDexfo · 2024-09-24T13:49:39Z

@RBT22 Any chances you'll resume work on this soon ?

RBT22 · 2024-09-27T14:14:35Z

@chrisDexfo I'm still having higher priority tasks at the moment. It will likely take about two more weeks to resolve. I'll be sure to get back to this project as soon as possible after that. Thanks for your understanding!

flutter-dashboard · 2024-10-17T14:18:03Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

RBT22 · 2024-10-22T10:14:40Z

I’ve done some refactoring. @robert-ancell, could you please take a look?

The issue I mentioned earlier with the missing GDK_TOUCH_END events still persists. We use our app with GNOME gestures disabled, so I didn’t explore that further. I’m not entirely sure where the event synthesis should occur. What I did find is that the leave_notify_event_cb is being triggered in those cases, but with a time value of zero.

robert-ancell · 2024-10-22T12:33:44Z

shell/platform/linux/fl_engine.cc

+  }
+}
+
+void fl_engine_send_pointer_event(FlEngine* self,


The engine has methods for each type of event, e.g. fl_engine_send_pointer_pan_zoom_event - adding this general event method doesn't fit in with the existing architecture. How many new pointer events are required? Could there be specific methods for these?

I feel the naming could be clearer. It might make more sense as fl_engine_send_touch_pointer_event. Otherwise, if I'm getting this right, we’d end up needing five separate methods for each phase we use. Should I go ahead with that approach?

I would say yes, make five methods to match the existing code. We can always consolidate them later if necessary.

shell/platform/linux/fl_engine.cc

robert-ancell · 2024-11-19T22:59:40Z

shell/platform/linux/fl_touch_manager.cc

+struct _FlTouchManager {
+  GObject parent_instance;
+
+  GWeakRef view_delegate;


This can be replaced with a reference to the engine - the view delegate only forwards the events to the engine. The existing delegates have been reduced or removed as part of the multi-view work.

robert-ancell · 2024-11-19T23:17:22Z

shell/platform/linux/sequential_id_generator.cc

@@ -0,0 +1,77 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


This class makes this very hard to understand. It's copied from another project, it's in C++ style which doesn't fit in well with the other code. A number of methods aren't being used. It's not used by any other code. I think this could be simplified and merged into FlTouchManager, though that's not a blocker and could be done at a later stage.

robert-ancell

I think with the removal of the view delegate this should be pretty close to landing. I can see some parts of it being refactored later, but it's not worth blocking on. You will need to add some tests though. Thanks for your patience!

dkwingsmt · 2024-12-02T22:21:40Z

I wonder how you see the relationship between the touch manager and the pointer manager.

I was guessing that you'd like touch manager manages touch screens, while the pointer manager manages mouse. (There will be some duplicate code but it might be acceptable, since handling multi-pointer is different from handling buttons.) However, in this case the touch manager should not need to handle buttons at all.
Or you might want to create a unified handler that handles both multi-pointer and buttons. But then you'll want to remove the pointer manager, which this PR doesn't.

RBT22 · 2024-12-03T08:23:49Z

@dkwingsmt I see what you mean about the relationship between the touch manager and pointer manager. Maybe it’d make sense to move a simplified state to the fl_touch_manager from the engine to clean things up a bit?

As for a unified handler, I get the idea, but I’m not up for tackling that myself right now. :)

…wn, move, up, and remove events

…rst part

…Table for touch ID management

RBT22 · 2024-12-11T11:24:47Z

I moved and simplified the state to FtTouchManager. Also, I integrated the ID generation.

robert-ancell

Looks great, thanks!

robert-ancell · 2024-12-11T21:08:21Z

@dkwingsmt do want to do a review before landing this?

dkwingsmt

LGTM, thanks for your contribution!

dkwingsmt · 2024-12-11T22:02:57Z

shell/platform/linux/fl_touch_manager_test.cc

+  EXPECT_EQ(pointer_events[4].timestamp,
+            1000lu);  // Milliseconds -> Microseconds
+  EXPECT_EQ(pointer_events[4].phase, kRemove);
+}


I'm surprised that EOF empty line isn't enforced. Just if you're interested:

Suggested change

}

}

…4214)

…160128) flutter/engine@b8034f1...e352461 2024-12-11 jonahwilliams@google.com [Impeller] remove std::vector usage in render pass vk. (flutter/engine#57132) 2024-12-11 43723477+RBT22@users.noreply.github.com [Linux] Add Multi-Touch Support for Linux (flutter/engine#54214) 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 jonahwilliams@google.com,zra@google.com 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

This draft PR aims to address the lack of multi-touch support under Linux, leveraging the existing implementation used for Windows. As I am not an expert in this domain, I would greatly appreciate feedback on the implementation. flutter#133239 flutter#52202 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

chrisDexfo · 2024-12-19T17:52:26Z

will this be included in next official release ?

github-actions bot added affects: desktop platform-linux labels Jul 30, 2024

robert-ancell reviewed Aug 13, 2024

View reviewed changes

RBT22 force-pushed the linux-multi-touch branch from 0c5aaff to 8c74a1e Compare October 22, 2024 09:02

robert-ancell reviewed Oct 22, 2024

View reviewed changes

shell/platform/linux/fl_engine.cc Outdated Show resolved Hide resolved

robert-ancell reviewed Oct 22, 2024

View reviewed changes

shell/platform/linux/fl_engine.cc Outdated Show resolved Hide resolved

loic-sharma requested a review from dkwingsmt October 28, 2024 17:23

RBT22 requested a review from robert-ancell November 13, 2024 08:16

robert-ancell reviewed Nov 19, 2024

View reviewed changes

robert-ancell suggested changes Nov 19, 2024

View reviewed changes

RBT22 force-pushed the linux-multi-touch branch from 0e416b6 to 45e850f Compare December 2, 2024 08:56

RBT22 requested a review from robert-ancell December 2, 2024 08:59

RBT22 force-pushed the linux-multi-touch branch from 3e02739 to 1ae0500 Compare December 2, 2024 13:40

RBT22 added 17 commits December 10, 2024 09:39

Fix typo

81a3c30

Rename method to match case convention

d6b33c4

Move PointerState def

e2f4e66

Fix merge error

83336cd

Remove FlTouchViewDelegate

29409a3

Refactor touch event handling to use dedicated functions for touch do…

a11185e

…wn, move, up, and remove events

Remove sequential_id_generator.h BUILD.gn

d26ef0c

Use event time for touch events

7f11d68

Add unit tests for FlTouchManager to validate touch event handling fi…

4045fca

…rst part

Add function descriptions

4771f1f

Add additional tests for FlTouchManager to validate touch event handling

3b4e4be

FIx linting

f4bdd3c

Update license references

99f830c

Fix memory management in FlEngine and FlTouchManager

72edc29

Simplify touch state

06f25e2

Remove SequentialIdGenerator and refactor FlTouchManager to use GHash…

45ffe87

…Table for touch ID management

Fix device ID handling

59190a5

RBT22 force-pushed the linux-multi-touch branch from 49ceb1d to 59190a5 Compare December 10, 2024 08:39

robert-ancell approved these changes Dec 11, 2024

View reviewed changes

dkwingsmt approved these changes Dec 11, 2024

View reviewed changes

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

auto-submit bot merged commit 0b34fae into flutter:main Dec 11, 2024
31 checks passed

engine-flutter-autoroll mentioned this pull request Dec 11, 2024

Roll Flutter Engine from b8034f152711 to e3524615fb69 (2 revisions) flutter/flutter#160128

Merged

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

0b34faefc [Linux] Add Multi-Touch Support for Linux (flutter/engine#5…

e12f575

…4214)

RBT22 deleted the linux-multi-touch branch December 12, 2024 07:33

		@@ -0,0 +1,77 @@
		// Copyright 2013 The Flutter Authors. All rights reserved.

[Linux] Add Multi-Touch Support for Linux #54214

[Linux] Add Multi-Touch Support for Linux #54214

Uh oh!

Conversation

RBT22 commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jul 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robert-ancell commented Aug 13, 2024

Uh oh!

RBT22 commented Aug 21, 2024

Uh oh!

robert-ancell commented Aug 22, 2024

Uh oh!

chrisDexfo commented Sep 6, 2024

Uh oh!

RBT22 commented Sep 6, 2024

Uh oh!

chrisDexfo commented Sep 24, 2024

Uh oh!

RBT22 commented Sep 27, 2024

Uh oh!

flutter-dashboard bot commented Oct 17, 2024

Uh oh!

RBT22 commented Oct 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robert-ancell left a comment

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented Dec 2, 2024

Uh oh!

RBT22 commented Dec 3, 2024

Uh oh!

RBT22 commented Dec 11, 2024

Uh oh!

robert-ancell left a comment

Choose a reason for hiding this comment

Uh oh!

robert-ancell commented Dec 11, 2024

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chrisDexfo commented Dec 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

RBT22 commented Jul 30, 2024 •

edited

Loading