# FB-05 — Closed-loop routes in Route Planner **Depends on** FB-04 (same file, heavily edited — sequence after it merges) · **Size** M · **Status** Done ## Goal Route planning currently only supports a one-way A→B→C path. Add the ability to close the route into a loop that returns to its starting pin, updating the drawn polyline and the distance stat to include the closing segment. ## Context Direct user feedback (`docs/FEEDBACK.md`, "Route" section): > ...there is no way to create a loop with the pins, it only creates a unidirectional > path. ### Data model — no loop concept exists today `lib/src/domain/models.dart`, `RoutePlan` (~lines 229-276): ```dart class RoutePlan { const RoutePlan({ this.id = 0, required this.name, required this.createdAt, this.activity = Activity.motorcycle, this.distanceM = 0.0, this.estimatedMillis, this.geometry, }); final int id; final String name; final int createdAt; final Activity activity; final double distanceM; final int? estimatedMillis; final String? geometry; RoutePlan copyWith({ int? id, String? name, int? createdAt, Activity? activity, double? distanceM, int? estimatedMillis, String? geometry, }) => RoutePlan( id: id ?? this.id, name: name ?? this.name, createdAt: createdAt ?? this.createdAt, activity: activity ?? this.activity, distanceM: distanceM ?? this.distanceM, estimatedMillis: estimatedMillis ?? this.estimatedMillis, geometry: geometry ?? this.geometry, ); } ``` `Waypoint` (~lines 278-309) has an `ordinal` for ordering, and waypoints are only ever appended at the end (`RoutePlanRepository.addWaypoint`, below) — there is no "return to start" field or concept anywhere in the model. ### Polyline and distance — strictly open, no wraparound `lib/src/ui/routes/route_planner_screen.dart`, the `PolylineLayer` (~lines 182-201): ```dart if (waypoints.length >= 2) PolylineLayer( polylines: [ Polyline( points: [ for (final w in waypoints) ll.LatLng(w.latitude, w.longitude), ], strokeWidth: 4, pattern: StrokePattern.dashed(segments: const [8, 6]), color: colors.primary, ), ], ), ``` `lib/src/data/route_plan_repository.dart`, distance recomputation (full relevant section): ```dart Future addWaypoint(int routeId, double latitude, double longitude) async { final existing = await _db.waypointsForRoute(routeId); await _db.insertWaypoint( Waypoint(routeId: routeId, ordinal: existing.length, latitude: latitude, longitude: longitude), ); await _recomputeDistance(routeId); } Future _recomputeDistance(int routeId) async { final waypoints = await _db.waypointsForRoute(routeId); final distance = geo.pathLengthMeters([ for (final w in waypoints) geo.LatLon(w.latitude, w.longitude), ]); await _db.setRoutePlanDistance(routeId, distance); } ``` `geo.pathLengthMeters` (`lib/src/geo/geo.dart` ~line 185) sums consecutive-pair distances generically over whatever point list it's given — it needs no changes itself, just a point list that includes the closing segment when appropriate. `FloatingPill`'s Distance/Est. Time/Pins stats read straight off `route.distanceM`/ `route.estimatedMillis`/`waypoints.length` (`route_planner_screen.dart` ~lines 280-292) — Distance updates for free once `_recomputeDistance` accounts for the closing segment; Pins and Est. Time are unaffected by this ticket. ### Schema — current version and migration pattern to follow `lib/src/data/database.dart`: ```dart @DataClassName('RoutePlanRow') class RoutePlans extends Table { @override String get tableName => 'route_plans'; IntColumn get id => integer().autoIncrement()(); TextColumn get name => text()(); IntColumn get createdAt => integer()(); TextColumn get activity => textEnum().withDefault(const Constant('motorcycle'))(); RealColumn get distanceM => real().withDefault(const Constant(0))(); IntColumn get estimatedMillis => integer().nullable()(); TextColumn get geometry => text().nullable()(); } @DriftDatabase(tables: [Trips, Segments, TrackPoints, RoutePlans, Waypoints]) class AppDatabase extends _$AppDatabase { @override int get schemaVersion => 3; @override MigrationStrategy get migration => MigrationStrategy( onCreate: (m) => m.createAll(), onUpgrade: (m, from, to) async { if (from < 2) { await m.addColumn(trips, trips.activity); } if (from < 3) { await m.createTable(routePlans); await m.createTable(waypoints); } }, ... ``` The existing `if (from < 2) { await m.addColumn(trips, trips.activity); }` is the exact pattern to follow for a new nullable/defaulted column. `RoutePlanRepository` insert/conversion glue, in `database.dart`: ```dart Future insertRoutePlan(domain.RoutePlan route) => into(routePlans).insert( RoutePlansCompanion.insert( name: route.name, createdAt: route.createdAt, activity: Value(route.activity), distanceM: Value(route.distanceM), estimatedMillis: Value(route.estimatedMillis), geometry: Value(route.geometry), ), ); Future setRoutePlanDistance(int id, double distanceM) => (update(routePlans) ..where((r) => r.id.equals(id))).write(RoutePlansCompanion(distanceM: Value(distanceM))); domain.RoutePlan _toRoutePlan(RoutePlanRow r) => domain.RoutePlan( id: r.id, name: r.name, createdAt: r.createdAt, activity: r.activity, distanceM: r.distanceM, estimatedMillis: r.estimatedMillis, geometry: r.geometry, ); ``` ### Overflow menu — where the toggle belongs `route_planner_screen.dart`, `_OverflowMenu` (~lines 493-530-ish) already hosts Rename/Download/Delete as a `PopupMenuButton` inside a `GlassPanel`: ```dart class _OverflowMenu extends StatelessWidget { const _OverflowMenu({ required this.hasWaypoints, required this.onRename, required this.onDownload, required this.onDelete, }); final bool hasWaypoints; final VoidCallback onRename; final VoidCallback? onDownload; final VoidCallback onDelete; @override Widget build(BuildContext context) => GlassPanel( borderRadius: const BorderRadius.all(Radius.circular(999)), child: PopupMenuButton( key: const Key('route-overflow-menu'), icon: const Icon(Icons.more_vert), onSelected: (value) => switch (value) { 'rename' => onRename(), 'download' => onDownload?.call(), 'delete' => onDelete(), _ => null, }, itemBuilder: (context) => [ const PopupMenuItem(key: Key('rename-route'), value: 'rename', child: Text('Rename')), ... ``` And it's constructed at the call site (~lines 298-310): ```dart _OverflowMenu( hasWaypoints: waypoints.isNotEmpty, onRename: () => _rename(context, repo, route), onDownload: waypoints.isEmpty ? null : () => _downloadOfflineTiles(context, waypoints), onDelete: () async { await repo.deleteRoutePlan(widget.routeId); widget.onBack?.call(); }, ), ``` ## Design - **Schema**: add `BoolColumn get isClosedLoop => boolean().withDefault(const Constant(false))();` to `RoutePlans` in `database.dart`. Bump `schemaVersion` from `3` to `4`; add `if (from < 4) { await m.addColumn(routePlans, routePlans.isClosedLoop); }` to `onUpgrade`. - **Domain model**: add `final bool isClosedLoop;` to `RoutePlan` (default `false` in the constructor), thread it through `copyWith`. - **DB glue**: add `isClosedLoop: Value(route.isClosedLoop)` to `insertRoutePlan`'s `RoutePlansCompanion.insert(...)` call, `isClosedLoop: r.isClosedLoop` to `_toRoutePlan`, and a new method mirroring `setRoutePlanDistance`: ```dart Future setRoutePlanClosedLoop(int id, bool value) => (update(routePlans) ..where((r) => r.id.equals(id))).write(RoutePlansCompanion(isClosedLoop: Value(value))); ``` - **Repository**: add to `RoutePlanRepository`: ```dart Future setClosedLoop(int routeId, bool value) async { await _db.setRoutePlanClosedLoop(routeId, value); await _recomputeDistance(routeId); // the closing segment changes the total } ``` Update `_recomputeDistance` to fetch the route's own flag and append the first waypoint's coordinates when closed, before summing: ```dart Future _recomputeDistance(int routeId) async { final route = await _db.getRoutePlan(routeId); final waypoints = await _db.waypointsForRoute(routeId); final points = [for (final w in waypoints) geo.LatLon(w.latitude, w.longitude)]; if (route?.isClosedLoop == true && points.length >= 2) { points.add(points.first); } final distance = geo.pathLengthMeters(points); await _db.setRoutePlanDistance(routeId, distance); } ``` (`_recomputeDistance` is already called from every waypoint-mutating method — `addWaypoint`, `moveWaypoint`, `deleteWaypoint`, `reorderWaypoint` — so the closing segment is kept correct automatically as pins are edited, with no other call site changes needed.) - **UI toggle**: add a 4th item to `_OverflowMenu`, enabled only when there are enough waypoints for a loop to mean anything (`hasWaypoints` alone isn't enough — need at least 2 to form any segment at all; reuse the existing `hasWaypoints` bool but also thread through whether there are `>= 2` waypoints, or simplify by passing a new `canCloseLoop: waypoints.length >= 2` param): ```dart class _OverflowMenu extends StatelessWidget { const _OverflowMenu({ required this.hasWaypoints, required this.canCloseLoop, required this.isClosedLoop, required this.onRename, required this.onDownload, required this.onToggleLoop, required this.onDelete, }); ... final bool canCloseLoop; final bool isClosedLoop; final VoidCallback onToggleLoop; @override Widget build(BuildContext context) => GlassPanel( ... child: PopupMenuButton( ... onSelected: (value) => switch (value) { 'rename' => onRename(), 'download' => onDownload?.call(), 'closeLoop' => onToggleLoop(), 'delete' => onDelete(), _ => null, }, itemBuilder: (context) => [ const PopupMenuItem(key: Key('rename-route'), value: 'rename', child: Text('Rename')), ... PopupMenuItem( key: const Key('toggle-closed-loop'), value: 'closeLoop', enabled: canCloseLoop, child: Text(isClosedLoop ? 'Open the loop' : 'Close the loop'), ), ... ], ), ); } ``` Wire it at the call site: ```dart _OverflowMenu( hasWaypoints: waypoints.isNotEmpty, canCloseLoop: waypoints.length >= 2, isClosedLoop: route.isClosedLoop, onRename: () => _rename(context, repo, route), onDownload: waypoints.isEmpty ? null : () => _downloadOfflineTiles(context, waypoints), onToggleLoop: () => repo.setClosedLoop(widget.routeId, !route.isClosedLoop), onDelete: () async { ... }, ), ``` - **Polyline**: append the closing point when the route is closed and there are at least 2 waypoints: ```dart if (waypoints.length >= 2) PolylineLayer( polylines: [ Polyline( points: [ for (final w in waypoints) ll.LatLng(w.latitude, w.longitude), if (route.isClosedLoop) ll.LatLng(waypoints.first.latitude, waypoints.first.longitude), ], strokeWidth: 4, pattern: StrokePattern.dashed(segments: const [8, 6]), color: colors.primary, ), ], ), ``` Same `Polyline` object, same styling — just one more point in the list, so the closing segment renders with identical dashed styling to the rest of the route. - **`FloatingPill`/`_initialFit`**: no changes needed — Distance updates automatically via `_recomputeDistance`; Pins/Est. Time are correctly unaffected; `_initialFit`'s bounds-fitting is based on waypoint positions regardless of whether the last segment loops back (the closing point is always one of the existing waypoints' own coordinates, already inside the fitted bounds). ## Implementation 1. Add `isClosedLoop` column to `RoutePlans` in `database.dart`, bump `schemaVersion` to `4`, add the `onUpgrade` migration step. 2. Add `isClosedLoop` field to `RoutePlan` domain model + `copyWith`. 3. Add `isClosedLoop` to `insertRoutePlan`'s companion and `_toRoutePlan` in `database.dart`; add `setRoutePlanClosedLoop`. 4. Add `RoutePlanRepository.setClosedLoop`; update `_recomputeDistance` to append the closing point when `isClosedLoop`. 5. Extend `_OverflowMenu` with the new toggle item and params; wire it at the call site in `route_planner_screen.dart`. 6. Extend the `PolylineLayer`'s point list with the conditional closing point. ## Acceptance criteria - [ ] "Close the loop" appears in the overflow menu, disabled when there are fewer than 2 waypoints, enabled otherwise. - [ ] Toggling it on redraws the polyline with a visible segment from the last waypoint back to the first, same dashed styling as the rest of the route. - [ ] Toggling it on increases the Distance stat by the length of the new closing segment; toggling it back off returns Distance to the open-path total. - [ ] The menu label reflects current state ("Close the loop" when open, "Open the loop" when already closed). - [ ] Adding/moving/deleting a waypoint on a closed-loop route keeps the closing segment correct automatically (it's recomputed via the existing `_recomputeDistance` call already present in every waypoint-mutating method). - [ ] Deleting waypoints down to fewer than 2 while closed doesn't crash — the `waypoints.length >= 2` guards in both the polyline and distance code make the closing point simply disappear along with the rest of the route drawing, same as today's behavior for an open path with 0-1 waypoints. - [ ] A pre-existing (schema v3) database opens cleanly post-migration with `isClosedLoop` defaulting to `false` on every existing route. - [ ] `flutter analyze` clean, `flutter test` green, test count only goes up. ## Tests - **Migration test** (`test/migration_test.dart`, following the exact pattern of the existing `'a v2 database (V3-07) gains route_plans/waypoints and keeps its trips'` test): seed a v3 database (v1 seed + `activity` column + `route_plans`/`waypoints` tables created via raw SQL, `PRAGMA user_version = 3;`), open it with `AppDatabase`, insert a route via `db.insertRoutePlan`, and assert `isClosedLoop` reads back `false` by default, and that toggling it via `setRoutePlanClosedLoop` persists. - **Repository tests** (`test/route_plan_repository_test.dart`, alongside the existing `'adding waypoints appends in order and updates distance live'` etc.): - `setClosedLoop(true)` on a route with 2+ waypoints increases `distanceM` by exactly the closing segment's length (compute the expected delta directly with `geo.pathLengthMeters`/a manual haversine call between the last and first waypoint, matching how other distance tests in this file already assert exact values). - `setClosedLoop(false)` after `true` returns `distanceM` to the pre-toggle value. - Adding a waypoint to an already-closed route keeps the distance correct (recomputed including the new closing segment against the new last-added point, not the old one). - **Widget tests** (`test/route_planner_screen_test.dart`): - The "Close the loop" menu item is disabled with 0-1 waypoints, enabled with 2+ (mirrors the existing "offline-tiles download menu item is disabled with no pins" test's structure — open the overflow menu, inspect the `PopupMenuItem.enabled` property via its key). - Tapping it toggles `route.isClosedLoop` (assert via `repo.routePlanById(id)?.isClosedLoop`, the same pattern the existing rename test uses to verify writes through to the repository) and the label switches to "Open the loop". - With the loop closed, `find.byType(Polyline)` (or inspecting the `PolylineLayer`'s `polylines` list directly, matching whatever existing pattern this test file uses to inspect polyline data) has one more point than the waypoint count. ## Risks - None significant — this is additive (new column, new optional toggle) and every touched call site (`_recomputeDistance`) already runs on every mutation, so there's no new code path that could silently skip recomputation. ## Out of scope Turn-by-turn following of a closed loop (V3-09, already deferred). Any UI indication of loop direction/rotation. Road-snapped routing for the closing segment (V3-08) — it stays a straight line like every other segment in this ticket's scope. ## Outcome Implemented exactly as designed, with one small addition to the migration step beyond what the ticket spelled out: - Added `isClosedLoop` (`BoolColumn`, default `false`) to `RoutePlans` in `lib/src/data/database.dart`, bumped `schemaVersion` from 3 to 4, and regenerated `database.g.dart` via `dart run build_runner build`. - **Deviation**: the migration guard needed to be `if (from >= 3 && from < 4)` rather than the ticket's plain `if (from < 4)`. A v2 → v4 upgrade runs the existing `if (from < 3)` branch first, which calls `m.createTable(routePlans)` — and since that table definition already includes `isClosedLoop` (Drift generates `CREATE TABLE` from the *current* Dart schema, not a historical snapshot), an unconditional `from < 4` then tried to `ADD COLUMN isClosedLoop` a second time and hit a duplicate-column SQLite error. Restricting the `addColumn` step to routes that already had the table before v4 (`from >= 3`) fixes it; a v1/v2 → v4+ upgrade gets the column for free from the fresh `createTable` instead. Added a v3-seed migration test (`test/migration_test.dart`, `'a v3 database (FB-05) gains isClosedLoop defaulting to false'`) covering exactly this path, plus verifying a v2 → v4+ jump still works via the existing v2 test. - Added `isClosedLoop` to the `RoutePlan` domain model, `copyWith`, the `insertRoutePlan`/`_toRoutePlan` glue, and a new `setRoutePlanClosedLoop` method in `database.dart`. - Added `RoutePlanRepository.setClosedLoop` and updated `_recomputeDistance` to append the first waypoint's coordinates as a closing point when the route is closed and has 2+ waypoints, exactly per the design. - Extended `_OverflowMenu` in `route_planner_screen.dart` with `canCloseLoop`/ `isClosedLoop`/`onToggleLoop` and a "Close the loop" / "Open the loop" menu item, wired at the call site. - Extended the `PolylineLayer`'s point list with the conditional closing point. - FB-04 had already changed the surrounding `route` access to a guarded `AsyncValue` pattern (no more `.valueOrNull`) by the time this ticket landed; the new code reads `route.isClosedLoop` off the same already-resolved `route` value the rest of the build method uses, so no adjustment to that pattern was needed. Added migration, repository, and widget tests per the ticket's Tests section (in `test/migration_test.dart`, `test/route_plan_repository_test.dart`, and `test/route_planner_screen_test.dart`). `flutter analyze`: clean (only 4 pre-existing, unrelated `info`-level lints remain). `flutter test`: all green, **411 tests passing** (up from the 404 baseline).