Skip to content
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

[pigeon] Add an initial example app #3761

Merged
merged 13 commits into from
Apr 20, 2023

Conversation

stuartmorgan
Copy link
Contributor

This creates an initial example app covering Android (Kotlin), iOS and macOS (Swift), and Windows, showing integration of Pigeon into an app directly, without using a plugin. This serves as both a simple runnable example for clients, and as a future source for snippet extraction for documentation. It includes a simple integration test so that CI will ensure that it's actually working.

Since we can't demonstrate Java and Objective-C in the same application, we could consider adding a second example app covering those in the future.

Currently it shows only host API, and only a single trivial method, but we can expand over time as is useful for documentation.

Part of flutter/flutter#66511

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.

@stuartmorgan
Copy link
Contributor Author

You'll want to skip the first several commits when reviewing this; the first is just the initial flutter create output, and then the next couple are cleaning that up.

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.

Not seeing anything disagreeable here.

Do you think there is benefit to be had in having additional integration testing here at all?

cppSourceOut: 'windows/runner/messages.g.cpp',
kotlinOut:
'android/app/src/main/kotlin/dev/flutter/pigeon_example_app/Messages.g.kt',
// This file is also used by the macOS project.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think having multiple outputs is something anyone would be interested in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that we have sharedDarwinSource for plugins (which allows macOS and iOS to share the same source directory) I think it's less likely. The use cases are now:

  • apps, as with this case, and
  • plugins that support macOS and iOS, with the pigeon API, but with completely unrelated implementations.

I think it's fine to just wait and see if we get any real-world requests for it.

@stuartmorgan
Copy link
Contributor Author

Do you think there is benefit to be had in having additional integration testing here at all?

We'll probably want to add integration tests that cover other functionality you add for examples. E.g., if we add a FlutterApi to this, which seems likely, we should have a test for that too.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 19, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 20, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Apr 20, 2023

auto label is removed for flutter/packages, pr: 3761, due to - The status or check suite repo_checks has failed. Please fix the issues identified (or deflake) before re-applying this label.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 20, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 20, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Apr 20, 2023

auto label is removed for flutter/packages, pr: 3761, due to - The status or check suite linux-custom_package_tests CHANNEL:master has failed. Please fix the issues identified (or deflake) before re-applying this label.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 20, 2023
@auto-submit auto-submit bot merged commit 746750e into flutter:main Apr 20, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 20, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 20, 2023
flutter/packages@88591be...746750e

2023-04-20 stuartmorgan@google.com [pigeon] Add an initial example app (flutter/packages#3761)
2023-04-20 imagipioneer@gmail.com [google_maps_flutter_web] Allow marker position updates (flutter/packages#3697)
2023-04-19 tarrinneal@gmail.com [Tool] [Code Excerpt] allow excerpts in example readme (flutter/packages#3758)
2023-04-19 47866232+chunhtai@users.noreply.github.com [go_router] migrates test for route information.location deprecation (flutter/packages#3763)

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 flutter-ecosystem@google.com,rmistry@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@cyanglaz
Copy link
Contributor

@stuartmorgan stuartmorgan deleted the pigeon-example-app branch April 20, 2023 18:12
@stuartmorgan
Copy link
Contributor Author

@keyonghan Did you track down what caused "Error Domain=DVTDownloadableErrors Code=23 \"'com.apple.platform.watchos' is an unknown platform identifier.\" UserInfo={NSLocalizedDescription='com.apple.platform.watchos' is an unknown platform identifier.}" errors? This come up previously in flutter/plugins#6682

@stuartmorgan
Copy link
Contributor Author

I'm re-running to see if it's an Apple-server-side flake issue.

@keyonghan
Copy link
Contributor

@keyonghan Did you track down what caused "Error Domain=DVTDownloadableErrors Code=23 \"'com.apple.platform.watchos' is an unknown platform identifier.\" UserInfo={NSLocalizedDescription='com.apple.platform.watchos' is an unknown platform identifier.}" errors? This come up previously in flutter/plugins#6682

IIRC, after updating the runtime, the CI passed. I didn't get a chance to track down to the root cause though.

stuartmorgan added a commit that referenced this pull request Apr 21, 2023
stuartmorgan added a commit that referenced this pull request Apr 21, 2023
Reverts #3761

It's failing on stable; is not yet clear why so referring while I
investigate.
pro100andrey added a commit to pro100andrey/packages that referenced this pull request Apr 25, 2023
* main: (144 commits)
  Update test golden images for the latest Skia roll (flutter#3787)
  [various] Adds Android namespace (flutter#3791)
  [shared_preferences] Update gradle/agp in example apps (flutter#3809)
  [go_router] Adds name to TypedGoRoute (flutter#3702)
  [webview_flutter] Adds support to receive permission requests (flutter#3543)
  [google_sign_in] Fix Android Java warnings (flutter#3762)
  [local_auth] Fix enum return on Android (flutter#3780)
  [pigeon] Warn when trying to use enums in collections (flutter#3782)
  [webview_flutter_android] [webview_flutter_wkwebview] Platform implementations for supporting permission requests (flutter#3792)
  [pigeon] Update for compatibility with a future change to the analyzer. (flutter#3789)
  [camera_android] Fix Android lint warnings  (flutter#3716)
  [webview_flutter_platform_interface] Adds method to receive permission requests (flutter#3767)
  [image_picker_ios] In unit test write and read kCGImagePropertyExifUserComment property (flutter#3783)
  [go_router_builder] Fixed the return value of the generated push method (flutter#3650)
  [image_picker] Mention `launchMode: singleInstance` in README (flutter#3759)
  Revert "[pigeon] Add an initial example app" (flutter#3785)
  [google_maps_flutter] Add examples for different iOS versions (flutter#3757)
  [pigeon] Add an initial example app (flutter#3761)
  [google_maps_flutter_web] Allow marker position updates (flutter#3697)
  [Tool] [Code Excerpt] allow excerpts in example readme (flutter#3758)
  ...

# Conflicts:
#	packages/camera/camera/pubspec.yaml
auto-submit bot pushed a commit that referenced this pull request Apr 28, 2023
Relands #3761, with the native runners re-created from `stable`. The important change was the principle class in the macOS Info.plist being NSApplication instead of `FlutterApplication`, since the latter doesn't exist on stable.

(The pigeon versions also changed because when I re-created the app I re-ran pigeon, and had a slightly newer version.)
nploi pushed a commit to nploi/packages that referenced this pull request Jul 16, 2023
This creates an initial example app covering Android (Kotlin), iOS and macOS (Swift), and Windows, showing integration of Pigeon into an app directly, without using a plugin. This serves as both a simple runnable example for clients, and as a future source for snippet extraction for documentation. It includes a simple integration test so that CI will ensure that it's actually working.

Since we can't demonstrate Java and Objective-C in the same application, we could consider adding a second example app covering those in the future.

Currently it shows only host API, and only a single trivial method, but we can expand over time as is useful for documentation.

Part of flutter/flutter#66511
nploi pushed a commit to nploi/packages that referenced this pull request Jul 16, 2023
Reverts flutter#3761

It's failing on stable; is not yet clear why so referring while I
investigate.
nploi pushed a commit to nploi/packages that referenced this pull request Jul 16, 2023
Relands flutter#3761, with the native runners re-created from `stable`. The important change was the principle class in the macOS Info.plist being NSApplication instead of `FlutterApplication`, since the latter doesn't exist on stable.

(The pigeon versions also changed because when I re-created the app I re-ran pigeon, and had a slightly newer version.)
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.

4 participants