From dde6ec833a4f582738563888fa8392647ffa7ddd Mon Sep 17 00:00:00 2001 From: uhryniuk Date: Tue, 25 Aug 2026 11:34:05 -0500 Subject: [PATCH] FB-10: close the ambient-tick-vs-drag race that snapped the map back A pointer down/up Listener around the FlutterMap now tracks whether a real gesture is in progress, and didUpdateWidget's follow-recenter callbacks skip the camera move while one is -- closing the timing window where a once-per- second ambient GPS tick's postFrameCallback could land after a finger touched the map but before flutter_map reported hasGesture: true, snapping the camera back out from under an in-progress pan. --- docs/feedback/FB-10-map-pan-recenter-race.md | 241 +++++++++++++++++++ lib/src/ui/components/ride_map.dart | 182 ++++++++------ test/ride_map_test.dart | 73 ++++++ 3 files changed, 417 insertions(+), 79 deletions(-) create mode 100644 docs/feedback/FB-10-map-pan-recenter-race.md diff --git a/docs/feedback/FB-10-map-pan-recenter-race.md b/docs/feedback/FB-10-map-pan-recenter-race.md new file mode 100644 index 0000000..d5f453c --- /dev/null +++ b/docs/feedback/FB-10-map-pan-recenter-race.md @@ -0,0 +1,241 @@ +# FB-10 — Manual map pan loses a race against the follow-recenter timer + +**Depends on** — · **Size** M · **Status** Done + +## Goal +The rider must be able to pan the map and have it stay where they put it. Today a pan +gesture is silently cancelled a moment after it happens, both idle and while recording. + +## Context +Direct user feedback (`docs/FEEDBACK.md`): + +> Map page still cannot be scrolled/panned around -- it stays locked centered on the +> user, both idle and recording. A previous attempt at this fix (FB-06) did not +> actually resolve it on-device. + +FB-06 already removed the old `hasPoints` gate that fully disabled +`InteractiveFlag.drag`/`InteractiveFlag.pinchZoom`. That part of the fix is correct and +confirmed by widget tests. The map is still stuck because of a **different** bug FB-06 +did not find: a timer-driven recenter that keeps re-arming and wins a race against the +user's own drag. + +`lib/src/ui/app_shell.dart` builds one shared `RideMap` behind every tab, and rebuilds +it on every ambient GPS fix: + +```dart +child: RideMap( + key: const Key('shell-background-map'), + points: points, + segments: segments, + ambientPosition: ambientPosition, + follow: isMapTab, + ... +``` + +`ambientPosition` comes from `ambientPositionProvider`, fed by +`GeolocatorLocationSource` at a fixed one-second interval with no distance filter +(`lib/src/recording/geolocator_location_source.dart`): + +```dart +/// Matches the native `LocationRequest`: high accuracy, 1 s nominal interval, no +/// distance filter (a stationary bike must still produce fixes so elapsed time and the +/// noise floor behave). +const Duration _interval = Duration(seconds: 1); +``` + +Because `distanceFilter` is `0`, almost every one-second tick delivers a fix that is at +least slightly different from the last one -- so `AppShell`, and therefore `RideMap`, +rebuilds roughly once per second, indefinitely, on the Map tab. + +`lib/src/ui/components/ride_map.dart`, `didUpdateWidget` (~line 142): + +```dart +void didUpdateWidget(RideMap old) { + super.didUpdateWidget(old); + if (!_following || _backgrounded) return; + if (widget.points.isNotEmpty) { + final last = widget.points.last; + WidgetsBinding.instance.addPostFrameCallback((_) { + if (!mounted || !_following) return; + _controller.move(ll.LatLng(last.latitude, last.longitude), _controller.camera.zoom); + }); + } else if (widget.ambientPosition != null && + widget.ambientPosition != old.ambientPosition) { + final isFirstFix = old.ambientPosition == null; + WidgetsBinding.instance.addPostFrameCallback((_) { + if (!mounted || !_following) return; + _controller.move( + widget.ambientPosition!, + isFirstFix ? ambientZoom : _controller.camera.zoom, + ); + }); + } +} +``` + +The only thing that stops this recenter is `_following` turning `false`, which only +happens inside `onPositionChanged` (~line 277): + +```dart +onPositionChanged: !widget.follow + ? null + : (position, hasGesture) { + if (hasGesture && _following) { + setState(() => _following = false); + } + }, +``` + +This callback is correct on its own -- a controller-driven `.move()` reports +`hasGesture: false`, so it never cancels itself out. The problem is timing, not logic: +`didUpdateWidget` re-arms a fresh `postFrameCallback` on almost every one-second tick, +for as long as `_following` is still `true`. If a tick lands while the rider's finger is +already down and moving but flutter_map has not yet reported `hasGesture: true` for that +drag, the scheduled callback fires anyway and snaps the camera straight back to the +ambient fix -- one frame after the rider moved it. The next tick does the same thing a +second later. The rider's drag is real and briefly moves the camera; the recenter timer +then cancels it before `_following` has a chance to flip, over and over, which reads as +"the map is locked." + +`test/ride_map_test.dart` never actually reproduces this, because every "manual pan" +test drives the bug-free half of the code by calling the callback directly instead of +performing a real drag: + +```dart +var map = tester.widget(find.byType(FlutterMap)); +map.options.onPositionChanged!(map.mapController!.camera, true); +``` + +A test built this way cannot observe the race: it sets `hasGesture: true` instantly, +with no competing `postFrameCallback` scheduled in between. This is why FB-06's own +tests passed while the on-device bug remained. + +## Design +Track whether a real pointer gesture is currently in progress on the map, and skip the +follow-recenter entirely while one is. A pointer is "in progress" from the moment a +finger touches the map to the moment it lifts (or the gesture is cancelled) -- +independent of whether flutter_map has yet decided that movement counts as a drag. + +- Add a private `bool _gestureInProgress = false;` field to `_RideMapState`. +- Wrap the `FlutterMap` widget in a `Listener` that only observes raw pointer events, + it must not consume or claim them, so flutter_map's own gesture recognizers keep + working exactly as they do today: + ```dart + Listener( + onPointerDown: (_) => _gestureInProgress = true, + onPointerUp: (_) => _gestureInProgress = false, + onPointerCancel: (_) => _gestureInProgress = false, + child: FlutterMap(...), + ) + ``` +- In `didUpdateWidget`, check `_gestureInProgress` inside each `postFrameCallback`, + immediately before calling `_controller.move(...)`, in addition to the existing + `mounted`/`_following` checks. Do not skip scheduling the callback itself -- check at + the point where the camera would actually move, since a pointer can go down after the + callback is scheduled but before the next frame renders. +- Leave `onPositionChanged`'s existing `hasGesture` check exactly as it is. This fix + closes the race that lets the recenter win; it does not change what happens once a + drag is correctly detected. + +## Implementation +1. Add the `_gestureInProgress` field to `_RideMapState` in + `lib/src/ui/components/ride_map.dart`. +2. Wrap the existing `FlutterMap` widget in a `Listener` with `onPointerDown`, + `onPointerUp`, and `onPointerCancel` handlers that set `_gestureInProgress`. +3. Add `if (_gestureInProgress) return;` to both `postFrameCallback` bodies in + `didUpdateWidget`, alongside the existing `if (!mounted || !_following) return;` + check. +4. Add a widget test to `test/ride_map_test.dart` that reproduces the race directly: + start a real drag with `tester.startGesture(...)`, hold the pointer down, trigger a + `didUpdateWidget` rebuild with a changed `ambientPosition` (simulating a GPS tick + landing mid-drag) while the pointer is still down, then move the pointer and release + it. Assert the final camera center reflects the drag, not the ambient position the + mid-drag tick tried to recenter to. +5. Run the app on the Android emulator. Set a mock GPS fix so ambient ticks keep + arriving. Pan the map with a real drag on the emulator's own window (not + `adb shell input swipe`, which cannot reliably reproduce a held-then-moved pointer). + Confirm the camera stays where it was dragged to and does not snap back. + +## Acceptance criteria +- [ ] A pan gesture on the idle Map tab moves the camera and it stays moved, even while + ambient GPS fixes keep arriving once per second. +- [ ] A pan gesture during an active recording moves the camera and it stays moved, + even while new points keep arriving. +- [ ] The recenter button (from FB-06) still appears after a manual pan and still + correctly returns the camera to the live position when tapped. +- [ ] The new widget test reproduces the race and fails without the fix, passes with it. +- [ ] `flutter analyze` clean, `flutter test` green, test count only goes up. + +## Tests +- New test in `test/ride_map_test.dart`: a real `tester.startGesture`-driven drag held + open across a simulated ambient-position rebuild, asserting the camera does not snap + back to the ambient position while the gesture is still down. +- Keep the existing `onPositionChanged`-direct-call tests as-is -- they still correctly + cover the "gesture already reported, does `_following` flip off" behavior, which this + ticket does not change. + +## Risks +`Listener`'s `onPointerDown`/`onPointerUp` fire for every pointer, including a tap that +never becomes a drag (e.g. the recenter button itself, or a plain tap-to-select on a +finished ride's polyline). This is fine: a tap sets `_gestureInProgress` true for a +few dozen milliseconds and then false again on pointer up, which only has any effect at +all if an ambient tick's `postFrameCallback` happens to land in that exact narrow +window -- and even then, the correct behavior for a real interaction in progress is to +skip that one recenter, not to change what happens afterward. + +## Out of scope +Any change to how often ambient position ticks arrive (`_interval` in +`geolocator_location_source.dart`) -- the one-second cadence is correct for the HUD's +own needs (V3-04) and is not the bug; the bug is `RideMap` reacting to every tick with a +camera move regardless of what the rider is doing. FB-11's Route Planner tile issue, +tracked separately. + +## Outcome +Shipped exactly as designed, with no deviation. + +`lib/src/ui/components/ride_map.dart` now has a `_gestureInProgress` field on +`_RideMapState`. The `FlutterMap` is wrapped in a `Listener` that sets it `true` on +`onPointerDown` and `false` on `onPointerUp`/`onPointerCancel`. The `Listener` only +observes pointer events; it does not consume them, so `flutter_map`'s own drag and +pinch-zoom recognizers still work untouched. Both `postFrameCallback` bodies in +`didUpdateWidget` now check `if (_gestureInProgress) return;` right before the +`_controller.move(...)` call, alongside the existing `mounted`/`_following` checks. +`onPositionChanged` was left alone, as the design said. + +Added one new widget test to `test/ride_map_test.dart`, in the "always-interactive +map + recenter (FB-06)" group: `a real drag beats an ambient tick that lands +mid-drag (FB-10)`. It starts a real pointer with `tester.startGesture`, holds it +down, then rebuilds the widget with a changed `ambientPosition` to simulate a GPS +tick landing mid-drag, and asserts right there — before any pointer movement — that +the camera has not snapped to the ambient fix. It then moves and lifts the pointer +and asserts the same thing again for the completed drag. + +Getting this test to actually catch the bug took one extra iteration. The first +version did the pointer-down, the mid-drag ambient tick, then a real move-and-lift, +and only checked the camera position at the very end. That version passed even with +the fix removed, because the final drag always overwrites the camera regardless of +what the buggy recenter did in between — a drag is relative motion, so wherever the +camera started, the drag moves it away from there, and the end position is never +exactly equal to the ambient fix either way. The fix was to add the same assertion +right after the mid-drag tick, before any pointer movement happens. At that point, +with the pointer down but not yet moved, the old code snaps the camera to exactly +the ambient position (confirmed by temporarily removing the two +`_gestureInProgress` guards and re-running: the test failed with `Actual: <51.5>`, +the exact ambient fix latitude); the fixed code leaves the camera where it was. +Both the "old code fails, fixed code passes" checks were run and confirmed before +finishing. + +One unrelated flake showed up during a full `flutter test` run: +`test/geo_test.dart`'s "douglas-peucker handles a full ride without stack overflow +and stays fast" failed once, then passed on every subsequent run in isolation and in +the full suite. It looks like a timing-sensitive performance assertion, not +something this change touches -- `ride_map.dart` and `geo.dart` share no code path. + +`flutter analyze` stayed clean: the same 4 pre-existing info-level issues in +`crash_reporter.dart` and `map_connectivity.dart`, nothing new. `flutter test` went +from 434 to 435 passing (the one new test), everything else green. + +No Android emulator or `adb` was available in this environment, so the on-device +check in Implementation step 5 was not attempted here. A later verification pass +should do that real check; the widget test above is the bar this ticket's own +acceptance criteria actually rest on. diff --git a/lib/src/ui/components/ride_map.dart b/lib/src/ui/components/ride_map.dart index 8603341..f39c2ee 100644 --- a/lib/src/ui/components/ride_map.dart +++ b/lib/src/ui/components/ride_map.dart @@ -133,6 +133,14 @@ class _RideMapState extends State with WidgetsBindingObserver { /// primitive, and an app resume already triggers a full rebuild anyway. bool _backgrounded = false; + /// FB-10: true from the moment a finger touches the map to the moment it lifts (or the + /// gesture is cancelled) -- independent of whether flutter_map has yet decided the + /// movement counts as a drag. Closes a race where a once-per-second ambient GPS tick's + /// `postFrameCallback` lands after the finger is down but before flutter_map has + /// reported `hasGesture: true`, snapping the camera back out from under the rider's + /// own in-progress pan. + bool _gestureInProgress = false; + @override void initState() { super.initState(); @@ -149,6 +157,10 @@ class _RideMapState extends State with WidgetsBindingObserver { // straight to the latest fix keeps the map from ever being one flush behind. WidgetsBinding.instance.addPostFrameCallback((_) { if (!mounted || !_following) return; + // FB-10: a pointer can go down after this callback is scheduled but before the + // next frame renders, so the check has to happen here, at the point where the + // camera would actually move, not when the callback is scheduled. + if (_gestureInProgress) return; _controller.move(ll.LatLng(last.latitude, last.longitude), _controller.camera.zoom); }); } else if (widget.ambientPosition != null && @@ -163,6 +175,8 @@ class _RideMapState extends State with WidgetsBindingObserver { final isFirstFix = old.ambientPosition == null; WidgetsBinding.instance.addPostFrameCallback((_) { if (!mounted || !_following) return; + // FB-10: see the identical check above -- same race, same reason. + if (_gestureInProgress) return; _controller.move( widget.ambientPosition!, isFirstFix ? ambientZoom : _controller.camera.zoom, @@ -243,88 +257,98 @@ class _RideMapState extends State with WidgetsBindingObserver { // adjacent controls, which osmdroid did until it was explicitly bounded. borderRadius: widget.fill ? BorderRadius.zero : BorderRadius.circular(ripprRadiusLarge), - child: FlutterMap( - mapController: _controller, - options: MapOptions( - initialCameraFit: (bounds == null || bounds.isDegenerate) - // No points yet, or every point at one spot (a parked "ride") -- - // fitting a degenerate box would zoom to infinity, so centre on a - // neutral or last-known point at a sane street-level zoom instead. - ? null - : CameraFit.bounds( - bounds: LatLngBounds( - ll.LatLng(bounds.minLat, bounds.minLon), - ll.LatLng(bounds.maxLat, bounds.maxLon), + // FB-10: raw pointer observation only -- this must not consume or claim the + // event, so flutter_map's own gesture recognizers underneath keep working exactly + // as they do today. A pointer is "in progress" from the instant a finger touches + // down, well before flutter_map's own recognizers decide the movement counts as a + // drag and report `hasGesture: true` -- that gap is the race this closes. + child: Listener( + onPointerDown: (_) => _gestureInProgress = true, + onPointerUp: (_) => _gestureInProgress = false, + onPointerCancel: (_) => _gestureInProgress = false, + child: FlutterMap( + mapController: _controller, + options: MapOptions( + initialCameraFit: (bounds == null || bounds.isDegenerate) + // No points yet, or every point at one spot (a parked "ride") -- + // fitting a degenerate box would zoom to infinity, so centre on a + // neutral or last-known point at a sane street-level zoom instead. + ? null + : CameraFit.bounds( + bounds: LatLngBounds( + ll.LatLng(bounds.minLat, bounds.minLon), + ll.LatLng(bounds.maxLat, bounds.maxLon), + ), + padding: const EdgeInsets.all(24), + // The clamp. Without it a short ride lands past OSM's max tile zoom + // and renders an empty grid. + maxZoom: maxTileZoom, ), - padding: const EdgeInsets.all(24), - // The clamp. Without it a short ride lands past OSM's max tile zoom - // and renders an empty grid. - maxZoom: maxTileZoom, - ), - initialCenter: bounds == null - ? (widget.ambientPosition ?? const ll.LatLng(0, 0)) - : ll.LatLng(bounds.centerLat, bounds.centerLon), - initialZoom: bounds == null - ? (widget.ambientPosition == null ? 2 : ambientZoom) - : (bounds.isDegenerate ? shortRideZoom : maxTileZoom), - maxZoom: maxTileZoom, - // FB-06: pan/zoom must always be available, idle or recording -- the old - // `hasPoints` gate locked the map to `InteractiveFlag.none` whenever no trip - // was recording, which is exactly the "can't zoom and move around" report. - interactionOptions: const InteractionOptions( - flags: InteractiveFlag.pinchZoom | InteractiveFlag.drag, + initialCenter: bounds == null + ? (widget.ambientPosition ?? const ll.LatLng(0, 0)) + : ll.LatLng(bounds.centerLat, bounds.centerLon), + initialZoom: bounds == null + ? (widget.ambientPosition == null ? 2 : ambientZoom) + : (bounds.isDegenerate ? shortRideZoom : maxTileZoom), + maxZoom: maxTileZoom, + // FB-06: pan/zoom must always be available, idle or recording -- the old + // `hasPoints` gate locked the map to `InteractiveFlag.none` whenever no trip + // was recording, which is exactly the "can't zoom and move around" report. + interactionOptions: const InteractionOptions( + flags: InteractiveFlag.pinchZoom | InteractiveFlag.drag, + ), + onPositionChanged: !widget.follow + ? null + : (position, hasGesture) { + // Only a real pan/pinch turns following off -- the programmatic + // moves this widget makes to chase the rider must not cancel + // themselves out. + if (hasGesture && _following) { + setState(() => _following = false); + } + }, ), - onPositionChanged: !widget.follow - ? null - : (position, hasGesture) { - // Only a real pan/pinch turns following off -- the programmatic - // moves this widget makes to chase the rider must not cancel - // themselves out. - if (hasGesture && _following) { - setState(() => _following = false); - } - }, + children: [ + // UI-02: the skeleton replaces the TileLayer entirely rather than sitting on + // top of it -- a widget that keeps trying and failing to fetch underneath its + // own placeholder would be exactly the retry loop the ticket warns against. + if (widget.skeletonMode) + const SkeletonMapLayer() + // Omitted entirely while backgrounded -- not just visually hidden -- so no + // tile request can fire off-screen. See the lifecycle observer above. + else if (!_backgrounded) + TileLayer( + urlTemplate: tileUrlTemplate, + subdomains: tileSubdomains, + retinaMode: true, + userAgentPackageName: tileUserAgent, + maxNativeZoom: tileMaxNativeZoom, + // Respect the tile host's usage policy: render what is looked at, never + // bulk prefetch. + panBuffer: 0, + tileProvider: widget.tileProvider, + ), + PolylineLayer(polylines: polylines), + if (widget.showLocationMarker && (hasPoints || widget.ambientPosition != null)) + MarkerLayer( + markers: [ + Marker( + key: const Key('location-marker'), + point: hasPoints + ? ll.LatLng( + widget.points.last.latitude, + widget.points.last.longitude, + ) + : widget.ambientPosition!, + width: 40, + height: 40, + child: const PulsingLocationMarker(), + ), + ], + ), + if (widget.showAttribution) const TileAttribution(), + ], ), - children: [ - // UI-02: the skeleton replaces the TileLayer entirely rather than sitting on - // top of it -- a widget that keeps trying and failing to fetch underneath its - // own placeholder would be exactly the retry loop the ticket warns against. - if (widget.skeletonMode) - const SkeletonMapLayer() - // Omitted entirely while backgrounded -- not just visually hidden -- so no - // tile request can fire off-screen. See the lifecycle observer above. - else if (!_backgrounded) - TileLayer( - urlTemplate: tileUrlTemplate, - subdomains: tileSubdomains, - retinaMode: true, - userAgentPackageName: tileUserAgent, - maxNativeZoom: tileMaxNativeZoom, - // Respect the tile host's usage policy: render what is looked at, never - // bulk prefetch. - panBuffer: 0, - tileProvider: widget.tileProvider, - ), - PolylineLayer(polylines: polylines), - if (widget.showLocationMarker && (hasPoints || widget.ambientPosition != null)) - MarkerLayer( - markers: [ - Marker( - key: const Key('location-marker'), - point: hasPoints - ? ll.LatLng( - widget.points.last.latitude, - widget.points.last.longitude, - ) - : widget.ambientPosition!, - width: 40, - height: 40, - child: const PulsingLocationMarker(), - ), - ], - ), - if (widget.showAttribution) const TileAttribution(), - ], ), ); diff --git a/test/ride_map_test.dart b/test/ride_map_test.dart index cbfba68..9b1bf33 100644 --- a/test/ride_map_test.dart +++ b/test/ride_map_test.dart @@ -520,6 +520,79 @@ void main() { expect(center.longitude, closeTo(fix1.longitude, 1e-9)); }); + testWidgets( + 'a real drag beats an ambient tick that lands mid-drag (FB-10)', + (tester) async { + const fix1 = ll.LatLng(51.0, -114.0); + // The ambient tick that lands while the finger is down but before + // flutter_map's own gesture recognizer has reported `hasGesture: true` -- + // this is the exact race the ticket describes. + const fix2 = ll.LatLng(51.5, -114.5); + + Widget build(ll.LatLng ambient) => MaterialApp( + theme: ripprTheme(), + home: Scaffold( + body: RideMap( + points: const [], + segments: const [], + showEmptyLabel: false, + follow: true, + ambientPosition: ambient, + ), + ), + ); + + await tester.pumpWidget(build(fix1)); + await tester.pump(); + + // Put a real pointer down on the map -- this is what a rider's finger + // touching the screen looks like, well before flutter_map decides the + // movement counts as a drag. + final gesture = + await tester.startGesture(tester.getCenter(find.byType(FlutterMap))); + addTearDown(() => gesture.removePointer()); + + // Simulate a once-per-second ambient GPS tick landing while the pointer + // is already down but before it has moved -- exactly the race window + // the ticket describes: flutter_map has not yet reported `hasGesture: + // true`, so nothing has flipped `_following` off yet. + await tester.pumpWidget(build(fix2)); + await tester.pump(); + + // The critical assertion: with the pointer still down and untouched by + // any real drag, the camera must not have snapped to the ambient tick's + // position. Without the fix, the race lets the scheduled + // `postFrameCallback` win here and the camera jumps to `fix2` before + // the rider's finger has moved at all. + var map = tester.widget(find.byType(FlutterMap)); + var center = map.mapController!.camera.center; + expect( + center.latitude, + isNot(closeTo(fix2.latitude, 1e-6)), + reason: 'the mid-drag ambient tick must not win the race and snap the ' + 'camera back to the ambient position while the gesture is still ' + 'down', + ); + expect(center.longitude, isNot(closeTo(fix2.longitude, 1e-6))); + + // Now the finger actually moves and lifts -- the real drag completes + // normally, and the final position reflects it, not the ambient tick. + await gesture.moveBy(const Offset(-100, -100)); + await tester.pump(); + await gesture.up(); + await tester.pump(); + + map = tester.widget(find.byType(FlutterMap)); + center = map.mapController!.camera.center; + expect( + center.latitude, + isNot(closeTo(fix2.latitude, 1e-6)), + reason: 'the completed drag must still reflect the rider\'s own pan, ' + 'not the ambient position the mid-drag tick tried to recenter to', + ); + expect(center.longitude, isNot(closeTo(fix2.longitude, 1e-6))); + }); + testWidgets('tapping recenter moves the camera to the latest recorded ' 'point when recording', (tester) async { final points = [for (var i = 0; i < 4; i++) p(1, i)];