PB-2058: Fix geolocation zoom level #1488
Conversation
ismailsunni
force-pushed
the
fix-pb-2058-geolocation-zoom-level
branch
from
7197e5d to
275bd15
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the order of operations in handleNewGeolocationPosition to ensure that map centering occurs before position updates, preventing a potential race condition where setCenter might disable tracking. The code has also been reorganized to add debug logging when tracking is disabled.
- Moved position/accuracy update logic to execute after the map centering logic
- Added debug logging for when
setCenterIfInBoundsis skipped due to tracking being false - Added clarifying comment about the importance of the execution order
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/viewer/src/store/modules/geolocation/utils/handleNewGeolocationPosition.ts
Outdated
Show resolved
Hide resolved
packages/viewer/src/store/modules/geolocation/utils/handleNewGeolocationPosition.ts
Outdated
Show resolved
Hide resolved
pakb
left a comment
pakb
left a comment
There was a problem hiding this comment.
On my phone I am unable to disable geolocation anymore, the tracking "flag" always kicks in and reset the button to the first state (and we need the button to be in the "second" state and click it to disable the geolocation)
Are you able to test that out yourself, being outside the zone covered by the map? 🙈
|
@pakb I can test it with FakeGPS. So, the issue is only on mobile, or is it also on desktop/web? By the way, do we have documentation on how this geolocation/tracking feature should work (i.e., the expected behavior)? |
ismailsunni
force-pushed
the
fix-pb-2058-geolocation-zoom-level
branch
from
b6b1777 to
cf688f4
Compare
ismailsunni
force-pushed
the
fix-pb-2058-geolocation-zoom-level
branch
from
7d1e58b to
8a429f1
Compare
pakb
left a comment
pakb
left a comment
There was a problem hiding this comment.
Like the "compass" button that only shows up whenever the map rotation isn't "zero", I'd hide the "re-center" button when we are tracking, instead of switching it to red/active
|
@pakb, if we hide the "re-center" button while tracking, how do we "recommend" a way for a user to disable the tracking without disabling the geolocation? Pan the map is possible, though. |
|
@pakb I implemented it :) |
- Recenter button now uses distance-based check (>1m from user location) instead of tracking flag - Fix compass button not hiding when rotation reset to north (falsy check bug) - Adjust compass button visibility based on geolocation state - Add comments documenting button visibility conditions
…tion. Only reset rotation to north when auto-rotation was active, preserving manual user rotations when geolocation is disabled.
…after the first geolocation activation.
…vent. - Restore hasTrackingFeedback behavior that was missing from TS migration - Add circle icon overlay for visual feedback when tracking is disabled - Change event listener from movestart to pointerdrag to only disable tracking on deliberate map drag, not on zoom/rotate - Add post-activation tracking check - Comment out RecenterButton (to be re-enabled later)
- Fix TODO bug in setCenter that disabled tracking when geolocation updated the position - Check dispatcher name to distinguish between geolocation updates and user map movements - Always enable tracking unconditionally after geolocation activation - This fixes the 3-phase bug where tracking would be disabled immediately after activation
Geolocation button now only toggles location detection on/off. Recenter button controls tracking and auto-rotation. Tracking is disabled by default when geolocation activates.
…tes it Include RecenterButton in geolocation dispatcher check to prevent setCenter from immediately disabling tracking when it gets enabled.
Register faArrowsToCircle in FontAwesome library and use it in RecenterButton for better visual indication of tracking mode.
ismailsunni
force-pushed
the
fix-pb-2058-geolocation-zoom-level
branch
from
0cc78ef to
31642a5
Compare
| active: false, | ||
| denied: false, | ||
| tracking: true, | ||
| tracking: false, |
There was a problem hiding this comment.
I'd maybe change a bit how tracking is handled.
- App start-up: keep tracking
trueas the default- first time we press on the geolocation button, we track
- If geolocation is disabled, set tracking to
false- All subsequent activation of the geolocation don't track directly, and we show the "re-center" button instead
Tracking each time we activate the geolocation is something we tried to avoid with the previous buttons, so I think we should stick to this approach
| const isFromGeolocation = | ||
| dispatcher.name === 'GeolocButton.vue' || | ||
| dispatcher.name === 'RecenterButton.vue' || | ||
| dispatcher.name === 'OpenLayersGeolocationFeedback.vue' |
There was a problem hiding this comment.
Can you add an options to this action to manage this use case?
I don't really like that we rely on dispatcher name to change the logic of the application
Test link
Geolocation Button System - Behavior Summary
1. Geolocation Button (Location Arrow Icon)
Purpose
Toggles geolocation detection on/off
Visual States
Behavior
When clicked (geolocation OFF):
When clicked (geolocation ON):
Tooltip
Always Visible
Yes (unless permission denied)
2. Recenter Button (Arrows-to-Circle Icon)
Purpose
Toggles continuous position tracking and auto-rotation
Visual States
Behavior
When clicked (tracking OFF):
When clicked (tracking ON):
Tooltip
Visibility
Shows when:
Hides when:
3. Compass Button (North Arrow Icon)
Purpose
Resets map rotation to north and disables auto-rotation
Behavior
When clicked:
Tooltip
Visibility
Shows when:
Hides when:
Test link