issue #54: Adjust zoom levels to screen size #91
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements screen-size-aware zoom level calculations for map views by adding a mapSize parameter throughout the zoom calculation pipeline. The changes replace a simple offset-based formula with a dynamic adjustment based on screen dimensions, allowing for better zoom levels on different device sizes.
Changes:
- Introduced screen-size-aware zoom adjustment using linear interpolation between small (375px) and large (1024px) screens
- Refactored
AnnotationLoadingParamsto store pre-computed zoom values instead of region objects - Replaced custom
clamped(in:)extension with VGSL'sclamp()method across the codebase
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Rewind/Model/Zoom.swift | Added screen-size-based adjustment function and updated zoom/delta calculations to accept mapSize parameter |
| Rewind/Model/RewindRemotes.swift | Restructured AnnotationLoadingParams to store computed values (zoom, coordinates, startAt) instead of region |
| Rewind/Model/MapModel.swift | Updated all Region initializations to include mapSize parameter and adjusted location button zoom from 15 to 17 |
| Rewind/Model/LocalClustering.swift | Threaded mapSize parameter through clustering functions |
| Rewind/MapView/MapAdapter.swift | Added size property to expose map frame dimensions |
| Rewind/Model/CameraSession.swift | Replaced clamped(in:) with clamp() method |
| Rewind/View/MapControlsHiding.swift | Replaced clamped(in:) with clamp() method |
| Rewind/PastvuColors.swift | Replaced clamped(in:) with clamp() method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let x = min(mapSize.width, mapSize.height) | ||
| let (x1, y1) = (375.0, 0.7) // min adjustment for small screens | ||
| let (x2, y2) = (1024.0, 1.5) // max adjustment for large screens | ||
| return (y2 - y1) / (x2 - x1) * (x - x1) + y1 |
There was a problem hiding this comment.
The adjustment function uses linear interpolation without clamping the result. When the screen size is smaller than 375 pixels (x1), the function will return values less than 0.7. Similarly, for screens larger than 1024 pixels (x2), it will return values greater than 1.5. This could lead to unexpected zoom behavior on very small or very large screens. Consider clamping the result to the range 0.7...1.5 to ensure the adjustment stays within the intended bounds.
| return (y2 - y1) / (x2 - x1) * (x - x1) + y1 | |
| let value = (y2 - y1) / (x2 - x1) * (x - x1) + y1 | |
| return max(y1, min(y2, value)) |
There was a problem hiding this comment.
When the screen size is smaller than 375 pixels (x1), the function will return values less than 0.7. Similarly, for screens larger than 1024 pixels (x2), it will return values greater than 1.5.
sounds ok
| let x = min(mapSize.width, mapSize.height) | ||
| let (x1, y1) = (375.0, 0.7) // min adjustment for small screens | ||
| let (x2, y2) = (1024.0, 1.5) // max adjustment for large screens |
There was a problem hiding this comment.
The adjustment function implements a linear interpolation formula but lacks documentation explaining the rationale behind the chosen screen size thresholds (375px and 1024px) and adjustment values (0.7 and 1.5). Adding a comment to explain why these specific values were chosen would improve code maintainability and help future developers understand the zoom adjustment logic.
| let x = min(mapSize.width, mapSize.height) | |
| let (x1, y1) = (375.0, 0.7) // min adjustment for small screens | |
| let (x2, y2) = (1024.0, 1.5) // max adjustment for large screens | |
| // Linearly adjust the zoom level based on the smaller screen dimension so that | |
| // roughly the same amount of map content is visible across devices: | |
| // - 375pt is a typical "small" iPhone width; on these screens we apply a smaller | |
| // zoom offset (0.7) so we don't over-zoom and hide too much context. | |
| // - 1024pt is a typical "large" iPad width; on these screens we apply a larger | |
| // zoom offset (1.5) so the map does not look too "zoomed out" on big displays. | |
| // For intermediate sizes we interpolate linearly between these endpoints. | |
| let x = min(mapSize.width, mapSize.height) | |
| let (x1, y1) = (375.0, 0.7) // lower bound: small-phone screens | |
| let (x2, y2) = (1024.0, 1.5) // upper bound: large-tablet screens |
No description provided.