dilly/rippr
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

FB-10: close the ambient-tick-vs-drag race that snapped the map back

Browse Source 
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.
This commit is contained in:
uhryniuk
2026-08-25 11:34:05 -05:00
parent 4051416add
commit dde6ec833a
3 changed files with 417 additions and 79 deletions
Download Patch File Download Diff File Expand all files Collapse all files

241
docs/feedback/FB-10-map-pan-recenter-race.md Normal file
View File

@@ -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<FlutterMap>(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.

182
lib/src/ui/components/ride_map.dart
View File

@@ -133,6 +133,14 @@ class _RideMapState extends State<RideMap> 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<RideMap> 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<RideMap> 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<RideMap> 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(),
],
),
);

73
test/ride_map_test.dart
View File

@@ -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<FlutterMap>(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<FlutterMap>(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)];