feat: map snapshot for mobile platforms #726
Conversation
gabbopalma
added
enhancement
There was a problem hiding this comment.
Pull request overview
This PR adds map snapshot functionality for mobile platforms (iOS and Android), building upon existing web snapshot capabilities. The implementation introduces a unified API that allows taking snapshots of maps with custom camera positions, dimensions, and optional markers.
Changes:
- Removed the old
translucent_full_map.dartfile from the diff (though the file still exists) - Split map snapshot functionality into platform-specific examples:
map_snapshot_web.dartandmap_snapshot_mobile.dart - Added new
startMapSnapshotfunction andMapMarkerclass to the global API - Implemented native iOS snapshot support via
MapSnapshotter.swift - Implemented native Android snapshot support via
MapSnapshotWrapper.java
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| maplibre_gl_example/lib/translucent_full_map.dart | File deletion from diff (file still exists in repository) |
| maplibre_gl_example/lib/main.dart | Updated imports to use platform-specific snapshot pages |
| maplibre_gl_example/lib/examples/advanced/map_snapshot_web.dart | Renamed class from MapSnapshotPage to MapSnapshotPageWeb |
| maplibre_gl_example/lib/examples/advanced/map_snapshot_mobile.dart | New example demonstrating mobile snapshot functionality with markers and camera controls |
| maplibre_gl_example/ios/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme | Added LLDB initialization file configuration |
| maplibre_gl/lib/src/global.dart | Added startMapSnapshot function and MapMarker class for snapshot API |
| maplibre_gl/ios/maplibre_gl/Sources/maplibre_gl/MapSnapshotter.swift | New iOS implementation for map snapshots |
| maplibre_gl/ios/maplibre_gl/Sources/maplibre_gl/MapLibreMapsPlugin.swift | Integrated MapSnapshotter into iOS plugin |
| maplibre_gl/android/src/main/java/org/maplibre/maplibregl/MapSnapshotWrapper.java | New Android implementation for map snapshots |
| maplibre_gl/android/src/main/java/org/maplibre/maplibregl/GlobalMethodHandler.java | Integrated MapSnapshotWrapper into Android plugin |
Comments suppressed due to low confidence (1)
maplibre_gl_example/lib/translucent_full_map.dart:1
- The diff shows this entire file being deleted (all lines prefixed with -), but according to the repository structure, this file still exists. This appears to be a diff formatting issue or the file shouldn't be in this PR's changes. If the file is meant to remain, it should not appear in the diff. If it's meant to be deleted, the actual deletion is incomplete.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| let size = CGSize(width: width, height: height) | ||
| let camera = MLNMapCamera(lookingAtCenter: CLLocationCoordinate2D(latitude: 0, longitude: 0), fromDistance: 1000, pitch: 0, heading: 0) |
There was a problem hiding this comment.
The camera is initialized with hardcoded default values (0, 0 coordinates and 1000 distance) but these values are immediately overridden by the camera position from arguments. This temporary camera initialization is unnecessary and confusing. Consider initializing the camera directly with the target coordinates and zoom level from cameraPosition, or use a more meaningful placeholder if required by the API.
| let size = CGSize(width: width, height: height) | ||
| let camera = MLNMapCamera(lookingAtCenter: CLLocationCoordinate2D(latitude: 0, longitude: 0), fromDistance: 1000, pitch: 0, heading: 0) | ||
| let options = MLNMapSnapshotOptions(styleURL: URL(string: styleUrl)!, camera: camera, size: size) |
There was a problem hiding this comment.
Force-unwrapping the URL with ! can cause a crash if styleUrl is not a valid URL. This should use guard-let or if-let to safely unwrap the URL and return a FlutterError if it's invalid.
| let size = CGSize(width: width, height: height) | |
| let camera = MLNMapCamera(lookingAtCenter: CLLocationCoordinate2D(latitude: 0, longitude: 0), fromDistance: 1000, pitch: 0, heading: 0) | |
| let options = MLNMapSnapshotOptions(styleURL: URL(string: styleUrl)!, camera: camera, size: size) | |
| guard let styleURL = URL(string: styleUrl) else { | |
| result(FlutterError( | |
| code: "INVALID_ARGUMENTS", | |
| message: "Invalid styleUrl", | |
| details: nil | |
| )) | |
| return | |
| } | |
| let size = CGSize(width: width, height: height) | |
| let camera = MLNMapCamera(lookingAtCenter: CLLocationCoordinate2D(latitude: 0, longitude: 0), fromDistance: 1000, pitch: 0, heading: 0) | |
| let options = MLNMapSnapshotOptions(styleURL: styleURL, camera: camera, size: size) |
|
|
||
| // Configure camera | ||
| if let zoom = cameraPosition["zoom"] as? Double { | ||
| print("Setting zoom level to: \(zoom)") |
There was a problem hiding this comment.
Debug print statements should be removed from production code or replaced with proper logging that can be controlled via build configurations. Consider using NSLog or a conditional logging mechanism.
| print("Setting zoom level to: \(zoom)") | |
| #if DEBUG | |
| NSLog("Setting zoom level to: \(zoom)") | |
| #endif |
| } | ||
|
|
||
| private func addMarkers(to image: UIImage, markers: [[String: Any]], snapshot: MLNMapSnapshot) -> UIImage { | ||
| print("iOS image size: \(image.size), scale: \(image.scale)") |
There was a problem hiding this comment.
Multiple debug print statements should be removed from production code or replaced with proper logging that can be controlled via build configurations.
|
|
||
| // Create icon from data | ||
| guard let iconImage = UIImage(data: iconData.data) else { continue } | ||
| print("iOS iconImage size: \(iconImage.size), scale: \(iconImage.scale)") |
There was a problem hiding this comment.
Multiple debug print statements should be removed from production code or replaced with proper logging that can be controlled via build configurations.
| private final MethodChannel channel; | ||
| private final android.content.Context context; | ||
| private MapSnapshotter snapshotter; | ||
| private MethodChannel.Result result; | ||
|
|
||
| public MapSnapshotWrapper(MethodChannel channel, android.content.Context context) { | ||
| this.channel = channel; |
There was a problem hiding this comment.
The channel field is stored but never used in the MapSnapshotWrapper class. It should be removed if not needed.
| private final MethodChannel channel; | |
| private final android.content.Context context; | |
| private MapSnapshotter snapshotter; | |
| private MethodChannel.Result result; | |
| public MapSnapshotWrapper(MethodChannel channel, android.content.Context context) { | |
| this.channel = channel; | |
| private final android.content.Context context; | |
| private MapSnapshotter snapshotter; | |
| private MethodChannel.Result result; | |
| public MapSnapshotWrapper(MethodChannel channel, android.content.Context context) { |
| case "startMapSnapshot": | ||
| Map<String, Object> snapshotArgs = (Map<String, Object>) methodCall.arguments; | ||
| if (mapSnapshotter == null) { | ||
| mapSnapshotter = new MapSnapshotWrapper(new MethodChannel(messenger, "plugins.flutter.io/maplibre_gl"), context); |
There was a problem hiding this comment.
Creating a new MethodChannel instance here is unnecessary since the MapSnapshotWrapper doesn't use it. The channel parameter should be removed from the MapSnapshotWrapper constructor, or if it's intended for future use, it should be documented.
| mapSnapshotter = new MapSnapshotWrapper(new MethodChannel(messenger, "plugins.flutter.io/maplibre_gl"), context); | |
| // MapSnapshotWrapper's MethodChannel parameter is currently unused; pass null to avoid creating an unnecessary instance. | |
| mapSnapshotter = new MapSnapshotWrapper(null, context); |
|
|
||
| public class MapLibreMapsPlugin: NSObject, FlutterPlugin { | ||
| static var downloadOfflineRegionChannelHandler: OfflineChannelHandler? = nil | ||
| static var mapSnapshotter: MapSnapshotter? = nil |
There was a problem hiding this comment.
Using a static variable for mapSnapshotter could lead to issues if multiple snapshot requests occur simultaneously. Consider using instance variables or implementing proper lifecycle management for concurrent snapshot requests.
| Bitmap scaledIcon = iconBitmap; // Use original bitmap directly | ||
|
|
||
| // Draw icon centered at the point | ||
| float left = point.x - scaledWidth / 2; | ||
| float top = point.y - scaledHeight; | ||
| canvas.drawBitmap(scaledIcon, left, top, paint); | ||
|
|
||
| // Only recycle scaledIcon if it's a different bitmap | ||
| if (scaledIcon != iconBitmap) { | ||
| scaledIcon.recycle(); |
There was a problem hiding this comment.
The variable name 'scaledIcon' is misleading since the comment indicates it's using the original bitmap directly without scaling. The variable should be renamed to 'iconToRender' or similar to reflect its actual purpose.
| Bitmap scaledIcon = iconBitmap; // Use original bitmap directly | |
| // Draw icon centered at the point | |
| float left = point.x - scaledWidth / 2; | |
| float top = point.y - scaledHeight; | |
| canvas.drawBitmap(scaledIcon, left, top, paint); | |
| // Only recycle scaledIcon if it's a different bitmap | |
| if (scaledIcon != iconBitmap) { | |
| scaledIcon.recycle(); | |
| Bitmap iconToRender = iconBitmap; // Use original bitmap directly | |
| // Draw icon centered at the point | |
| float left = point.x - scaledWidth / 2; | |
| float top = point.y - scaledHeight; | |
| canvas.drawBitmap(iconToRender, left, top, paint); | |
| // Only recycle iconToRender if it's a different bitmap | |
| if (iconToRender != iconBitmap) { | |
| iconToRender.recycle(); |
| // Only recycle scaledIcon if it's a different bitmap | ||
| if (scaledIcon != iconBitmap) { | ||
| scaledIcon.recycle(); | ||
| } |
There was a problem hiding this comment.
Since scaledIcon is directly assigned to iconBitmap (line 160), the condition scaledIcon != iconBitmap will always be false, making this recycling code unreachable. This appears to be leftover code from a previous implementation where scaling occurred. The condition and the associated recycle call should be removed since they're dead code.
| // Only recycle scaledIcon if it's a different bitmap | |
| if (scaledIcon != iconBitmap) { | |
| scaledIcon.recycle(); | |
| } |
2 participants
Continue of @EyreFree #610 PR.
This will introduce the snapshot for mobile platforms and try to create a unique API and example.