[ios_platform_images] Support loading system images

[cadenkriese]

trafficstars

I've implemented a method to retrieve system images known as 'sfsymbols' as one of the first steps in implementing https://flutter.dev/go/sf-symbols. At the same time, I've updated the example app to contain a demo, as well as added integration tests.

Issues are fixed by this PR. flutter/flutter#82208 flutter/flutter#60034 (partially)

Pre-launch Checklist

• [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
• [x] I read and followed the [relevant style guides] and ran [the auto-formatter]. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
• [x] I signed the [CLA].
• [x] The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• [x] I listed at least one issue that this PR fixes in the description above.
• [x] I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
• [x] I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style].
• [x] I updated/added relevant documentation (doc comments with ///).
• [x] I added new tests to check the change I am making.
• [x] All existing and new tests are passing.

[jmagman]

Failed to stop: Error getting access token for service account: 400 Bad Request

Can you rebase onto main? We had to refresh our auth tokens.

[stuartmorgan-g]

What's the status of this PR? Is this ready for another round of review?

[cadenkriese]

Sorry, school has been getting intense for me. I implemented the trivial parts of your review and agree that Pigeon is the way to go, so I've looked at some examples but have yet to implement it.

[stuartmorgan-g]

Okay thanks, I'll go ahead and mark this as a draft for now so it won't show up in triage sweeps, and you can mark it as ready for review once you've updated it.

[cadenkriese]

Apologies for my delays getting back to this. I believe the most significant parts of this PR are correct unless I went too far out of scope with the pigeon implementation.

The one thing I haven't been able to figure out on my own, however, is why the error case integration test fails.

[stuartmorgan-g]

Sorry for the delay in looking at the error you mentioned; unfortunately the logs have expired so I can't see what the failure was.

There are also a lot of other failures here (unit tests, format, etc.) Could you push an update to fix those issue, and then we can look at the new run of the integration test failure to see what the failure message is?

[stuartmorgan-g]

It looks like the native code implementing the Pigeon interface isn't compiling in this version.

[stuartmorgan-g]

@cadenkriese Is this still something you are planning on finishing? Should we mark it as a draft for now?

[cadenkriese]

@cadenkriese Is this still something you are planning on finishing? Should we mark it as a draft for now?

Yes, apologies for the delay (again). I'm trying to figure out unit testing with pigeon but haven't found any resources for that? On a macro level are unit tests making their own mock api implementation or does the framework handle that somehow?

[stuartmorgan-g]

Dart unit tests, or native unit tests? For Dart, you can just use Mockito to generate a mock implementation of the *Api class. Pigeon has a way of generating a test implementation that lets you mock the method channel, but that's more of a transitional tool for migrating from a pre-Pigeon implementation without fully rewriting the tests; mocking the API class is less fragile than mocking the binary messenger.

[cadenkriese]

Thanks for the tips, let me know if there's anything wrong with how I've implemented it.

The last thing I believe is the pesky error I'm getting with the native test error case, where it isn't throwing the error at the right time? My reasoning for that guess is that the test fails due to an ArgumentError, but that's what I'm expecting on this line https://github.com/flutter/plugins/blob/5700f92423c309edf7ff67ffc362795fd32527ec/packages/ios_platform_images/example/integration_test/ios_platform_images_integration_test.dart#L56

edit: also not sure how to proceed with the test as a whole, see https://github.com/flutter/plugins/pull/4803#discussion_r832622499

[cadenkriese]

@stuartmorgan, apologies for the silence on my end. I want to try to finish this since the 1-year mark is coming up. Here's a summary of the issues I'm facing and looking for guidance on:

Most importantly, I'm trying to figure out how to proceed with the failure integration test; I've tried 3 or 4 different implementations, but nothing can handle the asynchrony properly and report back to the test framework. The current state of the relevant code is here. https://github.com/flutter/plugins/blob/6b322afbaac00d8f3e3622562d13ac542074812b/packages/ios_platform_images/example/integration_test/ios_platform_images_integration_test.dart#L39-L58

I also can't get the integration test to run on my machine (Runner crashes), and it looks like LUCI can't either https://ci.chromium.org/ui/p/flutter/builders/try/Mac_x64%20ios_platform_tests_3_of_4%20master/817/overview. So ultimately, I'm very lost when it comes to all of this, I would appreciate some help getting back on track.

Also, the analyzer isn't liking that Pigeon generates an unused function, what should I do about this?

/Volumes/Work/s/w/ir/x/w/plugins/packages/ios_platform_images/ios/Classes/PlatformImagesApi.g.m:22:11: error: unused function 'GetNullableObject' [-Werror,-Wunused-function]
static id GetNullableObject(NSDictionary *dict, id key) {
          ^
1 error generated.

Thank you again for your persistence with this!

[stuartmorgan-g]

I also can't get the integration test to run on my machine (Runner crashes)

In general, or when running the specific integration test you referenced above?

Also, the analyzer isn't liking that Pigeon generates an unused function, what should I do about this?

That's a Pigeon bug; we can fix it and then you can update this PR with the new version of Pigeon.

[cadenkriese]

In general, or when running the specific integration test you referenced above?

It appears to be due to the test since it doesn't occur when I comment it out, here's the output when running native-test in the toolchain.

When running flutter test integration_test in ios_platform_images/example I get:

00:18 +2: (tearDownAll) - did not complete [E]
00:18 +2: Some tests failed.
unhandled error during finalization of test:
/Users/caden/Developer/flutter/plugins/packages/ios_platform_images/example/integration_test/ios_platform_images_integra
tion_test.dart
Exception: Unable to terminate com.example.iosPlatformImagesExample on C7B6F397-FEAB-474B-B347-69DCAC6624C1:
ProcessException: Process exited abnormally:
An error was encountered processing the command (domain=NSPOSIXErrorDomain, code=3):
The operation couldn’t be completed. found nothing to terminate
found nothing to terminate
  Command: /usr/bin/arch -arm64e xcrun simctl terminate C7B6F397-FEAB-474B-B347-69DCAC6624C1
  com.example.iosPlatformImagesExample
#0      throwToolExit (package:flutter_tools/src/base/common.dart:10:3)
#1      SimControl.stopApp (package:flutter_tools/src/ios/simulators.dart:274:7)
<asynchronous suspension>
#2      IOSSimulator.stopApp (package:flutter_tools/src/ios/simulators.dart:545:13)
<asynchronous suspension>
#3      IntegrationTestTestDevice.kill (package:flutter_tools/src/test/integration_test_device.dart:124:12)
<asynchronous suspension>
#4      FlutterPlatform._startTest.<anonymous closure> (package:flutter_tools/src/test/flutter_platform.dart:507:9)
<asynchronous suspension>
#5      FlutterPlatform._startTest (package:flutter_tools/src/test/flutter_platform.dart:563:11)
<asynchronous suspension>

That's a Pigeon bug; we can fix it and then you can update this PR with the new version of Pigeon.

Sounds good 👍

[stuartmorgan-g]

I don't know of a good way to do native debugging of a Dart integration test, but the workaround that I use is to locally copy the code from the problematic test into the example app itself, then run the example app in debug mode from Xcode, so you're attached with a native debugger. Then you can see what's crashing.

[cadenkriese]

I don't know of a good way to do native debugging of a Dart integration test, but the workaround that I use is to locally copy the code from the problematic test into the example app itself, then run the example app in debug mode from Xcode, so you're attached with a native debugger. Then you can see what's crashing.

That worked, thank you! Now I'm back where I was originally, the ios system image error case test is failing due to an ArgumentError, but I'm calling await expectLater(completer.future, throwsArgumentError);

[stuartmorgan-g]

Now I'm back where I was originally, the ios system image error case test is failing due to an ArgumentError, but I'm calling await expectLater(completer.future, throwsArgumentError);

I didn't have time to fully debug this, but based on running it locally with breakpoints the errors don't seem to be propagating the way your test expects; completer.completeError(exception); is never even reached. I don't think your expectation is the problem; perhaps the error is not being passed through the future chain at some point.

[cadenkriese]

@stuartmorgan I finally made progress! The ImageStreamListener's onError method was getting called, but I had an (amateur) syntax error that slipped by. I'm still not sure how that was valid Dart code but oh well.

I also deleted the completeError call for the scaleCompleter since the bytesCompleter errors first and is thus caught. The scaleCompleter errors later down the line. I'm unsure what the implications of this completer never completing or erroring are. I would think it best to have it complete with an error and catch that error in the test, but I would have no idea where to begin with that because I don't know what state the scaleCompleter is in after the bytesCompleter errors. What do you think about this?

Finally, I took a stab at rearchitecting the other integration tests according to your previous feedback, but it's pretty bad. The previous state wasn't really testing anything at all, and now it's much too quirky and hardcoded. I'm all ears if you have any ideas on what would be best here.

[cadenkriese]

I'm okay with hard-coded values in tests like this, as long as they pass in all expected environments. The symbols and therefore dimensions may change with future iOS versions, we can investigate that when it happens though.

They worked on two different iPhones in simulator but it's not passing in CI so I'm not sure if it's reliable.

[jmagman]

I'm okay with hard-coded values in tests like this, as long as they pass in all expected environments. The symbols and therefore dimensions may change with future iOS versions, we can investigate that when it happens though.

They worked on two different iPhones in simulator but it's not passing in CI so I'm not sure if it's reliable.

flutter: ══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
flutter: The following TestFailure was thrown running a test:
flutter: Expected: <96>
flutter:   Actual: <97>

Yeah... You could check it's greater than 0, or you could use a Matcher like https://api.flutter.dev/flutter/package-matcher_matcher/closeTo.html

[stuartmorgan-g]

We've just completed the migration of the plugin code to the flutter/packages repository, as described in https://flutter.dev/go/flutter-plugins-repo-migration, and this repository is now being archived. Unfortunately that means that all in-progress PRs here must be moved to flutter/packages.

Please see our instructions for an explanation of how to move your PR, and if you have any issues moving your PR please don't hesitate to reach out in the #hackers-ecosystem channel in Discord.

Our apologies that your PR was caught in this one-time transition. We're aware that it's disruptive in the short term, and appreciate your help in getting us to a better long-term state!