Skip to content

[pigeon] Use a const for custom type ids for gobject generated files (#156100) #9306

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

Merged

Conversation

JesseRiemens
Copy link
Contributor

Adds a custom type identifier to generated gobject headers for the user. Calling fl_value_new_custom_object is now possible with that constant.

This fixes flutter/flutter#156100

Pre-Review Checklist

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

Footnotes

  1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. 2 3

@JesseRiemens JesseRiemens requested a review from tarrinneal as a code owner May 22, 2025 14:39
@JesseRiemens JesseRiemens force-pushed the pigeon/gobject-custom-class-const-id branch from 9ef0df9 to b660a0c Compare May 22, 2025 15:37
@JesseRiemens JesseRiemens force-pushed the pigeon/gobject-custom-class-const-id branch 2 times, most recently from 62b41ae to da4196c Compare May 22, 2025 16:10
@tarrinneal tarrinneal requested a review from stuartmorgan-g May 22, 2025 18:41
@JesseRiemens JesseRiemens force-pushed the pigeon/gobject-custom-class-const-id branch 2 times, most recently from 77d7394 to 3947e4e Compare May 23, 2025 08:41
@JesseRiemens
Copy link
Contributor Author

The FLValue headers in some places use int, and in some places use uint8_t:

// fl_value.h:303
FlValue* fl_value_new_custom(int type, // uses int
                             gconstpointer value,
                             GDestroyNotify destroy_notify);

// fl_standard_message_codec.h:148
gboolean fl_standard_message_codec_write_value(FlStandardMessageCodec* codec,
                                               GByteArray* buffer,  // uses GByteArray*, which is used (subclassed) in generated files:
                                               FlValue* value,
                                               GError** error);
// core_tests.gen.cc:2422
static gboolean
core_tests_pigeon_test_message_codec_write_core_tests_pigeon_test_an_enum(
    FlStandardMessageCodec* codec, GByteArray* buffer, FlValue* value,
    GError** error) {
  uint8_t type = core_tests_pigeon_test_an_enum_type_id; // appends uint8_t to buffer
  g_byte_array_append(buffer, &type, sizeof(uint8_t));
  return fl_standard_message_codec_write_value(codec, buffer, value, error);
}

I think this might be related to #152916. Maybe it is best to leave it as is, and for this PR use int for the types.

@stuartmorgan-g
Copy link
Contributor

The FLValue headers in some places use int, and in some places use uint8_t:

Where is the header using uint8_t for the type? I'm seeing in your snippet:

  • A header declaration that uses int for the type.
  • A header declaration that doesn't have the type at all.
  • A file that's not an engine header.

@JesseRiemens
Copy link
Contributor Author

JesseRiemens commented May 23, 2025

My mistake, it's not the FlValue header that uses uint8_t, but only the generated code using the engine header.
I did not fully understand how it worked, but now I see that the generated dart code at the other side also uses uint8:

class DartGenerator extends StructuredGenerator<InternalDartOptions> {
  (...)
  void writeGeneralCodec() {
    (...)
          indent.writeln('buffer.putUint8(4);');

dart_generator.dart:432

That makes more sense to me now. I guess that means we can keep it like it is.

@JesseRiemens JesseRiemens force-pushed the pigeon/gobject-custom-class-const-id branch 4 times, most recently from 7aca7cf to c78a9d9 Compare May 28, 2025 08:22
@JesseRiemens

This comment was marked as off-topic.

Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

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

Pr looks good, just one question and I can lgtm

@JesseRiemens JesseRiemens force-pushed the pigeon/gobject-custom-class-const-id branch from c78a9d9 to 6e4d91e Compare June 5, 2025 15:09
@JesseRiemens JesseRiemens requested a review from tarrinneal June 5, 2025 15:10
@JesseRiemens JesseRiemens force-pushed the pigeon/gobject-custom-class-const-id branch 2 times, most recently from 75a101f to 70c893a Compare June 6, 2025 12:47
@JesseRiemens
Copy link
Contributor Author

JesseRiemens commented Jun 6, 2025

If you'd like to compare, this commit contains the changes since the last review

@JesseRiemens JesseRiemens force-pushed the pigeon/gobject-custom-class-const-id branch 3 times, most recently from 57edd16 to d5c4bde Compare June 10, 2025 15:40
…(#156100)

Adds a custom type identifier to generated gobject headers for the user. Calling fl_value_new_custom_object is now possible with that constant.
This fixes flutter/flutter#156100

Also updated the CONTRIBUTING.md file to include the gobject generator.
@JesseRiemens JesseRiemens force-pushed the pigeon/gobject-custom-class-const-id branch from d5c4bde to 2875eaf Compare June 11, 2025 12:29
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM

@stuartmorgan-g
Copy link
Contributor

@tarrinneal This just needs your final re-review.

Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

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

Sorry, thought I'd stamped this already!

@JesseRiemens
Copy link
Contributor Author

@tarrinneal Can you please autosubmit / merge?

@stuartmorgan-g stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 17, 2025
@auto-submit auto-submit bot merged commit 8929a93 into flutter:main Jun 17, 2025
80 checks passed
@JesseRiemens JesseRiemens deleted the pigeon/gobject-custom-class-const-id branch June 17, 2025 12:12
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2025
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Jun 17, 2025
flutter/packages@03a6abb...25d4fa4

2025-06-17 [email protected]
[google_maps_flutter] Add a new zIndexInt param to marker and deprecate
zIndex (flutter/packages#8012)
2025-06-17 [email protected] [pigeon] Use a
const for custom type ids for gobject generated files (#156100)
(flutter/packages#9306)
2025-06-16 [email protected]
[google_maps_flutter_platform_(web/android/ios)] Add a new zIndexInt
param to marker and deprecate zIndex (flutter/packages#9408)
2025-06-16 [email protected] [camera_avfoundation] fix race condition
when starting image stream on iOS (flutter/packages#8733)
2025-06-16 [email protected] Roll Flutter from
f79452e to 8303a96 (21 revisions) (flutter/packages#9433)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: pigeon platform-linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pigeon] Expose custom object IDs for testing on Linux
3 participants