fix(ros2,webrtc): restore picker-table selection by text, not row index#197
Merged
Conversation
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. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RP8RosW9FCsePNUttEYAXk
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
2 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.
Problem
ros2_dialog.hppandwebrtc_dialog.cpprestore their picker-table selection withsetSelectedRows, computing indices against their own delivery order.listRosTopicsissortingEnabled=trueon main, so after the user sorts the topic column the next discovery re-delivery highlights the wrong topics (reproduced live; the emit round-trip then corrupts the plugin's selected set).camerasListhas the same shape and goes live the moment #188 enables sorting on it.Both sites also carry a comment claiming the host only honors
selected_rowsfor tables — stale since PJ4#349 added text-keyedselected_itemsrestore forQTableWidget.Fix
Send
setSelectedItemskeyed by the first-column text (topic name / unique camera label), exactly likedata_load_mcapalready does. Text matching is sort-agnostic, and it deletes the index-computation loops. Defense-in-depth with PJ4#370 (host-side index translation): with both in place neither side depends on view order.Verification
webrtc_source_pluginbuilds clean.ros2_dialog.hppverified with a full-TU compile check against the pinned SDK 0.15.0 + rclcpp jazzy. Note theros2_stream_plugin_distroinner target does not build on main today for unrelated pre-existing reasons: feat(foxglove,ros2): demand-driven per-topic subscriptions (pj.topic_subscription.v1) #190 usesPJ::AvailableTopic/kCapabilityPerTopicPause/PJ_TOPIC_SUBSCRIPTION_EXTENSION_V1, which don't exist in the pinnedplotjuggler_sdk/0.15.0(they're in the unreleased 0.16.0).🤖 Generated with Claude Code
https://claude.ai/code/session_01RP8RosW9FCsePNUttEYAXk