diff --git a/docs/feedback/FB-01-live-street-level-map.md b/docs/feedback/FB-01-live-street-level-map.md new file mode 100644 index 0000000..af21e81 --- /dev/null +++ b/docs/feedback/FB-01-live-street-level-map.md @@ -0,0 +1,335 @@ +# FB-01 — Live, street-level map everywhere + +**Depends on** — · **Size** L · **Status** Not started + +## Goal +The map is too zoomed out on every screen, and the shared background map has zero +location awareness while idle — it should behave like Google/Apple Maps: zoomed in +enough to see the street you're on and the streets around it, and continuously tracking +the device's real position in real time, whether or not a ride is being recorded. + +## Context +Direct user feedback (`docs/FEEDBACK.md`, "All pages" section): + +> The map is too zoomed out. Start by having the map zoomed in enough where you could +> easily see what street the user is on and the streets around it. This goes for all +> screens that use the map, and even if the map is a background it should be updating +> real time to the person moving so if they aren't recording but they are riding in a +> car, it will update just like google maps. Essentially we want something that looks +> identical to google maps or apple maps. + +Today's behavior, exactly: + +`lib/src/ui/components/ride_map.dart`, `_RideMapState.build()` (~lines 200-237): + +```dart +geo.Bounds? bounds; +if (hasPoints) { + bounds = geo.bounds([ + for (final p in widget.points) geo.LatLon(p.latitude, p.longitude), + ]); +} +... +options: MapOptions( + initialCameraFit: (bounds == null || bounds.isDegenerate) + ? null + : CameraFit.bounds( + bounds: LatLngBounds(...), + padding: const EdgeInsets.all(24), + maxZoom: maxTileZoom, + ), + initialCenter: bounds == null + ? const ll.LatLng(0, 0) + : ll.LatLng(bounds.centerLat, bounds.centerLon), + initialZoom: bounds == null + ? 2 + : (bounds.isDegenerate ? shortRideZoom : maxTileZoom), + maxZoom: maxTileZoom, + ... +``` + +`bounds == null` happens whenever `widget.points` is empty — which is exactly the state +of the shared background map (`ShellScaffold` in `lib/src/ui/app_shell.dart`) any time +no trip is actively recording. In that state the map centers on **`(0, 0)` (Null +Island) at zoom 2** — the literal opposite of "zoomed in enough to see your street." + +The shared background map's data source, `ShellScaffold.build()` (`app_shell.dart` +~lines 59-72): + +```dart +final trip = ref.watch(activeTripProvider).valueOrNull; +final points = trip == null + ? const [] + : ref.watch(livePointsProvider(trip.id)).valueOrNull ?? const []; +final segments = trip == null + ? const [] + : ref.watch(liveSegmentsProvider(trip.id)).valueOrNull ?? const []; +``` + +`livePointsProvider`/`liveSegmentsProvider` are DB-backed streams of a trip's *stored* +points — they only ever produce data while `RecordingEngine` is actively writing to a +trip. There is no code path anywhere that feeds the background map a raw GPS position +independent of an active recording — confirmed by grep: `locationSourceProvider` +(`lib/src/app/providers.dart` ~line 54) is only ever watched by `recordingEngineProvider` +(~lines 79-89). **The background map is blind while idle**, which is the root cause of +both complaints at once (wrong zoom AND no live tracking) — there's simply no location +signal reaching it until a trip starts. + +`RideMap` already has exactly the chase-camera logic this needs, just scoped to +recorded points — `_RideMapState` (`ride_map.dart` ~lines 113-147): + +```dart +class _RideMapState extends State with WidgetsBindingObserver { + final _controller = MapController(); + late bool _following = widget.follow; + bool _backgrounded = false; + + @override + void didUpdateWidget(RideMap old) { + super.didUpdateWidget(old); + if (!_following || widget.points.isEmpty || _backgrounded) return; + final last = widget.points.last; + WidgetsBinding.instance.addPostFrameCallback((_) { + if (!mounted || !_following) return; + _controller.move(ll.LatLng(last.latitude, last.longitude), _controller.camera.zoom); + }); + } + ... +``` + +And the `onPositionChanged` cancel-on-manual-pan guard (`ride_map.dart` ~lines 243-252): + +```dart +onPositionChanged: !widget.follow + ? null + : (position, hasGesture) { + if (hasGesture && _following) { + setState(() => _following = false); + } + }, +``` + +This is the exact "chase the rider, but a real pan/pinch cancels it" behavior the +feedback is asking for — it just needs a second data source (ambient GPS) wired in for +when there are no recorded points to chase. + +**The shared `LocationSource` instance is process-wide — do not call `stop()` on it +from ambient code.** `lib/src/app/providers.dart`: + +```dart +final locationSourceProvider = Provider((ref) { + final source = GeolocatorLocationSource(); + ref.onDispose(source.dispose); + return source; +}); +``` + +One `GeolocatorLocationSource` for the whole app. Its `start()` is idempotent (safe to +call from two places — `lib/src/recording/geolocator_location_source.dart`: +`if (_subscription != null) return; // idempotent`), but its `stop()` is **not** +reference-counted — it unconditionally cancels the one underlying platform subscription: + +```dart +@override +Future stop() async { + await _subscription?.cancel(); + _subscription = null; +} +``` + +If ambient-mode code ever calls `locationSource.stop()` (e.g. from a provider's +`ref.onDispose`, or when the Map tab becomes invisible), and a real recording happens to +be in progress at that moment, **it would silently kill GPS delivery to the active +recording** — the engine has no way to know its location source was just stopped out +from under it by an unrelated consumer. This must not be possible. See Design below. + +## Design +- **New constant** in `lib/src/ui/components/ride_map.dart`, near `maxTileZoom`/ + `shortRideZoom`: `const double ambientZoom = 17.0;` — a distinct name from + `shortRideZoom` even though the value happens to match, since they mean different + things (one is "a very short recorded ride," the other is "no ride at all, just + ambient GPS"). +- **New provider**, in `lib/src/app/providers.dart`: + ```dart + /// 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. + }); + ``` + `LocationFix` and `LocationException` are both already imported/available via + `lib/src/recording/location_source.dart` (already imported in `providers.dart`). + `autoDispose` is correct here — this should stop listening the moment nothing watches + it (e.g. the app backgrounded, or a trip starts and `ShellScaffold` stops watching + this provider — see next bullet), same lifecycle discipline as every other + `StreamProvider.autoDispose` in this file. +- **`ShellScaffold.build()`** (`lib/src/ui/app_shell.dart`): only watch + `ambientPositionProvider` while idle, so it's never even subscribed during an active + recording: + ```dart + final trip = ref.watch(activeTripProvider).valueOrNull; + final ambientFix = trip == null ? ref.watch(ambientPositionProvider).valueOrNull : null; + final ambientPosition = ambientFix == null + ? null + : ll.LatLng(ambientFix.latitude, ambientFix.longitude); + ``` + (needs `import 'package:latlong2/latlong.dart' as ll;`) then pass `ambientPosition: + ambientPosition` into the `RideMap(...)` constructor call alongside the existing + `points`/`segments`/`follow`/etc. Leave `follow: isMapTab` and `showLocationMarker: + isMapTab` exactly as they are — ambient following should only run on the visible Map + tab, matching the existing recording-follow rationale already in that file's comments. +- **`RideMap`** (`lib/src/ui/components/ride_map.dart`): + - Add `final ll.LatLng? ambientPosition;` to the widget's fields (with a doc comment + explaining it's only meaningful when `points` is empty — a recorded path always + takes priority) and thread it through the constructor. + - In `build()`, change the `bounds == null` branch to use it: + ```dart + 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), + ``` + (`initialCenter`/`initialZoom` are read once at `FlutterMap` construction by + flutter_map — this only fixes the *first* placement; live tracking needs the + `didUpdateWidget` change below.) + - In `didUpdateWidget`, extend the chase logic to fall back to ambient position when + there are no recorded points: + ```dart + @override + 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) { + WidgetsBinding.instance.addPostFrameCallback((_) { + if (!mounted || !_following) return; + _controller.move(widget.ambientPosition!, _controller.camera.zoom); + }); + } + } + ``` + - `showLocationMarker`: today it only ever reads `widget.points.last` + (`MarkerLayer`/`PulsingLocationMarker` at ~lines 275-289). Add a fallback so the + pulsing marker also shows on ambient position: + ```dart + 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(), + ), + ], + ), + ``` +- **Permission prompt timing**: `LocationSource.start()` requests permission if not + already granted (`_ensurePermission` in `geolocator_location_source.dart`). This means + the very first time a user opens the app (or opens the Map tab) they may see a real OS + location-permission dialog before ever pressing Start — this is intentional per the + feedback ("even if they aren't recording... it will update just like Google Maps") and + matches how a real maps app behaves. A denial must degrade gracefully: catch + `LocationException` and yield `null` (already shown above) so the map simply falls + back to today's `(0,0)`/zoom-2 behavior rather than crashing or showing an error. + +## Implementation +1. Add `ambientZoom` constant to `ride_map.dart`. +2. Add `ambientPosition` field + constructor param to `RideMap`. +3. Update the `bounds == null` branch of `initialCenter`/`initialZoom`. +4. Update `didUpdateWidget` to chase `ambientPosition` when there are no recorded + points. +5. Update the `showLocationMarker` `MarkerLayer` to fall back to `ambientPosition`. +6. Add `ambientPositionProvider` to `lib/src/app/providers.dart`. +7. Wire it into `ShellScaffold.build()` in `app_shell.dart`, gated on `trip == null`. + +## Acceptance criteria +- [ ] With no active trip and no ambient fix yet available, the background map still + falls back to today's `(0,0)`/zoom-2 (no regression / no crash on first frame + before permission resolves). +- [ ] Once an ambient fix arrives (simulated via `FakeLocationSource.emit` in tests), + the background map centers on it at `ambientZoom` (street level) and continues to + re-center as new fixes arrive, exactly like the existing recorded-path chase + behavior. +- [ ] A manual pan/pinch on the Map tab cancels ambient following the same way it + cancels recording-follow today (reuses `_following`/`onPositionChanged` unchanged). +- [ ] The moment a trip starts recording, the background map switches to following the + trip's own recorded points (unchanged priority — `hasPoints` already wins in + `didUpdateWidget`), and `ShellScaffold` stops watching `ambientPositionProvider` + entirely (`trip == null` gate) so there's no duplicate GPS consumer during a ride. +- [ ] Turning `mapEnabledProvider` off stops the ambient GPS subscription (via the + provider's own `ref.watch(mapEnabledProvider)` short-circuit) — no tile or + location request fires while the map is disabled, matching the existing map-toggle + guarantee. +- [ ] `LocationSource.stop()` is never called by any code this ticket adds — grep the + diff to confirm. +- [ ] `flutter analyze` clean, `flutter test` green, test count only goes up. + +## Tests +- Widget test: `ShellScaffold` with `activeTripProvider` returning `null` and + `ambientPositionProvider` overridden/fed a fake `LocationFix` — assert the background + `RideMap`'s effective camera center/zoom reflects the ambient fix (read via + `MapController.camera` or by asserting the constructor args passed to `RideMap`, + whichever is more direct given the existing shell test patterns in + `test/widget_test.dart`'s `shell nav bar` group). +- Widget test: manually panning the map while an ambient fix is active stops further + auto-recentering on subsequent fixes (mirrors any existing recording-follow-cancel + test, if one exists — check `test/widget_test.dart`/`test/ride_map_test.dart`). +- Widget/provider test: `ambientPositionProvider` never calls `LocationSource.stop()` — + drive a `FakeLocationSource`, dispose the provider (e.g. via `container.dispose()` in + a `ProviderContainer`-based test), and assert `fakeSource.stopCalls == 0`. +- Widget test: `activeTripProvider` returning a non-null trip means the shell never + reads from `ambientPositionProvider` (e.g. assert no permission/`start()` call + happens when a trip is active and the fake source's `startCalls` was already + incremented by the recording path only). +- Existing `RideMap`/shell background tests continue to pass unmodified except where + they need a new `ambientPosition: null` default (should be a no-op given it's an + optional/nullable constructor param). + +## Risks +- **Battery**: `ambientPositionProvider` calling `start()` means GPS may run continuously + any time the Map tab (or the app in general, given the shell map is always mounted) + is open and the map is enabled, even with no ride ever recorded. This ticket + deliberately does not add a "stop after N minutes idle" or reference-counted shutdown + — that's a real product decision better made with actual battery data, not guessed at + here. Flag it in the Outcome section rather than solving it silently. +- **Permission timing**: as noted above, this may surface a permission dialog earlier + in the app's lifecycle than before (first Map-tab view rather than first Start press). + Confirmed intentional per the feedback; call out if it feels wrong in practice. + +## Out of scope +Route Planner's own map (it manages its own `FlutterMap` directly, not through +`RideMap`) — covered by FB-04, which should reuse `ambientZoom` from this ticket for +zoom-consistency but does not need ambient location following (Route Planner already +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. diff --git a/docs/feedback/FB-02-hide-idle-speed-panel.md b/docs/feedback/FB-02-hide-idle-speed-panel.md new file mode 100644 index 0000000..ae0dd6f --- /dev/null +++ b/docs/feedback/FB-02-hide-idle-speed-panel.md @@ -0,0 +1,193 @@ +# FB-02 — Hide the idle Speed panel until recording starts + +**Depends on** — · **Size** S · **Status** Not started + +## Goal +The Record screen's idle (not-recording) state currently shows a live "SPEED" readout +plus status text before the rider has pressed Start at all. It should show nothing but +the (live, per FB-01) map and the START control until a ride actually begins. + +## Context +Direct user feedback (`docs/FEEDBACK.md`, "Map Page" section): + +> Remove the "Speed" widget from the map screen when "start" hasn't been pressed yet. It +> blocks the entire screen, and you can't see the map at all. Speed and other widgets +> should only appear when recording starts. + +Today's idle branch — `lib/src/ui/record/record_screen.dart`, inside `build()`'s +`Stack` (~lines 196-223): + +```dart +child: Stack( + children: [ + if (ui.isIdle) + Center( + child: GlassPanel( + padding: const EdgeInsets.all(24), + child: Column( + mainAxisSize: MainAxisSize.min, + children: [ + // Current speed, not max: a max figure only moves when you beat + // it, which reads as a frozen screen while riding steadily. + // This is the 2.0.1 fix and it must not regress. + BigStat( + key: const Key('speed'), + label: 'SPEED', + value: speedValue, + unit: speedUnit, + scale: mountedMode ? mountedTextScale : 1.0, + ), + const SizedBox(height: 16), + // A resting state, not a zeroed ride -- otherwise the screen + // looks like a recording that is going nowhere. + const StatRow(label: 'Status', value: 'Ready'), + const StatRow(label: 'Last ride', value: 'See Rides'), + ], + ), + ), + ) + else + Positioned.fill( + bottom: _controlBarHeight(mountedMode), + child: HudEditOverlay( + metricBuilder: (context, metric) => + _HudMetricValue(metric: metric, ui: ui, units: units), + ), + ), + ... +``` + +`ui.isIdle` is `trip == null` (`RecordUiState.isIdle`, ~line 42). The `GlassPanel` sizes +to its content (`mainAxisSize: MainAxisSize.min`) so it isn't literally full-screen, but +it's centered, opaque-ish (`GlassPanel` is a near-opaque translucent fill per its own +doc comment elsewhere in this codebase), and shows a real live speed figure +(`_liveSpeed`, updated every GPS tick via `liveTelemetryProvider` — see `initState` +~lines 86-88) before any ride exists — which is exactly what the feedback is describing +as "blocking the screen" and confusing to see before pressing Start. + +## Design +Remove the entire `if (ui.isIdle) ... else ...` branching for HUD content — always +render the `HudEditOverlay` branch, and have the idle state simply show nothing there +(the HUD overlay area is empty when there's no trip, since there's nothing to edit/no +metrics to place — confirm `HudEditOverlay` degrades gracefully with an idle +`RecordUiState`, or gate it explicitly: only mount `HudEditOverlay` when `!ui.isIdle`, +and render an empty `SizedBox.shrink()` (or nothing at all) for the idle case, so the +full-bleed live map (per FB-01) is the entire idle screen behind the START control bar. + +Concretely: + +```dart +child: Stack( + children: [ + if (!ui.isIdle) + Positioned.fill( + bottom: _controlBarHeight(mountedMode), + child: HudEditOverlay( + metricBuilder: (context, metric) => + _HudMetricValue(metric: metric, ui: ui, units: units), + ), + ), + if (ui.isPaused) + ... +``` + +(i.e. drop the idle `Center(child: GlassPanel(...))` block entirely — no replacement +content, not even a "Ready" label. The now-live background map, per FB-01, is the idle +screen's entire content above the control bar.) + +`speedValue`/`speedUnit` (`formatSpeedParts(_liveSpeed, unit: units)`, ~line 176) become +unused once the idle panel is removed — check whether `_HudMetricValue`'s own +`HudMetric.speed` case (`formatSpeedParts(ui.currentSpeedKmh, unit: units).$1` at +~line 310) already computes this independently (it does — `ui.currentSpeedKmh` is +`_liveSpeed`), so the `final (speedValue, speedUnit) = ...` line in `build()` should +simply be deleted rather than left as dead code. Run `flutter analyze` to confirm no +other lingering unused-variable warnings from this removal. + +`BigStat`/`StatRow` imports in `record_screen.dart` (from `../components/stats.dart`) +may become unused if nothing else in this file still uses them — check before removing +the import; `confirmDialog` (also from `stats.dart`) is used elsewhere in this file for +the discard confirmation, so the import itself likely stays, just drop `BigStat`/ +`StatRow` specifically if nothing else references them. + +## Implementation +1. Remove the idle `Center(child: GlassPanel(...))` block from `record_screen.dart`'s + `build()`. +2. Gate `HudEditOverlay` on `!ui.isIdle` (it was previously the `else` branch of the + idle check — now it's the only branch, wrapped in its own `if`). +3. Delete the now-unused `final (speedValue, speedUnit) = formatSpeedParts(...)` line. +4. Run `flutter analyze` and remove any import that's now unused as a result. + +## Acceptance criteria +- [ ] Idle state (no active trip) shows no Speed/Status/"Last ride" content at all — + just the live background map and the control bar with only "START RECORDING" + visible (unchanged from today). +- [ ] The moment a trip starts recording, the full HUD (`HudEditOverlay` with the + default-visible metrics: Speed, Avg Speed, Distance, Elapsed Time) appears exactly + as it does today — this ticket only changes the idle state, not recording/paused. +- [ ] `flutter analyze` clean (no unused-variable/import warnings from the removal). +- [ ] `flutter test` green, test count only goes up (net effect after the test + migrations below should still be positive or flat, never negative). + +## Tests +Several existing tests assert against the idle Speed panel specifically and must be +migrated — not deleted — to seed an active recording and assert against the HUD's own +`Speed` widget instead, since the underlying regressions they guard (black-on-black +text, mounted-mode contrast/scale) are real risks that still apply once the same figure +lives inside `_HudMetricValue` instead of the idle `BigStat`. The HUD's per-metric +widgets are keyed `ValueKey(entry.key)` where `entry.key` is the `HudMetric` +(`lib/src/ui/components/hud_edit_overlay.dart` ~line 63) — use +`find.byKey(const ValueKey(HudMetric.speed))` to locate the speed widget in a recording +state, then descend into it the same way the old tests descended into `Key('speed')`. + +In `test/widget_test.dart`: + +- **`'idle shows Ready and only START'`** (~line 126): remove the + `expect(find.text('Ready'), findsOneWidget);` assertion (the "Ready"/"Status" text no + longer exists at all); keep the START/PAUSE/STOP key assertions and the + `find.text('Distance')` findsNothing check — both remain valid (this test's title may + want updating to `'idle shows only START'`). +- **`'the headline is live speed, not max speed'`** (~line 138): change to seed a + recording trip first (`await repo.startTrip(1000); await pumpLive(tester, + host(const RecordScreen(), map: false));` — see the identical pattern already used at + ~line 205-206 in `'tapping PAUSE and then STOP...'`), then assert + `find.text('SPEED')` findsOneWidget / `find.text('MAX SPEED')` findsNothing exactly as + before (Max Speed is not one of the four default-visible metrics — + `HudWidgetLayout.defaultFor`'s `visible: index < columns` with `columns = 4` and + `HudMetric.values` ordered `speed, avgSpeed, distance, elapsedTime, maxSpeed, ...` — + so this assertion still holds true in the recording state). +- **`'text is legible against the dark ground'`** (~line 235): seed a recording trip the + same way, then replace + `find.descendant(of: find.byKey(const Key('speed')), matching: find.text('0'))` with + `find.descendant(of: find.byKey(const ValueKey(HudMetric.speed)), matching: + find.text('0'))`. Same downstream assertions (`colour != ripprBackground`, + `colour != Colors.black`) unchanged. +- **`'the mounted theme is high-contrast and text scales up (V3-05)'`** (~line 380) and + **`'the mounted-mode speed digit is legible against GlassPanel's own translucent + surface...'`** (~line 400): both currently pump `RecordScreen()` idle with + `mountedMode: true` and inspect `Key('speed')`. Migrate both to seed a recording trip + first, then use `ValueKey(HudMetric.speed)` the same way. Confirm `_HudMetricValue`'s + value `Text` actually honors `mountedMode`'s scale/theme the same way the old idle + `BigStat` did (check `_HudMetricValue.build()` — if it currently has no mounted-mode + awareness at all, that's a **pre-existing gap this migration would newly expose**, not + something to paper over: if the mounted-mode font-size/contrast assertions fail + against the HUD widget, that's a real, separate bug worth calling out plainly in this + ticket's Outcome section rather than weakening the assertion to make it pass. +- **`'HUD widgets render over the map, not replacing it (UI-05)'`** (~line 219) already + seeds a recording trip and asserts `find.text('SPEED')`/`find.text('DISTANCE')` — no + change needed, but worth a quick sanity check that it still passes unmodified. + +## Risks +- The mounted-mode migration above may surface that `_HudMetricValue` never actually + had mounted-mode-aware styling (it was only ever exercised via the idle `BigStat` + before now) — if so, this ticket's Outcome section must say so explicitly rather than + silently deleting or weakening the assertion. Fixing that gap (if real) is reasonable + to fold into this ticket since it's directly caused by this change, but keep the fix + minimal (reuse whatever scale/theme mechanism `BigStat` already used) rather than + redesigning `_HudMetricValue`. + +## Out of scope +The HUD grid/drag-resize/text-auto-fit rework (FB-03) — this ticket only removes idle +content, it does not touch how the recording-state HUD widgets are laid out or sized. +The live-map-while-idle behavior itself (FB-01) — this ticket assumes it lands +separately; if FB-01 hasn't merged yet, the idle screen will just show today's +`(0,0)`/zoom-2 map, which is still a strict improvement over a blocking Speed panel. diff --git a/docs/feedback/FB-03-grid-snapped-hud-widgets.md b/docs/feedback/FB-03-grid-snapped-hud-widgets.md new file mode 100644 index 0000000..953debe --- /dev/null +++ b/docs/feedback/FB-03-grid-snapped-hud-widgets.md @@ -0,0 +1,603 @@ +# FB-03 — Grid-snapped HUD widgets with auto-fit, centered text + +**Depends on** — · **Size** L · **Status** Not started + +## Goal +Rework the customizable HUD telemetry widgets (built in UI-04) from free-form fractional +positioning into an Android-home-screen-style snapped grid — drag and resize stick to +grid cells, a placement that would overlap another widget is rejected rather than +silently stacking — and make each widget's label/value text scale to fill its own +current size instead of a fixed font that can overflow when small or look lost when +large. + +## Context +Direct user feedback (`docs/FEEDBACK.md`, "Map Page" section): + +> Speed and other widgets should only appear when recording starts. When toggling off +> different statistics, the flow and arrangement of the widgets goes crazy. This should +> be "drag and drop" but they stick to a grid much like the home screen on android, and +> they can be resizable like android widgets too. Make sure the font and values are +> centered in the widgets as well, and scale to the size of the widget (no over or +> underflow). + +(The "only appear when recording starts" half is FB-02, already scoped separately — +this ticket is the grid/drag/resize/text-fit half.) + +### Today's model — free-form fractions, no grid at all + +`lib/src/hud/hud_widget_layout.dart` (full file, 122 lines): + +```dart +const double hudMinWidthFraction = 0.20; +const double hudMaxWidthFraction = 0.70; +const double hudMinHeightFraction = 0.08; +const double hudMaxHeightFraction = 0.40; + +class HudWidgetLayout { + const HudWidgetLayout({ + required this.metric, + required this.x, + required this.y, + required this.width, + required this.height, + required this.visible, + }); + + final HudMetric metric; + final double x; // top-left, fraction of the HUD area + final double y; + final double width; + final double height; + final bool visible; + + HudWidgetLayout copyWith({double? x, double? y, double? width, double? height, bool? visible}) => ...; + + HudWidgetLayout clamped() { + final clampedWidth = width.clamp(hudMinWidthFraction, hudMaxWidthFraction); + final clampedHeight = height.clamp(hudMinHeightFraction, hudMaxHeightFraction); + final clampedX = x.clamp(0.0, 1.0 - clampedWidth); + final clampedY = y.clamp(0.0, 1.0 - clampedHeight); + return HudWidgetLayout(metric: metric, x: clampedX, y: clampedY, width: clampedWidth, height: clampedHeight, visible: visible); + } + + Map toJson() => {'x': x, 'y': y, 'width': width, 'height': height, 'visible': visible}; + + static HudWidgetLayout fromJson(HudMetric metric, Map json) { + try { + return HudWidgetLayout(metric: metric, x: (json['x'] as num).toDouble(), y: (json['y'] as num).toDouble(), + width: (json['width'] as num).toDouble(), height: (json['height'] as num).toDouble(), visible: json['visible'] as bool).clamped(); + } catch (_) { + return HudWidgetLayout.defaultFor(metric); + } + } + + factory HudWidgetLayout.defaultFor(HudMetric metric) { + const columns = 4; + const cellWidth = 0.22; + const cellHeight = 0.12; + const gap = 0.02; + final index = HudMetric.values.indexOf(metric); + final row = index ~/ columns; + final col = index % columns; + return HudWidgetLayout( + metric: metric, + x: 0.02 + col * (cellWidth + gap), + y: 0.06 + row * (cellHeight + gap), + width: cellWidth, + height: cellHeight, + visible: index < columns, // only the first 4 (Speed/Avg Speed/Dist/Time) start visible + ); + } +} +``` + +`HudMetric.values` order (`lib/src/hud/hud_metric.dart`, load-bearing — `defaultFor` uses +this indexing directly): `speed, avgSpeed, distance, elapsedTime, maxSpeed, movingTime, +elevationGain, pointsCaptured` (8 total; first 4 start visible). + +`lib/src/hud/hud_layout_controller.dart` (full file, 52 lines) — the in-memory, +authoritative layout during an edit session, persisted to `Config` only on exit: + +```dart +class HudLayoutController extends StateNotifier> { + HudLayoutController(this._config) + : super(_config?.hudLayout ?? {for (final m in HudMetric.values) m: HudWidgetLayout.defaultFor(m)}); + + final Config? _config; + + void updatePosition(HudMetric metric, double x, double y) { + final current = state[metric]; + if (current == null) return; + state = {...state, metric: current.copyWith(x: x, y: y).clamped()}; + } + + void updateSize(HudMetric metric, double width, double height) { + final current = state[metric]; + if (current == null) return; + state = {...state, metric: current.copyWith(width: width, height: height).clamped()}; + } + + void setVisible(HudMetric metric, bool visible) { + final current = state[metric] ?? HudWidgetLayout.defaultFor(metric); + state = {...state, metric: current.copyWith(visible: visible)}; + } + + Future persist() async => _config?.setHudLayout(state); +} +``` + +**Why "toggling widgets goes crazy" happens today**: `Config.hudLayout`'s getter +(`lib/src/config/config.dart` ~lines 95-113) always returns an entry for *every* +`HudMetric`, falling back to `HudWidgetLayout.defaultFor` for any metric never +explicitly saved — so `setVisible`'s `state[metric] ?? defaultFor(metric)` fallback is +essentially dead code; every metric already has a stored layout the moment the +controller exists. That stored layout is whatever it was the last time that metric was +visible (or its untouched `defaultFor` position if it's never been shown/moved). Nothing +about `setVisible`, `defaultFor`, or `clamped()` is aware of what other widgets +currently occupy — `clamped()` only keeps a single widget's own rectangle inside the +0.0–1.0 area, it has no concept of a sibling. So: drag widget A to overlap where widget +B's (currently hidden) default position sits, then re-enable B in Settings — B pops up +directly on top of A. Resize A larger (up to `hudMaxWidthFraction = 0.70`) and it can +swallow B's or C's default slot the same way. This reads as "the whole layout goes +crazy" even though, strictly, no *other* widget's own stored position ever actually +changes — the illusion is caused by newly-visible widgets landing on top of +already-visible ones with zero collision awareness. + +### Drag/resize gesture wiring (continuous, per-frame, into shared state) + +`lib/src/ui/components/draggable_resizable_hud_widget.dart` (`_DraggableResizableHudWidgetState.build`, +~lines 54-132) computes pixel position/size from `widget.layout`'s fractions × `widget.areaSize` +every build, and the drag/resize gesture handlers call the parent's callbacks **on every +frame of movement**, not just at gesture end: + +```dart +onLongPressMoveUpdate: (details) { + widget.onMoved( + layout.x + details.offsetFromOrigin.dx / widget.areaSize.width, + layout.y + details.offsetFromOrigin.dy / widget.areaSize.height, + ); +}, +... +onPanUpdate: (details) { // the resize handle + widget.onResized( + layout.width + details.delta.dx / widget.areaSize.width, + layout.height + details.delta.dy / widget.areaSize.height, + ); +}, +``` + +`onMoved`/`onResized` are wired straight to `HudLayoutController.updatePosition`/ +`updateSize` in `lib/src/ui/components/hud_edit_overlay.dart` (~lines 62-69): + +```dart +DraggableResizableHudWidget( + key: ValueKey(entry.key), + layout: entry.value, + areaSize: areaSize, + editing: _editing, + onMoved: (x, y) => notifier.updatePosition(entry.key, x, y), + onResized: (w, h) => notifier.updateSize(entry.key, w, h), + child: widget.metricBuilder(context, entry.key), +), +``` + +So every pixel of drag movement recomputes and commits a new fraction to the shared +`StateNotifier` (though `persist()` to `SharedPreferences` still only happens on exit — +that part is fine and unchanged). This continuous-commit model does not translate +directly to a grid: an integer `col`/`row` can't represent "40% of the way toward the +next cell," so this ticket changes drag/resize to track a **local, uncommitted pixel +offset** during the gesture and only calls the parent callback once, with final snapped +grid coordinates, at gesture end. See Design below. + +### Fixed-size text, no auto-fit + +`lib/src/ui/record/record_screen.dart`, `_HudMetricValue.build()` (~lines 321-346): + +```dart +@override +Widget build(BuildContext context) { + final colors = Theme.of(context).colorScheme; + return Column( + mainAxisSize: MainAxisSize.min, + children: [ + Text( + metric.label.toUpperCase(), + style: TextStyle(fontSize: 10, letterSpacing: 1, color: colors.onSurfaceVariant), + maxLines: 1, + overflow: TextOverflow.ellipsis, + ), + const SizedBox(height: 4), + Text( + _value, + style: monoDigits.copyWith(fontSize: 18, fontWeight: FontWeight.bold, color: _valueColor(colors)), + maxLines: 1, + overflow: TextOverflow.ellipsis, + ), + ], + ); +} +``` + +Font sizes are hardcoded regardless of the widget's actual current size — at the +smallest allowed size this can ellipsize; at the largest allowed size the same small +text looks lost in a big glass panel. `DraggableResizableHudWidget` just centers +whatever child it's given (`GlassPanel(child: Center(child: widget.child))`, +~line 64) — no text-scaling logic exists anywhere in this chain. + +### Persistence — no migration needed + +`HudWidgetLayout.fromJson`'s existing `try { ... } catch (_) { return +HudWidgetLayout.defaultFor(metric); }` already handles a field-shape change for free: an +old saved JSON blob has `x`/`y`/`width`/`height` keys (doubles); this ticket's new +`fromJson` reads `col`/`row`/`colSpan`/`rowSpan` (ints) and will throw on old data +(missing key, or a cast failure), falling back to `defaultFor` automatically — no schema +version, no explicit migration code. Confirmed reasonable: this is local +`SharedPreferences`, not a shared/synced format, and there's no production data to +preserve. + +## Design + +### Grid model +Fixed **4 columns × 8 rows** spanning the HUD area (matches the current 4-column +default-row assumption and gives ample vertical room for the 8-metric list). Replace +`HudWidgetLayout`'s `x`/`y`/`width`/`height` doubles with: + +```dart +const int hudGridColumns = 4; +const int hudGridRows = 8; +const int hudMinSpan = 1; +const int hudMaxColSpan = 4; +const int hudMaxRowSpan = 3; + +class HudWidgetLayout { + const HudWidgetLayout({ + required this.metric, + required this.col, + required this.row, + required this.colSpan, + required this.rowSpan, + required this.visible, + }); + + final HudMetric metric; + final int col; + final int row; + final int colSpan; + final int rowSpan; + final bool visible; + ... +} +``` + +- **`clampedToGrid()`** replaces `clamped()` — a *self-contained* bounds check with no + awareness of siblings (kept as its own method because `fromJson` and + `defaultFor`/`nextFreeSlot` all still need "is this rectangle even inside the grid" + independent of collision): + ```dart + HudWidgetLayout clampedToGrid() { + final clampedColSpan = colSpan.clamp(hudMinSpan, hudMaxColSpan); + final clampedRowSpan = rowSpan.clamp(hudMinSpan, hudMaxRowSpan); + final clampedCol = col.clamp(0, hudGridColumns - clampedColSpan); + final clampedRow = row.clamp(0, hudGridRows - clampedRowSpan); + return HudWidgetLayout(metric: metric, col: clampedCol, row: clampedRow, + colSpan: clampedColSpan, rowSpan: clampedRowSpan, visible: visible); + } + ``` +- **Collision helper**, a static/top-level function usable by both the layout file and + the controller: + ```dart + bool hudRectsOverlap(HudWidgetLayout a, HudWidgetLayout b) { + final aColEnd = a.col + a.colSpan; + final aRowEnd = a.row + a.rowSpan; + final bColEnd = b.col + b.colSpan; + final bRowEnd = b.row + b.rowSpan; + return a.col < bColEnd && aColEnd > b.col && a.row < bRowEnd && aRowEnd > b.row; + } + ``` +- **`defaultFor`/`nextFreeSlot`**: keep `defaultFor(metric)` for the *static* initial + layout (unchanged conceptually — still purely a function of the metric's own index, + no runtime state needed, since the 8 fixed default slots below never overlap each + other by construction): + ```dart + factory HudWidgetLayout.defaultFor(HudMetric metric) { + const columns = 4; + const rowSpan = 2; + final index = HudMetric.values.indexOf(metric); + final row = (index ~/ columns) * rowSpan; + final col = index % columns; + return HudWidgetLayout( + metric: metric, col: col, row: row, colSpan: 1, rowSpan: rowSpan, + visible: index < columns, + ); + } + ``` + (Speed/AvgSpeed/Distance/ElapsedTime → row 0, cols 0-3, visible; MaxSpeed/MovingTime/ + ElevationGain/PointsCaptured → row 2, cols 0-3, hidden — 8 distinct, non-overlapping + slots, same invariant the existing `defaultFor` test already checks.) + + Add a **new** function for the actual re-enable-without-collision fix: + ```dart + /// Scans row-major from (0,0) for the first colSpan×rowSpan slot that doesn't + /// overlap any `visible` entry in [occupied]. Falls back to (0,0) unconditionally + /// if the grid is fully packed -- a stacked default is better than a crash or an + /// exception the rider can't do anything about. + static HudWidgetLayout nextFreeSlot( + HudMetric metric, { + required Map occupied, + int colSpan = 1, + int rowSpan = 2, + }) { + final candidate = (int col, int row) => HudWidgetLayout( + metric: metric, col: col, row: row, colSpan: colSpan, rowSpan: rowSpan, visible: true, + ); + for (var row = 0; row <= hudGridRows - rowSpan; row++) { + for (var col = 0; col <= hudGridColumns - colSpan; col++) { + final c = candidate(col, row); + final overlapsAny = occupied.values + .where((l) => l.visible && l.metric != metric) + .any((other) => hudRectsOverlap(c, other)); + if (!overlapsAny) return c; + } + } + return candidate(0, 0); + } + ``` + +### Controller — collision-aware position/size updates, free-slot re-enable +`lib/src/hud/hud_layout_controller.dart`: + +```dart +void updatePosition(HudMetric metric, int col, int row) { + final current = state[metric]; + if (current == null) return; + final candidate = current.copyWith(col: col, row: row).clampedToGrid(); + if (_overlapsAnyOther(metric, candidate)) return; // reject: no-op, widget stays put + state = {...state, metric: candidate}; +} + +void updateSize(HudMetric metric, int colSpan, int rowSpan) { + final current = state[metric]; + if (current == null) return; + final candidate = current.copyWith(colSpan: colSpan, rowSpan: rowSpan).clampedToGrid(); + if (_overlapsAnyOther(metric, candidate)) return; + state = {...state, metric: candidate}; +} + +bool _overlapsAnyOther(HudMetric metric, HudWidgetLayout candidate) => state.values + .where((l) => l.visible && l.metric != metric) + .any((other) => hudRectsOverlap(candidate, other)); + +void setVisible(HudMetric metric, bool visible) { + var current = state[metric] ?? HudWidgetLayout.defaultFor(metric); + if (visible && _overlapsAnyOther(metric, current.copyWith(visible: true))) { + // The metric's own saved/default slot is now occupied by something else (the + // "goes crazy" bug this ticket exists to fix) -- find a genuinely free one + // instead of popping up on top of whatever's there. + current = HudWidgetLayout.nextFreeSlot( + metric, occupied: state, colSpan: current.colSpan, rowSpan: current.rowSpan, + ); + } + state = {...state, metric: current.copyWith(visible: visible)}; +} +``` + +**"Reject and no-op" is the collision rule for drag/resize** — simplest correct +behavior, and it's the same operation as "revert to last valid" here since `state` is +never mutated until the candidate passes the check (there's nothing to revert *from*, +the old value was never overwritten). A drag/resize that would overlap another visible +widget simply has no effect for that frame/gesture; the widget stays at its last valid +position. `setVisible` gets the one exception — it actively finds a free slot rather +than rejecting, since "the metric just doesn't turn on" would be a much worse user +experience than briefly landing somewhere else on the grid. + +### Drag/resize gesture — snap on gesture end, not live +`lib/src/ui/components/draggable_resizable_hud_widget.dart`: track a **local, transient +pixel offset** in `_DraggableResizableHudWidgetState` during the gesture (not committed +to the controller), paint the widget at `layout position (from grid) + local offset` +so the drag still feels smooth and continuous under the finger — then on +`onLongPressEnd`/`onPanEnd`, convert the final raw pixel position/size to the nearest +grid cell and call `widget.onMoved`/`onResized` exactly once with the snapped integer +coordinates, then reset the local offset to zero. + +```dart +class _DraggableResizableHudWidgetState extends State { + bool _grabbed = false; + Offset _dragOffset = Offset.zero; // pixels, uncommitted, live during a move gesture + Size _resizeDelta = Size.zero; // pixels, uncommitted, live during a resize gesture + + double get _cellWidth => widget.areaSize.width / hudGridColumns; + double get _cellHeight => widget.areaSize.height / hudGridRows; + + @override + Widget build(BuildContext context) { + final layout = widget.layout; + final left = layout.col * _cellWidth + _dragOffset.dx; + final top = layout.row * _cellHeight + _dragOffset.dy; + final width = layout.colSpan * _cellWidth + _resizeDelta.width; + final height = layout.rowSpan * _cellHeight + _resizeDelta.height; + ... + onLongPressMoveUpdate: (details) => setState(() => _dragOffset = details.offsetFromOrigin), + onLongPressEnd: (_) { + final snappedCol = ((layout.col * _cellWidth + _dragOffset.dx) / _cellWidth).round(); + final snappedRow = ((layout.row * _cellHeight + _dragOffset.dy) / _cellHeight).round(); + widget.onMoved(snappedCol, snappedRow); + setState(() { _grabbed = false; _dragOffset = Offset.zero; }); + }, + onLongPressCancel: () => setState(() { _grabbed = false; _dragOffset = Offset.zero; }), + ... + // resize handle: + onPanUpdate: (details) => setState(() => + _resizeDelta = Size(_resizeDelta.width + details.delta.dx, _resizeDelta.height + details.delta.dy)), + onPanEnd: (_) { + final snappedColSpan = ((layout.colSpan * _cellWidth + _resizeDelta.width) / _cellWidth).round(); + final snappedRowSpan = ((layout.rowSpan * _cellHeight + _resizeDelta.height) / _cellHeight).round(); + widget.onResized(snappedColSpan, snappedRowSpan); + setState(() => _resizeDelta = Size.zero); + }, +``` + +If the parent rejects the move/resize (collision), the widget simply rebuilds at its +unchanged `layout` — since `_dragOffset`/`_resizeDelta` are reset to zero right after +calling `onMoved`/`onResized` regardless of whether the controller accepted it, the +widget visually snaps back to wherever it actually ended up (its last accepted grid +cell) the instant the gesture ends. No separate "was it accepted?" callback needed. + +`widget.onMoved`/`onResized` function signatures change to `void Function(int col, int +row)` / `void Function(int colSpan, int rowSpan)`. + +`lib/src/ui/components/hud_edit_overlay.dart`: update the two callback wire-ups +(`onMoved: (col, row) => notifier.updatePosition(entry.key, col, row)`, `onResized: +(colSpan, rowSpan) => notifier.updateSize(entry.key, colSpan, rowSpan)`) — no other +change needed in this file; `areaSize` is still the raw pixel `Size` from its own +`LayoutBuilder`, cell-size division now happens inside +`DraggableResizableHudWidget` as shown above. + +### Text auto-fit — `FittedBox`, not a computed font size +`lib/src/ui/record/record_screen.dart`, `_HudMetricValue.build()`: wrap the existing +label+value `Column` in `FittedBox(fit: BoxFit.scaleDown)`, drop `maxLines`/ +`TextOverflow.ellipsis` on both `Text`s (structurally impossible to overflow once +`FittedBox` owns sizing), and keep the existing `fontSize: 10`/`18` values as the +"reference" size `FittedBox` scales down from — the ratio between label and value size +is preserved automatically as it scales: + +```dart +@override +Widget build(BuildContext context) { + final colors = Theme.of(context).colorScheme; + return FittedBox( + fit: BoxFit.scaleDown, + child: Column( + mainAxisSize: MainAxisSize.min, + children: [ + Text( + metric.label.toUpperCase(), + style: TextStyle(fontSize: 10, letterSpacing: 1, color: colors.onSurfaceVariant), + ), + const SizedBox(height: 4), + Text( + _value, + style: monoDigits.copyWith(fontSize: 18, fontWeight: FontWeight.bold, color: _valueColor(colors)), + ), + ], + ), + ); +} +``` + +`FittedBox` only scales *down* (`BoxFit.scaleDown` never enlarges past the reference +size) — so a widget resized to the grid's maximum span (4 cols × 3 rows) shows text at +its natural 10/18px reference size, not blown up to fill the space. That's an +intentional, reasonable simplification for this ticket (matching "no overflow" exactly +as asked; "looks proportionally larger in a bigger widget" is a nice-to-have, not named +in the feedback) — call this trade-off out plainly in the Outcome section rather than +silently under-delivering on it. + +`DraggableResizableHudWidget`'s own centering (`GlassPanel(child: Center(child: +widget.child))`) already satisfies "centered" — no change needed there. + +## Implementation +1. Rewrite `lib/src/hud/hud_widget_layout.dart`: grid constants, `col`/`row`/`colSpan`/ + `rowSpan` fields, `clampedToGrid()`, `hudRectsOverlap()`, updated `toJson`/ + `fromJson`, updated `defaultFor`, new `nextFreeSlot`. +2. Rewrite `lib/src/hud/hud_layout_controller.dart`: `updatePosition`/`updateSize` take + ints and reject-on-collision; `setVisible` uses `nextFreeSlot` when the metric's + current slot collides. +3. Rewrite `DraggableResizableHudWidget`: local pixel-offset drag state, snap-on-end, + updated callback signatures, pixel math from `areaSize / hudGridColumns` / + `hudGridRows`. +4. Update `HudEditOverlay`'s two callback closures to the new int signatures. +5. Wrap `_HudMetricValue`'s `Column` in `FittedBox(fit: BoxFit.scaleDown)`, drop + `maxLines`/`overflow` on its two `Text`s. +6. `flutter analyze`, fix any resulting type errors elsewhere that referenced the old + `x`/`y`/`width`/`height` fields (grep for `.x`/`.y`/`.width`/`.height` on + `HudWidgetLayout` instances across `lib/` and `test/` to be thorough). + +## Acceptance criteria +- [ ] Dragging a HUD widget in edit mode snaps to a grid cell on release; the widget + tracks the finger smoothly during the drag itself (no per-frame jump/snap while + still moving). +- [ ] Resizing via the handle snaps to whole grid cells on release, same smooth-during- + drag behavior. +- [ ] Dragging or resizing a widget onto a cell already occupied by another *visible* + widget rejects the move — the widget returns to its last valid position/size, + no crash, no silent overlap. +- [ ] Re-enabling a hidden metric whose saved/default slot is now occupied by another + visible widget places it in the next free grid cell instead of stacking on top of + the occupier — this is the concrete fix for the "goes crazy" feedback. +- [ ] Re-enabling a hidden metric whose slot is still free keeps its exact prior + position (unchanged from today's guarantee, still worth re-verifying under the + new model). +- [ ] HUD widget text (label + value) never overflows or gets ellipsized at any allowed + grid size, and stays visually centered within its widget, at both the smallest + (`1×1`) and largest (`hudMaxColSpan × hudMaxRowSpan`) allowed sizes. +- [ ] An old-shaped saved layout (pre-this-ticket JSON with `x`/`y`/`width`/`height` + keys) loads without crashing — falls back to `defaultFor` per metric, exactly as + `fromJson`'s existing try/catch already guarantees. +- [ ] `flutter analyze` clean, `flutter test` green, test count only goes up. + +## Tests +`test/hud_widget_layout_test.dart` needs a full rewrite for the new field names/types — +keep the same test *intents*, translated to grid coordinates: +- JSON round trip (encode/decode a `col`/`row`/`colSpan`/`rowSpan`/`visible` layout + exactly). +- A malformed/missing-field JSON entry falls back to `defaultFor` (unchanged intent, + new field names in the malformed input). +- `defaultFor`: every metric gets a distinct (non-overlapping, via `hudRectsOverlap`) + default rect; the first four metrics start visible, the rest hidden (unchanged + intent). +- `clampedToGrid`: a position/span pushed outside `[0, hudGridColumns)`/ + `[0, hudGridRows)` or `[hudMinSpan, hudMaxColSpan/RowSpan]` is corrected back inside + (parallel structure to the old `clamped` tests: past the right/bottom edge, past the + left/top edge, below the legibility floor, above the ceiling, shrink-before-reposition, + already-valid-is-unchanged). +- New: `hudRectsOverlap` — two identical rects overlap; two rects sharing only an edge + (touching, not overlapping) do not; two disjoint rects don't; a rect fully containing + another does. +- New: `nextFreeSlot` — given a set of `occupied` visible layouts that collide with a + metric's own `defaultFor` position, returns some other rect that doesn't collide with + any of them; given an empty/all-hidden `occupied` map, returns the same rect + `defaultFor` would have (or at least a valid non-colliding one at `(0,0)`). + +`test/hud_layout_controller_test.dart` needs a parallel rewrite for the new int +signatures and the new collision behavior: +- `updatePosition`/`updateSize` update only the given metric, still true. +- New: `updatePosition` to a cell already occupied by another visible metric is a + no-op (state for the target metric is unchanged). +- New: `updateSize` that would make a widget overlap a sibling is a no-op. +- `setVisible(false)` then `setVisible(true)` restores the last position **when that + position is still free** (keep this test, using coordinates guaranteed not to + collide with anything else default-visible). +- New: `setVisible(true)` when the metric's last position now collides with another + visible widget places it somewhere else instead (assert the resulting `state[metric]` + doesn't overlap anything, not a specific coordinate). +- `persist`/pre-persist-not-visible-to-Config tests carry over unchanged in spirit + (just using int fields). + +Widget-level: extend or add to `test/hud_edit_overlay_test.dart` — a drag gesture ending +over an occupied cell leaves the dragged widget's rendered position unchanged from +before the gesture; a resize past the point of colliding with a sibling leaves the +widget at its pre-gesture size. Also add a widget test putting `_HudMetricValue` (or a +representative HUD child) inside a very small `DraggableResizableHudWidget` (1×1 grid +cell) and a very large one (`hudMaxColSpan`×`hudMaxRowSpan`) and asserting no +`RenderFlex overflowed` exception/error is recorded by `FlutterError.onError` during +the pump (the standard way to assert "no overflow" in a widget test — check +`test/widget_test.dart`/other existing tests in this repo for the established pattern, +if any, otherwise use `tester.takeException()` after pumping and assert it's `null`). + +## Risks +- `FittedBox(fit: BoxFit.scaleDown)` never enlarges text past its reference size, so a + widget resized to the grid's maximum span shows the same 10/18px reference text as a + 1×1 widget, just with more empty space around it — not larger text filling the space. + This satisfies "no overflow" and "centered" exactly as asked; "text should grow to + fill a bigger widget" is not explicitly requested and is treated as future work, not a + silent gap — document this trade-off in the Outcome section. +- Existing saved HUD layouts (from anyone who ran a build before this ticket) reset to + defaults the first time they're loaded post-update, per the fromJson fallback. No + user-facing warning is added for this — acceptable given there's no real user base + yet; flag it anyway in Outcome for completeness. + +## Out of scope +FB-02 (hiding the idle Speed panel — separate ticket, this one only touches the +recording-state HUD widgets themselves). Making widget text grow to actually fill a +larger grid span (see Risks). Any change to which metrics exist or their default +visibility set — `HudMetric`'s own enum and `label`s are untouched. diff --git a/docs/feedback/FB-04-route-planner-map-render-fix.md b/docs/feedback/FB-04-route-planner-map-render-fix.md new file mode 100644 index 0000000..4f731ab --- /dev/null +++ b/docs/feedback/FB-04-route-planner-map-render-fix.md @@ -0,0 +1,216 @@ +# FB-04 — Fix Route Planner's map failing to render + match street-level zoom + +**Depends on** FB-01 (reuses its `ambientZoom` constant) · **Size** M · **Status** Not started + +## Goal +The Route Planner screen's map doesn't render at all for the user. Find the real cause +and fix it, and bring this screen's zoom in line with the rest of the app's new +street-level default (FB-01). + +## Context +Direct user feedback (`docs/FEEDBACK.md`, "Route" section): + +> This doesn't work at all, the map doesn't render at all... I want it to be the same +> zoomed in map view, then the user can zoom out and create the route. + +### High-confidence root cause + +`lib/src/ui/routes/route_planner_screen.dart`, `build()` (~lines 112-139): + +```dart +@override +Widget build(BuildContext context) { + final colors = Theme.of(context).colorScheme; + final route = ref.watch(routePlanProvider(widget.routeId)).valueOrNull; + final waypointsAsync = ref.watch(routeWaypointsProvider(widget.routeId)); + final waypoints = waypointsAsync.valueOrNull ?? const []; + final units = ref.watch(unitSystemProvider); + final repo = ref.read(routePlanRepositoryProvider); + + if (route == null) { + return Scaffold( + backgroundColor: Colors.transparent, + body: SafeArea( + child: Center( + child: Text( + 'This route no longer exists.', + style: TextStyle(color: colors.outline), + ), + ), + ), + ); + } + + return Scaffold( + backgroundColor: Colors.transparent, + body: SafeArea( + child: !waypointsAsync.hasValue + ? const Center(child: CircularProgressIndicator()) + : Stack( + children: [ + Positioned.fill( + child: FlutterMap( + ... +``` + +Both `route` and `waypointsAsync` come from the same kind of provider — +`routePlanProvider`/`routeWaypointsProvider` are both `StreamProvider.autoDispose.family` +(`lib/src/app/providers.dart` ~lines 198-203) — so both start in `AsyncLoading` on a +fresh navigation (e.g. tapping "+ New route" on `RoutesListScreen`, which calls +`repo.createRoutePlan` then immediately `onOpenRoute?.call(id)` — a brand-new +`routePlanProvider(id)` family instance with a DB stream that hasn't delivered its +first row yet). **`waypointsAsync` is correctly guarded** — `!waypointsAsync.hasValue` +shows a spinner instead of building `FlutterMap` prematurely, with a comment explaining +exactly why (`initialCenter`/`initialZoom` are read once at construction; building too +early freezes the camera at null-island forever). **`route` has no equivalent guard** — +`.valueOrNull` collapses "still loading" and "genuinely doesn't exist" into the same +`null`, so on that same cold navigation, this screen renders the **text-only "This route +no longer exists." fallback with zero `FlutterMap` in the tree** — for a route that is +completely valid, simply because its stream's first emission hasn't arrived yet. This is +a real, reproducible-on-a-slow-cold-start bug, and it would read to a user as exactly +"the map doesn't render at all" — because for however long that loading window lasts, +there is no map, no pins, nothing to interact with. + +This is the same shape of bug UI-06's own documented Outcome already found once in this +same file (a hardcoded, stale tile URL in `_DownloadDialog`): a place where near- +identical logic exists twice nearby and only one copy got the correct guard. + +### Zoom special-cases to replace + +Same `build()` method, ~line 157: + +```dart +initialZoom: waypoints.length <= 1 ? 14 : maxTileZoom - 3, +``` + +`initialZoom` is only ever used by flutter_map when `initialCameraFit` is null. +`initialCameraFit: _initialFit(waypoints)` (line ~153) is non-null for 2+ waypoints +(see `_initialFit`, ~lines 92-102, which returns `null` only for `waypoints.length < 2` +or degenerate bounds) — so `maxTileZoom - 3` is **dead, misleading code today**: it can +only ever apply when `_initialFit` already returned null, which for 2+ waypoints only +happens on a degenerate (single-point) bounds, in which case `waypoints.length <= 1` is +false but the fit is still null — meaning `maxTileZoom - 3` actually *does* fire for +that one edge case (all waypoints at the same spot). Still, `14` for 0-1 waypoints is +the literal reason a fresh, empty route (or one with a single pin) opens zoomed out +far past street level, exactly matching the "same zoomed in map view" ask. + +## Design +- **Fix the render bug**: change `route` from `.valueOrNull` to keeping the full + `AsyncValue`, and add a `hasValue` guard mirroring `waypointsAsync`'s + existing pattern: + ```dart + final routeAsync = ref.watch(routePlanProvider(widget.routeId)); + final waypointsAsync = ref.watch(routeWaypointsProvider(widget.routeId)); + final waypoints = waypointsAsync.valueOrNull ?? const []; + final units = ref.watch(unitSystemProvider); + final repo = ref.read(routePlanRepositoryProvider); + + if (!routeAsync.hasValue) { + return const Scaffold( + backgroundColor: Colors.transparent, + body: Center(child: CircularProgressIndicator()), + ); + } + final route = routeAsync.value; + if (route == null) { + return Scaffold( /* unchanged "This route no longer exists." body */ ); + } + ``` + This exactly mirrors the two-step pattern `waypointsAsync` already uses two lines + below it in the same method, just applied consistently to both providers. +- **Zoom fix**: replace the `initialZoom` line with FB-01's shared constant. FB-01 adds + `const double ambientZoom = 17.0;` to `lib/src/ui/components/ride_map.dart` — this + screen already imports named constants from that file (`show TileAttribution, + maxTileZoom, tileMaxNativeZoom, tileSubdomains, tileUrlTemplate, tileUserAgent`, near + the top of the file) — add `ambientZoom` to that same `show` clause and use it: + ```dart + initialZoom: waypoints.length <= 1 ? ambientZoom : maxTileZoom - 3, + ``` + **If FB-01 has not landed yet when this ticket is implemented**, add the same + constant directly to `ride_map.dart` yourself first (`const double ambientZoom = + 17.0;` near `maxTileZoom`/`shortRideZoom`) so this ticket doesn't block on ordering — + whichever of FB-01/FB-04 lands second will find the constant already defined and + should just reuse it rather than redefining it (grep for `ambientZoom` before adding + it, to avoid a duplicate-constant compile error). +- **Investigation order, if the above turns out not to be the whole story** (do this + first, before assuming the fix above is sufficient — reproduce, then fix): + 1. `flutter run` on the Android emulator (or a device), tap "+ New route" from + `RoutesListScreen` (`lib/src/ui/routes/routes_list_screen.dart`, `Key('new-route')` + button), watch closely for a flash of "This route no longer exists." text or a + fully blank screen right after navigation. + 2. If reproduced and the `hasValue` fix above resolves it, done — write up the root + cause and fix in the Outcome section. + 3. If it's *not* resolved (map still doesn't render even with the `hasValue` guard in + place), add a temporary `debugPrint('route=${routeAsync.runtimeType} ' + 'waypoints=${waypointsAsync.runtimeType}')` at the top of `build()` and watch the + console across a fresh navigation to see the actual state sequence — remove it + before committing. + 4. Check whether the map mounts (i.e. `FlutterMap` is in the tree, visible in the + Flutter inspector / a screenshot) but tiles themselves are blank — if so, look at + `cachedTileProviderProvider` (`lib/src/app/providers.dart` ~lines 234-238): it + returns `null` while `tileCacheProvider`'s underlying `FutureProvider` hasn't + resolved yet, and `TileLayer(tileProvider: null)` falls back to flutter_map's own + default network fetcher — verify that fallback actually fetches (it should; if it + silently doesn't, that's the bug). + 5. Diff this screen's tile setup token-for-token against `RideMap`'s + (`tileUrlTemplate`, `tileSubdomains`, `tileMaxNativeZoom`, `tileUserAgent` — all + already imported from the same `ride_map.dart` export, so a stale-URL-style + regression is unlikely, but `grep -n "tile.openstreetmap\|cartocdn"` across this + file to be certain nothing reintroduced UI-06's already-fixed bug). + +## Implementation +1. Change `route` to `routeAsync` (full `AsyncValue`), add the `hasValue` guard, keep + the rest of the "no longer exists" branch's body unchanged. +2. Add `ambientZoom` to this file's `ride_map.dart` import `show` clause (adding the + constant to `ride_map.dart` first if FB-01 hasn't landed yet — see Design). +3. Replace `waypoints.length <= 1 ? 14 : maxTileZoom - 3` with + `waypoints.length <= 1 ? ambientZoom : maxTileZoom - 3`. +4. Reproduce on an emulator per the Investigation order above; if the root cause is + something other than the `hasValue` gap, document the real cause and its fix in the + Outcome section instead of (or in addition to) the above. + +## Acceptance criteria +- [ ] Opening a brand-new route ("+ New route" from `RoutesListScreen`) always shows a + loading spinner briefly (if the stream hasn't emitted yet) and then the real map + canvas — never the "This route no longer exists." text for a route that + genuinely exists. +- [ ] Opening an existing, previously-created route with no waypoints shows the map + canvas at `ambientZoom` (street level), not zoom 14. +- [ ] A route that is actually deleted (or never existed — e.g. a stale/invalid id) + still correctly shows "This route no longer exists." — the fix must not weaken + this case, only stop it from firing on a merely-still-loading valid route. +- [ ] `flutter analyze` clean, `flutter test` green, test count only goes up. + +## Tests +- Widget test: pump `RoutePlannerScreen` for a route id that has a pending (not-yet- + resolved) `routePlanProvider` stream — assert a `CircularProgressIndicator` shows, + never the "no longer exists" text, then once the stream emits the real row, assert + the map (`FlutterMap`) appears. (Check how existing tests in + `test/route_planner_screen_test.dart` seed/await the repository — likely via + `repo.createRoutePlan` then pumping a frame or two before the provider's stream + delivers, similar to the existing `pumpMap` helper in that file's own comments about + needing a handful of frames for the waypoints stream's first value.) +- Widget test: pump `RoutePlannerScreen` for an id that was never created (or was + deleted) — assert "This route no longer exists." still shows once the stream settles + (not immediately, if the provider briefly reports loading first). +- Widget test: a route with zero waypoints renders at `ambientZoom`, not `14` — read + the `FlutterMap`'s `options.initialZoom` via `tester.widget(...)` the same + way other zoom-related assertions in this test suite already inspect `MapOptions` + (check existing patterns in `test/ride_map_test.dart`/ + `test/route_planner_screen_test.dart`). +- If the investigation reveals a different root cause than the `hasValue` gap, write a + regression test for the actual bug found, not just the hypothesis above. + +## Risks +- If the `hasValue` fix doesn't fully explain what the user saw, this ticket's + Outcome section must say so plainly and document whatever the actual root cause + turned out to be — don't claim a fix that wasn't verified to address the real + symptom. Reproducing on a real emulator/device before declaring this done is + important precisely because the original bug report is "doesn't work at all," a + strong signal, and a subtle timing bug is an easy thing to fix on paper without + confirming it was actually the cause. + +## Out of scope +Closed-loop routes (FB-05, sequenced after this ticket since it also edits this file +heavily). Turn-by-turn route following (V3-09, already explicitly deferred by UI-06). +Any change to `RoutesListScreen` itself. diff --git a/docs/feedback/FB-05-closed-loop-routes.md b/docs/feedback/FB-05-closed-loop-routes.md new file mode 100644 index 0000000..ed9504e --- /dev/null +++ b/docs/feedback/FB-05-closed-loop-routes.md @@ -0,0 +1,411 @@ +# FB-05 — Closed-loop routes in Route Planner + +**Depends on** FB-04 (same file, heavily edited — sequence after it merges) · **Size** M · **Status** Not started + +## Goal +Route planning currently only supports a one-way A→B→C path. Add the ability to close +the route into a loop that returns to its starting pin, updating the drawn polyline and +the distance stat to include the closing segment. + +## Context +Direct user feedback (`docs/FEEDBACK.md`, "Route" section): + +> ...there is no way to create a loop with the pins, it only creates a unidirectional +> path. + +### Data model — no loop concept exists today + +`lib/src/domain/models.dart`, `RoutePlan` (~lines 229-276): + +```dart +class RoutePlan { + const RoutePlan({ + this.id = 0, + required this.name, + required this.createdAt, + this.activity = Activity.motorcycle, + this.distanceM = 0.0, + this.estimatedMillis, + this.geometry, + }); + + final int id; + final String name; + final int createdAt; + final Activity activity; + final double distanceM; + final int? estimatedMillis; + final String? geometry; + + RoutePlan copyWith({ + int? id, String? name, int? createdAt, Activity? activity, + double? distanceM, int? estimatedMillis, String? geometry, + }) => RoutePlan( + id: id ?? this.id, name: name ?? this.name, createdAt: createdAt ?? this.createdAt, + activity: activity ?? this.activity, distanceM: distanceM ?? this.distanceM, + estimatedMillis: estimatedMillis ?? this.estimatedMillis, geometry: geometry ?? this.geometry, + ); +} +``` + +`Waypoint` (~lines 278-309) has an `ordinal` for ordering, and waypoints are only ever +appended at the end (`RoutePlanRepository.addWaypoint`, below) — there is no "return to +start" field or concept anywhere in the model. + +### Polyline and distance — strictly open, no wraparound + +`lib/src/ui/routes/route_planner_screen.dart`, the `PolylineLayer` (~lines 182-201): + +```dart +if (waypoints.length >= 2) + PolylineLayer( + polylines: [ + Polyline( + points: [ + for (final w in waypoints) ll.LatLng(w.latitude, w.longitude), + ], + strokeWidth: 4, + pattern: StrokePattern.dashed(segments: const [8, 6]), + color: colors.primary, + ), + ], + ), +``` + +`lib/src/data/route_plan_repository.dart`, distance recomputation (full relevant +section): + +```dart +Future addWaypoint(int routeId, double latitude, double longitude) async { + final existing = await _db.waypointsForRoute(routeId); + await _db.insertWaypoint( + Waypoint(routeId: routeId, ordinal: existing.length, latitude: latitude, longitude: longitude), + ); + await _recomputeDistance(routeId); +} + +Future _recomputeDistance(int routeId) async { + final waypoints = await _db.waypointsForRoute(routeId); + final distance = geo.pathLengthMeters([ + for (final w in waypoints) geo.LatLon(w.latitude, w.longitude), + ]); + await _db.setRoutePlanDistance(routeId, distance); +} +``` + +`geo.pathLengthMeters` (`lib/src/geo/geo.dart` ~line 185) sums consecutive-pair +distances generically over whatever point list it's given — it needs no changes itself, +just a point list that includes the closing segment when appropriate. + +`FloatingPill`'s Distance/Est. Time/Pins stats read straight off `route.distanceM`/ +`route.estimatedMillis`/`waypoints.length` (`route_planner_screen.dart` ~lines 280-292) +— Distance updates for free once `_recomputeDistance` accounts for the closing segment; +Pins and Est. Time are unaffected by this ticket. + +### Schema — current version and migration pattern to follow + +`lib/src/data/database.dart`: + +```dart +@DataClassName('RoutePlanRow') +class RoutePlans extends Table { + @override + String get tableName => 'route_plans'; + + IntColumn get id => integer().autoIncrement()(); + TextColumn get name => text()(); + IntColumn get createdAt => integer()(); + TextColumn get activity => textEnum().withDefault(const Constant('motorcycle'))(); + RealColumn get distanceM => real().withDefault(const Constant(0))(); + IntColumn get estimatedMillis => integer().nullable()(); + TextColumn get geometry => text().nullable()(); +} + +@DriftDatabase(tables: [Trips, Segments, TrackPoints, RoutePlans, Waypoints]) +class AppDatabase extends _$AppDatabase { + @override + int get schemaVersion => 3; + + @override + MigrationStrategy get migration => MigrationStrategy( + onCreate: (m) => m.createAll(), + onUpgrade: (m, from, to) async { + if (from < 2) { + await m.addColumn(trips, trips.activity); + } + if (from < 3) { + await m.createTable(routePlans); + await m.createTable(waypoints); + } + }, + ... +``` + +The existing `if (from < 2) { await m.addColumn(trips, trips.activity); }` is the exact +pattern to follow for a new nullable/defaulted column. + +`RoutePlanRepository` insert/conversion glue, in `database.dart`: + +```dart +Future insertRoutePlan(domain.RoutePlan route) => into(routePlans).insert( + RoutePlansCompanion.insert( + name: route.name, createdAt: route.createdAt, activity: Value(route.activity), + distanceM: Value(route.distanceM), estimatedMillis: Value(route.estimatedMillis), + geometry: Value(route.geometry), + ), +); + +Future setRoutePlanDistance(int id, double distanceM) => (update(routePlans) + ..where((r) => r.id.equals(id))).write(RoutePlansCompanion(distanceM: Value(distanceM))); + +domain.RoutePlan _toRoutePlan(RoutePlanRow r) => domain.RoutePlan( + id: r.id, name: r.name, createdAt: r.createdAt, activity: r.activity, + distanceM: r.distanceM, estimatedMillis: r.estimatedMillis, geometry: r.geometry, +); +``` + +### Overflow menu — where the toggle belongs + +`route_planner_screen.dart`, `_OverflowMenu` (~lines 493-530-ish) already hosts +Rename/Download/Delete as a `PopupMenuButton` inside a `GlassPanel`: + +```dart +class _OverflowMenu extends StatelessWidget { + const _OverflowMenu({ + required this.hasWaypoints, required this.onRename, required this.onDownload, required this.onDelete, + }); + + final bool hasWaypoints; + final VoidCallback onRename; + final VoidCallback? onDownload; + final VoidCallback onDelete; + + @override + Widget build(BuildContext context) => GlassPanel( + borderRadius: const BorderRadius.all(Radius.circular(999)), + child: PopupMenuButton( + key: const Key('route-overflow-menu'), + icon: const Icon(Icons.more_vert), + onSelected: (value) => switch (value) { + 'rename' => onRename(), + 'download' => onDownload?.call(), + 'delete' => onDelete(), + _ => null, + }, + itemBuilder: (context) => [ + const PopupMenuItem(key: Key('rename-route'), value: 'rename', child: Text('Rename')), + ... +``` + +And it's constructed at the call site (~lines 298-310): + +```dart +_OverflowMenu( + hasWaypoints: waypoints.isNotEmpty, + onRename: () => _rename(context, repo, route), + onDownload: waypoints.isEmpty ? null : () => _downloadOfflineTiles(context, waypoints), + onDelete: () async { + await repo.deleteRoutePlan(widget.routeId); + widget.onBack?.call(); + }, +), +``` + +## Design +- **Schema**: add `BoolColumn get isClosedLoop => boolean().withDefault(const + Constant(false))();` to `RoutePlans` in `database.dart`. Bump `schemaVersion` from + `3` to `4`; add `if (from < 4) { await m.addColumn(routePlans, routePlans.isClosedLoop); }` + to `onUpgrade`. +- **Domain model**: add `final bool isClosedLoop;` to `RoutePlan` (default `false` in + the constructor), thread it through `copyWith`. +- **DB glue**: add `isClosedLoop: Value(route.isClosedLoop)` to `insertRoutePlan`'s + `RoutePlansCompanion.insert(...)` call, `isClosedLoop: r.isClosedLoop` to + `_toRoutePlan`, and a new method mirroring `setRoutePlanDistance`: + ```dart + Future setRoutePlanClosedLoop(int id, bool value) => (update(routePlans) + ..where((r) => r.id.equals(id))).write(RoutePlansCompanion(isClosedLoop: Value(value))); + ``` +- **Repository**: add to `RoutePlanRepository`: + ```dart + Future setClosedLoop(int routeId, bool value) async { + await _db.setRoutePlanClosedLoop(routeId, value); + await _recomputeDistance(routeId); // the closing segment changes the total + } + ``` + Update `_recomputeDistance` to fetch the route's own flag and append the first + waypoint's coordinates when closed, before summing: + ```dart + Future _recomputeDistance(int routeId) async { + final route = await _db.getRoutePlan(routeId); + final waypoints = await _db.waypointsForRoute(routeId); + final points = [for (final w in waypoints) geo.LatLon(w.latitude, w.longitude)]; + if (route?.isClosedLoop == true && points.length >= 2) { + points.add(points.first); + } + final distance = geo.pathLengthMeters(points); + await _db.setRoutePlanDistance(routeId, distance); + } + ``` + (`_recomputeDistance` is already called from every waypoint-mutating method — + `addWaypoint`, `moveWaypoint`, `deleteWaypoint`, `reorderWaypoint` — so the closing + segment is kept correct automatically as pins are edited, with no other call site + changes needed.) +- **UI toggle**: add a 4th item to `_OverflowMenu`, enabled only when there are enough + waypoints for a loop to mean anything (`hasWaypoints` alone isn't enough — need at + least 2 to form any segment at all; reuse the existing `hasWaypoints` bool but also + thread through whether there are `>= 2` waypoints, or simplify by passing a new + `canCloseLoop: waypoints.length >= 2` param): + ```dart + class _OverflowMenu extends StatelessWidget { + const _OverflowMenu({ + required this.hasWaypoints, + required this.canCloseLoop, + required this.isClosedLoop, + required this.onRename, + required this.onDownload, + required this.onToggleLoop, + required this.onDelete, + }); + ... + final bool canCloseLoop; + final bool isClosedLoop; + final VoidCallback onToggleLoop; + + @override + Widget build(BuildContext context) => GlassPanel( + ... + child: PopupMenuButton( + ... + onSelected: (value) => switch (value) { + 'rename' => onRename(), + 'download' => onDownload?.call(), + 'closeLoop' => onToggleLoop(), + 'delete' => onDelete(), + _ => null, + }, + itemBuilder: (context) => [ + const PopupMenuItem(key: Key('rename-route'), value: 'rename', child: Text('Rename')), + ... + PopupMenuItem( + key: const Key('toggle-closed-loop'), + value: 'closeLoop', + enabled: canCloseLoop, + child: Text(isClosedLoop ? 'Open the loop' : 'Close the loop'), + ), + ... + ], + ), + ); + } + ``` + Wire it at the call site: + ```dart + _OverflowMenu( + hasWaypoints: waypoints.isNotEmpty, + canCloseLoop: waypoints.length >= 2, + isClosedLoop: route.isClosedLoop, + onRename: () => _rename(context, repo, route), + onDownload: waypoints.isEmpty ? null : () => _downloadOfflineTiles(context, waypoints), + onToggleLoop: () => repo.setClosedLoop(widget.routeId, !route.isClosedLoop), + onDelete: () async { ... }, + ), + ``` +- **Polyline**: append the closing point when the route is closed and there are at + least 2 waypoints: + ```dart + if (waypoints.length >= 2) + PolylineLayer( + polylines: [ + Polyline( + points: [ + for (final w in waypoints) ll.LatLng(w.latitude, w.longitude), + if (route.isClosedLoop) + ll.LatLng(waypoints.first.latitude, waypoints.first.longitude), + ], + strokeWidth: 4, + pattern: StrokePattern.dashed(segments: const [8, 6]), + color: colors.primary, + ), + ], + ), + ``` + Same `Polyline` object, same styling — just one more point in the list, so the closing + segment renders with identical dashed styling to the rest of the route. +- **`FloatingPill`/`_initialFit`**: no changes needed — Distance updates automatically + via `_recomputeDistance`; Pins/Est. Time are correctly unaffected; `_initialFit`'s + bounds-fitting is based on waypoint positions regardless of whether the last segment + loops back (the closing point is always one of the existing waypoints' own + coordinates, already inside the fitted bounds). + +## Implementation +1. Add `isClosedLoop` column to `RoutePlans` in `database.dart`, bump `schemaVersion` + to `4`, add the `onUpgrade` migration step. +2. Add `isClosedLoop` field to `RoutePlan` domain model + `copyWith`. +3. Add `isClosedLoop` to `insertRoutePlan`'s companion and `_toRoutePlan` in + `database.dart`; add `setRoutePlanClosedLoop`. +4. Add `RoutePlanRepository.setClosedLoop`; update `_recomputeDistance` to append the + closing point when `isClosedLoop`. +5. Extend `_OverflowMenu` with the new toggle item and params; wire it at the call + site in `route_planner_screen.dart`. +6. Extend the `PolylineLayer`'s point list with the conditional closing point. + +## Acceptance criteria +- [ ] "Close the loop" appears in the overflow menu, disabled when there are fewer than + 2 waypoints, enabled otherwise. +- [ ] Toggling it on redraws the polyline with a visible segment from the last waypoint + back to the first, same dashed styling as the rest of the route. +- [ ] Toggling it on increases the Distance stat by the length of the new closing + segment; toggling it back off returns Distance to the open-path total. +- [ ] The menu label reflects current state ("Close the loop" when open, "Open the + loop" when already closed). +- [ ] Adding/moving/deleting a waypoint on a closed-loop route keeps the closing segment + correct automatically (it's recomputed via the existing `_recomputeDistance` call + already present in every waypoint-mutating method). +- [ ] Deleting waypoints down to fewer than 2 while closed doesn't crash — the + `waypoints.length >= 2` guards in both the polyline and distance code make the + closing point simply disappear along with the rest of the route drawing, same as + today's behavior for an open path with 0-1 waypoints. +- [ ] A pre-existing (schema v3) database opens cleanly post-migration with + `isClosedLoop` defaulting to `false` on every existing route. +- [ ] `flutter analyze` clean, `flutter test` green, test count only goes up. + +## Tests +- **Migration test** (`test/migration_test.dart`, following the exact pattern of the + existing `'a v2 database (V3-07) gains route_plans/waypoints and keeps its trips'` + test): seed a v3 database (v1 seed + `activity` column + `route_plans`/`waypoints` + tables created via raw SQL, `PRAGMA user_version = 3;`), open it with `AppDatabase`, + insert a route via `db.insertRoutePlan`, and assert `isClosedLoop` reads back + `false` by default, and that toggling it via `setRoutePlanClosedLoop` persists. +- **Repository tests** (`test/route_plan_repository_test.dart`, alongside the existing + `'adding waypoints appends in order and updates distance live'` etc.): + - `setClosedLoop(true)` on a route with 2+ waypoints increases `distanceM` by + exactly the closing segment's length (compute the expected delta directly with + `geo.pathLengthMeters`/a manual haversine call between the last and first + waypoint, matching how other distance tests in this file already assert exact + values). + - `setClosedLoop(false)` after `true` returns `distanceM` to the pre-toggle value. + - Adding a waypoint to an already-closed route keeps the distance correct + (recomputed including the new closing segment against the new last-added point, + not the old one). +- **Widget tests** (`test/route_planner_screen_test.dart`): + - The "Close the loop" menu item is disabled with 0-1 waypoints, enabled with 2+ + (mirrors the existing "offline-tiles download menu item is disabled with no pins" + test's structure — open the overflow menu, inspect the `PopupMenuItem.enabled` + property via its key). + - Tapping it toggles `route.isClosedLoop` (assert via + `repo.routePlanById(id)?.isClosedLoop`, the same pattern the existing rename test + uses to verify writes through to the repository) and the label switches to "Open + the loop". + - With the loop closed, `find.byType(Polyline)` (or inspecting the `PolylineLayer`'s + `polylines` list directly, matching whatever existing pattern this test file uses + to inspect polyline data) has one more point than the waypoint count. + +## Risks +- None significant — this is additive (new column, new optional toggle) and every + touched call site (`_recomputeDistance`) already runs on every mutation, so there's + no new code path that could silently skip recomputation. + +## Out of scope +Turn-by-turn following of a closed loop (V3-09, already deferred). Any UI indication of +loop direction/rotation. Road-snapped routing for the closing segment (V3-08) — it stays +a straight line like every other segment in this ticket's scope. diff --git a/docs/feedback/README.md b/docs/feedback/README.md new file mode 100644 index 0000000..3b530c9 --- /dev/null +++ b/docs/feedback/README.md @@ -0,0 +1,41 @@ +# Post-launch feedback tickets + +Same shape as `docs/ui-redesign/`: one file per ticket, Goal · Context · Design · +Implementation · Acceptance criteria · Tests · Risks · Out of scope, written before +implementing, with an Outcome section appended after. + +Source: `docs/FEEDBACK.md` — hands-on feedback after using the redesigned app. Turned +into 5 tickets, each independently completable (self-contained enough for a fresh +subagent with no prior context to implement correctly). + +## The tickets + +| # | Ticket | Size | Depends on | Status | +|---|---|---|---|---| +| [FB-01](FB-01-live-street-level-map.md) | Live, street-level map everywhere | L | — | Not started | +| [FB-02](FB-02-hide-idle-speed-panel.md) | Hide the idle Speed panel until recording starts | S | — | Not started | +| [FB-03](FB-03-grid-snapped-hud-widgets.md) | Grid-snapped HUD widgets with auto-fit text | L | — | Not started | +| [FB-04](FB-04-route-planner-map-render-fix.md) | Fix Route Planner's map failing to render + zoom | M | FB-01 (shared zoom constant) | Not started | +| [FB-05](FB-05-closed-loop-routes.md) | Closed-loop routes in Route Planner | M | FB-04 (same file) | Not started | + +## Dependencies / dispatch order + +``` +Wave 1 (parallel): FB-01, FB-02, FB-03 +Wave 2 (after Wave 1 lands): FB-04 — reuses FB-01's ambientZoom constant +Wave 3 (after Wave 2 lands): FB-05 — heavily edits the same file FB-04 just touched +``` + +FB-02 and FB-03 both touch `lib/src/ui/record/record_screen.dart`, but disjoint +regions (the idle-state `Column` vs. `_HudMetricValue`) — low conflict risk. + +## Working method, per ticket + +1. Implement against the ticket's own Implementation section. +2. `flutter analyze` clean, `flutter test` green — count must not regress (374 passing + before this batch starts). +3. Outcome section appended to the ticket file: what shipped, deviations, test counts. +4. Commit, status flipped to Done in both the ticket file and this table. + +Android-emulator verification is best-effort only for this batch — lean on widget +tests as the primary bar; don't get stuck fighting emulator flakiness.