Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Linux trackpad gestures#31592

Merged
fluttergithubbot merged 7 commits intoflutter:mainfrom
moffatman:trackpad_gestures_linux
May 23, 2022
Merged

Linux trackpad gestures#31592
fluttergithubbot merged 7 commits intoflutter:mainfrom
moffatman:trackpad_gestures_linux

Conversation

@moffatman
Copy link
Contributor

@moffatman moffatman commented Feb 21, 2022

Start sending PointerPanZoom events from Linux when trackpad is used

Fixes flutter/flutter#23604

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 Hixie said 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
Copy link

flutter-dashboard bot commented Mar 13, 2022

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.

justinmc
justinmc reviewed Mar 30, 2022
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM at a high level, but maybe @cbracken or @stuartmorgan should take a look too.

Also, I tried the Mac part on my Mac but I don't think I have a trackpad that I could connect to my Linux machine to try it there.

And @moffatman there seem to be legitimate test failures.

../../flutter/shell/platform/linux/fl_engine_test.cc:99: Failure
Expected equality of these values:
  events[0].device_kind
    Which is: 4
  kFlutterPointerDeviceKindTouch
    Which is: 2

@moffatman moffatman force-pushed the trackpad_gestures_linux branch from 855e2c2 to 794f8fe Compare March 30, 2022 20:29
@moffatman
Copy link
Contributor Author

moffatman commented Mar 30, 2022 •
edited
Loading

For anyone trying to test this, you won't get trackpad gesture pan events if running via XWayland (scale/rotate work, but you'll get discrete scroll wheel events instead of pan). So make sure to clear $GDK_BACKEND if running from the VSCode terminal for example.

Not sure where I should document this as it would be useful information for a flutter user.

@stuartmorgan-g stuartmorgan-g requested a review from cbracken April 4, 2022 16:47
@dkwingsmt
Copy link
Contributor

dkwingsmt commented Apr 8, 2022

Looks like the fl_view changes are not tested? Is it because they're hard to test?

@moffatman
Copy link
Contributor Author

moffatman commented Apr 11, 2022

It will take a bit of reorganization to test fl_view. And I think gtk needs to be started for anything to work. I will take a stab at it though, that stuff should be tested.

@dkwingsmt
Copy link
Contributor

dkwingsmt commented Apr 11, 2022 •
edited
Loading

Yeah, I suggest checking if you can:

  1. Extract the scrolling-related part (or more so, the pointer-related part) into a separate class (ScrollingManager?)
  2. Extract everything the new ScrollingManager needs from FlView and the OS into an interface, which FlView implements.

In this way the FlView only handles the bare minimum work to connect ScrollingManager with others.
This is what I've been doing with the keyboard system: https://github.com/flutter/engine/pull/32305/files#diff-87603f83382091c7d78f2db674e8a6f6fd5d1981e3d02987d504b397d058842f

Feel free to let me know if you'd like me to handle this part. :)

@moffatman moffatman force-pushed the trackpad_gestures_linux branch from 794f8fe to f848ac5 Compare April 15, 2022 05:04
@moffatman
Copy link
Contributor Author

moffatman commented Apr 15, 2022

@dkwingsmt I think I have reorganized it like you described. Can you verify this is what you had in mind, if so I will add the comments and tests.

dkwingsmt
dkwingsmt suggested changes Apr 18, 2022
View reviewed changes
Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is what I meant! :) Please go ahead on unit tests!

dkwingsmt
dkwingsmt suggested changes Apr 29, 2022
View reviewed changes
Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change requests

@moffatman moffatman force-pushed the trackpad_gestures_linux branch from cca351e to a71b165 Compare April 29, 2022 04:09
@cbracken cbracken requested a review from robert-ancell April 29, 2022 17:22
robert-ancell
robert-ancell reviewed May 8, 2022
View reviewed changes
robert-ancell
robert-ancell reviewed May 8, 2022
View reviewed changes
robert-ancell
robert-ancell reviewed May 8, 2022
View reviewed changes
robert-ancell
robert-ancell suggested changes May 8, 2022
View reviewed changes
Copy link
Contributor

@robert-ancell robert-ancell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small requests but otherwise looks pretty good, thanks!

@moffatman moffatman force-pushed the trackpad_gestures_linux branch from fa3874f to cfe0628 Compare May 11, 2022 04:58
dkwingsmt
dkwingsmt approved these changes May 11, 2022
View reviewed changes
Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dkwingsmt
Copy link
Contributor

dkwingsmt commented May 19, 2022

@robert-ancell Would you like to make a second review of this PR?

robert-ancell
robert-ancell reviewed May 23, 2022
View reviewed changes
robert-ancell
robert-ancell reviewed May 23, 2022
View reviewed changes
@charafau charafau mentioned this pull request May 23, 2022
Open
@moffatman moffatman force-pushed the trackpad_gestures_linux branch from 8a644b4 to cdfb4e3 Compare May 23, 2022 20:03
robert-ancell
robert-ancell reviewed May 23, 2022
View reviewed changes
robert-ancell
robert-ancell approved these changes May 23, 2022
View reviewed changes
Copy link
Contributor

@robert-ancell robert-ancell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor issue with the documentation but otherwise great, thanks!

@dkwingsmt dkwingsmt added waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. affects: desktop labels May 23, 2022
@fluttergithubbot fluttergithubbot merged commit 178adb3 into flutter:main May 23, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 24, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 24, 2022
@engine-flutter-autoroll
178adb3 Linux trackpad gestures (flutter/engine#31592)
3b33c6d
houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022
@moffatman @houhuayong
Linux trackpad gestures (flutter#31592)
14b3636
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@cbracken cbracken Awaiting requested review from cbracken

3 more reviewers

@justinmc justinmc justinmc left review comments

@robert-ancell robert-ancell robert-ancell approved these changes

@dkwingsmt dkwingsmt dkwingsmt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

affects: desktop platform-linux waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Should support trackpad gestures from host

5 participants

@moffatman @dkwingsmt @robert-ancell @justinmc @fluttergithubbot