Skip to content

Conversation

@swift-kim
Copy link
Member

@swift-kim swift-kim commented Feb 3, 2023

Align the Tizen embedder with the new update_semantics_callback embedder API.

For details, please see flutter/engine#37404. I referred to Windows and macOS implementations.

@swift-kim swift-kim marked this pull request as ready for review February 3, 2023 09:26
Copy link

@loic-sharma loic-sharma left a comment

Choose a reason for hiding this comment

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

LGTM

@swift-kim swift-kim merged commit a152671 into flutter-tizen:master Feb 7, 2023
@loic-sharma
Copy link

@swift-kim Hello, we just realized that FlutterSemanticsUpdate is flawed: array indexing on FlutterSemanticsUpdate.nodes and FlutterSemanticsUpdate.custom_actions will break when we add new members to FlutterSemanticsNode or FlutterSemanticsCustomAction. Instead, you'll need to use the FlutterSemanticsNode.struct_size and FlutterSemanticsCustomAction.struct_size to determine the next element's pointer.

This code is bad:

FlutterSemanticsUpdate update = ...;
FlutterSemanticsNode first = update.nodes[0];
FlutterSemanticsNode second = update.nodes[1]; // BAD! This could crash.

You'll want something like this instead:

// Note: untested
FlutterSemanticsUpdate update = ...;
FlutterSemanticsNode first = update.nodes[0];
FlutterSemanticsNode* second = static_cast<FlutterSemanticsNode*>(
  static_cast<char*>(update.nodes) + 1 * first.struct_size);

I'd suggest reverting this change and staying on the old semantics update callbacks for now. Apologies for the inconvenience.

@swift-kim
Copy link
Member Author

@loic-sharma Thank you for informing us. We always stay on the Flutter SDK stable channel and the embedder.h and engine implementation will remain unchanged on our side until the next stable engine release. So the current code should work at least until then. I'll keep track of any changes to embedder.h and FlutterSemanticsNode and make necessary changes to our embedder when I update the engine next time, so you don't have to worry. Thanks again for your great work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants