Skip to content
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

Mac trackpad gestures #31593

Merged
merged 5 commits into from Apr 29, 2022
Merged

Mac trackpad gestures #31593

merged 5 commits into from Apr 29, 2022

Conversation

moffatman
Copy link
Contributor

@moffatman moffatman commented Feb 21, 2022

Start sending PointerPanZoom events from macOS 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

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 Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on 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.

@chinmaygarde
Copy link
Member

@cbracken Ping?

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

I tried this on my Macbook along with the framework PR (flutter/flutter#89944) using my demo app and it worked very well 👍 .

Would it be possible to write a unit test for this in FlutterViewControllerTest.mm?

@zanderso
Copy link
Member

From PR review triage: It looks like the next step on this one is for @moffatman to respond to the feedback.

@moffatman moffatman force-pushed the trackpad_gestures_mac branch 2 times, most recently from e75450c to 6ab5db4 Compare April 2, 2022 18:20
@moffatman moffatman requested a review from justinmc April 5, 2022 23:07
@chinmaygarde
Copy link
Member

Ping @justinmc.

@chinmaygarde
Copy link
Member

@dkwingsmt I believe this is good for another review.

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍 But @dkwingsmt should verify that the tests look ok.

Mostly just a bunch of nitpick comments on missing periods from me.

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 except for the periods.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

This looks great with one tiny nit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants