Skip to content

fix(mosaico): key table selection by name, not row index #201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Alvvalencia merged 1 commit into from
Jul 14, 2026
Merged

fix(mosaico): key table selection by name, not row index #201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions toolbox_mosaico/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,16 @@ if(BUILD_TESTING)
)
add_test(NAME ToolboxMosaicoTableSortTest COMMAND toolbox_mosaico_table_sort_test)

# Filter-exempt selection merge (text-keyed picker contract). Header-only.
add_executable(toolbox_mosaico_selection_merge_test
tests/selection_merge_test.cpp
)
target_link_libraries(toolbox_mosaico_selection_merge_test PRIVATE GTest::gtest_main)
target_include_directories(toolbox_mosaico_selection_merge_test PRIVATE
${CMAKE_CURRENT_SOURCE_DIR}/src
)
add_test(NAME ToolboxMosaicoSelectionMergeTest COMMAND toolbox_mosaico_selection_merge_test)

# Human-readable byte formatting (1024-based, PJ3 parity). Header-only.
add_executable(toolbox_mosaico_format_utils_test
tests/format_utils_test.cpp
Expand Down
154 changes: 52 additions & 102 deletions toolbox_mosaico/src/mosaico_dialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
#include <algorithm>
#include <chrono>
#include <pj_base/sdk/platform.hpp>
#include <set>
#include <string_view>
#include <utility>

Expand All @@ -32,6 +31,7 @@
#include "query/engine.h"
#include "query/query.h"
#include "query_filter.h"
#include "selection_merge.h"
#include "server_history.h"
#include "settings_store.hpp"
#include "table_sort.h"
Expand Down Expand Up @@ -544,8 +544,8 @@ std::string MosaicoDialog::widget_data() {
// worker; Cancel is live only during a fetch.
wd.setEnabled("buttonConnect", !state_.connecting && !state_.fetch_active);
wd.setEnabled(
"buttonFetch", state_.connected && !state_.selected_sequence.empty() && !state_.topic_selected_rows.empty() &&
!state_.fetch_active);
"buttonFetch",
state_.connected && !state_.selected_sequence.empty() && !state_.selected_topics.empty() && !state_.fetch_active);
// Per-column reload buttons re-list without a disconnect/reconnect (PJ3
// main_window.cpp:933-945): live only while connected and idle. The topic
// reload additionally needs a selected sequence (its topics are what reload).
Expand Down Expand Up @@ -635,8 +635,10 @@ std::string MosaicoDialog::widget_data() {
wd.setTableHeaders("seqTable", {"Name", "Date", "Size"});
wd.setTableRows("seqTable", cache.rows);
wd.setVisibleRows("seqTable", cache.visible);
if (state_.seq_selected_row >= 0) {
wd.setSelectedRows("seqTable", {state_.seq_selected_row});
if (!state_.selected_sequence.empty()) {
// Text-keyed (column-0 name match): survives the plugin-side re-sorts
// that shuffle row positions.
wd.setSelectedItems("seqTable", {state_.selected_sequence});
}
wd.setLabel("seqHeader", "Sequences");
}
Expand All @@ -661,8 +663,8 @@ std::string MosaicoDialog::widget_data() {
wd.setTableHeaders("topicTable", {"Name", "Size"});
wd.setTableRows("topicTable", rows);
wd.setVisibleRows("topicTable", visible);
if (!state_.topic_selected_rows.empty()) {
wd.setSelectedRows("topicTable", state_.topic_selected_rows);
if (!state_.selected_topics.empty()) {
wd.setSelectedItems("topicTable", state_.selected_topics);
}
if (state_.topics_loading) {
wd.setLabel("topicHeader", "Topics — loading…");
Expand All @@ -683,11 +685,7 @@ std::string MosaicoDialog::widget_data() {
if (state_.fetch_active) {
header = "Download progress";
info_text += state_.fetch_status + "\n\n";
for (int row : state_.topic_selected_rows) {
if (row < 0 || row >= static_cast<int>(state_.topic_names.size())) {
continue;
}
const std::string& tname = state_.topic_names[static_cast<size_t>(row)];
for (const std::string& tname : state_.selected_topics) {
std::int64_t bytes = 0;
if (auto it = state_.bytes_by_topic.find(tname); it != state_.bytes_by_topic.end()) {
bytes = it->second;
Expand Down Expand Up @@ -733,19 +731,20 @@ std::string MosaicoDialog::widget_data() {
info_text += buildSequenceInfoText(*seq_rec);
header = fmt::format("Info — {}", seq_rec->name);
}
for (int row : state_.topic_selected_rows) {
if (row < 0 || row >= static_cast<int>(state_.topic_names.size())) {
continue;
}
const std::string& tname = state_.topic_names[static_cast<size_t>(row)];
for (const std::string& tname : state_.selected_topics) {
info_text += "\n";
info_text += std::string(40, '-');
info_text += "\n\n";
if (auto it = state_.topic_meta.find(tname); it != state_.topic_meta.end()) {
info_text += buildTopicInfoText(it->second);
} else if (row < static_cast<int>(state_.topic_infos.size())) {
info_text += buildTopicInfoText(state_.topic_infos[static_cast<size_t>(row)]);
info_text += " (loading schema…)\n";
} else if (
auto pos = std::find(state_.topic_names.begin(), state_.topic_names.end(), tname);
pos != state_.topic_names.end()) {
const auto idx = static_cast<std::size_t>(pos - state_.topic_names.begin());
if (idx < state_.topic_infos.size()) {
info_text += buildTopicInfoText(state_.topic_infos[idx]);
info_text += " (loading schema…)\n";
}
}
}
wd.setPlainText("dataView", info_text);
Expand Down Expand Up @@ -991,9 +990,11 @@ bool MosaicoDialog::onClicked(std::string_view widget_name) {
return true;
}
seq = state_.selected_sequence;
for (int row : state_.topic_selected_rows) {
if (row >= 0 && row < static_cast<int>(state_.topic_names.size())) {
topics.push_back(state_.topic_names[row]);
// Only fetch topics that still exist in the current listing — a selection
// preserved across a filter or re-list may reference a topic that's gone.
for (const std::string& tname : state_.selected_topics) {
if (std::find(state_.topic_names.begin(), state_.topic_names.end(), tname) != state_.topic_names.end()) {
topics.push_back(tname);
}
}
// Step 7: convert proportional 0..100 % into absolute nanoseconds
Expand Down Expand Up @@ -1054,11 +1055,7 @@ bool MosaicoDialog::onClicked(std::string_view widget_name) {
// mark every not-yet-finished topic "Cancelling…" so the per-topic view
// updates without waiting for allFetchesComplete (PJ3 parity).
state_.fetch_status = "Cancelling…";
for (int row : state_.topic_selected_rows) {
if (row < 0 || row >= static_cast<int>(state_.topic_names.size())) {
continue;
}
const std::string& tname = state_.topic_names[static_cast<size_t>(row)];
for (const std::string& tname : state_.selected_topics) {
auto it = state_.topic_fetch_status.find(tname);
if (it == state_.topic_fetch_status.end() || it->second.empty() || it->second == "downloading…") {
state_.topic_fetch_status[tname] = "Cancelling…";
Expand Down Expand Up @@ -1226,10 +1223,9 @@ bool MosaicoDialog::onSelectionChanged(std::string_view widget_name, const std::
if (widget_name == "seqTable") {
if (selected.empty()) {
std::lock_guard<std::mutex> lock(state_.mu);
state_.seq_selected_row = -1;
state_.selected_sequence.clear();
state_.topic_names.clear();
state_.topic_selected_rows.clear();
state_.selected_topics.clear();
state_.topics_loading = false;
return true;
}
Expand All @@ -1242,15 +1238,8 @@ bool MosaicoDialog::onSelectionChanged(std::string_view widget_name, const std::
// persisted names.
state_.restore_selected_sequence.clear();
state_.restore_selected_topics.clear();
// Map the name back to a row index for the visible-row highlight.
for (size_t i = 0; i < state_.sequence_names.size(); ++i) {
if (state_.sequence_names[i] == selected.front()) {
state_.seq_selected_row = static_cast<int>(i);
break;
}
}
state_.topic_names.clear();
state_.topic_selected_rows.clear();
state_.selected_topics.clear();
state_.topics_loading = true; // header shows "loading…" until topicsReady
}
postCommand([w = worker_.get(), seq] { w->listTopicsAsync(seq); });
Expand All @@ -1261,19 +1250,15 @@ bool MosaicoDialog::onSelectionChanged(std::string_view widget_name, const std::
std::vector<std::string> need_metadata;
{
std::lock_guard<std::mutex> lock(state_.mu);
state_.topic_selected_rows.clear();
seq = state_.selected_sequence;
for (const auto& sel : selected) {
for (size_t i = 0; i < state_.topic_names.size(); ++i) {
if (state_.topic_names[i] == sel) {
state_.topic_selected_rows.push_back(static_cast<int>(i));
// Kick off a one-shot metadata fetch (Arrow schema + tag) for the
// Info panel if we don't already have the full record cached.
if (state_.topic_meta.find(sel) == state_.topic_meta.end()) {
need_metadata.push_back(sel);
}
break;
}
state_.selected_topics = mosaico::mergeReportedSelection(
state_.selected_topics, selected, state_.topic_names,
[&](const std::string& n) { return !nameMatches(n, state_.topic_filter, state_.topic_filter_regex); });
// Kick off a one-shot metadata fetch (Arrow schema + tag) for the Info
// panel for any newly selected topic without the full record cached.
for (const std::string& tname : state_.selected_topics) {
if (state_.topic_meta.find(tname) == state_.topic_meta.end()) {
need_metadata.push_back(tname);
}
}
}
Expand Down Expand Up @@ -1358,14 +1343,10 @@ void MosaicoDialog::persistState() {
lower = state_.range_lower;
upper = state_.range_upper;
// PJ3 parity: remember the last selection so the next connect can re-select
// it. Resolve the selected topic ROW indices back to names (names are
// stable across re-fetch; row indices are not).
// it (selection state is already name-keyed; names are stable across
// re-fetch).
selected_sequence = state_.selected_sequence;
for (int row : state_.topic_selected_rows) {
if (row >= 0 && row < static_cast<int>(state_.topic_names.size())) {
selected_topics.push_back(state_.topic_names[static_cast<std::size_t>(row)]);
}
}
selected_topics = state_.selected_topics;
}
SettingsStore settings(settings_);
settings.setString("mosaico/metadata_query", query);
Expand Down Expand Up @@ -1473,16 +1454,6 @@ void MosaicoDialog::sortSequencesLocked() {
for (const auto& s : state_.sequences) {
state_.sequence_names.push_back(s.name);
}
// Re-map the selected row to the selected sequence's new position.
state_.seq_selected_row = -1;
if (!state_.selected_sequence.empty()) {
for (std::size_t i = 0; i < state_.sequence_names.size(); ++i) {
if (state_.sequence_names[i] == state_.selected_sequence) {
state_.seq_selected_row = static_cast<int>(i);
break;
}
}
}
++state_.seq_epoch; // row order changed → invalidate the seqTable view cache
}

Expand All @@ -1494,15 +1465,8 @@ void MosaicoDialog::sortTopicsLocked() {
const bool asc = state_.topic_sort_asc;
const bool have_infos = state_.topic_infos.size() == state_.topic_names.size();

// Capture the selected topics by name so selection survives the reorder.
std::set<std::string> selected;
for (int r : state_.topic_selected_rows) {
if (r >= 0 && r < static_cast<int>(state_.topic_names.size())) {
selected.insert(state_.topic_names[static_cast<std::size_t>(r)]);
}
}

// Sort an index permutation, then apply it to the parallel name/info vectors.
// Selection is name-keyed, so the reorder can't desync it.
std::vector<std::size_t> perm;
if (col == 1 && have_infos) { // Size — numeric
std::vector<std::int64_t> keys;
Expand Down Expand Up @@ -1531,14 +1495,6 @@ void MosaicoDialog::sortTopicsLocked() {
if (have_infos) {
state_.topic_infos = std::move(new_infos);
}

// Re-map index-based selection from the captured names.
state_.topic_selected_rows.clear();
for (std::size_t i = 0; i < state_.topic_names.size(); ++i) {
if (selected.count(state_.topic_names[i]) > 0) {
state_.topic_selected_rows.push_back(static_cast<int>(i));
}
}
}

std::vector<std::string> MosaicoDialog::restoreSelectedTopicsLocked() {
Expand All @@ -1551,17 +1507,14 @@ std::vector<std::string> MosaicoDialog::restoreSelectedTopicsLocked() {
const std::vector<std::string> wanted = std::move(state_.restore_selected_topics);
state_.restore_selected_topics.clear();

state_.topic_selected_rows.clear();
for (std::size_t i = 0; i < state_.topic_names.size(); ++i) {
for (const std::string& name : wanted) {
if (state_.topic_names[i] == name) {
state_.topic_selected_rows.push_back(static_cast<int>(i));
// Fetch full metadata (Arrow schema + tag) for the Info panel, same as
// the manual topic-selection path, when not already cached.
if (state_.topic_meta.find(name) == state_.topic_meta.end()) {
need_metadata.push_back(name);
}
break;
state_.selected_topics.clear();
for (const std::string& name : wanted) {
if (std::find(state_.topic_names.begin(), state_.topic_names.end(), name) != state_.topic_names.end()) {
state_.selected_topics.push_back(name);
// Fetch full metadata (Arrow schema + tag) for the Info panel, same as
// the manual topic-selection path, when not already cached.
if (state_.topic_meta.find(name) == state_.topic_meta.end()) {
need_metadata.push_back(name);
}
}
}
Expand Down Expand Up @@ -1639,14 +1592,11 @@ void MosaicoDialog::onSequencesReady(std::vector<SequenceInfo> seqs) {
// session. One-shot — clear the restore slot once consumed so a later
// manual selection / refresh doesn't snap back.
if (!state_.restore_selected_sequence.empty() && state_.selected_sequence.empty()) {
for (std::size_t i = 0; i < state_.sequence_names.size(); ++i) {
if (state_.sequence_names[i] == state_.restore_selected_sequence) {
state_.selected_sequence = state_.restore_selected_sequence;
state_.seq_selected_row = static_cast<int>(i);
state_.topics_loading = true; // header shows "loading…" until topicsReady
reselect_sequence = state_.selected_sequence;
break;
}
if (std::find(state_.sequence_names.begin(), state_.sequence_names.end(), state_.restore_selected_sequence) !=
state_.sequence_names.end()) {
state_.selected_sequence = state_.restore_selected_sequence;
state_.topics_loading = true; // header shows "loading…" until topicsReady
reselect_sequence = state_.selected_sequence;
}
state_.restore_selected_sequence.clear();
if (reselect_sequence.empty()) {
Expand Down
8 changes: 5 additions & 3 deletions toolbox_mosaico/src/mosaico_dialog.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ struct DialogState {
bool seq_filter_regex = false;
bool topic_filter_regex = false;
// Column sort state — the plugin owns row ordering (built-in table widget
// sorting would desync the index-based selection/visibility). -1 = unsorted
// sorting would desync the index-based visibility filter). -1 = unsorted
// (server/load order). seqTable cols: 0=Name 1=Date 2=Size; topicTable: 0=Name 1=Size.
// PJ3 parity: both tables default to Name ascending (column 0) because the
// server's iteration order is unstable — without a deterministic sort, rows
Expand All @@ -80,8 +80,10 @@ struct DialogState {
bool seq_sort_asc = true;
int topic_sort_col = 0;
bool topic_sort_asc = true;
int seq_selected_row = -1;
std::vector<int> topic_selected_rows;
// Selection is keyed by NAME, not row index: names survive the plugin-side
// re-sorts and list refreshes that shuffle row positions (same text-keyed
// convention as the mcap/ros2/webrtc pickers).
std::vector<std::string> selected_topics;
std::string selected_sequence;

// PJ3 parity (main_window.cpp:1051-1052,1064-1065): the last sequence + topic
Expand Down
40 changes: 40 additions & 0 deletions toolbox_mosaico/src/selection_merge.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Copyright 2026 Davide Faconti
// SPDX-License-Identifier: MIT
#pragma once

#include <algorithm>
#include <functional>
#include <string>
#include <vector>

namespace mosaico {

// Merge the selection reported by the dialog host with the previously selected
// names. The host only reports rows currently visible under the view filter,
// so a previously selected name the filter hides must be kept explicitly —
// otherwise picking a topic while the filter conceals another would silently
// drop the hidden one (same contract as the ros2/foxglove pickers). A visible
// name missing from `reported` was deselected by the user and is dropped.
// `known` bounds the result to names present in the current listing;
// duplicates are dropped.
inline std::vector<std::string> mergeReportedSelection(
const std::vector<std::string>& previous, const std::vector<std::string>& reported,
const std::vector<std::string>& known, const std::function<bool(const std::string&)>& hidden) {
auto contains = [](const std::vector<std::string>& v, const std::string& s) {
return std::find(v.begin(), v.end(), s) != v.end();
};
std::vector<std::string> next;
for (const std::string& name : previous) {
if (contains(known, name) && hidden(name) && !contains(next, name)) {
next.push_back(name);
}
}
for (const std::string& name : reported) {
if (contains(known, name) && !contains(next, name)) {
next.push_back(name);
}
}
return next;
}

} // namespace mosaico
Loading
Loading