diff --git a/docs/feedback/FB-10-map-pan-recenter-race.md b/docs/feedback/FB-10-map-pan-recenter-race.md index ba8068a..d5f453c 100644 --- a/docs/feedback/FB-10-map-pan-recenter-race.md +++ b/docs/feedback/FB-10-map-pan-recenter-race.md @@ -1,6 +1,6 @@ # FB-10 — Manual map pan loses a race against the follow-recenter timer -**Depends on** — · **Size** M · **Status** Not started +**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 @@ -189,3 +189,53 @@ Any change to how often ambient position ticks arrive (`_interval` in 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)];