diff --git a/.agent/work-plans/issue-129/plan.md b/.agent/work-plans/issue-129/plan.md new file mode 100644 index 0000000..934b0ef --- /dev/null +++ b/.agent/work-plans/issue-129/plan.md @@ -0,0 +1,120 @@ +# Plan: Running tasks on chart + selection sync (P2) + +## Issue + +https://github.com/rolker/camp/issues/129 + +## Context + +P1 (#127 / PR #128) delivered a read-only `RunningTasksView` tree widget that +subscribes to `marine/status/mission_tasks` and renders `TaskFeedback` as a +structured tree. The widget emits `taskSelected(QString id)` on row activation +and exposes `setSelectedTask(id)` for external selection. Neither signal is yet +wired to anything outside the widget. + +P2 adds three things on top of that foundation: + +1. A per-task geometry overlay on CAMP's `QGraphicsScene` — point for single-pose + tasks (goto), polyline for multi-pose tasks (survey_line), using the existing + `GeoGraphicsItem` machinery but visually distinct from editable plan items. +2. Two-way selection sync between `RunningTasksView` and the overlay. +3. The CAMP-only glue class that owns the overlay items and wires the signals. + +A hasTask guard (carry-over from P1) is included: `taskSelected` must not fire +for group rows that have no poses, so map glue isn't triggered by clicks on +pure-structural parent rows. + +## Approach + +1. **Add `hasTaskPoses` to `RunningTasksModel`** — predicate that returns true when + the node at a `QModelIndex` has `task->message().poses` non-empty. Guard + `RunningTasksView::onCurrentRowChanged`: only emit `taskSelected` when + `model_->hasTaskPoses(current)` is true. + +2. **Add `tasksUpdated` signal to `RunningTasksView`** — emit from the end of + `applyPendingTasks()` with the current-task id and the raw `TaskInformation` + vector. This lets the CAMP glue layer refresh overlay geometry without a + second ROS subscription. + +3. **Add `TaskOverlayItem`** (`GeoGraphicsItem` subclass, lives in + `src/camp/running_tasks/`) — renders one task's poses on the scene. + - Single pose → filled circle (goto). + - Two or more poses → polyline (survey_line, transit, etc.). + - Visual states: current (emphasized, brighter), pending (muted), done (gray). + - Emits `clicked(QString id)` for map→tree selection sync. + - New type constant `RunningTaskType` added to `GeoGraphicsItem`'s enum. + +4. **Add `RunningTasksOverlay`** (`camp_ros::ROSWidget` subclass, lives in + `src/camp/running_tasks/`) — the CAMP-only glue. + - Initialized with the `QGraphicsScene*` and a `RunningTasksView*`. + - Holds `QMap` keyed by task id. + - `tasksUpdated` → rebuild overlay: call `getGeoCoordinate` (from ROSWidget) for + each pose, create/replace `TaskOverlayItem`s on the scene. + - `taskSelected` → set `is_selected_` on the matching item, clear others, + call `item->update()`. + - `TaskOverlayItem::clicked(id)` → `runningTasksView_->setSelectedTask(id)`. + +5. **Wire in `Platform`** — after `runningTasksView->setNode(node_)`, construct + `RunningTasksOverlay`, pass it the scene from the `AutonomousVehicleProject` + (via `Platform`'s parent chain or a passed-in pointer) and connect it to + `m_ui->runningTasksView`. `RunningTasksOverlay` inherits `nodeStarted` via + `ROSWidget`, so call `overlay_->nodeStarted(node_, transform_buffer_)` in + `Platform::onNodeUpdated`. + +6. **CMakeLists** — add `running_tasks/task_overlay_item.cpp` and + `running_tasks/running_tasks_overlay.cpp` to the `SOURCES` list. + +## Files to Change + +| File | Change | +|------|--------| +| `src/camp/running_tasks/running_tasks_model.h` | Add `hasTaskPoses(QModelIndex)` declaration | +| `src/camp/running_tasks/running_tasks_model.cpp` | Implement `hasTaskPoses` | +| `src/camp/running_tasks/running_tasks_view.h` | Add `tasksUpdated` signal | +| `src/camp/running_tasks/running_tasks_view.cpp` | Emit `tasksUpdated` from `applyPendingTasks`; guard `onCurrentRowChanged` with `hasTaskPoses` | +| `src/camp/running_tasks/task_overlay_item.h` | New file — `TaskOverlayItem` (`GeoGraphicsItem` subclass) | +| `src/camp/running_tasks/task_overlay_item.cpp` | New file — paint + click + coordinate update | +| `src/camp/running_tasks/running_tasks_overlay.h` | New file — `RunningTasksOverlay` (`camp_ros::ROSWidget`) | +| `src/camp/running_tasks/running_tasks_overlay.cpp` | New file — glue: rebuild items, sync selection | +| `src/camp/geographicsitem.h` | Add `RunningTaskType` to the type enum | +| `src/camp/platform_manager/platform.h` | Add `RunningTasksOverlay*` member | +| `src/camp/platform_manager/platform.cpp` | Construct overlay, call `nodeStarted`, connect signals | +| `CMakeLists.txt` | Add two new `.cpp` files to `SOURCES` | + +## Principles Self-Check + +| Principle | Consideration | +|---|---| +| Only what's needed | Two new classes; no third subscription or duplicate data path | +| Workspace vs. project separation | All new code in `camp/` project repo — no workspace infra touched | +| A change includes its consequences | `CMakeLists.txt` updated; `geographicsitem.h` enum extended | +| Improve incrementally | Single PR building on P1 foundation; P3 editing deferred | + +## ADR Compliance + +| ADR | Triggered | How addressed | +|---|---|---| +| ADR-0002 (Web-Mercator scene) | Yes | `TaskOverlayItem` uses `geoToPixel` for scene coordinates; no chart dependency | +| ADR-0008 (ROS 2 conventions) | Yes | Node-injection idiom for `RunningTasksView` unchanged; ROSWidget pattern for overlay | + +## Consequences + +| If we change... | Also update... | Included in plan? | +|---|---|---| +| `GeoGraphicsItem` type enum | Any switch/cast on item types | Yes — add enum value; no existing switch covers it | +| `RunningTasksView` signals | rqt plugin (future) | No — `tasksUpdated` is additive; rqt path unaffected | +| `Platform` construction | Any future subclass of Platform | No follow-up needed — single concrete class | + +## Open Questions + +- Which frame does the boat populate `poses[].header.frame_id` with in practice? + `getGeoCoordinate` will TF-lookup to earth regardless, but knowing the source + frame helps set timeout expectations (earth→map is quasi-static; a more dynamic + frame would need a tighter retry window). +- Should the overlay recenter the view on the selected task's first pose (tree→map)? + The issue says "and optionally recenters"; leaving optional — implement if trivial + once selection sync is working. + +## Estimated Scope + +Single PR. diff --git a/.agent/work-plans/issue-129/progress.md b/.agent/work-plans/issue-129/progress.md new file mode 100644 index 0000000..171567a --- /dev/null +++ b/.agent/work-plans/issue-129/progress.md @@ -0,0 +1,221 @@ +--- +issue: 129 +--- + +# Issue #129 — Running tasks on chart + selection sync (P2) + +## Issue Review +**Status**: complete +**When**: 2026-06-28 00:00 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Issue**: #129 +**Comment**: (best-effort post follows this entry; not recorded inline) +**Scope verdict**: well-scoped + +### Actions +- [ ] Resolve frame-handling strategy before or early in plan-task: determine which frame `poses[].header.frame_id` is populated with (earth/map frame vs. something requiring a TF lookup per update) and document the decision — either in code comments or a brief design note in the plan. +- [ ] Confirm or fix the P1 carry-over guard: `RunningTasksView::taskSelected` must not fire for synthetic group rows (only `hasTask` leaf nodes); verify this is enforced and add a test if not already present. +- [ ] Clarify `getGeoCoordinate` access path: `RunningTasksView` uses node-injection (not `camp_ros::ROSWidget`), so the CAMP-only glue layer (which has access to `camp_ros`) must own the frame conversion — confirm the architectural split is explicit in the plan so the core widget stays clean. +- [ ] Decide and document redraw cadence: rebuild-all vs. diff graphics items on each `TaskFeedback` republish; document the choice (a brief comment in the overlay class suffices unless significant). +- [ ] Dependency gate: confirm PR #128 (P1) is merged before starting implementation. + +## Plan Authored +**Status**: complete +**When**: 2026-06-28 12:00 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Plan**: `.agent/work-plans/issue-129/plan.md` at `e5094f8` +**Branch**: feature/issue-129 at `e5094f8` +**Phases**: single + +### Open questions +- [ ] Which frame does the boat populate `poses[].header.frame_id` with in practice — determines TF timeout tuning. +- [ ] Should tree→map selection optionally recenter the view on the task's first pose? + +## Plan Review +**Status**: complete +**When**: 2026-06-28 15:57 +00:00 +**By**: Claude Code Agent (Claude Opus) + +**Plan**: `.agent/work-plans/issue-129/plan.md` at `e5094f8` +**PR**: PR-less (`--issue` mode) +**Verdict**: approve-with-suggestions + +### Findings +- [x] (must-fix) No test planned for the `hasTaskPoses` guard — `review-issue` action #2 required one, P1 added none, and the gtest suite is a primary gate. Add `test/test_running_tasks_model.cpp` (empty-poses group row vs. leaf with poses) wired via `ament_add_gtest` (mirror `test_collision_monitor_converter`). — `plan.md:67-82` +- [x] (suggestion) Simplify scene access — `Platform` is a `GeoGraphicsItem` (`ShipTrack : public GeoGraphicsItem`), so use `Platform::scene()` directly rather than parent-chain/passed-in pointer. — `plan.md:57-62` +- [x] (suggestion) Close Open Question #1 in-plan — `TaskInformation.poses` is `geometry_msgs/PoseStamped[]` (per-pose headers); `getGeoCoordinate` already transforms each to `earth` with stale-stamp retry, so the frame is schema-answered. — `plan.md:108-113` +- [x] (suggestion) Document the redraw cadence (rebuild-all) in the overlay class — `review-issue` action #4. — `plan.md:50-55` +- [x] (suggestion) Note/guard the selection-sync loop (idempotent map→tree→map round-trip), mirroring the `QSignalBlocker` pattern in `applyPendingTasks`. — `plan.md:52-55` + +## Implementation +**Status**: complete +**When**: 2026-06-28 17:30 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Commit**: `3c916c7` on `feature/issue-129` +**Build**: clean (1 package, warnings only — pre-existing) +**Tests**: 5/5 passing (`test_running_tasks_model`) + +### Summary + +Implemented all three P2 deliverables — map overlay, two-way selection sync, +and CAMP glue — plus all five plan-review findings: + +**Model / View changes:** +- `running_tasks_model.h/.cpp`: added `hasTaskPoses(QModelIndex)` — returns + true when the node's task has at least one pose. +- `running_tasks_view.h/.cpp`: guarded `onCurrentRowChanged` with + `hasTaskPoses`; added `tasksUpdated(current, tasks)` signal emitted at end + of `applyPendingTasks` for the overlay to consume without a second ROS + subscription. + +**New files:** +- `task_overlay_item.h/.cpp`: `GeoGraphicsItem` + `QObject` subclass + (dual-inherit, `Q_INTERFACES(QGraphicsItem)`). Single pose→filled circle, + multi-pose→polyline. Color states: selected=orange, current=green, + done=gray, pending=blue-muted. Emits `clicked(id)` on mouse press. + `RunningTaskType` added to `GeoGraphicsItem` enum. +- `running_tasks_overlay.h/.cpp`: `camp_ros::ROSWidget` subclass. Takes + `QGraphicsScene*` (from `Platform::scene()` — plan-review finding #2) and + `RunningTasksView*`. Rebuild-all strategy on every `tasksUpdated` + (documented in class comment — finding #4). Idempotent selection guard + mirrors `QSignalBlocker` pattern (finding #5). `getGeoCoordinate` handles + `TaskInformation.poses` (geometry_msgs/PoseStamped[] per-pose headers with + stale-stamp retry — finding #3 closed in code comment). +- `test/test_running_tasks_model.cpp`: 5 gtests for `hasTaskPoses` (finding + #1); wired via `ament_add_gtest` mirroring `test_collision_monitor_converter`. + +**Platform wiring:** +- `platform.h`: forward-declare `RunningTasksOverlay*`; add member. +- `platform.cpp`: construct overlay in `onNodeUpdated` using `scene()` + directly (finding #2); call `nodeStarted` on re-entry. + +**CMakeLists.txt:** +- Added two new `.cpp` sources; added `test_running_tasks_model` gtest target. + +### Plan-review findings addressed +- [x] (must-fix) `test/test_running_tasks_model.cpp` wired via `ament_add_gtest`; 5 tests pass +- [x] (suggestion) `Platform::scene()` used directly — no parent-chain walk +- [x] (suggestion) Frame schema closed in `running_tasks_overlay.cpp` comment +- [x] (suggestion) Rebuild-all cadence documented in `RunningTasksOverlay` class comment +- [x] (suggestion) Selection-sync loop guarded with `id == selected_id_` early return + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-28 16:37 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: changes-requested + +**Branch**: feature/issue-129 at `6820371` +**Mode**: pre-push +**Depth**: Deep (reason: 736 lines / 15 files — both >200 lines and ≥10 files; cross-layer GUI+ROS) +**Must-fix**: 3 | **Suggestions**: 5 +**Round**: 1 | **Ship**: continue — genuine correctness bugs (multi-pose items unclickable; latent double-free on scene/overlay teardown) + +### Findings +- [x] (must-fix) Multi-pose `shape()` returns a zero-area open polyline → Qt hit-tests via `shape().contains()`, so survey_line/transit overlay items never receive clicks; map→tree selection broken for all multi-pose tasks (contradicts class doc). Stroke the path (`QPainterPathStroker`) for `shape()` — `src/camp/running_tasks/task_overlay_item.cpp:47-50` +- [x] (must-fix) Double ownership of `TaskOverlayItem`: `scene_->addItem(item)` gives the scene ownership (parent nullptr) yet `clearItems()`/dtor also `delete item`; if the scene is destroyed before the overlay → use-after-free + double-free. `scene_` is a raw pointer with no `QPointer` guard. Follow the codebase convention (Qt parent ownership; this is the only `addItem` call site) and/or use `QPointer` — `src/camp/running_tasks/running_tasks_overlay.cpp:88,93-101`, `running_tasks_overlay.h:53` +- [x] (must-fix) Marker radii are fixed scene units (6.0 hit circle, 3.0 endpoints), not zoom-scaled like `Waypoint::shape()` (`1/mapScale()`) → markers sub-pixel when zoomed out / oversized when zoomed in; degrades single-pose hit area. Deviates from ADR-0003 constant-pixel-footprint convention (`metresPerPixel`) — `src/camp/running_tasks/task_overlay_item.cpp:30,90` +- [x] (suggestion) Rebuild-all deletes/recreates every `TaskOverlayItem` on each ~1 Hz republish even when unchanged → avoidable allocation churn + scene-region invalidation/flicker (cross-pass confirmed; documented as intentional) — `src/camp/running_tasks/running_tasks_overlay.cpp:50-91` (deferred: rebuild-all is an approved plan-time tradeoff; the cheap "skip if message unchanged" mitigation is unsafe because pose frames may be `map` — `getGeoCoordinate` re-resolves earth coords via TF every cycle, so identical pose messages still need redrawing; a correct incremental diff is larger than this thin fix phase warrants — left for a follow-up if profiling shows it matters) +- [x] (suggestion) `rebuildItems` guards `transform_buffer_` but not `node_`; `getGeoCoordinate`'s catch path derefs `node_->get_logger()` — crash if `node_` null while buffer set (low risk in practice) — `src/camp/running_tasks/running_tasks_overlay.cpp:54` +- [x] (suggestion) `onItemClicked` comment claims it "mirrors the QSignalBlocker pattern" but uses only an id-equality guard (no `QSignalBlocker`) — comment/code mismatch — `src/camp/running_tasks/running_tasks_overlay.cpp:44-46` +- [x] (suggestion) `selected_id_` is never cleared/validated when the selected task disappears from a later republish; harmless but tree/overlay can drift — `src/camp/running_tasks/running_tasks_overlay.cpp:30-38` +- [x] (suggestion) Overlay is created only inside `onNodeUpdated` when `scene()` is non-null; if `scene()` is null on first call and `onNodeUpdated` never fires again the overlay is silently never created (no fallback) — `src/camp/platform_manager/platform.cpp:243` + +## Implementation +**Status**: complete +**When**: 2026-06-28 16:54 +00:00 +**By**: Claude Code Agent (Claude Opus) + +**Branch**: feature/issue-129 at `4338718` +**Addressed**: Local Review (Pre-Push) (2026-06-28 16:37 +00:00, `feature/issue-129` at `6820371`) +**Commits**: `70febed`, `872e961`, `4338718` + +### Actions +- [x] (must-fix) Multi-pose `shape()` is now stroked (`QPainterPathStroker`) into a constant-pixel-width clickable band, so survey_line/transit items hit-test and map→tree selection works — `src/camp/running_tasks/task_overlay_item.cpp:75-88` (`70febed`) +- [x] (must-fix) Scene/overlay teardown double-free fixed: `scene_` is now `QPointer` and `clearItems()` only deletes items while the scene is alive — `src/camp/running_tasks/running_tasks_overlay.h:55`, `running_tasks_overlay.cpp:101-117` (`872e961`) +- [x] (must-fix) Marker/hit radii are zoom-scaled to a constant display-pixel footprint via new `sceneRadius()` (ADR-0003) — `src/camp/running_tasks/task_overlay_item.cpp:32-42,50-55,67-73,125-131` (`70febed`) +- [x] (suggestion) `rebuildItems()` now also guards `node_` (getGeoCoordinate's catch path derefs `node_->get_logger()`) — `src/camp/running_tasks/running_tasks_overlay.cpp:55-58` (`872e961`) +- [x] (suggestion) `onItemClicked` comment corrected — it uses an id-equality guard, not `QSignalBlocker` — `src/camp/running_tasks/running_tasks_overlay.cpp:42-45` (`872e961`) +- [x] (suggestion) `selected_id_` is cleared when the selected task disappears from a republish — `src/camp/running_tasks/running_tasks_overlay.cpp:95-98` (`872e961`) +- [x] (suggestion) Overlay creation factored into `ensureRunningTasksOverlay()`, also called from periodic `update()`, so a late-attaching scene still gets an overlay — `src/camp/platform_manager/platform.{h,cpp}` (`4338718`) +- [x] (suggestion) Rebuild-all allocation churn / flicker — **deferred**: an approved plan-time tradeoff; the cheap "skip if message unchanged" mitigation is unsafe (pose frames may be `map`, so `getGeoCoordinate` re-resolves earth coords via TF each cycle even for an identical message), and a correct incremental diff exceeds this thin fix phase — `src/camp/running_tasks/running_tasks_overlay.cpp:51-99` (deferred: see reason) + +### Verification +- Build/gtest **not run in this worktree**: the lower ROS layers are unbuilt here + (`underlay_ws`/`core_ws`/… `install/` are empty, so camp's deps — + `marine_ais_msgs`, `marine_nav_interfaces`, etc. — are unavailable to CMake). + The fixes were verified by inspection against the cited Qt/codebase patterns + (`ShipTrack::drawTriangle`, `Waypoint::shape`, `QPointer` ownership). The + re-review (`review-code`) should build/run `test_running_tasks_model` in a + fully-provisioned environment. +- No changes to `running_tasks_model` (the gtest target), so existing model + tests are unaffected. + +### Next step +Lifecycle: **Implementation** → **review-code** (re-review the fixes). Hand off to a +fresh-context sub-agent: + + .agent/scripts/dispatch_subagent.sh --mode in-process --issue 129 --skill review-code + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-28 17:12 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: approved + +**Branch**: feature/issue-129 at `c35c139` +**Mode**: pre-push +**Depth**: Deep (reason: 876 lines / 15 files — both >200 lines and ≥10 files; cross-layer GUI+ROS) +**Must-fix**: 0 | **Suggestions**: 3 +**Round**: 2 | **Ship**: recommended — 0 must-fix; round-1's 3 must-fix all verified fixed; only low-impact suggestions remain + +Round-1 must-fixes verified correctly resolved (2 independent adversarial lenses + codebase-convention check): +multi-pose `shape()` now stroked (clickable); teardown double-free fixed via `QPointer`; +marker/hit radii zoom-scaled via `sceneRadius()`. Static analysis (cppcheck) clean. Build/gtest not re-run +here (lower ROS layers unbuilt); model/test untouched since last 5/5 pass — re-run in a provisioned env before merge. + +### Findings +- [ ] (suggestion) Duplicate task `id` in a republish overwrites `items_[id]` without removing the prior item from the scene → orphaned leak; add `if (items_.contains(id)) continue;` — `src/camp/running_tasks/running_tasks_overlay.cpp:81` +- [ ] (suggestion) `TaskOverlayItem::setSelected(bool)` shadows non-virtual `QGraphicsItem::setSelected`; rename to `setHighlighted` to avoid the footgun (not a live bug — nothing calls `scene->selectedItems()`) — `src/camp/running_tasks/task_overlay_item.h:38` +- [ ] (suggestion) `boundingRect()`/`shape()` are zoom-dependent but no `prepareGeometryChange()` on zoom → cached rect can briefly clip when zooming out; cosmetic, self-heals at ~1 Hz rebuild, matches existing `Waypoint` pattern — `src/camp/running_tasks/task_overlay_item.cpp:51-88` + +### Next step +Lifecycle: **Local Review (approved)** → push / open PR → **triage-reviews**. +Branch is shippable; the 3 suggestions are optional and may be applied pre-push or tracked as follow-ups. + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-28 17:43 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: approved + +**Branch**: feature/issue-129 at `16cd55f` +**Mode**: pre-push +**Depth**: Deep (reason: 923 lines / 16 files — both >200 lines and ≥10 files; cross-layer GUI+ROS) +**Must-fix**: 0 | **Suggestions**: 4 +**Round**: 3 | **Ship**: recommended — 0 must-fix; round-2's 3 suggestions all verified resolved; only optional low-impact suggestions remain + +Round-2's 3 suggestions verified resolved in `4bc1b58` + `16cd55f`: dup-id leak guard added +(`running_tasks_overlay.cpp:84`); `setSelected`→`setHighlighted` rename removes the non-virtual +`QGraphicsItem::setSelected` shadow (consistent across header, paint, and both call sites); +zoom-time geometry invalidation added via `updateMapScale` calling `prepareGeometryChange()` on +every scene `GeoGraphicsItem` (`autonomousvehicleproject.cpp:1186`, through the pre-existing public +wrapper at `geographicsitem.h:54`). cppcheck clean (only Qt `slots`-macro noise). Two fresh-context +adversarial passes (Lens A logic/edge-cases, Lens B lifecycle/concurrency/cross-cutting) found no +must-fix; Lens B confirmed the QPointer teardown, the GUI-thread-only `tasksUpdated`/`getGeoCoordinate` +signal path, and the new prepareGeometryChange loop are all sound. Build/gtest not re-run here +(lower ROS layers unbuilt in this worktree); model/test untouched since last 5/5 pass — re-run in a +provisioned env before merge. + +### Findings +- [ ] (suggestion) Selecting a no-pose group row leaves the previously-highlighted overlay item highlighted (tree shows the group row, map still shows the old task orange) — the `hasTaskPoses` early-return is the planned guard, but consider also emitting a deselect so the two panes don't drift — `src/camp/running_tasks/running_tasks_view.cpp:155`, `src/camp/running_tasks/running_tasks_overlay.cpp:30` +- [ ] (suggestion) `onItemClicked` updates `selected_id_` only indirectly (via `setSelectedTask`→`taskSelected`); if `indexForId` ever fails to resolve the id the map click is silently dropped — set the highlight locally before delegating for robustness — `src/camp/running_tasks/running_tasks_overlay.cpp:40` +- [ ] (suggestion) `sceneRadius` returns 0 if the projection collapses (`metresPerPixel`→0 or coincident probe pixels), yielding a zero-width hit stroke that makes a multi-pose item unclickable; add a small floor — `src/camp/running_tasks/task_overlay_item.cpp:32` +- [ ] (suggestion, low) `updateMapScale` `dynamic_cast`s every scene item per zoom tick; negligible at current scene sizes, could pre-filter on `type() >= UserType` if scenes grow large — `src/camp/autonomousvehicleproject.cpp:1186` + +### Next step +Lifecycle: **Local Review (approved)** → push / open PR → **triage-reviews**. +Branch is shippable; all 4 findings are optional suggestions — apply pre-push or track as follow-ups. diff --git a/CMakeLists.txt b/CMakeLists.txt index ff9642b..06baaa3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -110,6 +110,8 @@ set(SOURCES src/camp/helm_manager/helm_manager.cpp src/camp/running_tasks/running_tasks_model.cpp src/camp/running_tasks/running_tasks_view.cpp + src/camp/running_tasks/task_overlay_item.cpp + src/camp/running_tasks/running_tasks_overlay.cpp src/camp/roslink.cpp src/camp/nav_source.cpp ) @@ -420,6 +422,25 @@ if(BUILD_TESTING) Qt5::Positioning ) + # RunningTasksModel::hasTaskPoses guard (camp#129 P2) + ament_add_gtest(test_running_tasks_model + test/test_running_tasks_model.cpp + src/camp/running_tasks/running_tasks_model.cpp + ) + target_include_directories(test_running_tasks_model PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR}/src/camp + ) + ament_target_dependencies(test_running_tasks_model + rclcpp + geometry_msgs + marine_nav_interfaces + marine_nav_tasks + ) + target_link_libraries(test_running_tasks_model + Qt5::Core + Qt5::Widgets + ) + # earth<-frame transform cache / staleness decision (#59 — TF-gap flicker fix) ament_add_gtest(test_transform_cache test/test_transform_cache.cpp diff --git a/src/camp/autonomousvehicleproject.cpp b/src/camp/autonomousvehicleproject.cpp index 3fd2b89..2dee4b8 100644 --- a/src/camp/autonomousvehicleproject.cpp +++ b/src/camp/autonomousvehicleproject.cpp @@ -13,6 +13,7 @@ #include #include +#include "geographicsitem.h" #include "depth_raster.h" #include "waypoint.h" #include "trackline.h" @@ -1175,6 +1176,19 @@ void AutonomousVehicleProject::updateMapScale(qreal scale) // [#59 ADR-0003] Project-level map scale (driven by ProjectView::scaleChanged). // Glyph readers (Waypoint::shape, drawArrow, updateETE) read it via mapScale(); // no per-chart copy to update now that BackgroundRaster is retired. + // + // The scale change resizes constant-pixel glyphs (Waypoint markers/arrows, + // task-overlay markers, etc.), so their item-coordinate boundingRect changes. + // Without notifying the scene, it keeps the stale cached rect and clips the + // glyph when zooming out. Invalidate every scale-dependent scene item's + // geometry here (before the scale changes) so the scene re-indexes and + // repaints them at the new size. This covers all GeoGraphicsItems in the + // scene, including overlays (e.g. running-task items, camp#129) that live + // outside the mission-item tree. + if (m_scene) + for (QGraphicsItem* item : m_scene->items()) + if (auto* ggi = dynamic_cast(item)) + ggi->prepareGeometryChange(); m_map_scale = scale; } diff --git a/src/camp/geographicsitem.h b/src/camp/geographicsitem.h index 0b0e2c1..953dd47 100644 --- a/src/camp/geographicsitem.h +++ b/src/camp/geographicsitem.h @@ -31,6 +31,7 @@ class GeoGraphicsItem : public QGraphicsItem AvoidAreaType, CollisionMonitorType, FootprintType, + RunningTaskType, }; GeoGraphicsItem(QGraphicsItem *parentItem = Q_NULLPTR); diff --git a/src/camp/platform_manager/platform.cpp b/src/camp/platform_manager/platform.cpp index 8b84cc4..696c97b 100644 --- a/src/camp/platform_manager/platform.cpp +++ b/src/camp/platform_manager/platform.cpp @@ -3,6 +3,7 @@ #include #include "nav_source.h" #include "ros/ros_context.h" +#include "running_tasks/running_tasks_overlay.h" #include @@ -110,6 +111,10 @@ void Platform::update(const marine_interfaces::msg::Platform& platform) path_topic_ = "/" + platformNamespace + "/received_global_plan"; subscribeToPathTopic(); + // Fallback: if the scene wasn't attached when the node first arrived, create + // the overlay now that the platform is (very likely) in the scene. + ensureRunningTasksOverlay(); + m_width = platform.width; m_length = platform.length; m_reference_x = platform.reference_x; @@ -236,6 +241,26 @@ void Platform::onNodeUpdated() m_ui->missionManager->nodeStarted(node_, transform_buffer_); m_ui->runningTasksView->setNode(node_); subscribeToPathTopic(); + + // Create the overlay if possible; otherwise rebind the existing one to the + // (possibly changed) node. + if (running_tasks_overlay_) + running_tasks_overlay_->nodeStarted(node_, transform_buffer_); + else + ensureRunningTasksOverlay(); +} + +void Platform::ensureRunningTasksOverlay() +{ + // Platform is a GeoGraphicsItem so scene() gives us the shared QGraphicsScene + // directly. The scene may not be attached yet on the first node update, so + // this is also called from the periodic update(): the overlay is created on + // whichever call first sees both node_ and scene() non-null. + if (running_tasks_overlay_ || !node_ || !scene()) + return; + running_tasks_overlay_ = new RunningTasksOverlay( + scene(), m_ui->runningTasksView, this); + running_tasks_overlay_->nodeStarted(node_, transform_buffer_); } void Platform::subscribeToPathTopic() diff --git a/src/camp/platform_manager/platform.h b/src/camp/platform_manager/platform.h index 443da42..8750aad 100644 --- a/src/camp/platform_manager/platform.h +++ b/src/camp/platform_manager/platform.h @@ -14,6 +14,7 @@ namespace Ui class NavSource; class MissionManager; class HelmManager; +class RunningTasksOverlay; class Platform : public camp_ros::ROSWidget, public ShipTrack { @@ -55,6 +56,11 @@ public slots: void updateLabel(); void setColor(QColor color); void subscribeToPathTopic(); + /// Create the running-tasks overlay once both the ROS node and the scene are + /// available, then (re)bind it to the node. Idempotent — safe to call from + /// any hook (onNodeUpdated, periodic update()) so a scene that attaches after + /// the node still gets an overlay. + void ensureRunningTasksOverlay(); void pathCallback(const nav_msgs::msg::Path::SharedPtr msg); @@ -80,6 +86,8 @@ public slots: std::vector path_geopoints_; std::vector path_local_points_; + RunningTasksOverlay* running_tasks_overlay_ = nullptr; + }; #endif // PLATFORM_H diff --git a/src/camp/running_tasks/running_tasks_model.cpp b/src/camp/running_tasks/running_tasks_model.cpp index c629b14..89c88be 100644 --- a/src/camp/running_tasks/running_tasks_model.cpp +++ b/src/camp/running_tasks/running_tasks_model.cpp @@ -230,3 +230,11 @@ QModelIndex RunningTasksModel::indexForId(const QString& id) const return QModelIndex(); return createIndex(node->rowInParent, 0, node); } + +bool RunningTasksModel::hasTaskPoses(const QModelIndex& index) const +{ + Node* n = nodeForIndex(index); + if (!n || !n->task) + return false; + return !n->task->message().poses.empty(); +} diff --git a/src/camp/running_tasks/running_tasks_model.h b/src/camp/running_tasks/running_tasks_model.h index f2dcf5d..954471a 100644 --- a/src/camp/running_tasks/running_tasks_model.h +++ b/src/camp/running_tasks/running_tasks_model.h @@ -51,6 +51,10 @@ class RunningTasksModel : public QAbstractItemModel /// Index of the row carrying the given full id (invalid if not present). QModelIndex indexForId(const QString& id) const; + /// True when the node at index has at least one pose (i.e. it is a leaf + /// task with geometry, not a pure structural parent row). + bool hasTaskPoses(const QModelIndex& index) const; + private: struct Node { diff --git a/src/camp/running_tasks/running_tasks_overlay.cpp b/src/camp/running_tasks/running_tasks_overlay.cpp new file mode 100644 index 0000000..0bbfa0e --- /dev/null +++ b/src/camp/running_tasks/running_tasks_overlay.cpp @@ -0,0 +1,127 @@ +#include "running_tasks/running_tasks_overlay.h" + +#include + +#include "running_tasks/running_tasks_view.h" + +RunningTasksOverlay::RunningTasksOverlay(QGraphicsScene* scene, + RunningTasksView* view, + QWidget* parent) + : camp_ros::ROSWidget(parent), scene_(scene), view_(view) +{ + connect(view_, &RunningTasksView::tasksUpdated, + this, &RunningTasksOverlay::onTasksUpdated); + connect(view_, &RunningTasksView::taskSelected, + this, &RunningTasksOverlay::onTaskSelected); +} + +RunningTasksOverlay::~RunningTasksOverlay() +{ + clearItems(); +} + +void RunningTasksOverlay::onTasksUpdated( + const QString& current_task, + const std::vector& tasks) +{ + rebuildItems(current_task, tasks); +} + +void RunningTasksOverlay::onTaskSelected(const QString& id) +{ + if (id == selected_id_) + return; // idempotent — guard against map→tree→map round-trips + + selected_id_ = id; + for (auto it = items_.begin(); it != items_.end(); ++it) + it.value()->setHighlighted(it.key() == id); +} + +void RunningTasksOverlay::onItemClicked(const QString& id) +{ + // Guard the selection-sync loop so a map click doesn't trigger + // taskSelected → onTaskSelected → setHighlighted → redraw → re-click. An + // id-equality short-circuit suffices here (the round-trip ends once the id + // already matches), so no QSignalBlocker is needed. + if (id == selected_id_) + return; + // Apply the highlight locally first so a map click is reflected immediately + // and robustly even if the tree can't resolve the id (indexForId miss). The + // subsequent setSelectedTask → taskSelected → onTaskSelected is then a no-op + // (id already current), so there's no round-trip. + onTaskSelected(id); + view_->setSelectedTask(id); +} + +void RunningTasksOverlay::rebuildItems( + const QString& current_task, + const std::vector& tasks) +{ + // getGeoCoordinate's stale-stamp fallback logs via node_->get_logger(), so + // node_ must be set too — not just the transform buffer. + if (!scene_ || !transform_buffer_ || !node_) + return; + + clearItems(); + + for (const auto& task : tasks) + { + if (task.poses.empty()) + continue; + + // TaskInformation.poses is geometry_msgs/PoseStamped[] with per-pose headers. + // getGeoCoordinate transforms each to earth frame with stale-stamp retry, + // so the source frame (commonly map or earth) is handled transparently. + QList geo_poses; + for (const auto& ps : task.poses) + { + QGeoCoordinate gc = getGeoCoordinate(ps.pose, ps.header); + if (gc.isValid()) + geo_poses.append(gc); + } + if (geo_poses.isEmpty()) + continue; + + const QString id = QString::fromStdString(task.id); + // A republish shouldn't carry duplicate ids, but guard anyway: a second + // item with the same id would overwrite items_[id] and orphan the first + // one in the scene (leak, since clearItems only tracks items_). + if (items_.contains(id)) + continue; + const bool is_current = !current_task.isEmpty() && + id.split('/', Qt::SkipEmptyParts).join('/') == + current_task.split('/', Qt::SkipEmptyParts).join('/'); + + auto* item = new TaskOverlayItem(id, geo_poses, is_current, task.done); + item->setHighlighted(id == selected_id_); + + connect(item, &TaskOverlayItem::clicked, + this, &RunningTasksOverlay::onItemClicked); + + scene_->addItem(item); + items_[id] = item; + } + + // Drop a stale selection: if the previously-selected task is gone from this + // republish, clear selected_id_ so the overlay and tree don't drift. + if (!selected_id_.isEmpty() && !items_.contains(selected_id_)) + selected_id_.clear(); +} + +void RunningTasksOverlay::clearItems() +{ + if (scene_) + { + // Scene still alive: addItem() left these items parentless, so the scene + // and this overlay co-own them. Reclaim each from the scene and delete it. + for (auto* item : items_) + { + scene_->removeItem(item); + delete item; + } + } + // If scene_ is null the QGraphicsScene was destroyed first and already + // deleted its child items; the pointers in items_ now dangle, so drop our + // references without touching them (avoids the use-after-free / double-free). + items_.clear(); +} diff --git a/src/camp/running_tasks/running_tasks_overlay.h b/src/camp/running_tasks/running_tasks_overlay.h new file mode 100644 index 0000000..e44184d --- /dev/null +++ b/src/camp/running_tasks/running_tasks_overlay.h @@ -0,0 +1,63 @@ +#ifndef CAMP_RUNNING_TASKS_OVERLAY_H +#define CAMP_RUNNING_TASKS_OVERLAY_H + +#include + +#include +#include +#include +#include + +#include "marine_nav_interfaces/msg/task_information.hpp" +#include "ros/ros_widget.h" +#include "running_tasks/task_overlay_item.h" + +class RunningTasksView; + +/// CAMP-only glue that owns the running-task QGraphicsScene overlay. +/// +/// This is the camp_ros::ROSWidget side of the P2 feature: +/// - getGeoCoordinate (from ROSWidget) converts each PoseStamped to earth. +/// - tasksUpdated from RunningTasksView triggers a rebuild-all of overlay items. +/// - taskSelected from RunningTasksView highlights the matching item. +/// - TaskOverlayItem::clicked feeds back into RunningTasksView::setSelectedTask. +/// +/// Rebuild-all strategy: every tasksUpdated call removes all existing items and +/// recreates them. This is correct for the periodic ~1 Hz republish cadence; the +/// simplicity outweighs incremental-diff complexity at this update rate. +class RunningTasksOverlay : public camp_ros::ROSWidget +{ + Q_OBJECT + +public: + /// \param scene The QGraphicsScene to add overlay items to (from + /// Platform::scene() — Platform is a GeoGraphicsItem). + /// \param view The RunningTasksView whose signals we consume. + /// \param parent QWidget parent (typically Platform's widget parent). + explicit RunningTasksOverlay(QGraphicsScene* scene, RunningTasksView* view, + QWidget* parent = nullptr); + ~RunningTasksOverlay() override; + +private slots: + void onTasksUpdated( + const QString& current_task, + const std::vector& tasks); + void onTaskSelected(const QString& id); + void onItemClicked(const QString& id); + +private: + void rebuildItems( + const QString& current_task, + const std::vector& tasks); + void clearItems(); + + // QPointer so a QGraphicsScene destroyed before this overlay reads back as + // null instead of dangling — clearItems() then skips the (already-deleted) + // items rather than double-freeing them. + QPointer scene_; + RunningTasksView* view_; + QMap items_; + QString selected_id_; +}; + +#endif // CAMP_RUNNING_TASKS_OVERLAY_H diff --git a/src/camp/running_tasks/running_tasks_view.cpp b/src/camp/running_tasks/running_tasks_view.cpp index 7a62919..12e0d4c 100644 --- a/src/camp/running_tasks/running_tasks_view.cpp +++ b/src/camp/running_tasks/running_tasks_view.cpp @@ -147,11 +147,15 @@ void RunningTasksView::applyPendingTasks() tree_->setCurrentIndex(restored); } } + + emit tasksUpdated(current, tasks); } void RunningTasksView::onCurrentRowChanged(const QModelIndex& current, const QModelIndex& /*previous*/) { + if (!model_->hasTaskPoses(current)) + return; const QString id = model_->idForIndex(current); if (!id.isEmpty()) emit taskSelected(id); diff --git a/src/camp/running_tasks/running_tasks_view.h b/src/camp/running_tasks/running_tasks_view.h index 85d0f3a..eff7509 100644 --- a/src/camp/running_tasks/running_tasks_view.h +++ b/src/camp/running_tasks/running_tasks_view.h @@ -32,7 +32,13 @@ class RunningTasksView : public QWidget signals: /// User selected a task row (full id). Map glue (P2) consumes this. + /// Only emitted for rows where hasTaskPoses() is true. void taskSelected(QString id); + /// Emitted on the GUI thread after every model update with the current + /// navigation task id and the full raw task list. The RunningTasksOverlay + /// consumes this to rebuild overlay geometry without a second ROS subscription. + void tasksUpdated(QString current_task, + std::vector tasks); public slots: void updateRobotNamespace(QString robot_namespace); diff --git a/src/camp/running_tasks/task_overlay_item.cpp b/src/camp/running_tasks/task_overlay_item.cpp new file mode 100644 index 0000000..0a3586c --- /dev/null +++ b/src/camp/running_tasks/task_overlay_item.cpp @@ -0,0 +1,150 @@ +#include "running_tasks/task_overlay_item.h" + +#include + +#include +#include +#include +#include +#include +#include + +namespace +{ +// Desired on-screen marker/hit footprints in display pixels (held constant +// across zoom via sceneRadius — ADR-0003). +constexpr qreal kSinglePoseRadiusPx = 6.0; +constexpr qreal kEndpointRadiusPx = 3.0; +constexpr qreal kHitStrokePx = 5.0; +} // namespace + +TaskOverlayItem::TaskOverlayItem(const QString& id, + const QList& geo_poses, + bool is_current, bool is_done, + QGraphicsItem* parent) + : QObject(nullptr), GeoGraphicsItem(parent), + id_(id), geo_poses_(geo_poses), + is_current_(is_current), is_done_(is_done) +{ + setAcceptHoverEvents(false); +} + +qreal TaskOverlayItem::sceneRadius(const QGeoCoordinate& at, qreal pixels) const +{ + if (!at.isValid()) + return pixels; + // Real metres spanning `pixels` display pixels here, projected back through + // geoToPixel so the result is the equivalent radius in scene units. + const qreal metres = pixels * metresPerPixel(at); + const QPointF center = geoToPixel(at); + const QPointF edge = geoToPixel(at.atDistanceAndAzimuth(metres, 0.0)); + const qreal radius = std::hypot(center.x() - edge.x(), center.y() - edge.y()); + // Floor against a collapsed projection (zero metresPerPixel or coincident + // probe pixels): a zero-width hit stroke would make the item unclickable. + return radius > 0.0 ? radius : pixels; +} + +QPainterPath TaskOverlayItem::buildPath() const +{ + QPainterPath path; + if (geo_poses_.isEmpty()) + return path; + + if (geo_poses_.size() == 1) + { + // Single pose: filled circle at scene position (constant pixel footprint). + const QGeoCoordinate& c = geo_poses_.first(); + const qreal r = sceneRadius(c, kSinglePoseRadiusPx); + path.addEllipse(geoToPixel(c), r, r); + } + else + { + // Multi-pose: polyline through all scene positions. + path.moveTo(geoToPixel(geo_poses_.first())); + for (int i = 1; i < geo_poses_.size(); ++i) + path.lineTo(geoToPixel(geo_poses_[i])); + } + return path; +} + +QRectF TaskOverlayItem::boundingRect() const +{ + if (geo_poses_.isEmpty()) + return {}; + const qreal m = sceneRadius(geo_poses_.first(), kHitStrokePx + kSinglePoseRadiusPx); + return buildPath().boundingRect().marginsAdded(QMarginsF(m, m, m, m)); +} + +QPainterPath TaskOverlayItem::shape() const +{ + QPainterPath path = buildPath(); + if (geo_poses_.size() <= 1) + return path; // single-pose ellipse already encloses a clickable area + + // A multi-pose buildPath() is an open polyline: zero area, so Qt's + // shape().contains() hit-test never matches and the item is unclickable + // (breaking map->tree selection for survey_line/transit tasks). Stroke it + // into a clickable band of constant pixel width (ADR-0003). + QPainterPathStroker stroker; + stroker.setWidth(2.0 * sceneRadius(geo_poses_.first(), kHitStrokePx)); + return stroker.createStroke(path); +} + +void TaskOverlayItem::paint(QPainter* painter, + const QStyleOptionGraphicsItem* /*option*/, + QWidget* /*widget*/) +{ + if (geo_poses_.isEmpty()) + return; + + painter->save(); + + QColor color; + if (is_done_) + color = QColor(0x80, 0x80, 0x80, 180); // gray — completed + else if (is_highlighted_) + color = QColor(0xFF, 0xA5, 0x00, 230); // orange — selected + else if (is_current_) + color = QColor(0x00, 0xCC, 0x00, 220); // green — active + else + color = QColor(0x40, 0x80, 0xFF, 150); // blue-muted — pending + + QPen pen(color); + pen.setCosmetic(true); + pen.setWidth(is_highlighted_ ? 3 : 2); + painter->setPen(pen); + + if (geo_poses_.size() == 1) + { + painter->setBrush(QBrush(color)); + painter->drawPath(buildPath()); + } + else + { + painter->setBrush(Qt::NoBrush); + painter->drawPath(buildPath()); + // Draw small endpoint markers (constant pixel footprint — ADR-0003). + painter->setBrush(QBrush(color)); + for (const QGeoCoordinate& gc : geo_poses_) + { + if (gc.isValid()) + { + const qreal r = sceneRadius(gc, kEndpointRadiusPx); + painter->drawEllipse(geoToPixel(gc), r, r); + } + } + } + + painter->restore(); +} + +void TaskOverlayItem::setHighlighted(bool highlighted) +{ + is_highlighted_ = highlighted; + update(); +} + +void TaskOverlayItem::mousePressEvent(QGraphicsSceneMouseEvent* /*event*/) +{ + emit clicked(id_); +} diff --git a/src/camp/running_tasks/task_overlay_item.h b/src/camp/running_tasks/task_overlay_item.h new file mode 100644 index 0000000..3b26b0b --- /dev/null +++ b/src/camp/running_tasks/task_overlay_item.h @@ -0,0 +1,65 @@ +#ifndef CAMP_RUNNING_TASKS_TASK_OVERLAY_ITEM_H +#define CAMP_RUNNING_TASKS_TASK_OVERLAY_ITEM_H + +#include +#include +#include +#include + +#include "geographicsitem.h" + +/// Renders one running task's poses on the QGraphicsScene. +/// +/// Single pose → filled circle (goto-style task). +/// Two+ poses → polyline (survey_line, transit, etc.). +/// +/// Visual states: selected (bright/bold), active (current task), done (gray), +/// pending (muted). Emits clicked(id) on mouse press for map→tree selection. +/// +/// Uses geoToPixel() from GeoGraphicsItem (anchored to the Web-Mercator scene +/// origin at (0,0)); no setPos() needed — all rendering is in scene coordinates. +class TaskOverlayItem : public QObject, public GeoGraphicsItem +{ + Q_OBJECT + Q_INTERFACES(QGraphicsItem) + +public: + explicit TaskOverlayItem(const QString& id, + const QList& geo_poses, + bool is_current, bool is_done, + QGraphicsItem* parent = nullptr); + + int type() const override { return RunningTaskType; } + + QRectF boundingRect() const override; + void paint(QPainter* painter, const QStyleOptionGraphicsItem* option, + QWidget* widget) override; + QPainterPath shape() const override; + + /// Highlight this item as the map-selected task. Named to avoid shadowing the + /// non-virtual QGraphicsItem::setSelected (which drives scene->selectedItems()). + void setHighlighted(bool highlighted); + +signals: + void clicked(QString id); + +protected: + void mousePressEvent(QGraphicsSceneMouseEvent* event) override; + +private: + QPainterPath buildPath() const; + + /// Scene-unit radius that renders as \p pixels display pixels at \p at, + /// following the ADR-0003 constant-pixel-footprint convention (mirrors + /// ShipTrack::drawTriangle's no-heading marker) so markers hold their + /// on-screen size regardless of zoom. + qreal sceneRadius(const QGeoCoordinate& at, qreal pixels) const; + + QString id_; + QList geo_poses_; + bool is_current_; + bool is_done_; + bool is_highlighted_ = false; +}; + +#endif // CAMP_RUNNING_TASKS_TASK_OVERLAY_ITEM_H diff --git a/test/test_running_tasks_model.cpp b/test/test_running_tasks_model.cpp new file mode 100644 index 0000000..da08800 --- /dev/null +++ b/test/test_running_tasks_model.cpp @@ -0,0 +1,128 @@ +// Tests for RunningTasksModel::hasTaskPoses — the guard that prevents +// taskSelected from firing for synthetic group/parent rows that have no +// geometry. Covers: group row with empty poses returns false; leaf task with +// at least one pose returns true. + +#include + +#include +#include +#include + +#include +#include + +#include "marine_nav_interfaces/msg/task_information.hpp" +#include "running_tasks/running_tasks_model.h" + +namespace +{ + +marine_nav_interfaces::msg::TaskInformation makeTask( + const std::string& id, + const std::string& type, + bool with_pose = false) +{ + marine_nav_interfaces::msg::TaskInformation t; + t.id = id; + t.type = type; + t.priority = 10; + t.done = false; + if (with_pose) + { + geometry_msgs::msg::PoseStamped ps; + ps.header.frame_id = "map"; + ps.pose.position.x = 1.0; + ps.pose.position.y = 2.0; + t.poses.push_back(ps); + } + return t; +} + +} // namespace + +class RunningTasksModelTest : public ::testing::Test +{ +protected: + rclcpp::Node::SharedPtr node; + RunningTasksModel model; + + void SetUp() override + { + node = rclcpp::Node::make_shared("running_tasks_model_test"); + } + + void setTasks(const std::string& current, + const std::vector& tasks) + { + model.setTasks(QString::fromStdString(current), tasks, node->get_clock()); + } +}; + +TEST_F(RunningTasksModelTest, hasTaskPoses_invalidIndex_returnsFalse) +{ + EXPECT_FALSE(model.hasTaskPoses(QModelIndex())); +} + +TEST_F(RunningTasksModelTest, hasTaskPoses_groupRowWithEmptyPoses_returnsFalse) +{ + // A parent/group row whose TaskInformation has no poses (structural parent). + std::vector tasks = { + makeTask("survey_a", "survey", /*with_pose=*/false), + makeTask("survey_a/line_1", "survey_line", /*with_pose=*/true), + }; + setTasks("survey_a/line_1", tasks); + + // First top-level row should be the parent "survey_a" task (no poses). + const QModelIndex parent_idx = model.index(0, 0); + ASSERT_TRUE(parent_idx.isValid()); + EXPECT_EQ(model.idForIndex(parent_idx), QStringLiteral("survey_a")); + EXPECT_FALSE(model.hasTaskPoses(parent_idx)) + << "group row with empty poses must return false"; +} + +TEST_F(RunningTasksModelTest, hasTaskPoses_leafTaskWithPoses_returnsTrue) +{ + std::vector tasks = { + makeTask("survey_a", "survey", /*with_pose=*/false), + makeTask("survey_a/line_1", "survey_line", /*with_pose=*/true), + }; + setTasks("survey_a/line_1", tasks); + + // Child of the first top-level row: survey_a/line_1 (has a pose). + const QModelIndex parent_idx = model.index(0, 0); + ASSERT_TRUE(parent_idx.isValid()); + const QModelIndex leaf_idx = model.index(0, 0, parent_idx); + ASSERT_TRUE(leaf_idx.isValid()); + EXPECT_EQ(model.idForIndex(leaf_idx), QStringLiteral("survey_a/line_1")); + EXPECT_TRUE(model.hasTaskPoses(leaf_idx)) + << "leaf task with at least one pose must return true"; +} + +TEST_F(RunningTasksModelTest, hasTaskPoses_topLevelTaskWithPoses_returnsTrue) +{ + // A flat (non-hierarchical) task with poses — e.g. a simple goto. + std::vector tasks = { + makeTask("goto_wp1", "goto", /*with_pose=*/true), + }; + setTasks("goto_wp1", tasks); + + const QModelIndex idx = model.index(0, 0); + ASSERT_TRUE(idx.isValid()); + EXPECT_TRUE(model.hasTaskPoses(idx)); +} + +TEST_F(RunningTasksModelTest, hasTaskPoses_emptyModel_returnsFalse) +{ + // Nothing set — any index is invalid. + EXPECT_FALSE(model.hasTaskPoses(model.index(0, 0))); +} + +int main(int argc, char** argv) +{ + rclcpp::init(argc, argv); + ::testing::InitGoogleTest(&argc, argv); + int rc = RUN_ALL_TESTS(); + rclcpp::shutdown(); + return rc; +}