Skip to content

Conversation

@Fernthedev
Copy link

@Fernthedev Fernthedev commented Nov 17, 2023

It seems that when you have a pigeon configuration as follows, some C++ Utility classes are not written in the pigeon header which are required for the generated methods.

import 'package:pigeon/pigeon.dart';
@ConfigurePigeon(PigeonOptions(
  dartOut: 'lib/src/pigeon.g.dart',
  dartOptions: DartOptions(),
  cppOptions: CppOptions(namespace: "pigeon", ),
  cppHeaderOut: 'windows/runner/pigeon.g.h',
  cppSourceOut: 'windows/runner/pigeon.g.cpp',
  objcHeaderOut: 'macos/Runner/pigeon.g.h',
  objcSourceOut: 'macos/Runner/pigeon.g.m',
  // Set this to a unique prefix for your plugin or application, per Objective-C naming conventions.
  objcOptions: ObjcOptions(prefix: 'PGN'),
  // copyrightHeader: 'pigeons/copyright.txt',
  dartPackageName: 'desktop_adb_file_browser',
))

@FlutterApi()
abstract class Native2Flutter {
  void onClick(bool forward);
}

// For pigeon bug-testing
// Uncomment to generate FlutterError types 
// @HostApi()
// abstract class Flutter2Native {
//   void random(bool x);
// }

I'm not sure why this condition existed in the first place, as it was generating since v10.0.0. Regardless, it breaks compiles on Windows. The HostAPI methods still require FlutterError as shown below, so this clearly is necessary.

// Generated class from Pigeon that represents Flutter messages that can be called from C++.
class Native2Flutter {
 public:
  Native2Flutter(flutter::BinaryMessenger* binary_messenger);
  static const flutter::StandardMessageCodec& GetCodec();
  void OnClick(
    bool forward,
    std::function<void(void)>&& on_success,
    std::function<void(const FlutterError&)>&& on_error);

 private:
  flutter::BinaryMessenger* binary_messenger_;
};

List which issues are fixed by this PR. You must list at least one issue.

  • Generate FlutterError and other utilities in the C++ header as previously done (and currently required).

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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 relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

A few things to mention:

  • I was unable to sign the CLA, apparently some server issue or something?
  • Running flutter test in the packages/pigeon directory just spammed a lot of dart:mirror not supported errors.
  • I didn't file a bug report for this issue as noted in the Tree Hygiene wiki page, but I can do so if needed (again, small fix so I didn't bother)

Hopefully not a problem, but feel free to cherry pick or just manually implement the fix if these issues block the merge.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@Fernthedev Fernthedev changed the title Write FlutterError utilities for HostAPI [pigeon] Write FlutterError utilities for HostAPI Nov 17, 2023
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.

This will also need tests added to the swift_generator_tests.dart file testing that a file with only flutter api does generate the ErrorOr class.

If you don't want to deal with these changes, I can wrap it up into my pr that's about to land. let me know.

Thanks for catching this.


if (hasHostApi) {
// Always write?
if (hasFlutterApi || hasHostApi) {
Copy link
Contributor

Choose a reason for hiding this comment

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

the conditional isn't needed here. Just put _writeErrorOr above the pre-existing conditionals.

## 13.1.2

* Adds compatibilty with `analyzer` 6.x.
* [cpp] Fix C++ generator skipping FlutterError if no FlutterAPI is defined.
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need to be it's own version bump. With associated changes required (changelog, pubspec, and generator_tools)

@Fernthedev
Copy link
Author

If you have a PR about to land then feel free to supersede this PR. Appreciate the work!

P.S apologies for the messy issue, I was in a rush.

Good luck!

@tarrinneal
Copy link
Contributor

Closing as fixed in #5355

@tarrinneal tarrinneal closed this Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants