diff --git a/docs/v3/V3-02-settings-screen.md b/docs/v3/V3-02-settings-screen.md index 7d2b235..b70e264 100644 --- a/docs/v3/V3-02-settings-screen.md +++ b/docs/v3/V3-02-settings-screen.md @@ -1,6 +1,6 @@ # V3-02 — Settings screen -**Phase** Foundations · **Depends on** nothing (pairs with V3-01, V3-03) · **Size** S · **Status** Not started +**Phase** Foundations · **Depends on** nothing (pairs with V3-01, V3-03) · **Size** S · **Status** Done ## Goal One place for the preferences that currently have nowhere to live. @@ -47,3 +47,40 @@ without introducing a second source of truth is the only subtle part. ## Out of scope Account settings (v4). Theme selection (V3-16). + + +## Outcome + +"Move the map toggle off trip detail" (implementation step 3) turned out to be moot — +`mapEnabledProvider` already existed and drove `RideMap`'s visibility, but **no widget +anywhere ever offered a control to change it**. There was nothing to move. Settings adds +the first one. + +No `ConfigNotifier` was built — see V3-03's outcome for why the existing +`mapEnabledProvider`-style `StateProvider` pattern covers reactivity without it, and +`unitSystemProvider` was added the same way, in `providers.dart`, ahead of this ticket. + +Two implementation-step items shipped differently than drafted, both to avoid adding a +dependency disproportionate to an `S`-sized settings screen: + +- **No `package_info_plus`.** The About section's version string is a static literal + matching `pubspec.yaml`'s `1.0.0+1`, not a live package lookup. Fine today; would need + revisiting if the version ever needs to be authoritative from inside the running app + rather than copied by hand. +- **No privacy-policy link.** None is published yet (see `docs/LAUNCH.md`) and linking + to one that does not exist would be worse than omitting it. Shows a plain note instead. + Licences are still free: Flutter's built-in `showLicensePage` needed no new dependency. + +Endpoint validation accepts `http://` and `https://` with a non-empty host, and treats +an **empty** field as valid — that is how upload gets disabled, not an error state. A +non-empty invalid value is rejected with inline `errorText` and never reaches `Config`; +proven by a widget test that types garbage, taps Save, and asserts `Config.uploadEndpoint` +is still empty afterwards. + +`uploaderProvider` needed an explicit `ref.invalidate()` after saving the endpoint, for +the identical reason `unitSystemProvider` needed its own `StateProvider` rather than +reading through `configProvider` directly — a `Config` write never changes the `Config` +instance Riverpod is watching, so nothing downstream rebuilds unless told to. + +10 tests: 9 in `settings_screen_test.dart`, 1 confirming the record screen's settings +button is genuinely wired (not just present) in `widget_test.dart`. diff --git a/docs/v3/V3-03-units.md b/docs/v3/V3-03-units.md index 772b781..cdc957c 100644 --- a/docs/v3/V3-03-units.md +++ b/docs/v3/V3-03-units.md @@ -1,6 +1,6 @@ # V3-03 — Distance and speed units -**Phase** Foundations · **Depends on** V3-02 · **Size** S · **Status** Not started +**Phase** Foundations · **Depends on** V3-02 · **Size** S · **Status** Done ## Goal Imperial as well as metric, chosen once and applied everywhere. @@ -48,3 +48,40 @@ The obvious trap is converting too deep in the stack. Guard it with the export t ## Out of scope Temperature, pace (min/km) — pace is arguably right for running, revisit after V3-01. + + +## Outcome + +Built ahead of V3-02 in execution order, despite the ticket table listing it as +depending on V3-02 — the Settings screen needed something real to control, and the +formatting/`Config` plumbing itself has zero dependency on a screen existing. Both are +done; the numbering is unchanged. + +`UnitSystem` lives in `domain/models.dart`, not `ui/format.dart` as first drafted — +`Config` (a data/preferences-layer class) needed the enum too, and having it depend on +`ui/` read backwards. Moved to the domain layer alongside `Activity`, which every other +cross-cutting preference-like enum in this codebase already does. + +Reactivity reuses the exact pattern `mapEnabledProvider` already established — +`unitSystemProvider`, a `StateProvider` seeded from `Config` once and then +read/written directly by the UI — rather than introducing the heavier `ConfigNotifier` +class the ticket's implementation notes proposed. `Config` mutates its own backing +`SharedPreferences` in place, so a widget re-assigning the same `Config` instance to +`configProvider` was never going to notify anything; this sidesteps that without a new +abstraction. + +Threaded through all three screens plus the speed histogram's bucket labels, which +convert-and-round for display (`formatSpeedRangeLabel`) without changing how +`speedHistogram` itself bins — binning stays km/h always, matching the invariant that +storage and computation never see the display unit. Export was the one place explicitly +*not* touched: `gpx()`/`geoJson()` take no `UnitSystem` parameter at all, which is a +stronger guarantee than validating one. + +One real finding: `config_test.dart`'s locale-default test deliberately does not assert +a specific value, because it cannot know the test runner's own locale — and that caution +was immediately vindicated. `settings_screen_test.dart` first asserted a fresh `Config` +defaults to metric and failed, because this dev machine's own locale resolves to a +region in the imperial set. Fixed by seeding an explicit value before asserting, the +same technique already used elsewhere for exactly this reason. + +22 tests: 13 `format_test.dart`, 9 `config_test.dart`. diff --git a/lib/src/app/providers.dart b/lib/src/app/providers.dart index f99d479..187baf2 100644 --- a/lib/src/app/providers.dart +++ b/lib/src/app/providers.dart @@ -101,3 +101,12 @@ final recorderStateProvider = StreamProvider((ref) { final mapEnabledProvider = StateProvider( (ref) => ref.watch(configProvider)?.mapEnabled ?? true, ); + +/// Metric or imperial. Same shape as [mapEnabledProvider]: seeded from [Config] once, +/// then read and written directly by the UI so a change is visible immediately without +/// waiting on [configProvider]'s identity to change (it never does — `Config` mutates +/// its own backing preferences in place, so re-assigning the same instance would not +/// notify anything watching it). +final unitSystemProvider = StateProvider( + (ref) => ref.watch(configProvider)?.unitSystem ?? UnitSystem.metric, +); diff --git a/lib/src/config/config.dart b/lib/src/config/config.dart index 779f8dd..57bfb0b 100644 --- a/lib/src/config/config.dart +++ b/lib/src/config/config.dart @@ -4,13 +4,21 @@ /// recording must work with no server at all, and uploading is opt-in. library; +import 'dart:io'; import 'dart:math'; import 'package:shared_preferences/shared_preferences.dart'; +import '../domain/models.dart' show UnitSystem; + const _keyEndpoint = 'upload_endpoint'; const _keyDeviceId = 'device_id'; const _keyMapEnabled = 'map_enabled'; +const _keyUnitSystem = 'unit_system'; + +/// Countries that did not adopt metric for everyday distances. Not exhaustive — a +/// best-effort default, not a claim of authority. Anyone can override it in Settings. +const _imperialCountryCodes = {'US', 'LR', 'MM'}; class Config { Config(this._prefs); @@ -35,6 +43,32 @@ class Config { Future setMapEnabled(bool enabled) => _prefs.setBool(_keyMapEnabled, enabled); + /// Metric unless the device's own locale says otherwise. Explicit user choice, once + /// made, always wins over the locale guess. + UnitSystem get unitSystem { + final stored = _prefs.getString(_keyUnitSystem); + if (stored != null) { + return UnitSystem.values.firstWhere( + (u) => u.name == stored, + orElse: () => _localeDefaultUnitSystem(), + ); + } + return _localeDefaultUnitSystem(); + } + + Future setUnitSystem(UnitSystem system) => + _prefs.setString(_keyUnitSystem, system.name); + + static UnitSystem _localeDefaultUnitSystem() { + // "en_US", "en_US.UTF-8" and similar; the region is the second underscore- or + // hyphen-delimited segment when present. + final parts = Platform.localeName.split(RegExp('[_-]')); + final region = parts.length > 1 ? parts[1].toUpperCase() : ''; + return _imperialCountryCodes.contains(region) + ? UnitSystem.imperial + : UnitSystem.metric; + } + /// Stable per-install id so a server can distinguish riders in a group. String get deviceId { final existing = _prefs.getString(_keyDeviceId); diff --git a/lib/src/domain/models.dart b/lib/src/domain/models.dart index 065f9c4..4b6ef25 100644 --- a/lib/src/domain/models.dart +++ b/lib/src/domain/models.dart @@ -33,6 +33,15 @@ enum TripState { recording, paused, completed } /// `docs/v3/`. enum Activity { motorcycle, bicycle, scooter, skateboard, running, walking, other } +/// Metric or imperial, chosen once as a whole-app preference. +/// +/// **Display-only, forever.** Storage stays SI (metres, km/h) everywhere else in the +/// app — converting at the storage layer would corrupt every recorded ride and break the +/// cross-language parity harness, which compares raw metric values against the Kotlin +/// original. See `ui/format.dart`, the only place this enum's value should ever change a +/// number rather than just relabel one. See V3-03 in `docs/v3/`. +enum UnitSystem { metric, imperial } + /// One ride, from pressing Start to pressing Stop. /// /// The aggregate fields are denormalised on purpose. They are accumulated as points diff --git a/lib/src/ui/detail/trip_detail_screen.dart b/lib/src/ui/detail/trip_detail_screen.dart index 1628683..8396819 100644 --- a/lib/src/ui/detail/trip_detail_screen.dart +++ b/lib/src/ui/detail/trip_detail_screen.dart @@ -109,6 +109,7 @@ class TripDetailScreen extends ConsumerWidget { ), AsyncValue(hasValue: true, value: final d?) => _Body( detail: d, + units: ref.watch(unitSystemProvider), onEditActivity: () => _editActivity(context, ref, d.trip), ), AsyncValue(hasError: true, :final error) => EmptyState( @@ -219,14 +220,23 @@ class TripDetailScreen extends ConsumerWidget { } class _Body extends StatelessWidget { - const _Body({required this.detail, required this.onEditActivity}); + const _Body({ + required this.detail, + required this.units, + required this.onEditActivity, + }); final TripDetail detail; + final UnitSystem units; final VoidCallback onEditActivity; @override Widget build(BuildContext context) { final s = detail.summary; + final (distanceValue, distanceUnit) = formatDistanceParts( + s.distanceM, + unit: units, + ); return ListView( padding: const EdgeInsets.all(16), children: [ @@ -234,8 +244,8 @@ class _Body extends StatelessWidget { child: BigStat( key: const Key('distance'), label: 'DISTANCE', - value: (s.distanceM / 1000).toStringAsFixed(1), - unit: 'km', + value: distanceValue, + unit: distanceUnit, ), ), const SizedBox(height: 16), @@ -266,18 +276,21 @@ class _Body extends StatelessWidget { label: 'Stopped', value: formatDuration(s.stoppedMillis), ), - StatRow(label: 'Max speed', value: formatSpeed(s.maxSpeedKmh)), + StatRow( + label: 'Max speed', + value: formatSpeed(s.maxSpeedKmh, unit: units), + ), StatRow( label: 'Avg moving speed', - value: formatSpeed(s.avgMovingSpeedKmh), + value: formatSpeed(s.avgMovingSpeedKmh, unit: units), ), StatRow( label: 'Elevation gain', - value: formatElevation(s.elevationGainM), + value: formatElevation(s.elevationGainM, unit: units), ), StatRow( label: 'Elevation loss', - value: formatElevation(s.elevationLossM), + value: formatElevation(s.elevationLossM, unit: units), ), StatRow(label: 'Points', value: '${s.pointCount}'), StatRow(label: 'Segments', value: '${detail.segments.length}'), @@ -287,7 +300,7 @@ class _Body extends StatelessWidget { ), const SizedBox(height: 24), _SectionTitle('Speed distribution'), - _Histogram(buckets: detail.histogram), + _Histogram(buckets: detail.histogram, units: units), const SizedBox(height: 24), _SectionTitle('Elevation profile'), _ElevationChart(samples: detail.profile), @@ -351,9 +364,10 @@ class _SectionTitle extends StatelessWidget { /// Degrades to a message rather than an empty box: a ride of one point has no intervals /// to measure, and an unexplained blank looks like a bug. class _Histogram extends StatelessWidget { - const _Histogram({required this.buckets}); + const _Histogram({required this.buckets, required this.units}); final List buckets; + final UnitSystem units; @override Widget build(BuildContext context) { @@ -375,7 +389,7 @@ class _Histogram extends StatelessWidget { SizedBox( width: 64, child: Text( - b.label, + formatSpeedRangeLabel(b.fromKmh, b.toKmh, unit: units), style: TextStyle(fontSize: 12, color: colors.outline), ), ), diff --git a/lib/src/ui/format.dart b/lib/src/ui/format.dart index 5a9f809..1f5569f 100644 --- a/lib/src/ui/format.dart +++ b/lib/src/ui/format.dart @@ -5,12 +5,26 @@ /// Timestamps come from the platform location fix, which is UTC epoch millis, so /// everything here converts to the device zone. Formatting in UTC would show a 21:00 /// ride as tomorrow. +/// +/// ## Units (V3-01) +/// +/// Storage is SI everywhere — metres, km/h — forever. [UnitSystem] is a **display-only** +/// concern: every conversion happens here, at the last possible moment, and nowhere +/// else. Converting further up the stack would corrupt stored rides and break the +/// cross-language parity harness, which compares raw metric values against the Kotlin +/// original. Exports (`export/ride_export.dart`) deliberately do not take a +/// [UnitSystem] at all — GPX and GeoJSON are metric by specification, and changing that +/// would break every consumer that reads them. library; import 'package:intl/intl.dart'; import '../domain/models.dart'; +const double _milesPerMeter = 1 / 1609.344; +const double _feetPerMeter = 3.28084; +const double _mphPerKmh = 0.621371; + // Built per call rather than cached in a top-level final. The Kotlin original captured // Locale.getDefault() once at class-init; doing the same here would freeze the format // for the process lifetime and ignore a locale change. @@ -26,10 +40,61 @@ String formatFileTimestamp(int epochMillis) => /// A trip's own name, or a date-derived label when it has none. String tripLabel(Trip trip) => trip.name ?? formatDateTime(trip.startedAt); -String formatDistance(double meters) => meters < 1000 - ? '${meters.toInt()} m' - : '${(meters / 1000).toStringAsFixed(1)} km'; +String formatDistance(double meters, {UnitSystem unit = UnitSystem.metric}) { + if (unit == UnitSystem.imperial) { + final feet = meters * _feetPerMeter; + return feet < 1000 + ? '${feet.toInt()} ft' + : '${(meters * _milesPerMeter).toStringAsFixed(1)} mi'; + } + return meters < 1000 + ? '${meters.toInt()} m' + : '${(meters / 1000).toStringAsFixed(1)} km'; +} -String formatSpeed(double kmh) => '${kmh.toStringAsFixed(1)} km/h'; +String formatSpeed(double kmh, {UnitSystem unit = UnitSystem.metric}) => + unit == UnitSystem.imperial + ? '${(kmh * _mphPerKmh).toStringAsFixed(1)} mph' + : '${kmh.toStringAsFixed(1)} km/h'; -String formatElevation(double meters) => '${meters.toInt()} m'; +String formatElevation(double meters, {UnitSystem unit = UnitSystem.metric}) => + unit == UnitSystem.imperial + ? '${(meters * _feetPerMeter).toInt()} ft' + : '${meters.toInt()} m'; + +/// Value and unit split apart, for `BigStat`'s large-number-plus-small-label layout. +/// Always the "big" unit (km or mi) regardless of the short-distance threshold above — +/// a headline post-ride figure is never usefully shown in metres or feet. +(String value, String unit) formatDistanceParts( + double meters, { + UnitSystem unit = UnitSystem.metric, +}) => unit == UnitSystem.imperial + ? ((meters * _milesPerMeter).toStringAsFixed(1), 'mi') + : ((meters / 1000).toStringAsFixed(1), 'km'); + +/// As [formatDistanceParts], for the live speedo on the record screen. Zero decimal +/// places, matching that screen's existing display. +(String value, String unit) formatSpeedParts( + double kmh, { + UnitSystem unit = UnitSystem.metric, +}) => unit == UnitSystem.imperial + ? ((kmh * _mphPerKmh).toStringAsFixed(0), 'mph') + : (kmh.toStringAsFixed(0), 'km/h'); + +/// A speed-histogram bucket's `from–to` label, converted to the chosen unit and rounded +/// to whole units. A **relabel, not a re-bucketing**: `stats/ride_statistics.dart`'s +/// `speedHistogram` always bins in km/h regardless of the display unit, so switching +/// units changes what the numbers on this label mean without moving any data between +/// bars. +String formatSpeedRangeLabel( + int fromKmh, + int toKmh, { + UnitSystem unit = UnitSystem.metric, +}) { + if (unit == UnitSystem.imperial) { + final from = (fromKmh * _mphPerKmh).round(); + final to = (toKmh * _mphPerKmh).round(); + return '$from–$to'; + } + return '$fromKmh–$toKmh'; +} diff --git a/lib/src/ui/record/record_screen.dart b/lib/src/ui/record/record_screen.dart index 8608c49..972118c 100644 --- a/lib/src/ui/record/record_screen.dart +++ b/lib/src/ui/record/record_screen.dart @@ -35,9 +35,10 @@ class RecordUiState { } class RecordScreen extends ConsumerStatefulWidget { - const RecordScreen({super.key, this.onOpenTrips}); + const RecordScreen({super.key, this.onOpenTrips, this.onOpenSettings}); final VoidCallback? onOpenTrips; + final VoidCallback? onOpenSettings; @override ConsumerState createState() => _RecordScreenState(); @@ -116,6 +117,8 @@ class _RecordScreenState extends ConsumerState { final colors = Theme.of(context).colorScheme; final engine = ref.watch(recordingEngineProvider); final trip = ref.watch(activeTripProvider).valueOrNull; + final units = ref.watch(unitSystemProvider); + final (speedValue, speedUnit) = formatSpeedParts(_liveSpeed, unit: units); final ui = RecordUiState( trip: trip, @@ -155,9 +158,19 @@ class _RecordScreenState extends ConsumerState { color: colors.primary, ), ), - TextButton( - onPressed: widget.onOpenTrips, - child: const Text('Rides'), + Row( + children: [ + TextButton( + onPressed: widget.onOpenTrips, + child: const Text('Rides'), + ), + IconButton( + key: const Key('open-settings'), + icon: const Icon(Icons.settings_outlined), + tooltip: 'Settings', + onPressed: widget.onOpenSettings, + ), + ], ), ], ), @@ -169,8 +182,8 @@ class _RecordScreenState extends ConsumerState { BigStat( key: const Key('speed'), label: 'SPEED', - value: ui.currentSpeedKmh.toStringAsFixed(0), - unit: 'km/h', + value: speedValue, + unit: speedUnit, ), const SizedBox(height: 16), @@ -190,7 +203,10 @@ class _RecordScreenState extends ConsumerState { ] else ...[ StatRow( label: 'Distance', - value: formatDistance(ui.trip!.distanceM), + value: formatDistance( + ui.trip!.distanceM, + unit: units, + ), ), StatRow( label: 'Elapsed', @@ -202,7 +218,10 @@ class _RecordScreenState extends ConsumerState { ), StatRow( label: 'Max speed', - value: formatSpeed(ui.trip!.maxSpeedKmh), + value: formatSpeed( + ui.trip!.maxSpeedKmh, + unit: units, + ), ), StatRow( label: 'Points captured', @@ -210,7 +229,10 @@ class _RecordScreenState extends ConsumerState { ), StatRow( label: 'Elevation gain', - value: formatElevation(ui.trip!.elevationGainM), + value: formatElevation( + ui.trip!.elevationGainM, + unit: units, + ), ), if (ui.isPaused) const StatRow(label: 'Status', value: 'Paused'), diff --git a/lib/src/ui/router.dart b/lib/src/ui/router.dart index 2856f05..f21c189 100644 --- a/lib/src/ui/router.dart +++ b/lib/src/ui/router.dart @@ -10,12 +10,14 @@ import 'package:go_router/go_router.dart'; import 'detail/trip_detail_screen.dart'; import 'record/record_screen.dart'; +import 'settings/settings_screen.dart'; import 'trips/trips_screen.dart'; abstract final class Routes { static const record = '/'; static const trips = '/trips'; static const tripDetail = '/trip/:tripId'; + static const settings = '/settings'; static String detailFor(int tripId) => '/trip/$tripId'; } @@ -25,8 +27,15 @@ GoRouter buildRouter() => GoRouter( routes: [ GoRoute( path: Routes.record, + builder: (context, state) => RecordScreen( + onOpenTrips: () => context.push(Routes.trips), + onOpenSettings: () => context.push(Routes.settings), + ), + ), + GoRoute( + path: Routes.settings, builder: (context, state) => - RecordScreen(onOpenTrips: () => context.push(Routes.trips)), + SettingsScreen(onBack: () => context.pop()), ), GoRoute( path: Routes.trips, diff --git a/lib/src/ui/settings/settings_screen.dart b/lib/src/ui/settings/settings_screen.dart new file mode 100644 index 0000000..58b8f43 --- /dev/null +++ b/lib/src/ui/settings/settings_screen.dart @@ -0,0 +1,217 @@ +/// One place for the preferences that previously had nowhere to live — most of all the +/// upload endpoint, which has **never** had UI, in the native app or the port. See V3-02. +/// +/// Kept plain on purpose: this is not a screen anyone should spend time in. +library; + +import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; + +import '../../app/providers.dart'; +import '../../config/config.dart'; +import '../../domain/models.dart'; + +class SettingsScreen extends ConsumerWidget { + const SettingsScreen({super.key, this.onBack}); + + final VoidCallback? onBack; + + @override + Widget build(BuildContext context, WidgetRef ref) { + final config = ref.watch(configProvider); + + return Scaffold( + appBar: AppBar( + leading: IconButton( + key: const Key('back'), + icon: const Icon(Icons.arrow_back), + onPressed: onBack, + ), + title: const Text('Settings'), + ), + // 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. + body: config == null + ? const Center(child: CircularProgressIndicator()) + : _SettingsBody(config: config), + ); + } +} + +class _SettingsBody extends ConsumerStatefulWidget { + const _SettingsBody({required this.config}); + + final Config config; + + @override + ConsumerState<_SettingsBody> createState() => _SettingsBodyState(); +} + +class _SettingsBodyState extends ConsumerState<_SettingsBody> { + late final _endpointController = + TextEditingController(text: widget.config.uploadEndpoint); + String? _endpointError; + + @override + void dispose() { + _endpointController.dispose(); + super.dispose(); + } + + bool _looksLikeAnEndpoint(String value) { + if (value.isEmpty) return true; // empty disables upload -- always valid + final uri = Uri.tryParse(value); + return uri != null && + (uri.scheme == 'http' || uri.scheme == 'https') && + uri.host.isNotEmpty; + } + + Future _saveEndpoint() async { + final value = _endpointController.text.trim(); + if (!_looksLikeAnEndpoint(value)) { + setState( + () => _endpointError = 'Must be a full http:// or https:// address', + ); + return; + } + setState(() => _endpointError = null); + await widget.config.setUploadEndpoint(value); + // TelemetryUploader is built once per Config value inside uploaderProvider, which + // does not know a preference changed underneath the same Config instance -- see + // the note on unitSystemProvider for why this cannot rely on identity changing. + ref.invalidate(uploaderProvider); + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( + SnackBar(content: Text(value.isEmpty ? 'Upload disabled' : 'Endpoint saved')), + ); + } + + @override + Widget build(BuildContext context) { + final mapEnabled = ref.watch(mapEnabledProvider); + final units = ref.watch(unitSystemProvider); + + return ListView( + children: [ + const _SectionHeader('Map'), + SwitchListTile( + key: const Key('map-enabled-switch'), + title: const Text('Render maps on trip detail'), + subtitle: const Text( + 'Off means no tile is ever fetched -- not just hidden.', + ), + value: mapEnabled, + onChanged: (value) async { + await widget.config.setMapEnabled(value); + ref.read(mapEnabledProvider.notifier).state = value; + }, + ), + const Divider(), + const _SectionHeader('Units'), + Padding( + padding: const EdgeInsets.symmetric(horizontal: 16, vertical: 8), + child: SegmentedButton( + key: const Key('unit-system-selector'), + segments: const [ + ButtonSegment(value: UnitSystem.metric, label: Text('Metric')), + ButtonSegment(value: UnitSystem.imperial, label: Text('Imperial')), + ], + selected: {units}, + onSelectionChanged: (selection) async { + final chosen = selection.first; + await widget.config.setUnitSystem(chosen); + ref.read(unitSystemProvider.notifier).state = chosen; + }, + ), + ), + const Divider(), + const _SectionHeader('Sync'), + Padding( + padding: const EdgeInsets.symmetric(horizontal: 16), + child: TextField( + key: const Key('endpoint-field'), + controller: _endpointController, + decoration: InputDecoration( + labelText: 'Upload endpoint', + hintText: 'https://example.com/ingest', + helperText: 'Leave empty to keep recording fully local.', + errorText: _endpointError, + ), + keyboardType: TextInputType.url, + onSubmitted: (_) => _saveEndpoint(), + ), + ), + Padding( + padding: const EdgeInsets.only(left: 16, top: 4, bottom: 8), + child: Align( + alignment: Alignment.centerRight, + child: TextButton( + key: const Key('save-endpoint'), + onPressed: _saveEndpoint, + child: const Text('Save'), + ), + ), + ), + ListTile( + title: const Text('Device ID'), + subtitle: Text(widget.config.deviceId), + trailing: IconButton( + key: const Key('copy-device-id'), + icon: const Icon(Icons.copy), + tooltip: 'Copy', + onPressed: () async { + await Clipboard.setData( + ClipboardData(text: widget.config.deviceId), + ); + if (context.mounted) { + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar(content: Text('Device ID copied')), + ); + } + }, + ), + ), + const Divider(), + const _SectionHeader('About'), + const ListTile(title: Text('Version'), subtitle: Text('1.0.0')), + ListTile( + title: const Text('Privacy'), + subtitle: const Text( + 'No policy published yet -- see docs/LAUNCH.md before a public release.', + ), + ), + ListTile( + title: const Text('Open source licences'), + trailing: const Icon(Icons.chevron_right), + onTap: () => showLicensePage( + context: context, + applicationName: 'Rippr', + applicationVersion: '1.0.0', + ), + ), + ], + ); + } +} + +class _SectionHeader extends StatelessWidget { + const _SectionHeader(this.text); + + final String text; + + @override + Widget build(BuildContext context) => Padding( + padding: const EdgeInsets.fromLTRB(16, 16, 16, 4), + child: Text( + text.toUpperCase(), + style: TextStyle( + fontSize: 12, + letterSpacing: 2, + fontWeight: FontWeight.bold, + color: Theme.of(context).colorScheme.primary, + ), + ), + ); +} diff --git a/lib/src/ui/trips/trips_screen.dart b/lib/src/ui/trips/trips_screen.dart index 7bf949f..ef73e91 100644 --- a/lib/src/ui/trips/trips_screen.dart +++ b/lib/src/ui/trips/trips_screen.dart @@ -31,6 +31,7 @@ class TripsScreen extends ConsumerWidget { final colors = Theme.of(context).colorScheme; final tripsAsync = ref.watch(completedTripsProvider); final trips = tripsAsync.valueOrNull ?? const []; + final units = ref.watch(unitSystemProvider); // Drop ids that no longer exist, so a deleted trip cannot linger in the selection. final live = ref @@ -122,6 +123,7 @@ class TripsScreen extends ConsumerWidget { trip: trip, selected: live.contains(trip.id), selecting: selecting, + units: units, onTap: () { if (selecting) { _toggle(ref, trip.id); @@ -203,6 +205,7 @@ class _TripTile extends StatelessWidget { required this.trip, required this.selected, required this.selecting, + required this.units, required this.onTap, required this.onLongPress, }); @@ -210,6 +213,7 @@ class _TripTile extends StatelessWidget { final Trip trip; final bool selected; final bool selecting; + final UnitSystem units; final VoidCallback onTap; final VoidCallback onLongPress; @@ -233,9 +237,9 @@ class _TripTile extends StatelessWidget { style: const TextStyle(fontWeight: FontWeight.w600), ), subtitle: Text( - '${formatDistance(trip.distanceM)} · ' + '${formatDistance(trip.distanceM, unit: units)} · ' '${formatDuration(trip.movingMillis)} · ' - 'max ${formatSpeed(trip.maxSpeedKmh)}', + 'max ${formatSpeed(trip.maxSpeedKmh, unit: units)}', style: TextStyle(color: colors.outline), ), ), diff --git a/test/config_test.dart b/test/config_test.dart new file mode 100644 index 0000000..83875db --- /dev/null +++ b/test/config_test.dart @@ -0,0 +1,87 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:rippr/src/config/config.dart'; +import 'package:rippr/src/domain/models.dart'; +import 'package:shared_preferences/shared_preferences.dart'; + +/// `Config` had no dedicated tests before V3-03 — every existing widget test leaves +/// `configProvider` null, so this is its first direct coverage. +void main() { + Future freshConfig([Map initial = const {}]) async { + SharedPreferences.setMockInitialValues(initial); + return Config.load(); + } + + group('unitSystem', () { + test('an explicit choice always wins over the locale default', () async { + final config = await freshConfig(); + await config.setUnitSystem(UnitSystem.imperial); + expect(config.unitSystem, UnitSystem.imperial); + + await config.setUnitSystem(UnitSystem.metric); + expect(config.unitSystem, UnitSystem.metric); + }); + + test('with nothing stored, falls back to a locale default without throwing', () async { + // The exact value is environment-dependent (it reads the real platform locale), + // so this asserts the fallback path is safe to reach, not a specific answer. + final config = await freshConfig(); + expect(config.unitSystem, isA()); + }); + }); + + group('mapEnabled', () { + test('defaults to true', () async { + final config = await freshConfig(); + expect(config.mapEnabled, isTrue); + }); + + test('round-trips through set/get', () async { + final config = await freshConfig(); + await config.setMapEnabled(false); + expect(config.mapEnabled, isFalse); + }); + }); + + group('uploadEndpoint', () { + test('defaults to empty, not null or a placeholder', () async { + final config = await freshConfig(); + expect(config.uploadEndpoint, ''); + }); + + test('trims whitespace on write', () async { + final config = await freshConfig(); + await config.setUploadEndpoint(' https://example.test/ingest '); + expect(config.uploadEndpoint, 'https://example.test/ingest'); + }); + }); + + group('deviceId', () { + test('is generated once and then stable across reads', () async { + final config = await freshConfig(); + final first = config.deviceId; + final second = config.deviceId; + expect(first, second); + expect(first, isNotEmpty); + }); + + test('is a v4 UUID', () async { + final config = await freshConfig(); + // xxxxxxxx-xxxx-4xxx-{8,9,a,b}xxx-xxxxxxxxxxxx + final v4 = RegExp( + r'^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$', + ); + expect(v4.hasMatch(config.deviceId), isTrue, + reason: 'got ${config.deviceId}'); + }); + + test('persists across a fresh Config over the same preferences', () async { + SharedPreferences.setMockInitialValues({}); + final first = await Config.load(); + final id = first.deviceId; + + final second = await Config.load(); + expect(second.deviceId, id, + reason: 'a server distinguishing riders needs this to be stable'); + }); + }); +} diff --git a/test/format_test.dart b/test/format_test.dart new file mode 100644 index 0000000..a0b2788 --- /dev/null +++ b/test/format_test.dart @@ -0,0 +1,115 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:rippr/src/domain/models.dart'; +import 'package:rippr/src/export/ride_export.dart'; +import 'package:rippr/src/ui/format.dart'; + +/// V3-03: distance and speed units. Metric is the default everywhere, so every existing +/// call site that omits `unit:` keeps behaving exactly as before — these tests are about +/// what changes when a caller opts into imperial. +void main() { + group('formatDistance', () { + test('metric switches from metres to kilometres at 1000 m', () { + expect(formatDistance(999), '999 m'); + expect(formatDistance(1000), '1.0 km'); + expect(formatDistance(12345.6), '12.3 km'); + }); + + test('imperial switches from feet to miles at 1000 ft', () { + // 1000 ft is ~304.8 m. + expect(formatDistance(100, unit: UnitSystem.imperial), '328 ft'); + expect( + formatDistance(305, unit: UnitSystem.imperial), + '0.2 mi', + reason: 'just past the 1000 ft threshold (1000 ft is not 1 mile — 5280 ft is)', + ); + // 12345.6 m is ~7.67 mi. + expect(formatDistance(12345.6, unit: UnitSystem.imperial), '7.7 mi'); + }); + + test('metric is the default when unit is omitted', () { + expect(formatDistance(1500), formatDistance(1500, unit: UnitSystem.metric)); + }); + }); + + group('formatSpeed', () { + test('metric prints km/h to one decimal', () { + expect(formatSpeed(88.5), '88.5 km/h'); + }); + + test('imperial converts to mph', () { + // 100 km/h is ~62.1 mph. + expect(formatSpeed(100, unit: UnitSystem.imperial), '62.1 mph'); + }); + }); + + group('formatElevation', () { + test('metric prints whole metres', () { + expect(formatElevation(1045.7), '1045 m'); + }); + + test('imperial converts to whole feet', () { + // 1000 m is ~3280.84 ft. + expect(formatElevation(1000, unit: UnitSystem.imperial), '3280 ft'); + }); + }); + + group('parts helpers used by BigStat', () { + test('formatDistanceParts always uses the "big" unit, km or mi', () { + expect(formatDistanceParts(500), ('0.5', 'km')); + expect(formatDistanceParts(1609.344, unit: UnitSystem.imperial), ('1.0', 'mi')); + }); + + test('formatSpeedParts has zero decimal places, matching the live speedo', () { + expect(formatSpeedParts(88.6), ('89', 'km/h')); + expect(formatSpeedParts(100, unit: UnitSystem.imperial), ('62', 'mph')); + }); + }); + + group('formatSpeedRangeLabel', () { + test('metric passes the km/h boundaries through unchanged', () { + expect(formatSpeedRangeLabel(10, 20), '10–20'); + }); + + test('imperial converts and rounds the boundaries, not the underlying bucket', () { + // 10 km/h ~ 6 mph, 20 km/h ~ 12 mph. + expect(formatSpeedRangeLabel(10, 20, unit: UnitSystem.imperial), '6–12'); + }); + }); + + group('unit system never reaches export (the risk this ticket names)', () { + // gpx() and geoJson() take no UnitSystem parameter at all -- there is no argument to + // thread incorrectly. This proves the invariant one level up: formatting the same + // stored value under both units produces different text, but export always reads the + // one stored value, regardless of what a caller displays it as. + const trip = Trip( + id: 1, + startedAt: 1700000000000, + endedAt: 1700000600000, + state: TripState.completed, + distanceM: 12345.6, + maxSpeedKmh: 88.5, + ); + + test('display formatting differs by unit for the same stored value', () { + expect( + formatDistance(trip.distanceM, unit: UnitSystem.metric), + isNot(formatDistance(trip.distanceM, unit: UnitSystem.imperial)), + ); + }); + + test('export output is identical regardless of what unit the UI last showed', () { + // GPX carries no trip-level distance field at all (it is built from individual + // elements) -- deterministic regardless of unit is the property under + // test, not any particular substring. + final first = gpx(trip, const [], const [], 'Ride'); + final second = gpx(trip, const [], const [], 'Ride'); + expect(first, second); + + // GeoJSON does carry trip.distanceM in its properties, and it is the raw metric + // figure -- 12345.6, never a converted "7.7 mi". + final json = geoJson(trip, const [], const [], 'Ride'); + expect(json, contains('"distance_m": 12345.6')); + expect(json, isNot(contains('mi')), reason: 'export is metric by specification'); + }); + }); +} diff --git a/test/settings_screen_test.dart b/test/settings_screen_test.dart new file mode 100644 index 0000000..def3d68 --- /dev/null +++ b/test/settings_screen_test.dart @@ -0,0 +1,164 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:rippr/src/app/providers.dart'; +import 'package:rippr/src/config/config.dart'; +import 'package:rippr/src/domain/models.dart'; +import 'package:rippr/src/ui/settings/settings_screen.dart'; +import 'package:rippr/src/ui/theme.dart'; +import 'package:shared_preferences/shared_preferences.dart'; + +/// V3-02: every `Config` value must be viewable and editable here, and every write must +/// actually reach `Config` -- not just update local widget state that looks right until +/// the next app launch. +void main() { + late Config config; + + setUp(() async { + SharedPreferences.setMockInitialValues({}); + config = await Config.load(); + }); + + Widget host() => ProviderScope( + overrides: [configProvider.overrideWith((ref) => config)], + child: MaterialApp(theme: ripprTheme(), home: const SettingsScreen()), + ); + + testWidgets('the map switch reflects and writes through to Config', (tester) async { + await tester.pumpWidget(host()); + await tester.pumpAndSettle(); + + final initial = tester + .widget(find.byKey(const Key('map-enabled-switch'))) + .value; + expect(initial, isTrue, reason: 'Config.mapEnabled defaults to true'); + + await tester.tap(find.byKey(const Key('map-enabled-switch'))); + await tester.pumpAndSettle(); + + expect(config.mapEnabled, isFalse, + reason: 'the switch must write through to Config, not just local state'); + }); + + testWidgets('changing the map switch is visible to other providers immediately', + (tester) async { + // V3-02's acceptance criterion: "changing the map toggle takes effect without an + // app restart". mapEnabledProvider is what trip detail actually reads. + await tester.pumpWidget(host()); + await tester.pumpAndSettle(); + + final container = ProviderScope.containerOf( + tester.element(find.byType(SettingsScreen)), + ); + expect(container.read(mapEnabledProvider), isTrue); + + await tester.tap(find.byKey(const Key('map-enabled-switch'))); + await tester.pumpAndSettle(); + + expect(container.read(mapEnabledProvider), isFalse); + }); + + testWidgets('the unit selector writes through to Config', (tester) async { + // The default depends on the test runner's own locale (see config_test.dart), so + // start from an explicit, known value rather than assuming metric. + await config.setUnitSystem(UnitSystem.metric); + await tester.pumpWidget(host()); + await tester.pumpAndSettle(); + + await tester.tap(find.text('Imperial')); + await tester.pumpAndSettle(); + expect(config.unitSystem, UnitSystem.imperial); + + await tester.tap(find.text('Metric')); + await tester.pumpAndSettle(); + expect(config.unitSystem, UnitSystem.metric); + }); + + group('upload endpoint', () { + testWidgets('a valid https URL is saved', (tester) async { + await tester.pumpWidget(host()); + await tester.pumpAndSettle(); + + await tester.enterText( + find.byKey(const Key('endpoint-field')), + 'https://example.test/ingest', + ); + await tester.tap(find.byKey(const Key('save-endpoint'))); + await tester.pumpAndSettle(); + + expect(config.uploadEndpoint, 'https://example.test/ingest'); + expect(find.text('Endpoint saved'), findsOneWidget); + }); + + testWidgets('an invalid endpoint is rejected with a readable message, not ' + 'silently stored', (tester) async { + await tester.pumpWidget(host()); + await tester.pumpAndSettle(); + + await tester.enterText( + find.byKey(const Key('endpoint-field')), + 'not a url at all', + ); + await tester.tap(find.byKey(const Key('save-endpoint'))); + await tester.pumpAndSettle(); + + expect(config.uploadEndpoint, '', + reason: 'garbage input must never reach storage'); + expect(find.textContaining('http'), findsWidgets); + }); + + testWidgets('a non-http scheme is rejected', (tester) async { + await tester.pumpWidget(host()); + await tester.pumpAndSettle(); + + await tester.enterText( + find.byKey(const Key('endpoint-field')), + 'ftp://example.test/ingest', + ); + await tester.tap(find.byKey(const Key('save-endpoint'))); + await tester.pumpAndSettle(); + + expect(config.uploadEndpoint, ''); + }); + + testWidgets('clearing the endpoint is a valid way to disable upload', + (tester) async { + await config.setUploadEndpoint('https://example.test/ingest'); + await tester.pumpWidget(host()); + await tester.pumpAndSettle(); + + await tester.enterText(find.byKey(const Key('endpoint-field')), ''); + await tester.tap(find.byKey(const Key('save-endpoint'))); + await tester.pumpAndSettle(); + + expect(config.uploadEndpoint, ''); + expect(find.text('Upload disabled'), findsOneWidget); + }); + }); + + testWidgets('the device id is shown and a copy control exists', (tester) async { + await tester.pumpWidget(host()); + await tester.pumpAndSettle(); + + expect(find.text(config.deviceId), findsOneWidget); + expect(find.byKey(const Key('copy-device-id')), findsOneWidget); + + // Tapping must not throw even though no real clipboard exists in the test harness. + await tester.tap(find.byKey(const Key('copy-device-id'))); + await tester.pumpAndSettle(); + expect(tester.takeException(), isNull); + }); + + testWidgets('shows a spinner rather than crashing while Config is still loading', + (tester) async { + await tester.pumpWidget( + const ProviderScope( + child: MaterialApp(home: SettingsScreen()), + ), + ); + await tester.pump(); + + expect(find.byType(CircularProgressIndicator), findsOneWidget); + expect(tester.takeException(), isNull); + }); +} diff --git a/test/widget_test.dart b/test/widget_test.dart index 815bc13..1b173cd 100644 --- a/test/widget_test.dart +++ b/test/widget_test.dart @@ -10,6 +10,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/ui/detail/trip_detail_screen.dart'; +import 'package:rippr/src/ui/format.dart'; import 'package:rippr/src/ui/record/record_screen.dart'; import 'package:rippr/src/ui/theme.dart'; import 'package:rippr/src/ui/trips/trips_screen.dart'; @@ -67,13 +68,18 @@ void main() { /// /// The theme is deliberately included: the black-on-black bug was invisible to logic /// tests and only a rendered widget can catch its equivalent. - Widget host(Widget child, {bool map = true}) => ProviderScope( + Widget host( + Widget child, { + bool map = true, + UnitSystem units = UnitSystem.metric, + }) => ProviderScope( overrides: [ databaseProvider.overrideWithValue(db), locationSourceProvider.overrideWithValue(source), // The map is off by default in tests that are not about the map: it fetches // tiles, which a widget test cannot serve, and it changes scroll geometry. mapEnabledProvider.overrideWith((ref) => map), + unitSystemProvider.overrideWith((ref) => units), ], child: MaterialApp(theme: ripprTheme(), home: child), ); @@ -187,6 +193,21 @@ void main() { expect(colour, isNot(ripprBackground)); 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(); + + 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'); + }); }); group('trips list', () { @@ -380,5 +401,43 @@ void main() { expect(find.text(activityLabel(Activity.walking)), findsOneWidget); expect((await repo.tripById(id))!.activity, Activity.walking); }); + + screenTest('switching units changes the rendered distance label (V3-03)', + (tester) async { + final id = await seedCompletedTrip( + startedAt: 1000, endedAt: 5000, points: 11); + final trip = (await repo.tripById(id))!; + + await tester.pumpWidget( + host(TripDetailScreen(tripId: id), map: false), + ); + await tester.pumpAndSettle(); + final (metricValue, metricUnit) = formatDistanceParts(trip.distanceM); + expect(find.text(metricValue), findsWidgets); + // BigStat renders its unit as " $unit", with a leading space, next to the figure. + expect(find.text(' $metricUnit'), findsOneWidget); + + // A full unmount first: Riverpod's ProviderScope does not reliably re-seed a + // StateProvider's initial value on an override change alone if the same + // container survives the rebuild, so pumping a second host() directly on top of + // the first would silently keep reading the metric container. + await tester.pumpWidget(const SizedBox.shrink()); + await tester.pumpWidget( + host(TripDetailScreen(tripId: id), map: false, units: UnitSystem.imperial), + ); + await tester.pumpAndSettle(); + final (imperialValue, imperialUnit) = formatDistanceParts( + trip.distanceM, + unit: UnitSystem.imperial, + ); + expect(find.text(' $imperialUnit'), findsOneWidget); + expect(imperialUnit, isNot(metricUnit)); + // A short 11-point ride at ~11 m hops is short enough that the two rounded values + // could coincidentally match as text; the unit label changing is what this test is + // really proving, but assert the value differs too when it is safe to. + if (metricValue != imperialValue) { + expect(find.text(imperialValue), findsWidgets); + } + }); }); }