✨ Add photo_manager_location plugin (#1428) - #6
Conversation
An opt-in plugin that links CoreLocation to save assets with geographic coordinates on iOS/macOS. Apps that need location-tagged saves install this plugin (opting into the CoreLocation link and the NSLocationWhenInUseUsageDescription requirement); the core photo_manager package stays CoreLocation-free so the majority of apps are not flagged by Apple's static scan (#1428). - Darwin-only (iOS/macOS). Android location-save remains in the core package. - Independent method channel (com.fluttercandies/photo_manager_location). - Native save logic ports the proven CLLocation/PHAssetCreationRequest code previously in the core, with deduplicated class names (PMLocationPlugin / PMLocationManager) to avoid ObjC symbol collisions with the core. - Builds the full asset map (PMConvertUtils field set) so the returned AssetEntity is fully populated; FlutterResult is dispatched on the main thread.
- analyze: melos bootstrap, dart format check, flutter analyze lib per package, dart doc dry-run. - darwin: podspec lint + ObjC compile check for the Darwin plugin sources. - publishable: dart pub publish --dry-run per package. - All jobs checkout submodules recursively (flutter_photo_manager is a submodule that melos path-overrides for bootstrap).
- Scope melos exec to photo_manager_* (excludes the photo_manager submodule, which lint-checks in its own repo). - pod lint -> pod spec lint --quick (the correct command; --quick skips build-dependent checks). - Add LICENSE to the package (required by pub publish) and declare the license type in the podspec to clear the CocoaPods warning.
09722d2 to
27093f4
Compare
Required by pub publish validation.
PMPlugin.m imports <FlutterMacOS/FlutterMacOS.h>, which standalone clang cannot find without the framework path. Resolve the FlutterMacOS.framework from FLUTTER_ROOT and pass it via -F.
CaiJingLong
left a comment
There was a problem hiding this comment.
Review conclusion: Approve
Summary of changes
This PR introduces a new opt-in plugin photo_manager_location that links CoreLocation to save assets with geographic coordinates on iOS/macOS. It is the companion to flutter_photo_manager#1431, which removes CoreLocation from the core package so most apps are no longer flagged by Apple's static scan (issue #1428).
The plugin provides PhotoManagerLocation.editor with saveImage, saveImageWithPath, and saveVideo, routing calls through an independent method channel (com.fluttercandies/photo_manager_location) implemented by native PMLocationPlugin/PMLocationManager (deduplicated class names to avoid ObjC symbol collisions with the core).
Joint review with flutter_photo_manager#1431
The two PRs form one coherent change and were reviewed together. The contract between them is sound: the core no longer links CoreLocation, this plugin does, and both can coexist in one binary without ObjC symbol collisions. See the companion review on #1431 for the core-side analysis.
Review details
| Dimension | Conclusion | Notes |
|---|---|---|
| Correctness | Pass | Native save logic faithfully ports the proven CLLocation/PHAssetCreationRequest code previously in the core (three save methods, PMLocationHasValue null/NSNull guard mirroring isNilOrNull, performChanges + completionHandler with weak/strong self, placeholderForCreatedAsset.localIdentifier). mapFromAsset: field set (id, createDt, width, height, favorite, duration, type, modifiedDt, lng, lat, title, subtype) matches the core PMConvertUtils convertPHAssetToMap:needTitle: output the Dart ConvertUtils.convertMapToAsset expects. FlutterResult dispatched on the main queue (thread-safe per PHPhotoLibrary conventions). Dart _convertMapToAsset mirrors ConvertUtils.convertMapToAsset field-for-field. File-existence guards present for path-based saves. |
| Tests | Pass (with note) | No unit tests are included in this new package. This is acceptable for an initial plugin whose behavior is inherently platform-channel/native-driven and not unit-testable without device integration. CI "Verify Darwin (podspec + native)" compiles all .m sources and lints podspecs (pass); "Analyze" runs flutter analyze lib (pass); "Publish dry-run" passes. See non-blocking suggestion below. |
| Style | Pass | flutter analyze lib: No issues found (CI). dart format checked in CI (dart format packages --output=none --set-exit-if-changed, pass). Code follows the core package's conventions: same method-channel argument keys, same PHAssetCreationRequest flow, same doc template references ({@macro photo_manager.Editor.*}). analysis_options.yaml extends flutter_lints with require_trailing_commas. |
| Risk | Pass | New, isolated package — no regression surface for existing packages. Independent method channel avoids any override/registry coupling with the core. Deduplicated class names (PMLocationPlugin/PMLocationManager) prevent ObjC symbol collisions when both packages are linked. pubspec.yaml constrains photo_manager: ">=2.6.0 <4.0.0", compatible with the core's current 3.x line. podspec links Photos, PhotosUI, CoreLocation for both iOS and macOS (correct — this is the one place CoreLocation should be linked). |
| Docs | Pass | README.md explains the why, the Info.plist purpose-string requirement, the Android-unaffected note, and a usage example. CHANGELOG.md 1.0.0 entry present. Dart public API documented with doc comments and {@macro} references to the core's editor templates. |
| Repo constraints | Pass | Follows CONTRIBUTING.md (new plugin with linked issue #1428). No PR template present. CI all green: Analyze, Verify Darwin (podspec + native), Publish dry-run. |
Confirmed key points
- Correctness: Native save logic is a faithful port of the core's former implementation; asset-map field set matches the Dart converter; main-thread result dispatch; null/NSNull handling consistent with core's
isNilOrNull. - Tests: No unit tests (acceptable for initial native-channel plugin); CI native compilation, podspec lint, analyze, and publish dry-run all pass.
- Risk: Fully isolated new package with an independent channel and deduplicated symbols — no regression surface for existing packages; correct CoreLocation link in podspec.
Suggestions (non-blocking)
- Consider adding an integration-test scaffold (even a smoke test that verifies
PhotoManagerLocation.editoris non-null and the channel name) to guard against regressions as the package evolves. pubspec.lockis committed; for a published package this is typically omitted to let consumers resolve freely. Low priority — some Flutter plugin repos do keep it.- The
Package.swiftcarries aCopyright 2013 The Flutter AuthorsBSD-style header while the rest of the package uses the Apache-2.0 FlutterCandies header. If this file was adapted from a Flutter template, consider aligning the header or noting the provenance, for consistency.
This comment was generated by AI agent omp (model: devin/glm-5-2).
Published packages should let consumers resolve dependencies freely.
Summary
An opt-in plugin that links CoreLocation to save assets with geographic coordinates on iOS/macOS. This is the companion to flutter_photo_manager #1431, which removes CoreLocation from the core package so most apps are not flagged by Apple's static scan.
com.fluttercandies/photo_manager_location) — no override/registry coupling with the core.CLLocation/PHAssetCreationRequestcode previously in the core.PMLocationPlugin/PMLocationManager) to avoid ObjC symbol collisions when both packages are linked.PMConvertUtils) so the returnedAssetEntityis fully populated.FlutterResultdispatched on the main thread.Verification
clang -fsyntax-onlyonPMManager.m(the file containing CLLocation): clean.dart analyze lib: No issues found.dart pub get: resolves cleanly.Photos,PhotosUI,CoreLocation(boths.ios.frameworksands.osx.frameworks).