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

Remove single view assumption from pointer events #41602

Merged
auto-submit merged 1 commit into from
Apr 29, 2023
Merged

Remove single view assumption from pointer events #41602

auto-submit merged 1 commit into from
Apr 29, 2023

Conversation

@loic-sharma
Copy link
Member

@loic-sharma loic-sharma commented Apr 28, 2023 •
edited
Loading

This change removes the assumption that that all pointer events go to the view ID 0. This change also adds a test for pointer events.

Part of: flutter/flutter#112205

Background

All pointer events are sent to PlatformDispatcher.onPointerDataPacket. In the future, a viewId property will be added PointerData, which the framework (specifically, GestureBinding) will use to route the event to the proper view. This viewId property's value will be set by the embedders.

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.

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

@flutter-dashboard flutter-dashboard bot added the embedder Related to the embedder API label Apr 28, 2023
@loic-sharma loic-sharma changed the title Remove single window assumption from pointer events Remove single view assumption from pointer events Apr 28, 2023
gspencergoog
gspencergoog approved these changes Apr 28, 2023
View reviewed changes
Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

This looks good to me, I just had a question for my own edification.

runtime/runtime_controller.cc
const PointerDataPacket& packet) {
if (auto* platform_configuration = GetPlatformConfigurationIfAvailable()) {
TRACE_EVENT0("flutter", "RuntimeController::DispatchPointerDataPacket");
platform_configuration->get_window(0)->DispatchPointerDataPacket(packet);
Copy link
Contributor

@gspencergoog gspencergoog Apr 28, 2023

Choose a reason for hiding this comment

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

On the framework side, how does it know which window to associate the pointer data with? Or does it just not do that, and pointer data is just in global screen coordinates?

Copy link
Member Author

@loic-sharma loic-sharma Apr 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We'll be adding a viewId member to PointerData that will be set by embedders. However, that requires changing the embedder API which we'll do only after we've reviewed the multi-view engine spec.

dkwingsmt
dkwingsmt approved these changes Apr 28, 2023
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 Apr 28, 2023

It seems like the plan is to move all messaging methods out of Window, making it a data class?

@loic-sharma
Copy link
Member Author

loic-sharma commented Apr 28, 2023

It seems like the plan is to move all messaging methods out of Window, making it a data class?

Yup, the only other method - set viewport metrics - will also get lifted up to PlatformConfiguration.

cbracken
cbracken approved these changes Apr 29, 2023
View reviewed changes
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 -- I've got nothing! Nice!

@loic-sharma loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 29, 2023
@auto-submit auto-submit bot merged commit 0079bb4 into flutter:main Apr 29, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 29, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 29, 2023
@engine-flutter-autoroll
0079bb4a2 Remove single view assumption from pointer events (flutter/…
be73d2b 
…engine#41602)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 29, 2023
@engine-flutter-autoroll
Roll Flutter Engine from 8f04b29c1b98 to 0079bb4a20d0 (2 revisions) (#…
9417b2a 
…125734)

flutter/engine@8f04b29...0079bb4

2023-04-29 [email protected] Remove single view assumption from pointer events (flutter/engine#41602)
2023-04-29 [email protected] Roll Fuchsia Mac SDK from kNDuhglIFq_xEt5n3... to XeUTCh70VOPbIFXdz... (flutter/engine#41608)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from kNDuhglIFq_x to XeUTCh70VOPb

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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@loic-sharma loic-sharma deleted the multiview_dispatch_pointer branch May 1, 2023 16:04
@HansMuller HansMuller mentioned this pull request May 5, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@cbracken cbracken cbracken approved these changes

@dkwingsmt dkwingsmt dkwingsmt approved these changes

@gspencergoog gspencergoog gspencergoog approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App embedder Related to the embedder API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@loic-sharma @dkwingsmt @cbracken @gspencergoog