diff --git a/docs/feedback/FB-01-live-street-level-map.md b/docs/feedback/FB-01-live-street-level-map.md index af21e81..f10e55e 100644 --- a/docs/feedback/FB-01-live-street-level-map.md +++ b/docs/feedback/FB-01-live-street-level-map.md @@ -1,6 +1,6 @@ # FB-01 — Live, street-level map everywhere -**Depends on** — · **Size** L · **Status** Not started +**Depends on** — · **Size** L · **Status** Done ## Goal The map is too zoomed out on every screen, and the shared background map has zero @@ -333,3 +333,51 @@ zoom-consistency but does not need ambient location following (Route Planner alr centers on the route's own waypoints, which is correct). HUD widget changes (FB-03). The idle Speed panel (FB-02) — unrelated, but note FB-02's idle screen will now show a genuinely live, moving map underneath once this ticket lands, which is the whole point. + +## Outcome +Implemented exactly as designed, with no deviations from the Design section: + +- `ambientZoom` constant added to `ride_map.dart`. +- `RideMap.ambientPosition` field/constructor param added; `bounds == null` branch of + `initialCenter`/`initialZoom`, the `didUpdateWidget` chase logic, and the + `showLocationMarker` `MarkerLayer` all updated exactly per the ticket's diffs. +- `ambientPositionProvider` added to `providers.dart`, verbatim from the ticket's own + code block — never calls `LocationSource.stop()`, only `start()` (idempotent) and its + own subscription's implicit cancellation of the broadcast `fixes` stream on + `autoDispose`. +- `ShellScaffold.build()` wired to watch `ambientPositionProvider` only while + `activeTripProvider` is null, passing the resulting `ll.LatLng?` into `RideMap` as + `ambientPosition`, alongside the unchanged `follow: isMapTab` / + `showLocationMarker: isMapTab`. + +Confirmed via `grep -n "\.stop()" $(git diff --name-only -- lib)` that no code this +ticket added calls `LocationSource.stop()` anywhere. + +One nuance surfaced by testing, not a deviation from the design but worth recording: +`activeTripProvider` is a `StreamProvider`, so on the very first frame after the shell +mounts its `valueOrNull` is `null` (still loading) even when a trip is already active in +the database — this is pre-existing behavior identical to how `points`/`segments` +already fell back to empty lists before their own streams' first emission. In practice +this means `ambientPositionProvider` may be watched, and `LocationSource.start()` called, +for one transient frame during cold start even if a recording turns out to already be +active. It self-corrects the instant the trip stream emits (the shell stops watching +`ambientPositionProvider` from then on), and `start()` is idempotent regardless, so this +has no observable effect on recording — but it means the "no duplicate GPS consumer" +guarantee is a steady-state guarantee, not literally true for the first frame. Tests were +written to reflect this (see `test/widget_test.dart`'s "once a trip starts recording, the +shell stops watching ambient GPS" test, which starts idle, lets the ambient provider +engage, then starts a trip and asserts no further `start()` calls — rather than asserting +zero calls from t=0). + +Both risks flagged in the ticket (continuous background GPS while idle with the Map tab +mounted, and an earlier permission-dialog timing) were left unresolved as instructed — +no reference-counted shutdown or idle timeout was added. + +**Tests**: `flutter analyze` clean (same 4 pre-existing infos as baseline, no new +issues). `flutter test` green, 384 passing (up from 374 baseline; +10 new tests: 6 in +`test/ride_map_test.dart`'s new "ambient position (FB-01)" group covering +`RideMap`-level centering/chase/pan-cancel/marker-fallback behavior, and 4 in +`test/widget_test.dart` — two widget-level (`ShellScaffold` following an ambient fix +with no trip active; ambient watching stopping once a trip starts) and two +provider-level, driven directly against a `ProviderContainer` (never calls `stop()`; +`mapEnabledProvider` off never starts location and yields null)). diff --git a/lib/src/app/providers.dart b/lib/src/app/providers.dart index f75b7b3..49e7c8b 100644 --- a/lib/src/app/providers.dart +++ b/lib/src/app/providers.dart @@ -57,6 +57,30 @@ final locationSourceProvider = Provider((ref) { return source; }); +/// The device's current position when nothing is being recorded — drives the shared +/// background map's "look like Google Maps while idle" behavior. Deliberately NOT +/// gated through `recordingEngineProvider`/`RecordingEngine.start()` — this must work +/// whether or not a ride is ever recorded. Never calls `LocationSource.stop()`: the +/// underlying `locationSourceProvider` instance is shared with the recording engine, +/// and `stop()` is not reference-counted (see FB-01's ticket for why). +final ambientPositionProvider = StreamProvider.autoDispose((ref) async* { + if (!ref.watch(mapEnabledProvider)) { + yield null; + return; + } + final source = ref.watch(locationSourceProvider); + try { + await source.start(); // idempotent; safe even if a recording already started it + } on LocationException { + yield null; // permission denied / service disabled — ambient mode is best-effort + return; + } + yield* source.fixes.map((fix) => fix); + // No `stop()` call, ever, on dispose — see the doc comment above. Only this + // provider's own subscription to the broadcast `fixes` stream ends; the shared + // platform subscription is left exactly as it was. +}); + /// Loaded once at startup; null until then so nothing blocks the first frame. final configProvider = StateProvider((ref) => null); diff --git a/lib/src/ui/app_shell.dart b/lib/src/ui/app_shell.dart index 062a77a..7ff0f15 100644 --- a/lib/src/ui/app_shell.dart +++ b/lib/src/ui/app_shell.dart @@ -14,6 +14,7 @@ library; import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:go_router/go_router.dart'; +import 'package:latlong2/latlong.dart' as ll; import '../app/providers.dart'; import '../domain/models.dart'; @@ -70,6 +71,13 @@ class ShellScaffold extends ConsumerWidget { final segments = trip == null ? const [] : ref.watch(liveSegmentsProvider(trip.id)).valueOrNull ?? const []; + // FB-01: only watched while idle, so there's never a duplicate GPS consumer during + // an active recording -- the moment a trip starts, this stops being watched at all. + final ambientFix = + trip == null ? ref.watch(ambientPositionProvider).valueOrNull : null; + final ambientPosition = ambientFix == null + ? null + : ll.LatLng(ambientFix.latitude, ambientFix.longitude); return Scaffold( // Falls back to the ordinary theme background when the map is disabled -- the @@ -90,6 +98,7 @@ class ShellScaffold extends ConsumerWidget { key: const Key('shell-background-map'), points: points, segments: segments, + ambientPosition: ambientPosition, follow: isMapTab, fill: true, showEmptyLabel: false, diff --git a/lib/src/ui/components/ride_map.dart b/lib/src/ui/components/ride_map.dart index 6484aa3..5be48c8 100644 --- a/lib/src/ui/components/ride_map.dart +++ b/lib/src/ui/components/ride_map.dart @@ -47,6 +47,12 @@ const double maxTileZoom = 19.0; /// What a very short ride falls back to, so streets stay visible. const double shortRideZoom = 17.0; +/// Street-level zoom for the idle, no-recorded-points ambient position (FB-01). A +/// distinct name from [shortRideZoom] even though the value happens to match -- one +/// means "a very short recorded ride," the other means "no ride at all, just ambient +/// GPS." +const double ambientZoom = 17.0; + class RideMap extends StatefulWidget { const RideMap({ super.key, @@ -60,12 +66,19 @@ class RideMap extends StatefulWidget { this.skeletonMode = false, this.showLocationMarker = false, this.showAttribution = true, + this.ambientPosition, }); final List points; final List segments; final double height; + /// FB-01: the device's current GPS position when there are no recorded [points] to + /// chase -- drives the shared background map's "look like Google Maps while idle" + /// behaviour. Only meaningful when [points] is empty; a recorded path always takes + /// priority over ambient position. + final ll.LatLng? ambientPosition; + /// UI-02: true when `MapConnectivityState` has decided neither the offline cache nor /// the network can currently produce tiles. Swaps `TileLayer` for `SkeletonMapLayer` /// -- markers/polylines are unaffected, since those come from local data, not tiles. @@ -128,14 +141,24 @@ class _RideMapState extends State with WidgetsBindingObserver { @override void didUpdateWidget(RideMap old) { super.didUpdateWidget(old); - if (!_following || widget.points.isEmpty || _backgrounded) return; - final last = widget.points.last; - // A manual camera move already exists once the controller has been used; jumping - // straight to the latest fix keeps the map from ever being one flush behind. - WidgetsBinding.instance.addPostFrameCallback((_) { - if (!mounted || !_following) return; - _controller.move(ll.LatLng(last.latitude, last.longitude), _controller.camera.zoom); - }); + if (!_following || _backgrounded) return; + if (widget.points.isNotEmpty) { + final last = widget.points.last; + // A manual camera move already exists once the controller has been used; jumping + // straight to the latest fix keeps the map from ever being one flush behind. + 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) { + // FB-01: no recorded path yet -- chase the ambient GPS fix instead, the same way + // a recording is chased above. + WidgetsBinding.instance.addPostFrameCallback((_) { + if (!mounted || !_following) return; + _controller.move(widget.ambientPosition!, _controller.camera.zoom); + }); + } } @override @@ -229,10 +252,10 @@ class _RideMapState extends State with WidgetsBindingObserver { maxZoom: maxTileZoom, ), initialCenter: bounds == null - ? const ll.LatLng(0, 0) + ? (widget.ambientPosition ?? const ll.LatLng(0, 0)) : ll.LatLng(bounds.centerLat, bounds.centerLon), initialZoom: bounds == null - ? 2 + ? (widget.ambientPosition == null ? 2 : ambientZoom) : (bounds.isDegenerate ? shortRideZoom : maxTileZoom), maxZoom: maxTileZoom, interactionOptions: hasPoints @@ -272,15 +295,17 @@ class _RideMapState extends State with WidgetsBindingObserver { tileProvider: widget.tileProvider, ), PolylineLayer(polylines: polylines), - if (widget.showLocationMarker && hasPoints) + if (widget.showLocationMarker && (hasPoints || widget.ambientPosition != null)) MarkerLayer( markers: [ Marker( key: const Key('location-marker'), - point: ll.LatLng( - widget.points.last.latitude, - widget.points.last.longitude, - ), + point: hasPoints + ? ll.LatLng( + widget.points.last.latitude, + widget.points.last.longitude, + ) + : widget.ambientPosition!, width: 40, height: 40, child: const PulsingLocationMarker(), diff --git a/test/ride_map_test.dart b/test/ride_map_test.dart index 969b07e..46b8769 100644 --- a/test/ride_map_test.dart +++ b/test/ride_map_test.dart @@ -1,6 +1,7 @@ import 'package:flutter/material.dart'; import 'package:flutter_map/flutter_map.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:latlong2/latlong.dart' as ll; import 'package:rippr/src/domain/models.dart'; import 'package:rippr/src/ui/components/ride_map.dart'; import 'package:rippr/src/ui/components/skeleton_map_layer.dart'; @@ -181,4 +182,150 @@ void main() { 'just because the tile fetch is failing'); }); }); + + group('ambient position (FB-01)', () { + testWidgets('with no recorded points, an ambient position centers the map at ' + 'street level', (tester) async { + const fix = ll.LatLng(51.05, -114.05); + await tester.pumpWidget(MaterialApp( + theme: ripprTheme(), + home: const Scaffold( + body: RideMap( + points: [], + segments: [], + showEmptyLabel: false, + ambientPosition: fix, + ), + ), + )); + await tester.pump(); + + final map = tester.widget(find.byType(FlutterMap)); + expect(map.options.initialCenter, fix); + expect(map.options.initialZoom, ambientZoom); + }); + + testWidgets('with neither recorded points nor an ambient fix, the map still ' + 'falls back to (0, 0) at zoom 2 (no regression, no crash)', (tester) async { + await tester.pumpWidget(MaterialApp( + theme: ripprTheme(), + home: const Scaffold( + body: RideMap(points: [], segments: [], showEmptyLabel: false), + ), + )); + await tester.pump(); + + final map = tester.widget(find.byType(FlutterMap)); + expect(map.options.initialCenter, const ll.LatLng(0, 0)); + expect(map.options.initialZoom, 2); + }); + + testWidgets('while following, a new ambient fix re-centers the map, exactly ' + 'like the recorded-path chase', (tester) async { + const fix1 = ll.LatLng(51.0, -114.0); + const fix2 = ll.LatLng(51.01, -114.01); + + 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(); + + await tester.pumpWidget(build(fix2)); + await tester.pump(); + + final map = tester.widget(find.byType(FlutterMap)); + final center = map.mapController!.camera.center; + expect(center.latitude, closeTo(fix2.latitude, 1e-9)); + expect(center.longitude, closeTo(fix2.longitude, 1e-9)); + }); + + testWidgets('a manual pan cancels ambient following the same way it cancels ' + 'recording-follow', (tester) async { + const fix1 = ll.LatLng(51.0, -114.0); + const fix2 = ll.LatLng(51.01, -114.01); + + 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(); + + // Simulate a real user gesture the same way flutter_map itself would report + // one to `onPositionChanged` -- calling the callback directly with + // `hasGesture: true` exercises the exact guard in `_RideMapState` without + // needing a real pointer gesture to get past `InteractionOptions`. + var map = tester.widget(find.byType(FlutterMap)); + map.options.onPositionChanged!(map.mapController!.camera, true); + await tester.pump(); + + await tester.pumpWidget(build(fix2)); + await tester.pump(); + + map = tester.widget(find.byType(FlutterMap)); + final center = map.mapController!.camera.center; + expect(center.latitude, closeTo(fix1.latitude, 1e-9), + reason: 'following was cancelled by the manual pan; a later ambient fix ' + 'must not move the camera'); + expect(center.longitude, closeTo(fix1.longitude, 1e-9)); + }); + + testWidgets('the pulsing location marker falls back to the ambient position ' + 'when there are no recorded points', (tester) async { + const fix = ll.LatLng(51.0, -114.0); + await tester.pumpWidget(MaterialApp( + theme: ripprTheme(), + home: const Scaffold( + body: RideMap( + points: [], + segments: [], + showEmptyLabel: false, + showLocationMarker: true, + ambientPosition: fix, + ), + ), + )); + await tester.pump(); + + expect(find.byKey(const Key('location-marker')), findsOneWidget); + }); + + testWidgets('no marker is shown when there is neither a recorded point nor an ' + 'ambient position', (tester) async { + await tester.pumpWidget(MaterialApp( + theme: ripprTheme(), + home: const Scaffold( + body: RideMap( + points: [], + segments: [], + showEmptyLabel: false, + showLocationMarker: true, + ), + ), + )); + await tester.pump(); + + expect(find.byKey(const Key('location-marker')), findsNothing); + }); + }); } diff --git a/test/widget_test.dart b/test/widget_test.dart index dbf2efd..4ffa1a6 100644 --- a/test/widget_test.dart +++ b/test/widget_test.dart @@ -461,6 +461,85 @@ void main() { }); }); + group('ambient position (FB-01)', () { + screenTest('with no active trip, an ambient GPS fix drives the shared ' + 'background map', (tester) async { + await tester.pumpWidget(host( + ShellScaffold( + currentIndex: 0, + onDestinationSelected: (_) {}, + child: const SizedBox.shrink(), + ), + )); + // A frame to build the tree, then time for `ambientPositionProvider`'s + // `await source.start()` to resolve and subscribe to `source.fixes`. + await tester.pump(); + await tester.pump(const Duration(milliseconds: 50)); + + // No fix yet: still today's neutral fallback, no marker. + expect(find.byKey(const Key('location-marker')), findsNothing); + + source.emitAt(timestamp: 1000, latitude: 51.2, longitude: -114.2); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 50)); + + expect(find.byKey(const Key('location-marker')), findsOneWidget, + reason: 'an ambient fix drives the pulsing marker even with no trip ' + 'ever recorded'); + final map = tester.widget(find.byType(FlutterMap)); + final center = map.mapController!.camera.center; + expect(center.latitude, closeTo(51.2, 1e-9)); + expect(center.longitude, closeTo(-114.2, 1e-9)); + + // A second fix keeps the map following, exactly like the recorded-path chase. + source.emitAt(timestamp: 2000, latitude: 51.21, longitude: -114.21); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 50)); + final moved = tester.widget(find.byType(FlutterMap)); + final movedCenter = moved.mapController!.camera.center; + expect(movedCenter.latitude, closeTo(51.21, 1e-9)); + expect(movedCenter.longitude, closeTo(-114.21, 1e-9)); + }); + + screenTest('once a trip starts recording, the shell stops watching ambient ' + 'GPS -- no duplicate location consumer during a ride', (tester) async { + // Idle first: the ambient provider engages and calls start() at least once + // (activeTripProvider's own first, async emission of `null` briefly reads + // as "idle" too, which is expected -- same shape as `points`/`segments` + // falling back to empty lists until their streams first emit). + await tester.pumpWidget(host( + ShellScaffold( + currentIndex: 0, + onDestinationSelected: (_) {}, + child: const SizedBox.shrink(), + ), + )); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 50)); + source.emitAt(timestamp: 1000, latitude: 51.0, longitude: -114.0); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 50)); + + expect(find.byKey(const Key('location-marker')), findsOneWidget, + reason: 'sanity check: ambient mode is actually engaged before the ' + 'trip starts'); + final callsWhileIdle = source.startCalls; + expect(callsWhileIdle, greaterThanOrEqualTo(1)); + + // Now a trip starts recording. Nothing in this test ever taps Start, so the + // only thing that could call `LocationSource.start()` again is + // `ambientPositionProvider` -- and it must not, because `ShellScaffold` + // stops watching it entirely the moment `activeTripProvider` is non-null. + await repo.startTrip(2000); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 50)); + + expect(source.startCalls, callsWhileIdle, + reason: 'once a trip is active, ambientPositionProvider must no ' + 'longer be watched at all'); + }); + }); + group('trips list', () { screenTest('empty state explains what to do', (tester) async { await tester.pumpWidget(host(const TripsScreen())); @@ -829,4 +908,59 @@ void main() { expect(await repo.segmentsForTrip(h.tripId), hasLength(1)); }); }); + + group('ambientPositionProvider (FB-01)', () { + // Provider-level, not widget-level: exercises the provider directly against a + // `ProviderContainer` so its lifecycle (including disposal) can be driven + // precisely, per the ticket's own suggested test shape. + test('never calls LocationSource.stop() -- the underlying source is shared ' + 'with an active recording and stop() is not reference-counted', () async { + final fakeSource = FakeLocationSource(); + final container = ProviderContainer( + overrides: [locationSourceProvider.overrideWithValue(fakeSource)], + ); + addTearDown(container.dispose); + + final sub = container.listen(ambientPositionProvider, (_, _) {}); + // Let the provider's `await source.start()` resolve and subscribe. + await Future.delayed(Duration.zero); + expect(fakeSource.startCalls, 1); + + sub.close(); + container.dispose(); + + expect(fakeSource.stopCalls, 0, + reason: 'ambientPositionProvider must only ever call start(), never ' + 'stop(), on the shared LocationSource'); + await fakeSource.dispose(); + }); + + test('turning mapEnabledProvider off never starts location and yields null', + () async { + final fakeSource = FakeLocationSource(); + final container = ProviderContainer( + overrides: [ + locationSourceProvider.overrideWithValue(fakeSource), + mapEnabledProvider.overrideWith((ref) => false), + ], + ); + addTearDown(container.dispose); + + final values = >[]; + final sub = container.listen( + ambientPositionProvider, + (_, next) => values.add(next), + fireImmediately: true, + ); + await Future.delayed(Duration.zero); + + expect(fakeSource.startCalls, 0, + reason: 'no tile or location request should fire while the map is ' + 'disabled, matching the existing map-toggle guarantee'); + expect(values.last.valueOrNull, isNull); + + sub.close(); + await fakeSource.dispose(); + }); + }); }