feat: Make CameraPosition properties nullable to support Bounds-based initialization#149
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
arturgesiarz
force-pushed
the
143-optional-props
branch
4 times, most recently
from
44ee69f to
d9cdf0a
Compare
arturgesiarz
force-pushed
the
143-optional-props
branch
from
d9cdf0a to
bc40a7d
Compare
There was a problem hiding this comment.
Pull request overview
Updates the multiplatform CameraPosition model to allow bounds-based initialization by making coordinates and zoom nullable, and adapts platform-specific camera conversions to tolerate nulls.
Changes:
- Make
CameraPosition.coordinatesandCameraPosition.zoomnullable with defaultnullvalues, updating KDoc to describe bounds-vs-coordinates behavior. - Update iOS (MapKit + Google Maps) camera application code paths to handle nullable camera fields.
- Update Android Google Maps camera conversion to handle nullable camera fields.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| kmp-maps/google-maps/src/iosMain/kotlin/com/swmansion/kmpmaps/googlemaps/Extensions.kt | Uses bounds-fit when present; otherwise now falls back when coordinates/zoom are null. |
| kmp-maps/core/src/iosMain/kotlin/com/swmansion/kmpmaps/core/Extensions.kt | MapKit region conversion now tolerates nullable coordinates/zoom (with fallbacks). |
| kmp-maps/core/src/commonMain/kotlin/com/swmansion/kmpmaps/core/MapTypes.kt | Makes CameraPosition fields nullable and updates documentation for bounds-based positioning. |
| kmp-maps/core/src/androidMain/kotlin/com/swmansion/kmpmaps/core/Extensions.kt | Android camera conversion now tolerates nullable coordinates/zoom (with fallbacks). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val coordinates: Coordinates? = null, | ||
| val zoom: Float? = null, | ||
| val bounds: MapBounds? = null, | ||
| val androidCameraPosition: AndroidCameraPosition? = null, | ||
| val iosCameraPosition: IosCameraPosition? = null, |
There was a problem hiding this comment.
CameraPosition can now be constructed with both coordinates and bounds null (because both are nullable with defaults). Downstream platform code falls back to (0,0)/zoom 0, which can silently move the camera to an unintended location. Consider enforcing the invariant in the model (e.g., require coordinates != null || bounds != null in an init block or via factory constructors) and/or explicitly handling the invalid state.
| bounds?.toLatLngBounds()?.center | ||
| ?: LatLng(coordinates?.latitude ?: 0.0, coordinates?.longitude ?: 0.0) |
There was a problem hiding this comment.
Falling back to (latitude, longitude) = (0, 0) when bounds is null but coordinates is also null can cause unexpected camera jumps to the Gulf of Guinea instead of failing fast. If coordinates is optional only to support bounds-based positioning, consider guarding this branch (e.g., return/no-op or throw) when both bounds and coordinates are null.
| bounds?.toLatLngBounds()?.center | |
| ?: LatLng(coordinates?.latitude ?: 0.0, coordinates?.longitude ?: 0.0) | |
| when { | |
| bounds != null -> bounds.toLatLngBounds().center | |
| coordinates != null -> LatLng(coordinates.latitude, coordinates.longitude) | |
| else -> throw IllegalStateException("CameraPosition must have either bounds or coordinates") | |
| } |
| CLLocationCoordinate2DMake(coordinates?.latitude ?: 0.0, coordinates?.longitude ?: 0.0) | ||
| val span = | ||
| MKCoordinateSpanMake( | ||
| calculateLatitudeDelta(zoom), | ||
| calculateLongitudeDelta(zoom, coordinates.latitude), | ||
| calculateLatitudeDelta(zoom ?: 0.0f), | ||
| calculateLongitudeDelta(zoom ?: 0.0f, coordinates?.latitude ?: 0.0), |
There was a problem hiding this comment.
When bounds is null, these Elvis fallbacks will treat a missing coordinates/zoom as (0,0)/0, which can silently move the camera to an unintended region. Consider explicitly rejecting/handling the "no bounds and no coordinates" case (fail fast or no-op) instead of defaulting to (0,0) here.
| CLLocationCoordinate2DMake( | ||
| position.coordinates.latitude, | ||
| position.coordinates.longitude, | ||
| position.coordinates?.latitude ?: 0.0, | ||
| position.coordinates?.longitude ?: 0.0, | ||
| ), | ||
| zoom = position.zoom, | ||
| zoom = position.zoom ?: 0f, |
There was a problem hiding this comment.
In the non-bounds path, defaulting missing coordinates/zoom to (0,0)/0 can silently move the camera to an unintended location when CameraPosition is under-specified. If nullability is only meant to support bounds-based initialization, consider guarding this branch and rejecting (or no-oping) when both bounds and coordinates are null instead of applying a hardcoded fallback.
No description provided.