Synthesize buttons for embedders #8873

dkwingsmt · 2019-05-07T02:03:29Z

Description

This PR is a placeholder for button support for embedders. Currently the embedder API does not have button information at all, which leads to PointerData.buttons = 0 and blocks the corresponding framework change. This PR synthesizes a buttons = kPrimaryButton for events of down and move.

In the future when we have added FlutterPointerEvent::buttons (without breaking existing binaries), make sure to fix these 2 TODOs.

Related issues

Discussion: Desktop embedding mouse clicks do not register flutter#31884
Fixes: Embedder should provide "buttons" of events flutter#32052
Unblocks: Redo#2: Add buttons to gestures flutter#31935

stuartmorgan-g · 2019-05-07T03:44:38Z

lib/ui/window/pointer_data.h

 namespace flutter {

+// Must match the button constants in events.dart.
+const int64_t kPrimaryButton = 0x01;


Why is this a free constant rather than an enum class like the others below? In my experience it's standard in C++ to use enums for flags.

It would also be good to just go ahead and add the other mouse button constants from events.dart at the same time so the list here matches, rather than being a subset.

I'm not sure if it's ok to use enum as bitfield.

The reason I chose not to add other button constants is that only kPrimaryButton is used in code, since only it is used synthesis. When other buttons are involved they are directly copied from FlutterPointerEvent.buttons to PointerData.buttons.

I'll fix them if they're not a concern.

I'm not sure if it's ok to use enum as bitfield.

See https://github.com/flutter/engine/blob/master/lib/ui/semantics/semantics_node.h#L21 and later in the same file for example.

only kPrimaryButton is used in code

We'll need all of them when it becomes part of the embedding API, since we'll need to translate each public constant to the corresponding internal constant.

Note: Stuart and I discussed privately about whether to use enum types or int64_t.

Pros of enum class:

Type safe, such as preventing kMousePrimaryButton | kStylusSecondaryButton.

Grouping comes more natively than explicit namespace.

Cons:

Require to define operator& and operator|. It is magnified if we're to define all 4 sets of constants.

Combined values no longer fall into enum options, which might be confusing.

The cost to implement all operators is considerable, while the benefit seems insignificant because it's hard to predict the usage of the constants without knowing how they'll be used in engine. Therefore I might go forward with int64_t constants.

shell/platform/embedder/embedder.cc

stuartmorgan-g · 2019-05-07T03:49:06Z

shell/platform/embedder/embedder.cc

+inline int64_t ToPointerDataButtons(
+    int64_t buttons,
+    flutter::PointerData::Change change) {
+  // `buttons` should be non-zero when and only when the pointer is down.


Is the "and only when" actually true? For instance, is sending a cancel with a button set actually an error? I can think of cases where I would expect that to be the event created by the platform on desktop, unless it were explicitly worked around at the platform level.

Cancel is kind of special being the state of "termination". I can add it to the list (down, move or cancel), but other events should really not involve buttons.

For other events, there is a set of restrictions we hope the list of PointerData should obey, and the engine should regularize them before sending to framework. It is currently handled by PointerEventConverter . There was a plan to move them to the engine (leaving framework with only 1-to-1 converting) but was cancelled. I'm not sure we're still going this way though.

shell/platform/embedder/embedder.cc

dkwingsmt · 2019-05-07T22:51:24Z

@stuartmorgan I fixed all issues you raised. Notes:

I still think it's better to use int64_t for button constants, especially because the operator overload will have to be copied to all 4 series of button constants.
I changed ToPointerDataButtons so that they no longer force buttons = 0 for the events that are supposed to, but they should still log a warning in case of violation.

shell/platform/embedder/embedder.cc

lib/ui/window/pointer_data.h

stuartmorgan-g · 2019-05-07T23:37:15Z

lib/ui/window/pointer_data.h

+// Must match the button constants in events.dart.
+namespace PointerButton {
+const int64_t kPrimary = 1 << 0;
+const int64_t kSecondary = 1 << 1;


Are the raw values in event.dart actually used? I wouldn't expose them unless there's a need; it seemed like they were more for defining the device-specific constants.

Having them here encourages confusing usage (e.g., your use of PointerButton::kPrimary rather than PointerButton::kMousePrimary in this PR even though it's a mouse), and especially so given that they don't line up with the stylus names.

Yes. In fact, the general series of buttons are used more often, because most widgets are defined for cross-device behaviors, and by grouping the same "cross-device behavior" into the same value, more often than not widgets do not need to check the device kind. For example, primary mouse button, stylus contact, and touch contact are supposed to behave the same for most cases, so they share the same value 0x01, while secondary mouse button and primary stylus button share the same value 0x02. In this way, a button click will respond to kPrimaryButton, instead of kPrimaryMouseButton or kStylusContact, while a context menu will respond to kSecondaryButton.

I see, makes sense.

I still think we shouldn't expose them in the C++ code absent a reason though; we're creating device-specific data structs, rather than consuming data in a device-agnostic way. For creating them, using the device-specific values makes much more sense to me.

I agree. However I still need a value when synthesizing the primary button. Currently I'm using kPrimaryMouseButton but it's a little awkward. I wonder if you think it's acceptable. https://github.com/flutter/engine/pull/8873/files#diff-3784eecbfe20523d62f0a4c19cd363ebR730

Why is that awkward? The device type is hard-coded to mouse, so the primary mouse button seems like the right constant to me.

lib/ui/window/pointer_data.h

dkwingsmt · 2019-05-10T01:28:44Z

@stuartmorgan I fixed the requested changes. However,

I'm using the "no default" strategy you suggested in ToPointerDataButtons. I'm sure I listed all 7 options but still got warning C4715: 'ToPointerDataButtons': not all control paths return a value.
Issue I replied here Synthesize buttons for embedders #8873 (comment)

stuartmorgan-g

Some nits, but LGTM otherwise.

I'm using the "no default" strategy you suggested in ToPointerDataButtons. I'm sure I listed all 7 options but still got warning C4715: 'ToPointerDataButtons': not all control paths return a value.

You need a final return outside the switch; there's no actual guarantee that the value you're switching on is a valid enum value (consider someone passing (flutter::PointerData::Change)9999, for instance), so without a default reaching the end of the switch without returning is a possible code path.

lib/ui/window/pointer_data.h

stuartmorgan-g · 2019-05-10T03:43:31Z

lib/ui/window/pointer_data.h

+// Must match the button constants in events.dart.
+namespace PointerButton {
+const int64_t kPrimary = 1 << 0;
+const int64_t kSecondary = 1 << 1;


Why is that awkward? The device type is hard-coded to mouse, so the primary mouse button seems like the right constant to me.

flutter/engine@3822aec...6e3c043 git log 3822aec..6e3c043 --no-merges --oneline 6e3c043 Synthesize buttons for embedders (flutter/engine#8873) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (chinmaygarde@google.com), and stop the roller if necessary.

* Synthesize a buttons = kPrimaryButton for events of down and move * Add PointerEventButtons

dkwingsmt added 2 commits May 6, 2019 18:48

Add buttons

e55fa5e

Fix compile

0c14596

googlebot added the cla: yes label May 7, 2019

stuartmorgan-g suggested changes May 7, 2019

View reviewed changes

Fix according to change request

9ec2329

Style fix

e694717

stuartmorgan-g suggested changes May 7, 2019

View reviewed changes

dkwingsmt added 2 commits May 9, 2019 17:35

Make consts enum. Improve comment.

01d96ed

Fix enum values.

c3268cf

stuartmorgan-g approved these changes May 10, 2019

View reviewed changes

dkwingsmt added 2 commits May 10, 2019 10:56

Change enum names

6ceeba0

Fix comment

d9eb60e

dkwingsmt merged commit 6e3c043 into flutter:master May 10, 2019

dkwingsmt deleted the synthesize-buttons branch May 10, 2019 19:12

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 10, 2019

6e3c043 Synthesize buttons for embedders (flutter/engine#8873)

3224d00

engine-flutter-autoroll mentioned this pull request May 10, 2019

Roll engine 3822aeccd93f..6e3c0431410c (1 commits) flutter/flutter#32508

Merged

huqiuser pushed a commit to huqiuser/engine that referenced this pull request Jun 12, 2019

Synthesize buttons for embedders (flutter#8873)

99be710

* Synthesize a buttons = kPrimaryButton for events of down and move * Add PointerEventButtons

Synthesize buttons for embedders #8873

Synthesize buttons for embedders #8873

Uh oh!

Conversation

dkwingsmt commented May 7, 2019

Description

Related issues

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dkwingsmt commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

dkwingsmt commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dkwingsmt May 7, 2019 •

edited

Loading

dkwingsmt May 7, 2019 •

edited

Loading

dkwingsmt commented May 7, 2019 •

edited

Loading

dkwingsmt commented May 10, 2019 •

edited

Loading