Multi-view pointer event #46213
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -102,7 +102,7 @@ extern const intptr_t kPlatformStrongDillSize; | |||||
| const int32_t kFlutterSemanticsNodeIdBatchEnd = -1; | ||||||
| const int32_t kFlutterSemanticsCustomActionIdBatchEnd = -1; | ||||||
|
|
||||||
| static constexpr FlutterViewId kFlutterImplicitViewId = 0; | ||||||
|
|
||||||
| // A message channel to send platform-independent FlutterKeyData to the | ||||||
| // framework. | ||||||
|
|
@@ -2326,6 +2326,7 @@ FlutterEngineResult FlutterEngineSendPointerEvent( | |||||
| pointer_data.pan_delta_y = 0.0; | ||||||
| pointer_data.scale = SAFE_ACCESS(current, scale, 0.0); | ||||||
| pointer_data.rotation = SAFE_ACCESS(current, rotation, 0.0); | ||||||
| pointer_data.view_id = SAFE_ACCESS(current, view_id, 0); | ||||||
|
||||||
| pointer_data.view_id = SAFE_ACCESS(current, view_id, 0); | |
| pointer_data.view_id = SAFE_ACCESS(current, view_id, kFlutterImplicitViewId); |
There was a problem hiding this comment.
I'm kind of hesitant here. The benefit of using 0 as the default value is that we can flush the struct to be all 0s and be sure that it doesn't break even when new fields are added (in fact I'm planning to propose a macro that initiate an embedder API struct that with flushed bytes). But I also agree that the default value should likely be the implicit view. In this way, we should probably NOT allow embedders to customize the implicit view ID to be non-0.
There was a problem hiding this comment.
The benefit of using 0 as the default value is that we can flush the struct to be all 0s and be sure that it doesn't break even when new fields are added
I don't follow. The embedder API - including the new multi-view APIs - is designed for forwards/backwards ABI compatibility. The struct_size member lets us determine how to handle the view_id without needing to fallback to 0.
In this way, we should probably NOT allow embedders to customize the implicit view ID to be non-0.
I don't understand this either, can you expand on this? The embedder API design doc had a section that allows the embedder to customize the implicit view ID: https://docs.google.com/document/d/1kegGJ235y1778-vzyyv90MgSHmYQcROTupMuLtEDBq0/edit#heading=h.1enoc2wkpbna
There was a problem hiding this comment.
The struct_size member lets us determine how to handle the view_id without needing to fallback to 0.
Let me rephrase the problem (because I realized Flutter is currently doing it correctly. My point is still valid though).
I'm talking about recompiling the code that creates such a struct. For example, check out the following code from the GLFW example (simplified):
void GLFWcursorPositionCallbackAtPhase(GLFWwindow* window,
FlutterPointerPhase phase,
double x,
double y) {
FlutterPointerEvent event = {};
event.struct_size = sizeof(event);
event.phase = phase;
event.x = x * g_pixelRatio;
event.y = y * g_pixelRatio;
FlutterEngineSendPointerEvent(&event, 1);
}The line FlutterPointerEvent event = {}; zeros out the struct. This is important, because old apps do not assign event.view_id, and without zeroing out properties, once Flutter adds this new field view_id and this custom embedder is recompiled without editing, every FlutterPointerEvent Flutter received would have an uninitialized view_id, which would be a random value. And because FlutterPointerEvent is a C struct, this is the only way to initialize future properties as C structs don't accept explicit default values. (We have encountered similar issues, which was fixed by this PR.)
The struct_size guard is used for compatibility of compiled libraries.
Therefore, when we add new fields to a struct, we're assuming that all old code will send a zero value for this field, which should be the default value. And therefore the default value, the implicit view ID, must be 0.
I understand that the doc said that the implicit view ID should be customizable, and I agreed to it. But this is a new problem I just realized.
There was a problem hiding this comment.
There's two cases that are handled differently:
- Embedders in the engine repo - These automatically use the latest embedder API whenever the API changes.
- Custom embedders outside the engine repo - These must update their embedder API manually:
- Generate a
flutter_embedder.hfile - Copy the
flutter_embedder.hfile into the embedder's project - Update the embedder's logic to use the latest embedder API
- Recompile the embedder
- Generate a
See Chinmay's minimal embedder example.
The struct_size guard allows us to detect whether a custom embedder has updated their flutter_embedder.h file. If a custom embedder sends a default value for the view ID, it means the custom embedder updated their flutter_embedder.h file without updating their embedder logic - this would be an error by the custom embedder.
There was a problem hiding this comment.
Here's where custom embedders put their copy of flutter_embedder.h:
- Sony: https://github.com/sony/flutter-embedded-linux/blob/master/src/flutter/shell/platform/embedder/embedder.h
- Samsung Tizen: https://github.com/flutter-tizen/embedder/blob/master/flutter/shell/platform/embedder/embedder.h
- Toyota: https://github.com/toyota-connected/ivi-homescreen/blob/main/third_party/flutter/shell/platform/embedder/embedder.h
- Go Flutter: https://github.com/go-flutter-desktop/go-flutter/blob/master/embedder/embedder.h
- Raspberry pi: https://github.com/ardera/flutter-pi/blob/master/third_party/flutter_embedder_header/include/flutter_embedder.h
There was a problem hiding this comment.
I see. In that case we don't have to be limited to use 0 as the default value. And we don't have to use disable_implicit_view, since we can make it enable_implicit_view and make the default value (by struct_size) true.
There was a problem hiding this comment.
At least I don't think it's not worth the "customizability" that I doubt if many people would use.
It feels a bit late to change this aspect of the design. This design has been out for over a month and has been reviewed by several stakeholders. We can still pivot if this is truly not worth the effort, I'm not convinced that's the case (and it takes effort to review design updates!)
There was a problem hiding this comment.
And we don't have to use
disable_implicit_view, since we can make itenable_implicit_viewand make the default value (bystruct_size) true.
We discussed this with Chinmay but decided to stick to disable_implicit_view regardless. The poor name does minimize the impact to a custom embedder that is updated incorrectly (assuming they zero initialize their args)
There was a problem hiding this comment.
It feels a bit late to change this aspect of the design. This design has been out for over a month and has been reviewed by several stakeholders. We can still pivot if this is truly not worth the effort, I'm not convinced that's the case (and it takes effort to review design updates!)
It's never too late to make improvement as long as it has not been submitted into embedder.h. Not to mention the fact that the implicit view ID and its customization are probably among last features of this project to add. If we worry about the minimizing the impact to a custom embedder that is updated incorrectly for disable_implicit_view, then we probably want to make it similar for the view ID.
For this particular change though, I'll change it to kFlutterImplicitViewId since I agree that the default value should be the implicit view ID, no matter what the value is.
Uh oh!
There was an error while loading. Please reload this page.