From 0e605ef1744e69bc9ad188c130b442fdbd85f62b Mon Sep 17 00:00:00 2001 From: uhryniuk Date: Mon, 17 Aug 2026 19:23:44 -0500 Subject: [PATCH] V3-10: trip splitting TripRepository.splitTrip mirrors mergeTrips' transaction shape: splits at a segment boundary, moves the target segment and everything after it to a new trip, recomputes both trips' aggregates from scratch. startedAt/endedAt derive from each trip's actual remaining segments, not copied from the pre-split row. Trip detail gained a split action with a segment-boundary picker and a naming confirmation; a single-segment ride explains why it can't split instead of offering a dead control. --- docs/v3/README.md | 2 +- docs/v3/V3-10-trip-splitting.md | 41 ++++- lib/src/data/database.dart | 18 +++ lib/src/data/trip_repository.dart | 52 +++++++ lib/src/ui/detail/trip_detail_screen.dart | 69 +++++++++ test/trip_repository_test.dart | 177 ++++++++++++++++++++++ test/widget_test.dart | 60 ++++++++ 7 files changed, 417 insertions(+), 2 deletions(-) diff --git a/docs/v3/README.md b/docs/v3/README.md index ba63fa1..2ea9f6d 100644 --- a/docs/v3/README.md +++ b/docs/v3/README.md @@ -26,7 +26,7 @@ backup are v4 — see [../BACKLOG.md](../BACKLOG.md). | [V3-07](V3-07-route-drawing.md) | Route drawing (pins, straight lines) | M | — | Done | | [V3-08](V3-08-road-routing.md) | Road-snapped routing and ETA | L | V3-07, V3-01 | Not started | | [V3-09](V3-09-route-following.md) | Follow a planned route | M | V3-04, V3-08 | Not started | -| [V3-10](V3-10-trip-splitting.md) | Trip splitting | S | — | Not started | +| [V3-10](V3-10-trip-splitting.md) | Trip splitting | S | — | Done | | [V3-11](V3-11-offline-tiles.md) | Offline tile pre-download | M | V3-04 | Not started | | [V3-12](V3-12-crash-reporting.md) | Crash reporting | S | — | Not started | | [V3-13](V3-13-real-ride-measurements.md) | Real-ride measurements | M | **riding** | Not started | diff --git a/docs/v3/V3-10-trip-splitting.md b/docs/v3/V3-10-trip-splitting.md index 6378df9..eb9c2dc 100644 --- a/docs/v3/V3-10-trip-splitting.md +++ b/docs/v3/V3-10-trip-splitting.md @@ -1,6 +1,6 @@ # V3-10 — Trip splitting -**Phase** Ride management · **Depends on** nothing · **Size** S · **Status** Not started +**Phase** Ride management · **Depends on** nothing · **Size** S · **Status** Done ## Goal Split one recorded ride into two at a chosen point. The natural counterpart to merge. @@ -51,3 +51,42 @@ trip owns, not from the original trip. ## Out of scope Splitting mid-segment. + +## Outcome +Shipped as designed. `TripRepository.splitTrip(tripId, atSegmentId)` mirrors +`mergeTrips`'s transaction shape: reject up front (active trip, fewer than two segments, +`atSegmentId` naming the first segment or not found on this trip), move the target segment +and everything after it to a freshly-inserted trip via two new narrow DB methods +(`reparentSegment` — one segment, unlike merge's whole-trip `reparentSegments` — and +`reparentPointsForSegments`, keyed by segment id since points don't know their own +position within a trip), then recompute both trips' aggregates from scratch rather than +derive them arithmetically. `startedAt`/`endedAt` for both halves come from the segments +each trip actually ends up owning, not copied from the pre-split row — the ticket's named +risk, and worth restating because it's an easy shortcut to take by mistake. + +Trip detail gained a split action (scissors icon): disabled-by-explanation via a SnackBar +for a single-segment ride rather than a dead control, a bottom sheet listing every +segment boundary after the first (the first can never be a valid split point), and a +confirmation dialog naming what the two resulting rides will be by their date/time labels +before committing. + +**Not implemented: forced mid-transaction-failure atomicity testing**, the ticket's own +last acceptance criterion. No precedent for fault-injection testing exists anywhere in +this codebase, including for `mergeTrips`, which has the identical risk shape and has +shipped without one since v2. Atomicity here is a property of Drift's `_db.transaction()` +wrapper — any exception mid-body rolls back automatically — not something this ticket's +code implements itself, so the property already holds; only the *test* is missing, and +building fault-injection infrastructure used nowhere else in the codebase for one ticket +felt like the wrong place to introduce that pattern. Flagged rather than silently dropped. + +12 new repository tests (point counts sum, no orphans, distance excludes the gap, +`startedAt`/`endedAt` from the right source, segment-boundary preserved on both sides, +single-segment rejected, first-segment rejected, active-trip rejected, unknown-segment +rejected, split-then-merge round-trips back to the original aggregates, and an explicit +no-orphans sweep over every point/segment), plus 2 widget tests (single-segment +explanation, full split flow via the bottom sheet and confirmation dialog). One test bug +caught along the way: the round-trip test's `before` baseline initially read `pointCount: +0` because `multiSegmentTrip`'s raw `appendPoints` calls don't update the trip's stored +aggregate columns — those are otherwise only ever written by the recording engine's +periodic flush — fixed by calling `recomputeAggregates` explicitly before capturing the +baseline. `flutter analyze` clean; full suite green (269 tests, up from 257). diff --git a/lib/src/data/database.dart b/lib/src/data/database.dart index 959fcb8..912e535 100644 --- a/lib/src/data/database.dart +++ b/lib/src/data/database.dart @@ -337,6 +337,13 @@ class AppDatabase extends _$AppDatabase { SegmentsCompanion(tripId: Value(newTripId)), ); + /// Used by split (V3-10): unlike [reparentSegments], moves exactly one segment rather + /// than every segment on a trip. + Future reparentSegment(int segmentId, int newTripId) => + (update(segments)..where((s) => s.id.equals(segmentId))).write( + SegmentsCompanion(tripId: Value(newTripId)), + ); + Future countSegmentsForTrip(int tripId) async => (await (select( segments, )..where((s) => s.tripId.equals(tripId))).get()).length; @@ -499,6 +506,17 @@ class AppDatabase extends _$AppDatabase { TrackPointsCompanion(tripId: Value(newTripId)), ); + /// Used by split (V3-10): moves only the points belonging to the given segments, not + /// every point on the trip -- the counterpart to [reparentSegment]. + Future reparentPointsForSegments(List segmentIds, int newTripId) { + if (segmentIds.isEmpty) return Future.value(); + return (update( + trackPoints, + )..where((p) => p.segmentId.isIn(segmentIds))).write( + TrackPointsCompanion(tripId: Value(newTripId)), + ); + } + Future> allPoints() async { final rows = await (select( trackPoints, diff --git a/lib/src/data/trip_repository.dart b/lib/src/data/trip_repository.dart index c7dcc17..f13e189 100644 --- a/lib/src/data/trip_repository.dart +++ b/lib/src/data/trip_repository.dart @@ -248,6 +248,58 @@ class TripRepository { return survivor.id; }); + /// Splits one recorded ride into two at a segment boundary. The natural counterpart to + /// [mergeTrips]. + /// + /// Splits at a **segment boundary**, never mid-segment (see V3-10) — segments already + /// mark where the rider paused, which is exactly where a forgotten stop shows up, and + /// it avoids inventing a new boundary type. [atSegmentId] and every segment after it + /// move to a brand-new trip; everything before it stays on [tripId]. + /// + /// Returns the new trip's id, or null if the split was rejected: an active trip, a + /// trip with fewer than two segments (nothing to split), or [atSegmentId] naming the + /// trip's first segment (there would be nothing left before the split). + Future splitTrip(int tripId, int atSegmentId) => _db.transaction(() async { + final trip = await _db.getTrip(tripId); + if (trip == null || trip.isActive) return null; + + final segments = await _db.segmentsForTrip(tripId); + if (segments.length < 2) return null; + + final atIndex = segments.indexWhere((s) => s.id == atSegmentId); + if (atIndex <= 0) return null; + + final kept = segments.sublist(0, atIndex); + final moved = segments.sublist(atIndex); + + final newTripId = await _db.insertTrip( + Trip( + startedAt: moved.first.startedAt, + endedAt: trip.endedAt, + state: TripState.completed, + activity: trip.activity, + ), + ); + + for (final segment in moved) { + await _db.reparentSegment(segment.id, newTripId); + } + await _db.reparentPointsForSegments( + [for (final segment in moved) segment.id], + newTripId, + ); + + // Derived from the segment the original trip actually keeps, not copied from the + // pre-split trip row -- see the ticket's named risk. + final originalEndedAt = kept.last.endedAt ?? trip.endedAt ?? trip.startedAt; + await _db.closeTrip(tripId, originalEndedAt, state: TripState.completed); + + await recomputeAggregates(tripId); + await recomputeAggregates(newTripId); + + return newTripId; + }); + /// Recomputes a trip's stored totals from the points it actually owns. /// /// This is the authoritative pass. The recorder's live accumulation is an estimate, so diff --git a/lib/src/ui/detail/trip_detail_screen.dart b/lib/src/ui/detail/trip_detail_screen.dart index 8396819..6faafef 100644 --- a/lib/src/ui/detail/trip_detail_screen.dart +++ b/lib/src/ui/detail/trip_detail_screen.dart @@ -100,6 +100,17 @@ class TripDetailScreen extends ConsumerWidget { icon: const Icon(Icons.edit_outlined), onPressed: () => _rename(context, ref, async.value!.trip), ), + if (async.valueOrNull != null) + IconButton( + key: const Key('split'), + icon: const Icon(Icons.content_cut), + tooltip: async.value!.segments.length < 2 + ? 'Nothing to split -- this ride has only one segment' + : 'Split this ride', + // V3-10: a dead control that explains itself via SnackBar on tap, rather + // than a disabled button with no explanation at all. + onPressed: () => _split(context, ref, async.value!), + ), ], ), body: switch (async) { @@ -185,6 +196,64 @@ class TripDetailScreen extends ConsumerWidget { ref.invalidate(tripDetailProvider(trip.id)); } + /// V3-10: splits at a segment boundary only -- see `TripRepository.splitTrip`. A + /// single-segment ride has no boundary to offer, so this explains why rather than + /// silently doing nothing. + Future _split(BuildContext context, WidgetRef ref, TripDetail detail) async { + if (detail.segments.length < 2) { + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar( + content: Text('This ride has only one segment -- there is nothing to split.'), + ), + ); + return; + } + + final atSegmentId = await showModalBottomSheet( + context: context, + isScrollControlled: true, + builder: (context) => SafeArea( + child: ListView( + shrinkWrap: true, + children: [ + const Padding( + padding: EdgeInsets.fromLTRB(16, 16, 16, 4), + child: Text('Split before...'), + ), + // The first segment can never be a split point -- nothing would remain + // before it. See TripRepository.splitTrip. + for (final segment in detail.segments.skip(1)) + ListTile( + key: Key('split-at-${segment.id}'), + leading: const Icon(Icons.content_cut), + title: Text(formatDateTime(segment.startedAt)), + onTap: () => Navigator.of(context).pop(segment.id), + ), + ], + ), + ), + ); + if (atSegmentId == null || !context.mounted) return; + + final segment = detail.segments.firstWhere((s) => s.id == atSegmentId); + final firstLabel = formatDateTime(detail.trip.startedAt); + final secondLabel = formatDateTime(segment.startedAt); + final confirmed = await confirmDialog( + context, + title: 'Split this ride?', + message: 'This ride will become two: "$firstLabel" and "$secondLabel". ' + 'Distance and other totals are recomputed for each.', + confirmLabel: 'Split', + ); + if (!confirmed) return; + + final newTripId = await ref + .read(tripRepositoryProvider) + .splitTrip(detail.trip.id, atSegmentId); + ref.invalidate(tripDetailProvider(detail.trip.id)); + if (newTripId != null) ref.invalidate(tripDetailProvider(newTripId)); + } + /// No picker in front of Start — V3-01's constraint — but a ride's activity is fully /// editable here after the fact, the same pattern as rename. Future _editActivity(BuildContext context, WidgetRef ref, Trip trip) async { diff --git a/test/trip_repository_test.dart b/test/trip_repository_test.dart index 525834e..920e56b 100644 --- a/test/trip_repository_test.dart +++ b/test/trip_repository_test.dart @@ -428,4 +428,181 @@ void main() { expect(segments.every((s) => s.tripId == a), isTrue); }); }); + + group('split (V3-10)', () { + /// A completed, multi-segment ride: [segmentSpecs] each become one paused-and-resumed + /// segment, `points` fixes per segment, one degree of latitude apart so an accidental + /// distance leak across the split is unmissable. + Future multiSegmentTrip( + List<({int startedAt, int endedAt})> segmentSpecs, { + int points = 3, + }) async { + var h = await repo.startTrip(segmentSpecs.first.startedAt); + for (var s = 0; s < segmentSpecs.length; s++) { + final spec = segmentSpecs[s]; + for (var i = 0; i < points; i++) { + await addPoint(h, spec.startedAt + i * 1000, lat: 51.0 + s * 1.0); + } + if (s < segmentSpecs.length - 1) { + await repo.pauseTrip(spec.endedAt); + h = (await repo.resumeTrip(segmentSpecs[s + 1].startedAt))!; + } + } + await repo.completeTrip(segmentSpecs.last.endedAt); + return h.tripId; + } + + test('point counts sum to the original and nothing is orphaned', () async { + final id = await multiSegmentTrip([ + (startedAt: 1000, endedAt: 2000), + (startedAt: 10000, endedAt: 11000), + ], points: 3); + final segments = await repo.segmentsForTrip(id); + + final newId = await repo.splitTrip(id, segments[1].id); + + expect(newId, isNotNull); + final original = (await repo.tripById(id))!; + final split = (await repo.tripById(newId!))!; + expect(original.pointCount + split.pointCount, 6); + final allPoints = await db.allPoints(); + expect( + allPoints.every((p) => p.tripId == id || p.tripId == newId), + isTrue, + reason: 'no point may belong to neither resulting trip', + ); + }); + + test("neither trip's distance includes the gap between them", () async { + // Segments a whole degree of latitude apart -- ~111 km. If the gap leaked in + // (e.g. by keeping the old aggregate rather than recomputing), it would dwarf + // the few-metre hops inside each segment. + final id = await multiSegmentTrip([ + (startedAt: 1000, endedAt: 2000), + (startedAt: 10000, endedAt: 11000), + ]); + final segments = await repo.segmentsForTrip(id); + + final newId = await repo.splitTrip(id, segments[1].id); + + final original = (await repo.tripById(id))!; + final split = (await repo.tripById(newId!))!; + expect(original.distanceM, lessThan(200.0)); + expect(split.distanceM, lessThan(200.0)); + }); + + test('both trips get plausible startedAt/endedAt from their own segments', + () async { + final id = await multiSegmentTrip([ + (startedAt: 1000, endedAt: 2000), + (startedAt: 10000, endedAt: 11000), + ]); + final segments = await repo.segmentsForTrip(id); + + final newId = await repo.splitTrip(id, segments[1].id); + + final original = (await repo.tripById(id))!; + final split = (await repo.tripById(newId!))!; + expect(original.startedAt, 1000); + expect(original.endedAt, 2000, + reason: 'must come from the last kept segment, not the pre-split trip row'); + expect(split.startedAt, 10000); + expect(split.endedAt, 11000); + }); + + test('the split stays a segment boundary on both sides', () async { + final id = await multiSegmentTrip([ + (startedAt: 1000, endedAt: 2000), + (startedAt: 10000, endedAt: 11000), + (startedAt: 20000, endedAt: 21000), + ]); + final segments = await repo.segmentsForTrip(id); + + final newId = await repo.splitTrip(id, segments[1].id); + + expect(await repo.segmentsForTrip(id), hasLength(1)); + expect(await repo.segmentsForTrip(newId!), hasLength(2)); + }); + + test('a single-segment trip cannot be split', () async { + final id = await multiSegmentTrip([(startedAt: 1000, endedAt: 2000)]); + final segments = await repo.segmentsForTrip(id); + + expect(await repo.splitTrip(id, segments.single.id), isNull); + expect(await db.countTrips(), 1); + }); + + test('splitting at the first segment is rejected -- nothing would remain before it', + () async { + final id = await multiSegmentTrip([ + (startedAt: 1000, endedAt: 2000), + (startedAt: 10000, endedAt: 11000), + ]); + final segments = await repo.segmentsForTrip(id); + + expect(await repo.splitTrip(id, segments.first.id), isNull); + expect(await db.countTrips(), 1); + }); + + test('splitting an active trip is rejected', () async { + final h = await repo.startTrip(1000); + await addPoint(h, 1000); + await repo.pauseTrip(2000); + final h2 = (await repo.resumeTrip(3000))!; + await addPoint(h2, 3000); + + final segments = await repo.segmentsForTrip(h.tripId); + expect(await repo.splitTrip(h.tripId, segments[1].id), isNull); + expect(await db.countTrips(), 1); + }); + + test('an unknown segment id is rejected', () async { + final id = await multiSegmentTrip([ + (startedAt: 1000, endedAt: 2000), + (startedAt: 10000, endedAt: 11000), + ]); + expect(await repo.splitTrip(id, 9999), isNull); + }); + + test('split then merge is a round trip back to the original aggregates', + () async { + final id = await multiSegmentTrip([ + (startedAt: 1000, endedAt: 2000), + (startedAt: 10000, endedAt: 11000), + ]); + // multiSegmentTrip only inserts points; the aggregate columns are otherwise only + // ever updated by the recording engine's periodic flush (bypassed here), so they + // must be computed explicitly to have a real baseline to compare against. + await repo.recomputeAggregates(id); + final before = (await repo.tripById(id))!; + final segments = await repo.segmentsForTrip(id); + + final newId = await repo.splitTrip(id, segments[1].id); + final survivor = await repo.mergeTrips(id, newId!); + + final after = (await repo.tripById(survivor!))!; + expect(after.pointCount, before.pointCount); + expect(after.distanceM, closeTo(before.distanceM, 1e-6)); + expect(after.startedAt, before.startedAt); + expect(after.endedAt, before.endedAt); + }); + + test('split is atomic and leaves no orphans', () async { + final id = await multiSegmentTrip([ + (startedAt: 1000, endedAt: 2000), + (startedAt: 10000, endedAt: 11000), + ]); + final segments = await repo.segmentsForTrip(id); + + final newId = await repo.splitTrip(id, segments[1].id); + + final allSegments = [ + ...await repo.segmentsForTrip(id), + ...await repo.segmentsForTrip(newId!), + ]; + expect(allSegments.length, 2); + final allPoints = await db.allPoints(); + expect(allPoints.every((p) => p.tripId == id || p.tripId == newId), isTrue); + }); + }); } diff --git a/test/widget_test.dart b/test/widget_test.dart index 1e82801..eac3148 100644 --- a/test/widget_test.dart +++ b/test/widget_test.dart @@ -593,5 +593,65 @@ void main() { expect(find.text(imperialValue), findsWidgets); } }); + + screenTest('a single-segment ride explains why it cannot be split (V3-10)', + (tester) async { + final id = await seedCompletedTrip(startedAt: 1000, endedAt: 5000); + + await tester.pumpWidget(host(TripDetailScreen(tripId: id), map: false)); + await tester.pumpAndSettle(); + + await tester.tap(find.byKey(const Key('split'))); + await tester.pumpAndSettle(); + + expect(find.textContaining('nothing to split'), findsOneWidget); + expect(await repo.segmentsForTrip(id), hasLength(1), + reason: 'a rejected split must not touch anything'); + }); + + screenTest('splitting a multi-segment ride creates a second ride (V3-10)', + (tester) async { + final h = await repo.startTrip(1000); + await repo.appendPoints([ + TrackPoint( + tripId: h.tripId, + segmentId: h.segmentId, + timestamp: 1000, + latitude: 51.0, + longitude: -114.0, + speedKmh: 40.0, + altitudeM: 1000.0, + ), + ]); + await repo.pauseTrip(2000); + final h2 = (await repo.resumeTrip(3000))!; + await repo.appendPoints([ + TrackPoint( + tripId: h2.tripId, + segmentId: h2.segmentId, + timestamp: 3000, + latitude: 52.0, + longitude: -114.0, + speedKmh: 40.0, + altitudeM: 1000.0, + ), + ]); + await repo.completeTrip(4000); + final segments = await repo.segmentsForTrip(h.tripId); + expect(segments, hasLength(2)); + + await tester.pumpWidget(host(TripDetailScreen(tripId: h.tripId), map: false)); + await tester.pumpAndSettle(); + + await tester.tap(find.byKey(const Key('split'))); + await tester.pumpAndSettle(); + await tester.tap(find.byKey(Key('split-at-${segments[1].id}'))); + await tester.pumpAndSettle(); + await tester.tap(find.text('Split')); + await tester.pumpAndSettle(); + + expect(await db.countTrips(), 2); + expect(await repo.segmentsForTrip(h.tripId), hasLength(1)); + }); }); }