style(dialogs): unify picker tables (selection, edit, sort, header)#188
Merged
Conversation
Bring the topic/channel/camera/sequence picker tables onto one idiom: - Multi-select pickers all use MultiSelection (bare click toggles/accumulates) instead of ExtendedSelection (bare click silently clears the prior pick). Converts ros2 listRosTopics and mosaico topicTable. - Every picker gets NoEditTriggers, SelectRows, verticalHeaderVisible=false, and sortingEnabled=true (Qt's default indicator starts the table sorted by column 0 ascending). Fixes foxglove/pj_bridge topicsList and webrtc camerasList, which were accidentally editable and had no consistent header. - mosaico seqTable intentionally stays SingleSelection; mosaico sorting stays off (its index-based setVisibleRows filtering would desync under re-sort). Per-column resize-to-content is deliberately not addressed here: column sizing is owned by the host's installTreeLikeHeader() and cannot be expressed in a .ui file. That is tracked as a follow-up SDK protocol change (see docs/superpowers/specs/2026-07-03-dialog-table-column-resize-mode-design.md). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Design and TDD implementation plan for a follow-up SDK protocol addition that lets a plugin declare per-column Qt resize modes for dialog tables (name column Stretch, short data columns ResizeToContents) — the one picker-table axis that cannot be set from a .ui file because column sizing is owned by the host's installTreeLikeHeader(). Additive, header-only WidgetData::setColumnResizeMode / WidgetDataView:: columnResizeModes across plotjuggler_sdk -> PJ4 -> pj-official-plugins; no compiled-ABI break. Not yet implemented; this only captures the approved design. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
Codex review follow-up (pushed 973a78c)An independent Codex review flagged a real issue in the original branch: the host realizes plugin Per-picker selection contract (verified in code):
Fix in 973a78c: drop the Follow-up (not this PR): |
Contributor
Author
Follow-up: mcap + ros2 sort now fixed in-PR (0b994d3)Digging into the two pre-existing index-based pickers, the desync turned out to be latent behind a coincidence, not a live bug: both row vectors are already name-sorted (mcap Rather than leave that fragility, per request this PR now drops Final state — built-in sort is on iff selection restore is text-based:
|
The pickers now keep Qt's built-in column sorting (sortingEnabled, default
column-0 ascending) AND restore selection correctly, by identifying the
selection by first-column text instead of row index.
Background: the host realizes the .ui via QUiLoader, which honors sortingEnabled
and gives the widget a *sorted* view. Any selection restored by row index
(setSelectedRows) then desyncs — the plugin's row vector stays in its own order
while the widget re-orders. Restoring by text (setSelectedItems) is
sort-agnostic: the host matches items by their first-column string. This is the
same pattern foxglove/pj_bridge already use.
- ros2 listRosTopics: drop the name->index translation (and the now-unused
ordered_names vector); hand the host the selected topic names directly.
- webrtc camerasList: collect the selected rows' unique display labels (1:1 with
the stream id within a render) instead of row indices. Read-back already maps
labels back to ids, so the round-trip stays lossless.
Neither plugin uses setVisibleRows/setDisabledRows, so selection was their only
index dependency — converting it makes built-in sort fully safe. Verified:
webrtc compiles clean under -Werror -Wconversion.
Deferred (need cross-repo work, not in this PR): mcap tableWidget also disables
empty channels by row index (setDisabledRows, no text-keyed equivalent yet), and
mosaico filters by row index (setVisibleRows). Making those sort-correct wants
plugin-owned onHeaderClicked sorting, which needs the host to wire
QHeaderView::sectionClicked -> onHeaderClicked (declared in the SDK but currently
undelivered for tables) plus a sort-indicator API.
Docs: correct foxglove topicsList to 3 columns {Topic Name, DataType, Encoding}.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
facontidavide
force-pushed
the
feat/unify-picker-tables
branch
from
July 6, 2026 09:28
0b994d3 to
9c41ae1
Compare
Contributor
Author
Course correction: pickers are now genuinely sortable (force-pushed, 9c41ae1)I initially misread the goal and removed built-in sorting to dodge the index-desync bug. That was backwards — the ask is for sortable columns (default column-0). I've dropped that detour (force-pushed) and instead fixed the root cause the right way: restore selection by first-column text, not row index, so it survives Qt's built-in sort. This is the same pattern foxglove/pj_bridge already use. Now sortable (built-in sort on, default column 0):
Deferred to a follow-up (need cross-repo work):
Branch (3 commits): unify pickers → resize-mode docs → make ros2/webrtc sortable. mcap/mosaico left exactly as they were on |
This was referenced Jul 10, 2026
pabloinigoblasco
added a commit
that referenced
this pull request
Jul 13, 2026
…ex (#197) Both dialogs sent setSelectedRows computed against their own row order. With sortingEnabled (ros2 today; webrtc once #188 lands) index-keyed restore lands on the wrong rows after a user sort. setSelectedItems is honored for QTableWidget since PJ4#349 and matches rows by text, which is sort-agnostic — same pattern mcap already uses. Both code comments also claimed the host ignores selected_items, which is stale since PJ4#349. Claude-Session: https://claude.ai/code/session_01RP8RosW9FCsePNUttEYAXk Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Pablo Iñigo Blasco <pablo.inigo@ibrobotics.com>
Conflict resolutions, documented per file: - data_stream_ros2/distro/src/ros2_dialog.hpp, data_stream_webrtc/webrtc_dialog.cpp: taken from MAIN verbatim. This branch's commit 9c41ae1 and merged PR #197 implement the same fix (restore picker selection via setSelectedItems keyed by first-column text instead of row indices); #197 landed first, so 9c41ae1's payload is superseded and contributes nothing to the net diff. - data_stream_foxglove_bridge/foxglove_client.ui: took MAIN's structure (PR #200 reworked the dialog layout: SectionHeaderBand + filter header) and re-applied this branch's picker properties onto the topicsList block (SelectRows, NoEditTriggers, sortingEnabled, hidden vertical header) — same payload, new surrounding layout. Auto-merged and audited (properties landed inside the right widget blocks of #200's reworked layouts): websocket_client.ui, datastream_webrtc.ui, datastream_ros2.ui, mosaico_panel.ui. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AP5H4GFFwKXLRbG17GGtzC
Since PJ4#356 (fix 4) the dialog host forces setEditTriggers(NoEditTriggers) on every protocol table (widget_binding.cpp), because the dialog protocol has no cell-edit event and an editable cell always discards the edit silently. Keeping per-.ui copies would re-introduce the per-plugin discipline that host fix removed (decision recorded in #199). The rest of the picker idiom (SelectRows, sortingEnabled, hidden vertical header) stays — that is the actual payload of this PR. Only the three .ui this branch added the property to are touched; ros2/mosaico carry it from main, outside this PR's diff. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AP5H4GFFwKXLRbG17GGtzC
The design itself is unchanged (still valid, still unimplemented). What rotted while the branch waited: - widget_binding.cpp line numbers predated PJ4#356's reshuffle — anchor by symbol instead of line. - Edit-triggers are no longer a per-.ui concern: PJ4#356 forces every protocol table read-only in the host. - The mosaico non-goal claimed index-based setVisibleRows *cannot* work under sort; since PJ4#356 the host translates index-keyed state, and mosaico re-sorts via its own onHeaderClicked sorter — restated as a scope decision, not a technical impossibility. - "bump minor" SDK versioning predated upstream taking 0.16.x (our SDK line is 0.17.0 now) — point at "next SDK release" instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AP5H4GFFwKXLRbG17GGtzC
Contributor
Branch updated: merged
|
| Decision | Why |
|---|---|
ros2_dialog.hpp / webrtc_dialog.cpp conflicts → resolved to main |
This branch's 9c41ae15 and merged #197 are the same fix (restore selection via setSelectedItems by first-column text). #197 landed first; the commit's payload is fully absorbed — the net diff has zero C++ changes left. |
foxglove_client.ui conflict → main's layout + this PR's picker props |
#200 reworked the dialog layout (SectionHeaderBand). The branch's payload (SelectRows / sortingEnabled / hidden v-header) was re-applied onto the reworked topicsList block. |
Dropped the NoEditTriggers additions (foxglove / pj_bridge / webrtc .ui) |
Redundant since PJ4#356 (fix 4): the host forces every protocol table read-only, because the protocol has no cell-edit event. Keeping per-.ui copies re-introduces the per-plugin discipline that host fix removed (decision recorded in #199). |
| Refreshed the resize-mode spec/plan facts | The design is untouched (still valid, still unimplemented), but its "verified facts" rotted: widget_binding.cpp line anchors predate #356's reshuffle; the mosaico non-goal claimed a technical impossibility that #356's index translation removed (now stated as a scope decision); SDK versioning now points at "next release" (0.17.0 line) instead of "bump minor". |
What the PR now ships (net vs main):
| Change | Files |
|---|---|
Picker idiom: SelectRows + sortingEnabled + hidden vertical header |
foxglove / pj_bridge / webrtc .ui (+21 lines) |
ExtendedSelection → MultiSelection |
ros2 listRosTopics, mosaico topicTable (1 line each) |
| Resize-mode design spec + implementation plan (docs only) | docs/superpowers/ (2 files) |
Sorting these three pickers is safe on two independent grounds now: their selection restore is text-keyed (#197 / already was), and PJ4#356 translates index-keyed state through the sorted view host-side.
Two calls that deserve an explicit ✓ rather than riding under "style":
- MultiSelection on ros2/mosaico — consistent with the other three pickers and with the "Select one or multiple…" labels (bare click accumulates instead of silently clearing). The cost: shift-click range selection stops working, which is felt on ROS 2 systems with hundreds of topics. Fine by us as a deliberate trade-off — flagging it so it's chosen, not inherited.
docs/superpowers/location — this PR creates that convention (docs/doesn't exist onmaintoday). Since the resize-mode design spans three repos, a SDK-repo issue may be the better home. Your call as repo owner; the content is ready either way.
Proposed replacement PR description (the current one predates #197/#356/#200 and describes a main that no longer exists)
What
Unifies the topic / camera picker tables of the streaming dialogs on one idiom, now that the host is sort-safe (PJ4#356) and all pickers restore selection by text.
Picker idiom
foxglove topicsList, pj_bridge topicsList, webrtc camerasList gain, via .ui: SelectRows · sortingEnabled · hidden vertical header. (NoEditTriggers is not set per-.ui: the host forces every protocol table read-only since PJ4#356.)
Selection mode
ros2 listRosTopics and mosaico topicTable switch ExtendedSelection → MultiSelection, matching the other three pickers: in a picker, a bare click under ExtendedSelection silently clears the previous picks; MultiSelection makes it toggle/accumulate, as the "Select one or multiple…" labels promise. Deliberate trade-off: shift-click range selection is lost.
Docs
Adds the design spec + TDD implementation plan for per-column resize modes (WidgetData::setColumnResizeMode) — the one picker axis a .ui file cannot express, since column sizing is owned by the host's installTreeLikeHeader(). Not implemented in this PR.
facontidavide
added a commit
that referenced
this pull request
Jul 15, 2026
main migrated every streaming dialog to the canonical banner framework (PRs #188/#207); after the WHEP rewrite this dialog was the only holdout still using bold-QLabel headers + a hand-rolled filter row. Convert the three sections (WHEP Server / Streams / ICE Servers) to SectionHeaderBand banners wrapped in native QWidgets with the standard 9/6/9/9 margins, and host the path filter via the streams banner's filterFieldName (it creates the lineEditFilter the dialog already listens to, so no C++ change). The picker table also gains SelectRows + a hidden vertical header to match the other unified pickers. Pure .ui edit; webrtc_dialog.cpp is untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JC57A96uJnRMAkioFzRmdA
facontidavide
added a commit
that referenced
this pull request
Jul 16, 2026
…ompatible) (#205) * build(webrtc): scaffold whep_client module + ixwebsocket dep Add ixwebsocket/11.4.6 to conanfile.py, wire whep_client.{hpp,cpp} into the webrtc_source_plugin target, and register a whep_client_test binary. Functions are stubbed (empty/no-op); the smoke test intentionally fails, proving the module and test target are correctly wired ahead of the real WHEP implementation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(webrtc): whep_client URL join + Location resolution Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(webrtc): adopt PJ::Expected + chrono timeouts in whep_client API Quality-review follow-up: reuse PJ::Expected<T,WhepError> instead of a hand-rolled expected type, take std::chrono::seconds instead of raw int seconds, add [[nodiscard]] to the fallible calls. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(webrtc): WHEP postOffer/deleteSession with typed errors Implement postOffer/deleteSession in whep_client against ix::HttpClient: POST the SDP offer with Bearer auth + application/sdp, classify non-2xx responses into WhepErrorKind (401/403->kUnauthorized, 404->kNotFound, transport failures->kNetwork/kTimeout, anything else->kBadResponse), and resolve the answer's Location header into an absolute session URL. deleteSession issues a best-effort DELETE with the same auth header. TDD'd against a loopback ix::HttpServer stub fixture (StubWhepServer) that records every request and replays a canned response. Note: ixwebsocket 11.4.6's SocketServer::getPort() just echoes the ctor argument (no getsockname() for ephemeral-port resolution), so the stub binds a fixed port (18889) instead of an OS-assigned one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(webrtc): mediamtx Control API paths-list fetch Implement fetchPathsList: GET <api_url>/v3/paths/list, parse items[].{name,ready,tracks} via nlohmann::json, and classify HTTP/JSON failures through the existing WhepError kind taxonomy (kUnauthorized, kBadResponse, kNetwork/kTimeout via errorFromResponse). Also clamp HTTP timeouts to >=1s (review follow-up). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(webrtc): skip non-object items in paths-list JSON (review follow-up) nlohmann json::value() throws type_error.306 when the element is not an object; the Control API response is untrusted network input parsed on a worker thread with no exception boundary, so a single malformed items[] entry would std::terminate. Guard with is_object() and skip the entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(webrtc): skip paths-list items with wrong-typed fields (review follow-up) An items[] entry that is an object but carries a wrong-typed field (e.g. "ready":"yes") made json::value() throw type_error.302, escaping the PJ::Expected contract on the worker thread. Wrap the per-item parsing in try/catch so any wrong-typed field skips that entry and keeps the rest — total against future field additions and consistent with the plugin's worker-path catch convention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(webrtc): whep_client hardening from checkpoint review - Same-origin guard: absolute Location headers must match the request URL's scheme://host:port or postOffer returns kBadResponse. - Response body size cap (8 MiB): progress callback flips args->cancel (ix 11.4.6 ignores the callback's bool in the HTTP receive path); belt-and-braces body.size() check in postOffer/fetchPathsList as defense-in-depth. - Answer Content-Type check: present-but-not-application/sdp is kBadResponse; absent stays accepted (lenient for non-mediamtx servers). - fetchPathsList pagination: follow ?page=N up to min(pageCount, 50) pages (mediamtx defaults to 100 items/page); no partial results on a failing page. - Bearer token control-character rejection before any HTTP I/O (Authorization header injection); deleteSession silently refuses. - Test stub: scripted setReplies() fixture and reply state now read and written under the same mutex (fixes a test-only data race). Also documents the deliberate WHEP-scope limitations in whep_client.hpp. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(webrtc): WhepConnection data path + AU-level tests Add the Qt-free WhepConnection class: one PeerConnection carrying one recvonly H.264 track for one WHEP camera path. This lands the data path and test seams (normalizeAccessUnit, extractSpropForTest, feedAccessUnitForTest/primeFromAnswerForTest, swap-drain queue, idempotent close()) reusing video_emit's H264AnnexBNormalizer and whep_client's HTTP helpers. open()/runConnect() (live PeerConnection + WHEP offer/answer exchange) are stubbed for a follow-up task; webrtc_receiver/webrtc_signaling are untouched and keep building alongside it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JC57A96uJnRMAkioFzRmdA * fix(webrtc): detach Track-level callbacks in WhepConnection::close (review follow-up) pc_->resetCallbacks() only clears PeerConnection-level callbacks; rtc::Track (an rtc::Channel) carries its own resetCallbacks() for the per-track onFrame that Task 6 wires directly on track_. Detach it before tearing down the PC so an in-flight frame on a libdatachannel worker cannot re-enter onFrame during teardown, honoring the header's "detaches every callback" contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JC57A96uJnRMAkioFzRmdA * feat(webrtc): WhepConnection live offerer path (gather -> POST -> answer) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(webrtc): harden WhepConnection open/close lifecycle (review follow-up) - C1: guard open() against double-open (joinable worker => std::terminate; live pc_ => leaked wired callbacks) - I1: failed open() (offer setup path) detaches track/PC callbacks and resets both, leaving the object fully closed - I2: never hold gather_mutex_ across blocking HTTP — close-race DELETE now runs outside every mutex Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(webrtc): recv_probe speaks WHEP Replace the legacy WebrtcReceiver+WebrtcSignaling answerer probe with a minimal WHEP client that drives WhepConnection directly: connects to a WHEP endpoint (e.g. mediamtx), drains normalized Annex-B frames, and writes them to a file while printing live frame/keyframe/byte counts. Update the webrtc_recv_probe CMake target accordingly (whep_connection.cpp + whep_client.cpp + video_emit.cpp, plus ixwebsocket for the HTTP POST). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(webrtc): recv_probe fails fast on unwritable output path (review follow-up) Open and check the output ofstream BEFORE conn.open(): a bad --out path (missing dir / unwritable) previously left the stream silently failed -- out.write() became a no-op yet the probe still printed "Done: N frames" and exited 0, after wasting a full connect cycle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(webrtc): mediamtx demo rig; spike validated vs mediamtx v1.19.2 (cam0+cam1 frames+keyframes OK) REAL-mediamtx spike gate for the WHEP refactor. Validates the two external assumptions with actual binaries: mediamtx WHEP semantics and libdatachannel offerer-role negotiation. Results (mediamtx v1.19.2, ffmpeg publisher, webrtc_recv_probe): - /v3/paths/list: cam0 + cam1 ready, tracks ["H264"]. - probe cam0: exit 0, 237 frames / 7 keyframes; ffprobe codec_name=h264 640x480. - probe cam1: exit 0, 235 frames / 8 keyframes; ffprobe codec_name=h264. - negative /nope/whep: exit 2 in ~0.12s, typed kind=1 (kNotFound); mediamtx answers HTTP 404. - SDP priming: the WHEP answer carries NO sprop-parameter-sets; SPS/PPS arrive in-band (verified type-7/type-8 NAL units in the received Annex-B). Rig: demo/mediamtx.yml (moq:no to avoid cert-gen droppings) + publish_cameras.sh (two H.264 test cameras via RTSP, ffmpeg or gstreamer). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(webrtc): WhepConnection callback/lifecycle hardening (checkpoint review) - never invoke on_error_ while holding gather_mutex_ (unlock before the gathering-timeout callback) - centralize failure finalization in failConnect(): state -> kFailed + on_error_ on every runConnect error path - reset the normalizer in close() (no stale SPS/PPS across reopen) - bound the frame queue at 256, drop-oldest (stalled-consumer DoS) + test - prime SPS/PPS from the answer BEFORE setRemoteDescription (first-frame race) - scope the sprop search to the first m=video section + test - worker-thread ctor exception boundary with the same self-clean as offer setup - document the callback threading contract (no close()/destroy from callbacks) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(webrtc): whep_client content-type + pagination bounds (checkpoint review) - Content-Type check is now an exact, case-insensitive media-type match (parameters like '; charset=utf-8' ignored) + test; text/html still rejected - paths-list pagination bounded: page cap 50 -> 10 plus a cumulative 2000-item cap (discovery is best-effort for a picker UI; truncation is acceptable, unbounded fetching is not) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(webrtc): demo rig robustness (checkpoint review) - recv_probe: stop on terminal states (kFailed/kClosed/kDisconnected); check output write errors and make a write/close failure win the exit code - publish_cameras.sh: supervise children (pids array + EXIT/INT/TERM trap, wait -n, kill sibling, propagate the failing status) in both branches - mediamtx.yml: bind API/RTSP/WebRTC to loopback by default, with a LAN note (key names verified against the shipped default config) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(webrtc): source+dialog speak WHEP (per-path connections, Control API discovery) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(webrtc): make onStart authoritative + escalate persistent failures (review follow-up) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(webrtc)!: delete the bespoke signaling protocol (replaced by WHEP) The plugin now speaks standard WHEP end-to-end (whep_client, whep_connection, video_emit); the hand-rolled JSON signaling protocol and its WebSocket/HTTP receiver are dead code. Remove webrtc_receiver.*, webrtc_signaling.*, their test, PROTOCOL.md, and the legacy demo scripts (stream_server.py, send_camera(s).py); purge them from CMakeLists.txt (source list, the webrtc_receiver_test target, and the core-vs-Qt split comment); drop libdatachannel's with_websocket option (WHEP signaling is plain HTTP); update manifest.json description/keywords to describe WHEP support. with_websocket=False built from source cleanly in the full rebuild, so the option flip stuck (no fallback to True needed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(webrtc): source dtor + dialog api-url/selection/ipv6 robustness (checkpoint review) - HIGH: add ~WebrtcSource() that calls onStop() so every WhepConnection is closed (workers joined, callbacks detached) while the atomics + message queue its callbacks capture are still alive; the host can destroy the source without a preceding onStop(). Idempotent (onStop clears cameras_; second call no-ops). - MEDIUM: persist api_url_edited explicitly (saveConfig writes it, loadConfig reads api_url_edited, not api_url presence) so a never-edited API URL reloads as still-auto-derived and server_url edits keep re-deriving the host. - MEDIUM: preserve selections that are not a currently-rendered row (filter- hidden OR vanished from the catalog on a transient fetch failure); a selection is dropped only when the user deselects a rendered row. - LOW: make deriveApiUrl IPv6-bracket-aware (http://[::1]:8889 -> [::1]:9997). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(webrtc): mediamtx/WHEP demo quick start Rewrite data_stream_webrtc/demo/README.md for the WHEP-client rework: drop the deleted custom-broker (stream_server.py/send_camera*.py) docs and describe the current mediamtx.yml + publish_cameras.sh + recv_probe.cpp rig, dialog fields, discovery/auth behavior, and the WHEP-scope limitations and inotify/ENOSPC troubleshooting note. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(webrtc): detach Track frame callback on close + minor cleanups (final review) - HIGH: close() now calls track_->onFrame(nullptr) before resetCallbacks() — Track::onFrame installs a separate impl::Track frame callback that Channel::resetCallbacks() does not clear; detaching it first prevents an in-flight frame dispatch firing onFrame() (UAF) after close() returns - LOW A: synchronous open() failure now funnels through a new noteFailedAttemptAndMaybeEscalate() helper (shared with the async-loss path) so a persistently-failing sync open reaches the kError escalation - LOW B: corrected the error-callback comment — a connect-phase failure sets state kFailed but notifies via the error callback (failConnect() does not fire the state callback), which drives the lost/terminal atomics - LOW C: publish_cameras.sh GStreamer branch uses v4l2src for cam0 when a /dev/videoN device arg is passed (matches the ffmpeg branch and the README) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(webrtc): define NOMINMAX so whep_client compiles on MSVC <ixwebsocket/IXHttpClient.h> transitively pulls in <windows.h> on MSVC, which defines min/max as macros and mangles the std::max() call in makeArgs() ("illegal token on right side of ::"). Only Windows CI hit this; Linux/macOS were green. Define NOMINMAX before the first include so the macros are never introduced, matching the repo's existing idiom (reactive_script_editor.cpp, the mcap contrib reader, platform.hpp). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JC57A96uJnRMAkioFzRmdA * style(webrtc): adopt the SectionHeaderBand dialog framework main migrated every streaming dialog to the canonical banner framework (PRs #188/#207); after the WHEP rewrite this dialog was the only holdout still using bold-QLabel headers + a hand-rolled filter row. Convert the three sections (WHEP Server / Streams / ICE Servers) to SectionHeaderBand banners wrapped in native QWidgets with the standard 9/6/9/9 margins, and host the path filter via the streams banner's filterFieldName (it creates the lineEditFilter the dialog already listens to, so no C++ change). The picker table also gains SelectRows + a hidden vertical header to match the other unified pickers. Pure .ui edit; webrtc_dialog.cpp is untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JC57A96uJnRMAkioFzRmdA * fix(webrtc): define NOMINMAX in whep_client_test for MSVC windows.h (pulled in by ixwebsocket on MSVC) defines min/max macros that break std::min in the stub server reply selection. Same guard whep_client.cpp already uses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(webrtc): harden WHEP client per review findings - ResponseByteCounter: response-size cap now cumulative across chunked reads (Socket::readBytes restarts 'current' per chunk; a current drop or total change banks the previous call's bytes) - HttpAbort: cross-thread cancel for postOffer/fetchPathsList (flips the atomic args->cancel ixwebsocket polls in connect+transfer loops); re-checked between pagination pages - WhepConnection: close() aborts an in-flight POST; teardownPeer() deduplicates the 3 copy-pasted PC/track teardown sequences and gives the open() failure paths the missing onFrame(nullptr) detach - webrtc_source: onStop/restart close cameras in parallel instead of serially; backoff/escalation counters only reset after ~10s of stable connection (flapping servers keep escalating); onStart rejects an empty server URL - webrtc_dialog: abort the paths fetch on reject/destroy (bounds the UI join); previously-selected paths survive transient no-tracks catalog reports instead of being silently deselected; OK gated on a non-empty server URL; manual path input trimmed and validated Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(webrtc): initialize Winsock in whep_client_test so it passes on Windows whep_client_test's in-process ixwebsocket stub server never entered the listening state on Windows: ix::initNetSystem() (WSAStartup) is required before any socket op there and was never called, so server_.listen() failed and every assertion requiring the stub blew up (27/30). On Linux/macOS initNetSystem() is a no-op, so only the Windows job failed. Give the test its own main() that brackets the run with initNetSystem()/uninitNetSystem(), and link GTest::gtest instead of GTest::gtest_main so that main() is the entry point. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
facontidavide
added a commit
to PlotJuggler/PlotJuggler
that referenced
this pull request
Jul 20, 2026
… sorting (#356) * fix(dialog_host): suspend table sorting while populating rows QTableWidget physically re-sorts its model on every setItem/setText when sorting is enabled. applyTableRows writes cells by model-row index, so a mid-loop re-sort remapped the indices and the remaining writes landed on the wrong rows — leaving blank cells, scrambling name↔value pairs, and duplicating rows. This surfaced once a user clicked a column header to sort a streaming picker table while topics were still arriving. Suspend sorting before the first cell write and restore it once at the end, so Qt applies a single clean sort. Suspending lazily keeps the same-shape path's minimal-churn intent: a re-delivery that changes no cell never toggles sorting and pays no re-sort. Text-keyed selection restore (selected_items) runs after this, once the sort has settled, so it keeps matching rows correctly. This is the host-side root cause behind the picker tables that PlotJuggler/pj-official-plugins#188 makes sortable; ros2 and mcap already set sortingEnabled=true and were exposed independently. Complements #349 (text-keyed selection restore), which is unaffected. Adds three TableBinding regression tests covering populate-while-sortable, sorted full rebuild (blank cells without the fix), and sorted in-place text update (duplicated row without the fix). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K8fuH9N8XLRHkN6ze87BzW * fix(dialog_host): translate table row indices between plugin order and sorted view Plugins key table state (selected_rows, visible_rows, disabled_rows, radio_checked_row) and interpret emitted indices (table_radio_row, item_double_clicked_index) against the row order they delivered rows in. With sortingEnabled the user re-orders the view, so every index-keyed exchange landed on the wrong rows. The host now records the delivered order on each rows delivery (_pj_plugin_row_keys / _pj_plugin_key_col) and translates indices in both directions, matching rows by the recorded key column text — the same column tableRowKeyText reads, one contract by construction. Duplicate keys pair positionally; tables never fed by a delivery keep raw-index semantics, and a recorded order that desyncs warns before degrading. Maps are built only when a delivery carries an index-keyed aspect, so rows-only streaming ticks pay nothing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RP8RosW9FCsePNUttEYAXk * fix(dialog_host): render cell tooltips under sorted views The dialog protocol carries per-cell tooltips (WidgetData::setCellTooltip, consumed via WidgetDataView::cellTooltips), but the host never applied them, so a plugin's tooltip (e.g. MCAP's "always loaded" locked rows) never showed. Apply them on the table binding path, translating the plugin-order (row, col) to the current sorted-view row via the same plugin_to_view map the index-keyed state uses. A delivery that carries cell_tooltips states the complete set, so every item tooltip is cleared first — a tooltip the plugin dropped must not linger on a stale cell after an in-place row rewrite. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016CCvzveKLxvwqwUGsJnWAx * fix(dialog_host): force protocol tables read-only The dialog protocol has no cell-edit event (only selection, double-click and radio), so an edited cell can never be read back by the plugin — an editable cell silently discards the edit on accept. Several plugin pickers (pj_bridge, foxglove, webrtc) never set NoEditTriggers in their .ui, so their topic cells looked editable while dropping edits. Force read-only on every protocol table at the host binding path, so no picker looks editable when it isn't and no future plugin has to remember the property. Persistent and idempotent, so setting it on each delivery is free. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016CCvzveKLxvwqwUGsJnWAx * fix(dialog_host): disambiguate duplicate key-column rows under sort The plugin-order <-> view-order row translation matched rows by key-column text, so two rows sharing identical key text (e.g. two same-named topics disambiguated by another column) became indistinguishable once the view was sorted: radio_checked_row, table_radio_row, and item_double_clicked_index could resolve to the wrong one of the two. Tag each QTableWidgetItem/QListWidgetItem with its plugin-delivery index directly (a custom item-data role) instead of re-deriving identity from text. Qt relocates item pointers (not their data) on sort, so the tag rides along with the correct row through any resort. This also removes the QHash-based text matching, the _pj_plugin_row_keys/_pj_plugin_item_order property bookkeeping, and the desync-warning fallback entirely -- the mechanism is now structurally correct instead of detected-and-recovered. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Davide Faconti <davide.faconti@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Brings the topic / channel / camera / sequence picker tables across the plugin dialogs onto one consistent idiom, and captures a follow-up SDK design for the one axis that can't live in a
.uifile (per-column resize behavior).Picker idiom (the 5 multi-select lists)
All now share: MultiSelection · SelectRows · NoEditTriggers · sortingEnabled · verticalHeaderVisible=false.
topicsListtopicsListcamerasListlistRosTopicstopicTableWhy MultiSelection over ExtendedSelection: in a picker, a bare click under ExtendedSelection silently clears the previous selection — a footgun when users expect to accumulate picks. MultiSelection makes a bare click toggle/accumulate, which matches the "Select one or multiple…" labels these dialogs already show.
Deliberate exceptions (unchanged):
seqTablestays SingleSelection (single-select by design, now explicit).setVisibleRows, and user re-sorting would desync that index mapping.Not addressed here (tracked)
Per-column resize-to-content (e.g. MCAP
Encoding,Msg Counthugging their content while the name column stretches) can't be expressed in a.uifile — column sizing is owned by the host'sinstallTreeLikeHeader(). The second commit adds a design spec + TDD implementation plan for an additive, header-onlyWidgetData::setColumnResizeMode/WidgetDataView::columnResizeModesprotocol addition spanningplotjuggler_sdk→PJ4→ this repo. Not implemented in this PR.Notes
.uifiles are embedded verbatim and parsed at runtime (nouic); all 5 were validated well-formed..uiattributes + docs.🤖 Generated with Claude Code