diff --git a/docs/ui-redesign/README.md b/docs/ui-redesign/README.md index dee99a9..ed3ea4d 100644 --- a/docs/ui-redesign/README.md +++ b/docs/ui-redesign/README.md @@ -19,7 +19,7 @@ v3 held to. | # | Ticket | Size | Depends on | Status | |---|---|---|---|---| -| [UI-01](UI-01-tab-shell-background-map.md) | Persistent tab shell with an always-visible background map | L | — | Not started | +| [UI-01](UI-01-tab-shell-background-map.md) | Persistent tab shell with an always-visible background map | L | — | Done | | [UI-02](UI-02-offline-skeleton-map.md) | Offline / no-connection skeleton map | S | UI-01 | Not started | | [UI-03](UI-03-glass-component-kit.md) | Shared floating-glass component kit | M | UI-08 | Not started | | [UI-04](UI-04-customizable-hud-widgets.md) | Customizable HUD telemetry widgets (drag, resize, visibility toggle) | L | UI-03 | Not started | diff --git a/docs/ui-redesign/UI-01-tab-shell-background-map.md b/docs/ui-redesign/UI-01-tab-shell-background-map.md index d66577e..3398d4a 100644 --- a/docs/ui-redesign/UI-01-tab-shell-background-map.md +++ b/docs/ui-redesign/UI-01-tab-shell-background-map.md @@ -1,6 +1,6 @@ # UI-01 — Persistent tab shell with an always-visible background map -**Depends on** nothing · **Size** L · **Status** Not started +**Depends on** nothing · **Size** L · **Status** Done ## Goal Replace the current push/pop stack rooted at Record with a persistent 4-tab shell (Map, @@ -98,3 +98,85 @@ from a persistent bar. The skeleton/offline map state (UI-02). Any visual redesign of what's inside a tab (UI-04/05/06) — this ticket only has to make the shell and shared background map exist, correctly, with today's screen content still working inside it. + +## Outcome + +Shipped as designed: `lib/src/ui/router.dart` now builds a single `StatefulShellRoute +.indexedStack` with four branches (Map `/`, Rides `/rides`, Plan `/plan`, Settings +`/settings`), each with Trip Detail / the route planner nested as a child `GoRoute` +inside its own branch so pushing/popping there never disturbs the other tabs or the +active tab index. `lib/src/ui/app_shell.dart` is new: `AppShell` is a thin go_router +adapter around `ShellScaffold`, a plain-parameter (`currentIndex`/`onDestinationSelected` +/`child`) `ConsumerWidget` that owns the one shared `RideMap` instance, the dimming scrim, +and the bottom `NavigationBar`. Splitting the two was necessary for testability — +go_router only ever constructs a real `StatefulNavigationShell` itself, so widget tests +drive `ShellScaffold` directly with a plain `int` instead. + +Every top-level tab screen (`RecordScreen`, `TripsScreen`, `RoutesListScreen`, +`SettingsScreen`) had its `AppBar`/back-button header removed and its `Scaffold` +background set to transparent so the shared map shows through. `RecordScreen` also lost +its per-screen `onOpenTrips`/`onOpenSettings`/`onOpenRoutes` navigation callbacks and its +embedded `_LiveMap` card entirely — navigation is the shell's nav bar now, and the map is +the shell's permanent background rather than something each screen constructs for +itself. One deliberate temporary trade: the mounted-mode toggle's only remaining button +was on `RecordScreen`'s removed header; its sole access point until UI-05 restyles the +Record screen is now Settings' existing `mounted-mode-switch`. + +**Bug found and fixed during this ticket, not anticipated by the plan:** `RideMap` +returned a structurally different widget tree for empty vs. non-empty points in its +`showEmptyLabel: false` (background) mode — a bare `FlutterMap` vs. `ClipRRect > +FlutterMap`. Once the map became a long-lived shell background instead of a +freshly-mounted per-screen widget, this became visible: the moment a live ride's first +point arrived, Flutter unmounted/remounted the differing subtree mid-flight, and +`RideMap`'s own `didUpdateWidget`-scheduled `_controller.camera` post-frame callback fired +against the stale element, throwing "Looking up a deactivated widget's ancestor is +unsafe." Fixed by unifying the `showEmptyLabel: false` and non-empty branches into one +tree shape (`ClipRRect > FlutterMap` always, `MapOptions`/children varying only by +whether points exist); the `showEmptyLabel: true` path (a finished-ride card, which never +transitions live) was left as its original text-only branch since it carries no such +risk and an existing test (`ride_map_test.dart`) already asserted no `FlutterMap` renders +there. + +**Tests:** `flutter analyze` clean (pre-existing `deprecated_member_use` infos only, +unrelated to this ticket). `flutter test` green at 315 tests (was 316 before this ticket; +net -1 after removing two now-meaningless per-screen entry-point tests and adding one +shell-nav-bar test — see below). Two tests referencing the removed +`onOpenSettings`/`onOpenRoutes` callbacks were deleted outright (the concept no longer +exists); a new "shell nav bar" group in `test/widget_test.dart` asserts all four +`NavigationDestination`s are present and that tapping each calls back with the right +index. The two V3-04 background-map tests were rewritten to pump `ShellScaffold` and +assert on `shell-background-map`/`TileLayer` instead of the old per-screen `live-map` key +— both pass, including the lifecycle-driven tile-drop/restore test that exposed the bug +above. + +**Android emulator verification** (`Medium_Phone_API_35`, API 35, `flutter run --debug`): +confirmed visually for all four tabs — +- Map tab: full-opacity, interactive map, no header, `START RECORDING` visible. +- Rides/Plan/Settings tabs: same map dimmed via the `0xB3000000` scrim, non-interactive, + no header, each tab's real content on top. +- Started a live recording (with `adb emu geo fix` supplying a location) and switched + tabs repeatedly: the point count and elapsed timer kept advancing across tab switches + (11 → 31 → 41 points, uninterrupted) and the camera position did not reset — confirming + the shared instance survives tab switches rather than being torn down and recreated. +- Backgrounding via the real HOME key (not just the synthetic lifecycle message the + widget test uses) dropped the map to a flat, untiled gray — the same lifecycle-based + tile-teardown from V3-04 firing correctly in the shell context, not just in tests. +- One visual red herring investigated and ruled out: a flat gray rectangle appeared + transiently in the dimmed background on the Rides/Plan tabs. Ruled out as an app bug by + reproducing it identically on two different tab screens at the same absolute screen + position, and by confirming a fresh app launch never shows it — it is flutter_map's + normal "tile chunk not yet loaded" placeholder at the low zoom level the background + map starts at before any location fix narrows it, not a shell defect. +- Known pre-existing rough edge, not a regression: the background map's follow-mode + moves the camera center to the latest point but does not adjust zoom on the first fix, + so a freshly-started ride's background map can look zoomed far out until the user + manually zooms. This behavior predates UI-01 (the same `_controller.move(point, current + Zoom)` call existed in the old standalone `_LiveMap`) and is out of scope here. + +Deferred, not a gap: a widget test asserting Trip Detail/route-planner push-and-pop stays +within its own tab (rather than affecting `currentIndex`) was not written — the +`StatefulShellBranch`-per-tab nesting in `router.dart` structurally guarantees this via +go_router's own navigator-per-branch semantics, and it was verified manually on-device +that `router.dart`'s nested `GoRoute`s compile and resolve correctly. A future ticket +touching Rides/Plan navigation should add explicit coverage rather than relying on this +note. diff --git a/lib/src/ui/app_shell.dart b/lib/src/ui/app_shell.dart new file mode 100644 index 0000000..a4fb9a2 --- /dev/null +++ b/lib/src/ui/app_shell.dart @@ -0,0 +1,157 @@ +/// UI-01: the persistent 4-tab shell. Replaces the old push/pop stack rooted at Record. +/// +/// One background map, always mounted, shared by every tab -- not four separate map +/// instances. Full opacity and interactive on the Map tab; dimmed and non-interactive +/// (taps pass through to the tab's real content) everywhere else. This is what lets the +/// map survive a tab switch with its camera position and live path intact, and what +/// makes "the map is active on every tab" true without refetching tiles four times over. +/// +/// No `AppBar` anywhere in this shell or in any of the four tab roots -- the point of the +/// redesign is showing as much map as possible, and a persistent bottom nav plus zero +/// header chrome is what buys that space back. +library; + +import 'package:flutter/material.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:go_router/go_router.dart'; + +import '../app/providers.dart'; +import '../domain/models.dart'; +import 'components/ride_map.dart'; + +/// The thin go_router adapter. Kept separate from [ShellScaffold] so tests can drive the +/// actual shell logic (background map, dimming, nav bar) with a plain `currentIndex` and +/// `child`, without needing to construct a real [StatefulNavigationShell] -- which +/// go_router only ever builds internally. +class AppShell extends StatelessWidget { + const AppShell({super.key, required this.navigationShell}); + + final StatefulNavigationShell navigationShell; + + @override + Widget build(BuildContext context) => ShellScaffold( + currentIndex: navigationShell.currentIndex, + onDestinationSelected: (index) => navigationShell.goBranch( + index, + // Tapping the already-active tab pops it back to its own root, matching the + // conventional "tap the active tab to go home" behaviour of a bottom-tab app. + initialLocation: index == navigationShell.currentIndex, + ), + child: navigationShell, + ); +} + +class ShellScaffold extends ConsumerWidget { + const ShellScaffold({ + super.key, + required this.currentIndex, + required this.onDestinationSelected, + required this.child, + }); + + final int currentIndex; + final ValueChanged onDestinationSelected; + final Widget child; + + static const mapTabIndex = 0; + + @override + Widget build(BuildContext context, WidgetRef ref) { + final isMapTab = currentIndex == mapTabIndex; + // The Settings toggle's whole point (see its own doc comment) is that no tile is + // ever fetched when off, not merely hidden -- that guarantee must survive this + // screen becoming a permanent background across every tab, not just apply to the + // Map tab the way it used to. + final mapEnabled = ref.watch(mapEnabledProvider); + 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 []; + + return Scaffold( + // Falls back to the ordinary theme background when the map is disabled -- the + // per-tab screens are transparent now, relying on this shell to paint something. + backgroundColor: Theme.of(context).scaffoldBackgroundColor, + body: Stack( + children: [ + // The one shared map instance. IgnorePointer rather than a per-tab rebuild: + // keeping this the same widget across tab switches is what preserves camera + // position and avoids refetching tiles. Not constructed at all when the map is + // disabled -- no widget means no tile request can fire, the same real + // short-circuit `mapEnabledProvider` always guaranteed. + if (mapEnabled) + Positioned.fill( + child: IgnorePointer( + ignoring: !isMapTab, + child: RideMap( + key: const Key('shell-background-map'), + points: points, + segments: segments, + follow: isMapTab, + fill: true, + showEmptyLabel: false, + tileProvider: ref.watch(cachedTileProviderProvider), + ), + ), + ), + // Dims the map behind every tab except Map itself -- decoration there, not a + // control surface; the scrim also absorbs taps so they can't reach the map. + if (mapEnabled && !isMapTab) + const Positioned.fill( + child: IgnorePointer( + child: ColoredBox(color: Color(0xB3000000)), + ), + ), + Positioned.fill(child: child), + ], + ), + bottomNavigationBar: _ShellNavBar( + currentIndex: currentIndex, + onDestinationSelected: onDestinationSelected, + ), + ); + } +} + +class _ShellNavBar extends StatelessWidget { + const _ShellNavBar({required this.currentIndex, required this.onDestinationSelected}); + + final int currentIndex; + final ValueChanged onDestinationSelected; + + @override + Widget build(BuildContext context) => NavigationBar( + key: const Key('shell-nav-bar'), + selectedIndex: currentIndex, + onDestinationSelected: onDestinationSelected, + destinations: const [ + NavigationDestination( + key: Key('nav-map'), + icon: Icon(Icons.map_outlined), + selectedIcon: Icon(Icons.map), + label: 'Map', + ), + NavigationDestination( + key: Key('nav-rides'), + icon: Icon(Icons.directions_bike_outlined), + selectedIcon: Icon(Icons.directions_bike), + label: 'Rides', + ), + NavigationDestination( + key: Key('nav-plan'), + icon: Icon(Icons.route_outlined), + selectedIcon: Icon(Icons.route), + label: 'Plan', + ), + NavigationDestination( + key: Key('nav-settings'), + icon: Icon(Icons.settings_outlined), + selectedIcon: Icon(Icons.settings), + label: 'Settings', + ), + ], + ); +} diff --git a/lib/src/ui/components/ride_map.dart b/lib/src/ui/components/ride_map.dart index 84dbbb7..1b091fc 100644 --- a/lib/src/ui/components/ride_map.dart +++ b/lib/src/ui/components/ride_map.dart @@ -45,12 +45,24 @@ class RideMap extends StatefulWidget { this.height = 320, this.follow = false, this.tileProvider, + this.fill = false, + this.showEmptyLabel = true, }); final List points; final List segments; final double height; + /// UI-01: fills whatever space the parent gives it (a `Positioned.fill`/`Expanded` + /// ancestor) instead of the fixed [height] -- for the persistent full-screen + /// background map behind every tab, where there is no card to size it. + final bool fill; + + /// False for the persistent background: an idle app with no ride yet is the normal + /// state there, not an error to explain with text -- it should just look like a map, + /// centred on a neutral default, with nothing overlaid. + final bool showEmptyLabel; + /// V3-11: when supplied, tiles are read from (and written through to) the offline /// tile cache instead of flutter_map's own uncapped default. Null falls back to /// ordinary networked tiles -- used whenever the cache isn't ready yet, or in tests. @@ -118,13 +130,23 @@ class _RideMapState extends State with WidgetsBindingObserver { return grouped.values.toList(); } + // `SizedBox.expand`, not `Positioned.fill` -- this widget is used both as a direct + // `Stack` child (fine either way) and wrapped in an `IgnorePointer` first (the shared + // shell background), where a `Positioned` is invalid because it isn't a direct child + // of the `Stack`. `SizedBox.expand` fills the available space either way. + Widget _sized({required Widget child}) => + widget.fill ? SizedBox.expand(child: child) : SizedBox(height: widget.height, child: child); + @override Widget build(BuildContext context) { final colors = Theme.of(context).colorScheme; + final hasPoints = widget.points.isNotEmpty; - if (widget.points.isEmpty) { - return SizedBox( - height: widget.height, + // The `showEmptyLabel && !hasPoints` case (a finished ride card with no points, e.g. + // a corrupt/empty trip) renders text only, no map underneath -- that usage never + // transitions live, so there's no risk to the element tree here. + if (!hasPoints && widget.showEmptyLabel) { + return _sized( child: Center( child: Text( 'No path recorded', @@ -134,69 +156,86 @@ class _RideMapState extends State with WidgetsBindingObserver { ); } - final all = [ - for (final p in widget.points) geo.LatLon(p.latitude, p.longitude), - ]; - final bounds = geo.bounds(all)!; - final polylines = _buildPolylines(colors); + // UI-01 bug, found by a widget test: the empty and non-empty states used to return + // structurally different widget trees (a bare `FlutterMap` versus + // `ClipRRect > FlutterMap`) for the `showEmptyLabel: false` background-map usage. + // The moment a live ride's first point arrived, Flutter treated that as a full + // element swap rather than a rebuild of the same element -- unmounting the old + // `FlutterMap` out from under a `MapController` that a `didUpdateWidget`-scheduled + // post-frame callback was about to use, which threw "Looking up a deactivated + // widget's ancestor is unsafe." Building exactly one shape always for this usage, + // varying only the `MapOptions`/children by `hasPoints`, removes the swap entirely. + geo.Bounds? bounds; + if (hasPoints) { + bounds = geo.bounds([ + for (final p in widget.points) geo.LatLon(p.latitude, p.longitude), + ]); + } + final polylines = hasPoints ? _buildPolylines(colors) : const []; - return SizedBox( - height: widget.height, + final map = ClipRRect( // The map draws to the edge of its box; clipping keeps it from painting over // adjacent controls, which osmdroid did until it was explicitly bounded. - child: ClipRRect( - borderRadius: BorderRadius.circular(12), - child: FlutterMap( - mapController: _controller, - options: MapOptions( - initialCameraFit: bounds.isDegenerate - // Every point at one spot — a parked "ride". Fitting this would zoom to - // infinity, so centre and use a sane street-level zoom instead. - ? null - : CameraFit.bounds( - bounds: LatLngBounds( - ll.LatLng(bounds.minLat, bounds.minLon), - ll.LatLng(bounds.maxLat, bounds.maxLon), - ), - padding: const EdgeInsets.all(24), - // The clamp. Without it a short ride lands past OSM's max tile zoom - // and renders an empty grid. - maxZoom: maxTileZoom, + borderRadius: widget.fill ? BorderRadius.zero : BorderRadius.circular(12), + child: FlutterMap( + mapController: _controller, + options: MapOptions( + initialCameraFit: (bounds == null || bounds.isDegenerate) + // No points yet, or every point at one spot (a parked "ride") -- + // fitting a degenerate box would zoom to infinity, so centre on a + // neutral or last-known point at a sane street-level zoom instead. + ? null + : CameraFit.bounds( + bounds: LatLngBounds( + ll.LatLng(bounds.minLat, bounds.minLon), + ll.LatLng(bounds.maxLat, bounds.maxLon), ), - initialCenter: ll.LatLng(bounds.centerLat, bounds.centerLon), - initialZoom: bounds.isDegenerate ? shortRideZoom : maxTileZoom, - maxZoom: maxTileZoom, - interactionOptions: const InteractionOptions( - flags: InteractiveFlag.pinchZoom | InteractiveFlag.drag, - ), - onPositionChanged: !widget.follow - ? null - : (position, hasGesture) { - // Only a real pan/pinch turns following off -- the programmatic - // moves this widget makes to chase the rider must not cancel - // themselves out. - if (hasGesture && _following) { - setState(() => _following = false); - } - }, - ), - children: [ - // Omitted entirely while backgrounded -- not just visually hidden -- so no - // tile request can fire off-screen. See the lifecycle observer above. - if (!_backgrounded) - TileLayer( - urlTemplate: 'https://tile.openstreetmap.org/{z}/{x}/{y}.png', - userAgentPackageName: tileUserAgent, - maxNativeZoom: maxTileZoom.toInt(), - // Respect OSM's usage policy: render what is looked at, never bulk prefetch. - panBuffer: 0, - tileProvider: widget.tileProvider, - ), - PolylineLayer(polylines: polylines), - ], + padding: const EdgeInsets.all(24), + // The clamp. Without it a short ride lands past OSM's max tile zoom + // and renders an empty grid. + maxZoom: maxTileZoom, + ), + initialCenter: bounds == null + ? const ll.LatLng(0, 0) + : ll.LatLng(bounds.centerLat, bounds.centerLon), + initialZoom: bounds == null + ? 2 + : (bounds.isDegenerate ? shortRideZoom : maxTileZoom), + maxZoom: maxTileZoom, + interactionOptions: hasPoints + ? const InteractionOptions( + flags: InteractiveFlag.pinchZoom | InteractiveFlag.drag, + ) + : const InteractionOptions(flags: InteractiveFlag.none), + onPositionChanged: !widget.follow + ? null + : (position, hasGesture) { + // Only a real pan/pinch turns following off -- the programmatic + // moves this widget makes to chase the rider must not cancel + // themselves out. + if (hasGesture && _following) { + setState(() => _following = false); + } + }, ), + children: [ + // Omitted entirely while backgrounded -- not just visually hidden -- so no + // tile request can fire off-screen. See the lifecycle observer above. + if (!_backgrounded) + TileLayer( + urlTemplate: 'https://tile.openstreetmap.org/{z}/{x}/{y}.png', + userAgentPackageName: tileUserAgent, + maxNativeZoom: maxTileZoom.toInt(), + // Respect OSM's usage policy: render what is looked at, never bulk prefetch. + panBuffer: 0, + tileProvider: widget.tileProvider, + ), + PolylineLayer(polylines: polylines), + ], ), ); + + return _sized(child: map); } /// One polyline per speed run within each segment. diff --git a/lib/src/ui/record/record_screen.dart b/lib/src/ui/record/record_screen.dart index 932053d..fe0be4d 100644 --- a/lib/src/ui/record/record_screen.dart +++ b/lib/src/ui/record/record_screen.dart @@ -11,7 +11,6 @@ import '../../domain/models.dart'; import '../../recording/location_source.dart'; import '../../recording/wakelock_controller.dart'; import '../../telemetry/telemetry.dart'; -import '../components/ride_map.dart'; import '../components/stats.dart'; import '../format.dart'; import '../theme.dart'; @@ -38,16 +37,7 @@ class RecordUiState { } class RecordScreen extends ConsumerStatefulWidget { - const RecordScreen({ - super.key, - this.onOpenTrips, - this.onOpenSettings, - this.onOpenRoutes, - }); - - final VoidCallback? onOpenTrips; - final VoidCallback? onOpenSettings; - final VoidCallback? onOpenRoutes; + const RecordScreen({super.key}); @override ConsumerState createState() => _RecordScreenState(); @@ -158,7 +148,6 @@ class _RecordScreenState extends ConsumerState { // colour scheme straight off a locally-built theme rather than `Theme.of(context)`, // which would still report the app-wide (dark) theme here. final theme = mountedMode ? ripprMountedTheme() : Theme.of(context); - final colors = theme.colorScheme; final engine = ref.watch(recordingEngineProvider); final trip = ref.watch(activeTripProvider).valueOrNull; final units = ref.watch(unitSystemProvider); @@ -173,7 +162,13 @@ class _RecordScreenState extends ConsumerState { : (_now - trip.startedAt).clamp(0, 1 << 62), ); + // UI-01: no header, no embedded map card -- the map is the shared background behind + // this whole tab (`AppShell`), full opacity here. A quick mounted-mode toggle used to + // live in a header row; that row is gone, and the toggle's only home for now is + // Settings' existing switch (see the ticket's Outcome for why this is a deliberate, + // temporary trade rather than an oversight -- UI-05 restyles this screen properly). return Theme(data: theme, child: Scaffold( + backgroundColor: Colors.transparent, // Scrollable, but still centred when there is room. // // With a ride active the card grows to six stat rows, which overflows a short @@ -190,56 +185,6 @@ class _RecordScreenState extends ConsumerState { child: Column( mainAxisAlignment: MainAxisAlignment.center, children: [ - Row( - mainAxisAlignment: MainAxisAlignment.spaceBetween, - children: [ - Text( - 'RIPPR', - style: TextStyle( - fontSize: 34, - fontWeight: FontWeight.w900, - letterSpacing: 6, - color: colors.primary, - ), - ), - Row( - children: [ - TextButton( - key: const Key('open-routes'), - onPressed: widget.onOpenRoutes, - child: const Text('Routes'), - ), - TextButton( - onPressed: widget.onOpenTrips, - child: const Text('Rides'), - ), - IconButton( - key: const Key('mounted-mode-toggle'), - icon: Icon(mountedMode - ? Icons.motorcycle - : Icons.motorcycle_outlined), - tooltip: mountedMode - ? 'Mounted mode on' - : 'Mounted mode off', - color: mountedMode ? colors.primary : null, - onPressed: () async { - final next = !mountedMode; - await ref.read(configProvider)?.setMountedMode(next); - ref.read(mountedModeProvider.notifier).state = next; - }, - ), - IconButton( - key: const Key('open-settings'), - icon: const Icon(Icons.settings_outlined), - tooltip: 'Settings', - onPressed: widget.onOpenSettings, - ), - ], - ), - ], - ), - const SizedBox(height: 20), - // 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. @@ -252,17 +197,6 @@ class _RecordScreenState extends ConsumerState { ), const SizedBox(height: 16), - // V3-04: RecordingEngine has no idea this exists -- it is fed - // entirely from a Drift stream, the same seam the trip-detail map - // already uses. Off by default (mapEnabledProvider) until battery - // impact is measured (V3-13), and only while a ride is actually - // active -- an idle screen has nothing to draw and no reason to - // hold a tile layer alive. - if (ui.trip != null && ref.watch(mapEnabledProvider)) ...[ - _LiveMap(tripId: ui.trip!.id), - const SizedBox(height: 16), - ], - Card( child: Padding( padding: const EdgeInsets.all(16), @@ -352,28 +286,6 @@ class _RecordScreenState extends ConsumerState { } } -/// Thin adapter from the two live Drift streams to [RideMap]. Kept out of -/// [_RecordScreenState] so a provider read/write cycle here can't be confused with the -/// engine's own state -- this widget only ever reads. -class _LiveMap extends ConsumerWidget { - const _LiveMap({required this.tripId}); - - final int tripId; - - @override - Widget build(BuildContext context, WidgetRef ref) { - final points = ref.watch(livePointsProvider(tripId)).valueOrNull ?? const []; - final segments = ref.watch(liveSegmentsProvider(tripId)).valueOrNull ?? const []; - return RideMap( - key: const Key('live-map'), - points: points, - segments: segments, - height: 220, - follow: true, - tileProvider: ref.watch(cachedTileProviderProvider), - ); - } -} class _Controls extends StatelessWidget { const _Controls({ diff --git a/lib/src/ui/router.dart b/lib/src/ui/router.dart index 321d1f3..e7f6490 100644 --- a/lib/src/ui/router.dart +++ b/lib/src/ui/router.dart @@ -1,13 +1,13 @@ -/// Ported from `com.rippr.ui.RipprNavHost`. -/// -/// Three destinations, same shape as the Compose original. `go_router` replaces -/// `navigation-compose`; the path parameter is parsed explicitly, mirroring the Kotlin -/// comment about ids arriving as the wrong type when the argument type is left implicit. +/// UI-01: a persistent 4-tab shell (`StatefulShellRoute`), replacing the push/pop stack +/// rooted at Record that this app used through v3. Each branch keeps its own navigator, +/// so pushing Trip Detail from Rides -- or the route planner from Plan -- doesn't +/// disturb the other tabs' state or the active tab index. library; import 'package:flutter/material.dart'; import 'package:go_router/go_router.dart'; +import 'app_shell.dart'; import 'detail/trip_detail_screen.dart'; import 'record/record_screen.dart'; import 'routes/route_planner_screen.dart'; @@ -17,65 +17,81 @@ import 'trips/trips_screen.dart'; abstract final class Routes { static const record = '/'; - static const trips = '/trips'; - static const tripDetail = '/trip/:tripId'; + static const trips = '/rides'; + static const tripDetail = '/rides/:tripId'; static const settings = '/settings'; - static const routes = '/routes'; - static const routePlanner = '/routes/:routeId'; + static const routes = '/plan'; + static const routePlanner = '/plan/:routeId'; - static String detailFor(int tripId) => '/trip/$tripId'; - static String plannerFor(int routeId) => '/routes/$routeId'; + static String detailFor(int tripId) => '/rides/$tripId'; + static String plannerFor(int routeId) => '/plan/$routeId'; } GoRouter buildRouter() => GoRouter( initialLocation: Routes.record, routes: [ - GoRoute( - path: Routes.record, - builder: (context, state) => RecordScreen( - onOpenTrips: () => context.push(Routes.trips), - onOpenSettings: () => context.push(Routes.settings), - onOpenRoutes: () => context.push(Routes.routes), - ), - ), - GoRoute( - path: Routes.routes, - builder: (context, state) => RoutesListScreen( - onOpenRoute: (id) => context.push(Routes.plannerFor(id)), - onBack: () => context.pop(), - ), - ), - GoRoute( - path: Routes.routePlanner, - builder: (context, state) { - final id = int.tryParse(state.pathParameters['routeId'] ?? ''); - if (id == null) return const _NotFound(); - return RoutePlannerScreen(routeId: id, onBack: () => context.pop()); - }, - ), - GoRoute( - path: Routes.settings, - builder: (context, state) => - SettingsScreen(onBack: () => context.pop()), - ), - GoRoute( - path: Routes.trips, - builder: (context, state) => TripsScreen( - onOpenTrip: (id) => context.push(Routes.detailFor(id)), - onBack: () => context.pop(), - ), - ), - GoRoute( - path: Routes.tripDetail, - builder: (context, state) { - // Parsed explicitly. A malformed or missing id must land on a real screen - // saying so, never on a blank one or a crash. - final id = int.tryParse(state.pathParameters['tripId'] ?? ''); - if (id == null) { - return const _NotFound(); - } - return TripDetailScreen(tripId: id, onBack: () => context.pop()); - }, + StatefulShellRoute.indexedStack( + builder: (context, state, navigationShell) => + AppShell(navigationShell: navigationShell), + branches: [ + // Branch 0: Map. The Record screen, headerless -- see AppShell's doc comment. + StatefulShellBranch( + routes: [ + GoRoute(path: Routes.record, builder: (context, state) => const RecordScreen()), + ], + ), + // Branch 1: Rides. Trip Detail pushes within this branch's own navigator. + StatefulShellBranch( + routes: [ + GoRoute( + path: Routes.trips, + builder: (context, state) => TripsScreen( + onOpenTrip: (id) => context.push(Routes.detailFor(id)), + ), + routes: [ + GoRoute( + path: ':tripId', + builder: (context, state) { + final id = int.tryParse(state.pathParameters['tripId'] ?? ''); + if (id == null) return const _NotFound(); + return TripDetailScreen(tripId: id, onBack: () => context.pop()); + }, + ), + ], + ), + ], + ), + // Branch 2: Plan. The route planner pushes within this branch's own navigator. + StatefulShellBranch( + routes: [ + GoRoute( + path: Routes.routes, + builder: (context, state) => RoutesListScreen( + onOpenRoute: (id) => context.push(Routes.plannerFor(id)), + ), + routes: [ + GoRoute( + path: ':routeId', + builder: (context, state) { + final id = int.tryParse(state.pathParameters['routeId'] ?? ''); + if (id == null) return const _NotFound(); + return RoutePlannerScreen(routeId: id, onBack: () => context.pop()); + }, + ), + ], + ), + ], + ), + // Branch 3: Settings. + StatefulShellBranch( + routes: [ + GoRoute( + path: Routes.settings, + builder: (context, state) => const SettingsScreen(), + ), + ], + ), + ], ), ], errorBuilder: (context, state) => const _NotFound(), @@ -86,13 +102,11 @@ class _NotFound extends StatelessWidget { @override Widget build(BuildContext context) => Scaffold( - appBar: AppBar( - leading: IconButton( - icon: const Icon(Icons.arrow_back), - onPressed: () => - context.canPop() ? context.pop() : context.go(Routes.record), + body: Center( + child: TextButton( + onPressed: () => context.canPop() ? context.pop() : context.go(Routes.record), + child: const Text('That ride could not be found. Back'), ), ), - body: const Center(child: Text('That ride could not be found.')), ); } diff --git a/lib/src/ui/routes/routes_list_screen.dart b/lib/src/ui/routes/routes_list_screen.dart index face432..9ee1e7f 100644 --- a/lib/src/ui/routes/routes_list_screen.dart +++ b/lib/src/ui/routes/routes_list_screen.dart @@ -10,10 +10,9 @@ import '../components/stats.dart'; import '../format.dart'; class RoutesListScreen extends ConsumerWidget { - const RoutesListScreen({super.key, this.onOpenRoute, this.onBack}); + const RoutesListScreen({super.key, this.onOpenRoute}); final void Function(int routeId)? onOpenRoute; - final VoidCallback? onBack; @override Widget build(BuildContext context, WidgetRef ref) { @@ -21,7 +20,10 @@ class RoutesListScreen extends ConsumerWidget { final routes = ref.watch(routePlansProvider).valueOrNull ?? const []; final units = ref.watch(unitSystemProvider); + // UI-01: no back/title header -- Plan is a tab now. "New route" is functional, not + // chrome, so it stays, right-aligned on its own. return Scaffold( + backgroundColor: Colors.transparent, body: SafeArea( child: Column( crossAxisAlignment: CrossAxisAlignment.stretch, @@ -30,15 +32,6 @@ class RoutesListScreen extends ConsumerWidget { padding: const EdgeInsets.fromLTRB(16, 12, 16, 4), child: Row( children: [ - TextButton( - key: const Key('back'), - onPressed: onBack, - child: const Text('‹ Record'), - ), - const Padding( - padding: EdgeInsets.only(left: 8), - child: Text('ROUTES', style: TextStyle(letterSpacing: 2)), - ), const Spacer(), IconButton( key: const Key('new-route'), diff --git a/lib/src/ui/settings/settings_screen.dart b/lib/src/ui/settings/settings_screen.dart index 25c2276..373301d 100644 --- a/lib/src/ui/settings/settings_screen.dart +++ b/lib/src/ui/settings/settings_screen.dart @@ -13,23 +13,16 @@ import '../../config/config.dart'; import '../../domain/models.dart'; class SettingsScreen extends ConsumerWidget { - const SettingsScreen({super.key, this.onBack}); - - final VoidCallback? onBack; + const SettingsScreen({super.key}); @override Widget build(BuildContext context, WidgetRef ref) { final config = ref.watch(configProvider); + // UI-01: no AppBar -- Settings is a tab now, reached via the persistent bottom nav, + // not a pushed screen with somewhere to back out to. return Scaffold( - appBar: AppBar( - leading: IconButton( - key: const Key('back'), - icon: const Icon(Icons.arrow_back), - onPressed: onBack, - ), - title: const Text('Settings'), - ), + backgroundColor: Colors.transparent, // Config loads once at startup (see main.dart) and is normally already present by // the time anyone reaches this screen, but the seam is real: show a spinner rather // than crash on a null read in the small window before it resolves. diff --git a/lib/src/ui/trips/trips_screen.dart b/lib/src/ui/trips/trips_screen.dart index ef73e91..35fd765 100644 --- a/lib/src/ui/trips/trips_screen.dart +++ b/lib/src/ui/trips/trips_screen.dart @@ -21,10 +21,9 @@ final _selectionProvider = StateProvider.autoDispose>( ); class TripsScreen extends ConsumerWidget { - const TripsScreen({super.key, this.onOpenTrip, this.onBack}); + const TripsScreen({super.key, this.onOpenTrip}); final void Function(int tripId)? onOpenTrip; - final VoidCallback? onBack; @override Widget build(BuildContext context, WidgetRef ref) { @@ -44,18 +43,22 @@ class TripsScreen extends ConsumerWidget { void clearSelection() => ref.read(_selectionProvider.notifier).state = {}; + // UI-01: no back/title header -- Rides is a tab now. The selection toolbar (Cancel/ + // count/Merge/Delete) is functional, not chrome, so it stays -- it only appears + // while `selecting` is true. return Scaffold( + backgroundColor: Colors.transparent, body: SafeArea( child: Padding( padding: const EdgeInsets.symmetric(horizontal: 16), child: Column( crossAxisAlignment: CrossAxisAlignment.stretch, children: [ - Padding( - padding: const EdgeInsets.symmetric(vertical: 12), - child: Row( - children: [ - if (selecting) ...[ + if (selecting) + Padding( + padding: const EdgeInsets.symmetric(vertical: 12), + child: Row( + children: [ TextButton( onPressed: clearSelection, child: const Text('Cancel'), @@ -67,27 +70,7 @@ class TripsScreen extends ConsumerWidget { style: TextStyle(fontSize: 14, color: colors.outline), ), ), - ] else ...[ - TextButton( - key: const Key('back'), - onPressed: onBack, - child: const Text('‹ Record'), - ), - Padding( - padding: const EdgeInsets.only(left: 8), - child: Text( - 'RIDES', - style: TextStyle( - fontSize: 18, - fontWeight: FontWeight.bold, - letterSpacing: 3, - color: colors.primary, - ), - ), - ), - ], - const Spacer(), - if (selecting) ...[ + const Spacer(), TextButton( key: const Key('merge'), onPressed: canMerge @@ -104,9 +87,8 @@ class TripsScreen extends ConsumerWidget { ), ), ], - ], + ), ), - ), Expanded( child: switch (tripsAsync) { AsyncValue(hasValue: true, value: final list) diff --git a/test/widget_test.dart b/test/widget_test.dart index eac3148..8479524 100644 --- a/test/widget_test.dart +++ b/test/widget_test.dart @@ -12,6 +12,7 @@ import 'package:rippr/src/domain/models.dart'; import 'package:rippr/src/ui/activity_display.dart'; import 'package:rippr/src/recording/location_source.dart'; import 'package:rippr/src/recording/wakelock_controller.dart'; +import 'package:rippr/src/ui/app_shell.dart'; import 'package:rippr/src/ui/detail/trip_detail_screen.dart'; import 'package:rippr/src/ui/format.dart'; import 'package:rippr/src/ui/record/record_screen.dart'; @@ -203,58 +204,37 @@ void main() { expect(colour, isNot(Colors.black)); }); - screenTest('a settings entry point exists and is wired (V3-02)', (tester) async { - var opened = false; - await tester.pumpWidget( - host(RecordScreen(onOpenSettings: () => opened = true)), - ); - await tester.pumpAndSettle(); + // UI-01: Settings/Routes/Rides entry points are no longer per-screen callback + // buttons on RecordScreen -- they're destinations on the shell's persistent bottom + // nav bar. See the `AppShell`/`ShellScaffold` group below for their coverage. - expect(find.byKey(const Key('open-settings')), findsOneWidget); - await tester.tap(find.byKey(const Key('open-settings'))); - await tester.pumpAndSettle(); - - expect(opened, isTrue, - reason: 'the button must actually invoke the callback that navigates'); - }); - - screenTest('a routes entry point exists and is wired (V3-07)', (tester) async { - var opened = false; - await tester.pumpWidget( - host(RecordScreen(onOpenRoutes: () => opened = true)), - ); - await tester.pumpAndSettle(); - - expect(find.byKey(const Key('open-routes')), findsOneWidget); - await tester.tap(find.byKey(const Key('open-routes'))); - await tester.pumpAndSettle(); - - expect(opened, isTrue); - }); - - screenTest('the live map appears only while recording and the toggle is on ' - '(V3-04)', (tester) async { - // Idle, toggle on: no trip to draw, so no map at all. - await tester.pumpWidget(host(const RecordScreen())); - await tester.pumpAndSettle(); - expect(find.byKey(const Key('live-map')), findsNothing); - - // Recording, toggle off: RecordingEngine has produced a trip, but the map must - // not be constructed at all -- not just hidden. + screenTest('the background map appears only while the map toggle is on ' + '(V3-04, moved to the shell by UI-01)', (tester) async { + // Toggle off: the shell must not construct the map at all -- not just hidden. await repo.startTrip(1000); - await tester.pumpWidget(const SizedBox.shrink()); - await pumpLive(tester, host(const RecordScreen(), map: false)); - expect(find.byKey(const Key('live-map')), findsNothing); + await pumpLive( + tester, + host( + ShellScaffold(currentIndex: 0, onDestinationSelected: (_) {}, child: const RecordScreen()), + map: false, + ), + ); + expect(find.byKey(const Key('shell-background-map')), findsNothing); expect(find.byType(FlutterMap), findsNothing); - // Recording, toggle on: the map is drawn. + // Toggle on: the map is drawn. await tester.pumpWidget(const SizedBox.shrink()); - await pumpLive(tester, host(const RecordScreen())); - expect(find.byKey(const Key('live-map')), findsOneWidget); + await pumpLive( + tester, + host( + ShellScaffold(currentIndex: 0, onDestinationSelected: (_) {}, child: const RecordScreen()), + ), + ); + expect(find.byKey(const Key('shell-background-map')), findsOneWidget); }); - screenTest('backgrounding the app drops the tile layer (V3-04)', - (tester) async { + screenTest('backgrounding the app drops the tile layer (V3-04, moved to the ' + 'shell by UI-01)', (tester) async { final h = await repo.startTrip(1000); await repo.appendPoints([ TrackPoint( @@ -267,7 +247,12 @@ void main() { altitudeM: 1000.0, ), ]); - await pumpLive(tester, host(const RecordScreen())); + await pumpLive( + tester, + host( + ShellScaffold(currentIndex: 0, onDestinationSelected: (_) {}, child: const RecordScreen()), + ), + ); expect(find.byType(TileLayer), findsOneWidget, reason: 'foregrounded: tiles render normally'); @@ -278,6 +263,7 @@ void main() { await tester.binding.defaultBinaryMessenger .handlePlatformMessage('flutter/lifecycle', message, (_) {}); await tester.pump(); + await tester.pump(); expect(find.byType(TileLayer), findsNothing, reason: 'backgrounded: no tile layer means no tile request can fire'); @@ -288,6 +274,7 @@ void main() { await tester.binding.defaultBinaryMessenger .handlePlatformMessage('flutter/lifecycle', resumed, (_) {}); await tester.pump(); + await tester.pump(); expect(find.byType(TileLayer), findsOneWidget, reason: 'foregrounding again must resume tiles'); }); @@ -364,6 +351,41 @@ void main() { }); }); + group('shell nav bar', () { + // UI-01: Settings/Routes/Rides are no longer per-screen callback buttons on + // RecordScreen -- they're destinations on the shell's persistent bottom nav bar. + // This replaces the old "a settings/routes entry point exists and is wired" + // per-screen tests. + screenTest('all four destinations are present and switching tabs calls back ' + 'with the tapped index', (tester) async { + var lastIndex = -1; + await tester.pumpWidget(host( + ShellScaffold( + currentIndex: 0, + onDestinationSelected: (i) => lastIndex = i, + child: const RecordScreen(), + ), + map: false, + )); + await tester.pumpAndSettle(); + + expect(find.byKey(const Key('shell-nav-bar')), findsOneWidget); + expect(find.byKey(const Key('nav-map')), findsOneWidget); + expect(find.byKey(const Key('nav-rides')), findsOneWidget); + expect(find.byKey(const Key('nav-plan')), findsOneWidget); + expect(find.byKey(const Key('nav-settings')), findsOneWidget); + + await tester.tap(find.byKey(const Key('nav-settings'))); + expect(lastIndex, 3); + + await tester.tap(find.byKey(const Key('nav-rides'))); + expect(lastIndex, 1); + + await tester.tap(find.byKey(const Key('nav-plan'))); + expect(lastIndex, 2); + }); + }); + group('trips list', () { screenTest('empty state explains what to do', (tester) async { await tester.pumpWidget(host(const TripsScreen()));