Merge FB-10: fix map pan-recenter race
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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(),
|
||||
],
|
||||
),
|
||||
);
|
||||
|
||||
|
||||
@@ -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)];
|
||||
|
||||
Reference in New Issue
Block a user