Fix/camera state stalls #84
Conversation
Archdoog
requested review from
Copilot,
ianthetechie and
hactar
and removed request for
ianthetechie
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a camera state stalling issue by addressing race conditions between camera updates through modern concurrency. Key changes include:
- Converting test methods to async to better simulate asynchronous camera updates.
- Replacing the old updateCamera API with the new applyCameraChangeFromStateUpdate method.
- Updating camera-related models and dependencies (e.g., MapViewCamera, Package.swift).
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Tests/MapLibreSwiftUITests/MapViewCoordinator/MapViewCoordinatorCameraTests.swift | Converted tests to async and updated camera update calls. |
| Sources/MapLibreSwiftUI/Models/MapCamera/MapViewCamera.swift | Added Equatable and Sendable conformances to MapViewCamera. |
| Sources/MapLibreSwiftUI/Models/MapCamera/CameraState.swift | Added Equatable and Sendable conformances to CameraState. |
| Sources/MapLibreSwiftUI/MapViewCoordinatorCamera.swift | Added new concurrency-based camera update methods, including handling gestures asynchronously. |
| Sources/MapLibreSwiftUI/MapViewCoordinator.swift | Updated region change handling to use the new gesture-based API and removed old camera update code. |
| Sources/MapLibreSwiftUI/MapView.swift | Replaced updateCamera with applyCameraChangeFromStateUpdate; added a debug print for initial camera state. |
| Sources/MapLibreSwiftUI/Extensions/MapLibre/MLNMapViewCameraUpdating.swift | Added new properties for centerCoordinate and zoomLevel. |
| Package.swift | Updated swift-snapshot-testing dependency from 1.17.7 to 1.18.3. |
Comments suppressed due to low confidence (1)
Package.swift:33
- Ensure that updating the snapshot-testing dependency to version 1.18.3 does not introduce breaking changes in the snapshot tests.
.package(url: "https://github.com/pointfreeco/swift-snapshot-testing", from: "1.18.3"),
There was a problem hiding this comment.
Pull Request Overview
This PR addresses race conditions in camera updates by leveraging modern concurrency constructs and refactoring camera update handling throughout the codebase. Key changes include:
- Transition to async/await in camera update tests and coordinator methods.
- Replacement of the legacy updateCamera API with applyCameraChangeFromStateUpdate.
- Updates to protocol definitions and dependency versions to support the new concurrency model.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/MapLibreSwiftUITests/MapViewCoordinator/MapViewCoordinatorCameraTests.swift | Updated tests to use async/await and simulateCameraUpdateAndWait for verifying camera updates. |
| Sources/MapLibreSwiftUI/Models/MapCamera/MapViewCamera.swift | Added Equatable and Sendable conformances to support concurrent usage. |
| Sources/MapLibreSwiftUI/Models/MapCamera/CameraState.swift | Added Equatable and Sendable conformances. |
| Sources/MapLibreSwiftUI/MapViewCoordinatorCamera.swift | Introduced an async approach in applyCameraChangeFromStateUpdate and gesture-based update handling. |
| Sources/MapLibreSwiftUI/MapViewCoordinator.swift | Removed the legacy updateCamera method and refactored regionDidChangeWith to trigger gesture-based updates. |
| Sources/MapLibreSwiftUI/MapView.swift | Refactored to call the new applyCameraChangeFromStateUpdate method. |
| Sources/MapLibreSwiftUI/Extensions/MapLibre/MLNMapViewCameraUpdating.swift | Extended the protocol with centerCoordinate and zoomLevel properties for better camera state control. |
| Package.swift | Updated the snapshot-testing dependency version. |
Comments suppressed due to low confidence (2)
Sources/MapLibreSwiftUI/MapViewCoordinator.swift:180
- Only gesture-driven camera changes invoke applyCameraChangeFromGesture; non-gesture (programmatic) camera updates are no longer propagated to the parent binding. Please confirm that this behavior is intentional and does not omit necessary state synchronization.
switch changeReason {
Sources/MapLibreSwiftUI/Extensions/MapLibre/MLNMapViewCameraUpdating.swift:12
- The new centerCoordinate and zoomLevel properties have been added to the protocol. It is recommended to add or verify documentation and implementation details to ensure consistent behavior across all conforming types.
@MainActor var centerCoordinate: CLLocationCoordinate2D { get set }
| // This must be weak, the UIViewRepresentable owns the MLNMapView. | ||
| weak var mapView: MLNMapView? | ||
| var parent: MapView<T> | ||
|
|
||
| // Storage of variables as they were previously; these are snapshot | ||
| // every update cycle so we can avoid unnecessary updates | ||
| private var snapshotUserLayers: [StyleLayerDefinition] = [] | ||
| private var snapshotCamera: MapViewCamera? | ||
| var snapshotCamera: MapViewCamera? |
There was a problem hiding this comment.
This feels like it should be private since it's internal state that we don't want to modify. I assume the change is due to a lot of this functionality moving to an extension, which is in another file.
IMO this is a minor language design flaw, but we could still keep things in a separate extension and just move the code to the same file if you want that organizational barrier. But I'm not really sure opening up write access to these variables to get smaller files is the right design.
There was a problem hiding this comment.
Yep. Reverted.
I just very much enjoy scoped extensions especially working through these systems where tons of unrelated behaviors look similar.
Tests/MapLibreSwiftUITests/MapViewCoordinator/MapViewCoordinatorCameraTests.swift
Outdated
Show resolved
Hide resolved
Issue/Motivation
Tasklist