Skip to content

fix(setup): camera wizard UX follow-ups from #262 review (#263) - #268

Open
Adityakk9031 wants to merge 1 commit into
robocurve:mainfrom
Adityakk9031:feature/263-wizard-camera-ux-followups
Open

fix(setup): camera wizard UX follow-ups from #262 review (#263)#268
Adityakk9031 wants to merge 1 commit into
robocurve:mainfrom
Adityakk9031:feature/263-wizard-camera-ux-followups

Conversation

@Adityakk9031

Copy link
Copy Markdown
Contributor

Closes #263.

Addresses three non-blocking camera UX follow-ups deferred from the #262 review:

  1. Duplicate camera detection: Maps candidate and assigned paths to physical camera identity (_CameraNode.camera / sysfs USB device directory or canonical resolved node) before comparing, catching duplicates across by-id and by-path aliases.
  2. Fresh listing row rescan after replug: Rescans inventory and rebuilds by_id_devices and by_path_devices after a successful u unplug-to-identify flow, clearing stale rows.
  3. Toggle advertisement cleanup: Suppresses the p prompt when by_id_rows == by_path_rows.

Verification

  • ruff check . — All checks passed.
  • ruff format --check . — All 123 files formatted.
  • pytest tests/test_setup.py — 113 passed, 43 skipped (gated on POSIX/symlinks).

@Adityakk9031

Copy link
Copy Markdown
Contributor Author

@aris-zhu and @jeqcho have a look

@jeqcho jeqcho left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Adityakk9031 — this lands all three follow-ups from #263 cleanly, and the physical-identity duplicate check in particular is exactly the shape the issue hoped for: mapping both sides through _CameraNode.camera with a resolved-path fallback is robust, and the by-id-vs-by-path duplicate test genuinely exercises it (it fails on main, as a good regression test should). The in-place clear()/extend() rebuild after u is also the right call, since the row lists are shared by reference across slots.

Two things I'd like addressed before merge, plus a couple of optional notes:

  1. test_camera_view_state_suppresses_toggle_when_rows_are_identical doesn't exercise the new code (tests/test_setup.py, ~line 3819). Its inventory record has by_id set and a unique serial, so the pre-existing inventory branch already returns advertise_path_toggle=False — the test passes without this PR's change. To cover the new suppression line, use a record with by_id=None (and a by-path name): the old code advertises the toggle for it, both views then render the identical by-path row, and the new by_id_rows == by_path_rows guard is what flips it to False.

  2. Reprint the active listing when the post-u rescan changes the rows (src/inspect_robots/_setup.py, ~lines 567-575). The numbered listing is printed once at section start; after a replug re-rolls udev links, the rebuilt rows can diverge from what's on screen, so a number typed at a later slot silently selects a different device than the one displayed — the very race this change is meant to close. A cheap guard: capture the old active rows, and if they differ after the rebuild, call _print_camera_listing for the active view.

Optional, non-blocking:

  • A small test for the rescan path (scripted u with a rescan_inventory that returns changed rows, asserting the row lists were rebuilt) would protect item 2 of the issue, which currently has no coverage.
  • advertise_path_toggle isn't recomputed after the rescan, so a replug that changes by-id availability leaves the p advertisement stale for later slots — fine to defer, just noting it.
  • Tiny style nit: physical_id is re-defined on every loop iteration and the kind == "v4l2" and inventory guard appears twice; hoisting the helper above the loop would tidy this.

Also a heads-up that the branch conflicts with main (the _duplicated_serials_ambiguous_identities refactor touched _camera_view_state), so the rebase will need a little care in exactly the function this PR edits.

Thanks again — with the test fixed and the listing reprint added, this is good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

setup: wizard camera UX follow-ups from the #262 review

2 participants