diff --git a/CHANGELOG.md b/CHANGELOG.md index 7373100..b30b9e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,43 @@ All notable changes to `plotjuggler_sdk` are recorded here. Versioning policy is ## [0.18.0] +### Feature: typed table sort keys — numeric columns sort numerically (MINOR) + +A table cell crossed the dialog protocol as text only, so the host could compare +nothing but the rendered string and every numeric column sorted +lexicographically (`720` before `7` before `65`). Sort keys now travel beside the +display text. Backward-compatible JSON additions — no C ABI change, +`PJ_DIALOG_PROTOCOL_VERSION` unchanged, `abi/baseline.abi` unchanged: + +- `PJ::TableItem { std::string text; std::optional value; }` — the + display string and the ordering truth. Constructors cover a text cell, a + numeric cell (`std::to_string` rendering), and a numeric cell with plugin-owned + rendering (`TableItem(v, "5.60536e+08")`), which also expresses a *hidden* key: + display a date, sort on `int64` nanoseconds. `NumericValue` keeps the native + width, so `int64`/`uint64` values past 2⁵³ round-trip exactly rather than + degrading through a `double`. +- `WidgetData::setTableRows(name, vector>)` — an overload beside + the string one. Emits display text as the usual `rows` plus a **sparse** + `column_values` map (`{"": [v0, v1, …]}`): only columns where some cell has + a value, with valueless cells as `null`, and the key omitted entirely when no + cell has one. Old hosts never look for it and read `rows` exactly as before. + Deriving both keys from one `TableItem` matrix means display and value cannot + desync. +- `WidgetData::setTableSortIndicator(name, column, ascending)` → `sort_indicator` + — draws the header arrow for a table that sorts itself via `onHeaderClicked`, + which Qt otherwise leaves unpainted because its own sorting is off. Cosmetic + only. +- `WidgetDataView::tableColumnValues(name)` / `tableSortIndicator(name)` for host + implementations. `tableColumnValues` drops a column whose value count disagrees + with `rows` or whose key is not a non-negative integer, rather than sorting some + rows by number and the rest by text. +- `WidgetDataView::tableRows()` hardening: a non-string cell now yields an empty + string instead of being skipped. Skipping shifted every later cell one column + left and mis-aligned the row against its headers. +- The plain-string `setTableRows` overload now erases any `column_values` a + previous typed delivery left on the same table, so alternating overloads can + never pair fresh rows with stale sort keys. + ### Feature: batch table deltas for large QTableWidgets (MINOR) Mutate a table without resending the whole `rows` array (backward-compatible @@ -24,7 +61,7 @@ JSON addition; no C ABI change, `PJ_DIALOG_PROTOCOL_VERSION` unchanged): omit-unchanged-fields pattern (with measured costs) and the delta ops. - SDK-side only: hosts apply `table_delta` from the companion PlotJuggler change onward; older hosts ignore the key (harmless no-op). - + ### Feature: QDateTimeEdit event surface (MINOR) The dialog protocol's QDateTimeEdit setters (`setDateTime` / `setDateTimeRange`, diff --git a/conanfile.py b/conanfile.py index 506d6fd..2b49a87 100644 --- a/conanfile.py +++ b/conanfile.py @@ -6,7 +6,7 @@ plugin_sdk — umbrella for plugin authors (base + dialog SDK + parser SDK) plugin_host — umbrella for host loaders (data_source/parser/toolbox/dialog) -A consuming Conan recipe declares e.g. `plotjuggler_sdk/0.16.2` and then: +A consuming Conan recipe declares e.g. `plotjuggler_sdk/0.18.0` and then: find_package(plotjuggler_sdk REQUIRED COMPONENTS plugin_sdk) target_link_libraries(my_plugin PRIVATE plotjuggler_sdk::plugin_sdk) @@ -30,29 +30,6 @@ class PlotjugglerSdkConan(ConanFile): name = "plotjuggler_sdk" - # 0.14.0 unified markers + transforms into the single host service - # `pj.data_processors.v1` via a `kind` discriminator (removed the old - # `pj.markers.v1` and the interim `pj.generators.v1`; generalized - # `create_data_processor`/`validate_data_processor_script` with - # kind/language/flags) and added the dialog-protocol additions (radio - # column + interactive sub-panel). Shipped as a MINOR bump rather than - # 1.0.0 because no public tag had ever carried `pj.data_processors.v1`, - # so no released plugin broke. - # 0.15.0 adds DataSource per-topic pause (advertise + demand-driven - # subscription) — strictly additive. See CHANGELOG.md. - # 0.16.0 removes the host-side PluginRuntimeCatalog (duplicate-resolution - # policy moved to the app, pj_runtime). No plugin links it and - # abi/baseline.abi is unchanged, so per the plugin-impact rule this is a - # MINOR, not a MAJOR. See CHANGELOG.md. - # 0.16.1 fixes plugin_data_api.hpp double formatting on Apple deployment - # targets older than macOS 13.3 (FP std::to_chars unavailable there) — - # PATCH, installed-header bug fix. See CHANGELOG.md. - # 0.17.0 extends the dialog protocol with backward-compatible additions - # (list_deletable / list_placeholder / chart_placeholder keys and the - # item_delete_index event) — MINOR. See CHANGELOG.md. - # 0.18.0 adds the QDateTimeEdit event surface (datetime_iso event, - # WidgetEvent::dateTimeChanged, DialogPluginTyped::onDateTimeChanged) — - # MINOR, header-only additions. See CHANGELOG.md. version = "0.18.0" # Apache-2.0 covers the whole SDK (pj_base + pj_plugins). See LICENSE. license = "Apache-2.0" diff --git a/docs/dialog-sdk-reference.md b/docs/dialog-sdk-reference.md index bb91732..7ad1fa8 100644 --- a/docs/dialog-sdk-reference.md +++ b/docs/dialog-sdk-reference.md @@ -74,7 +74,9 @@ For the full tutorial, see [dialog-plugin-guide.md](../pj_plugins/docs/dialog-pl | Method | Description | |--------|-------------| | `setTableHeaders(name, vector)` | Set column headers | -| `setTableRows(name, vector>)` | Set row data | +| `setTableRows(name, vector>)` | Set row data as plain text. Every column sorts **lexicographically** — `"9"` after `"10"`. | +| `setTableRows(name, vector>)` | Set row data with a per-cell sort key, so numeric columns sort numerically. `TableItem{text, optional}`: `TableItem("x")` sorts by text, `TableItem(v)` renders and sorts on `v`, `TableItem(v, "display")` sorts on `v` but shows `display` (lossy or decorated rendering, or a hidden key such as a date over `int64` ns). Pass the native type — `NumericValue` keeps `int64`/`uint64` exact past 2⁵³. | +| `setTableSortIndicator(name, column, ascending)` | Draw the header arrow **without** enabling Qt's sorting — for tables that sort themselves via `onHeaderClicked` (Qt paints an arrow only for its own sorting). Cosmetic; never reorders rows. | | `setSelectedRows(name, vector)` | Set selected row indices | | `setDisabledRows(name, vector)` | Grey out rows (non-selectable) | | `setTableRadioColumn(name, column, checked_row)` | Render `column` as an exclusive radio group; `checked_row` is selected (-1 = none). Fires `onTableRadioSelected`. | @@ -82,6 +84,11 @@ For the full tutorial, see [dialog-plugin-guide.md](../pj_plugins/docs/dialog-pl | `updateTableCells(name, seq, vector)` | Delta: rewrite individual cells (`{row, col, text}`, plugin row space) | | `removeTableRows(name, seq, vector)` | Delta: remove plugin-space row indexes | +> A table must not combine `sortingEnabled=true` in its `.ui` with +> `onHeaderClicked` — Qt would sort the view while the plugin reorders the model, +> and the plugin's order loses. See `pj_plugins/docs/dialog-plugin-guide.md` → +> "Sortable Tables". + ### QFrame Chart Container | Method | Description | diff --git a/pj_plugins/dialog_protocol/CLAUDE.md b/pj_plugins/dialog_protocol/CLAUDE.md index 4d55ac9..5cb968d 100644 --- a/pj_plugins/dialog_protocol/CLAUDE.md +++ b/pj_plugins/dialog_protocol/CLAUDE.md @@ -11,6 +11,13 @@ Local traps not visible from the headers: the XML, or the dialog renders with no OK/Cancel and no compile error. - `QTextEdit` / model-based `QTableView` are unsupported by the widget binding — use `QPlainTextEdit` / `QTableWidget`. See `../docs/dialog-plugin-guide.md`. +- A table must not combine `sortingEnabled=true` in its `.ui` XML with + `onHeaderClicked`: Qt sorts the view while the plugin reorders the model and the + plugin's order loses. Sort keys (`setTableRows` with `TableItem`) or + `onHeaderClicked` — one per table, never both. +- Sortable tables must also leave item drag/drop (`dragEnabled`, `InternalMove`) + OFF: Qt reconstructs dropped cells from serialized display roles, which strips + the typed sort key and leaves a column mixing keyed and keyless cells. - Headers here install into the SAME `pj_plugins/` include tree as the parent module (merged at install); keep names distinct. - `DialogHandle::borrowed()` / `fromBorrowed()` wrap a source/toolbox-owned diff --git a/pj_plugins/dialog_protocol/include/pj_plugins/host/widget_data_view.hpp b/pj_plugins/dialog_protocol/include/pj_plugins/host/widget_data_view.hpp index a979544..872d3e6 100644 --- a/pj_plugins/dialog_protocol/include/pj_plugins/host/widget_data_view.hpp +++ b/pj_plugins/dialog_protocol/include/pj_plugins/host/widget_data_view.hpp @@ -113,6 +113,10 @@ class WidgetDataView { for (const auto& cell : row) { if (cell.is_string()) { cells.push_back(cell.get()); + } else { + // Keep the row's shape. Dropping an unrecognized cell would shift every + // column after it and silently mis-align the whole row. + cells.emplace_back(); } } result.push_back(std::move(cells)); @@ -120,6 +124,68 @@ class WidgetDataView { return result; } + /// Sparse per-column sort keys (see WidgetData::setTableRows for TableItem rows). + /// Maps column index -> one entry per row, in the same order as tableRows(). A + /// nullopt entry, or a column absent from the map, sorts by the cell's text. + /// Columns whose value count disagrees with `rows` are dropped: a partial key + /// column would sort some rows by number and others by text. + [[nodiscard]] std::map>> tableColumnValues(std::string_view name) const { + std::map>> result; + const nlohmann::json* w = widget(name); + if (!w) { + return result; + } + auto rows_it = w->find("rows"); + const std::size_t row_count = (rows_it != w->end() && rows_it->is_array()) ? rows_it->size() : 0; + auto it = w->find("column_values"); + if (it == w->end() || !it->is_object()) { + return result; + } + for (auto kv = it->begin(); kv != it->end(); ++kv) { + const nlohmann::json& values = kv.value(); + const auto col = parseNumber(kv.key()); + if (!col.has_value() || *col < 0 || !values.is_array() || values.size() != row_count) { + continue; + } + std::vector> decoded; + decoded.reserve(values.size()); + for (const auto& v : values) { + // The wire collapses every integer width to a JSON integer, so decode to the + // widest lossless alternative rather than guessing the plugin's original type. + if (v.is_number_unsigned()) { + decoded.emplace_back(NumericValue(v.get())); + } else if (v.is_number_integer()) { + decoded.emplace_back(NumericValue(v.get())); + } else if (v.is_number_float()) { + decoded.emplace_back(NumericValue(v.get())); + } else { + decoded.emplace_back(std::nullopt); + } + } + result.emplace(*col, std::move(decoded)); + } + return result; + } + + /// Sort arrow a plugin-sorted table asked the host to draw: {column, ascending}. + /// Purely cosmetic — it never changes row order (see setTableSortIndicator). + [[nodiscard]] std::optional> tableSortIndicator(std::string_view name) const { + const nlohmann::json* w = widget(name); + if (!w) { + return std::nullopt; + } + auto it = w->find("sort_indicator"); + if (it == w->end() || !it->is_object()) { + return std::nullopt; + } + auto col = it->find("col"); + auto asc = it->find("asc"); + if (col == it->end() || !col->is_number_integer() || asc == it->end() || !asc->is_boolean()) { + return std::nullopt; + } + return std::make_pair(col->get(), asc->get()); + } + /// Column to render as an exclusive radio-button group (see setTableRadioColumn). [[nodiscard]] std::optional tableRadioColumn(std::string_view name) const { return getInt(name, "radio_column"); diff --git a/pj_plugins/dialog_protocol/include/pj_plugins/sdk/widget_data.hpp b/pj_plugins/dialog_protocol/include/pj_plugins/sdk/widget_data.hpp index fb74037..297b870 100644 --- a/pj_plugins/dialog_protocol/include/pj_plugins/sdk/widget_data.hpp +++ b/pj_plugins/dialog_protocol/include/pj_plugins/sdk/widget_data.hpp @@ -5,10 +5,16 @@ #include #include #include +#include #include #include +#include +#include +#include #include +#include "pj_base/types.hpp" + namespace PJ { /// A single point in a chart series (used by setChartSeries). @@ -17,6 +23,49 @@ struct ChartPoint { double y; }; +/// A table cell carrying both what it displays and what it sorts by. +/// +/// `text` is rendered verbatim — the plugin owns formatting (`%g`, `%.2f`, units, +/// "N/A"). `value` is the ordering truth. Without it the host can only compare the +/// rendered string, so a numeric column sorts lexicographically ("9" lands after +/// "10"); supplying it makes the host sort on the number instead. +/// +/// `value` need not resemble `text`: `TableItem(ns_since_epoch, "2026-07-17 10:23")` +/// displays a date and sorts on int64 nanoseconds. Leave it unset (the string +/// constructors do) for text columns — those sort by `text`. +/// +/// Pass the native type: `NumericValue` keeps each width exactly, so a uint64 count +/// or an int64 nanosecond timestamp never round-trips through a double (which is +/// only exact to 2^53). +/// +/// Non-finite doubles (NaN, ±infinity) do not survive the JSON wire — dump() +/// serializes them as null — so such a cell arrives at the host keyless and +/// sorts in the text rank with the other keyless cells, not at the numeric +/// extremes. Map infinities to a finite sentinel first if their ordering +/// matters. +struct TableItem { + std::string text; + std::optional value; + + TableItem() = default; + TableItem(std::string display) : text(std::move(display)) {} + TableItem(const char* display) : text(display) {} + + /// Numeric cell rendered with the default representation of `v`. long double + /// is excluded up front — NumericValue has no alternative for it, and letting + /// it past the constraint would fail with an incomprehensible variant error. + template < + typename T, typename = std::enable_if_t< + std::is_arithmetic_v && !std::is_same_v && !std::is_same_v>> + TableItem(T v) : text(std::to_string(v)), value(NumericValue(v)) {} + + /// Numeric cell with plugin-controlled rendering — `display` is shown, `v` is sorted on. + template < + typename T, typename = std::enable_if_t< + std::is_arithmetic_v && !std::is_same_v && !std::is_same_v>> + TableItem(T v, std::string display) : text(std::move(display)), value(NumericValue(v)) {} +}; + /// A named series of XY points for chart display (used by setChartSeries). /// If `color` is non-empty (e.g. "#ff7f0e"), it overrides the built-in palette /// color for this series; otherwise the host's chart palette picks one. @@ -243,7 +292,75 @@ class WidgetData { } WidgetData& setTableRows(std::string_view name, const std::vector>& rows) { - entry(name)["rows"] = rows; + auto& e = entry(name); + e["rows"] = rows; + // Plain rows carry no sort keys: drop any keys a previous TableItem delivery + // left behind, or — when the row count happens to match — the host would pair + // these rows with the STALE keys and silently sort them wrong. + e.erase("column_values"); + return *this; + } + + /// Rows that carry a sort key per cell (see TableItem). Emits the display text as + /// the usual `rows`, plus the keys as a sparse per-column map — so a host that + /// predates this overload still reads `rows` and behaves exactly as before. + /// Columns where no cell has a value are omitted entirely and sort by text. + WidgetData& setTableRows(std::string_view name, const std::vector>& rows) { + auto& e = entry(name); + + nlohmann::json text_rows = nlohmann::json::array(); + std::size_t max_cols = 0; + for (const auto& row : rows) { + nlohmann::json cells = nlohmann::json::array(); + for (const auto& item : row) { + cells.push_back(item.text); + } + text_rows.push_back(std::move(cells)); + if (row.size() > max_cols) { + max_cols = row.size(); + } + } + e["rows"] = std::move(text_rows); + + nlohmann::json columns = nlohmann::json::object(); + for (std::size_t c = 0; c < max_cols; ++c) { + bool any_value = false; + for (const auto& row : rows) { + if (c < row.size() && row[c].value.has_value()) { + any_value = true; + break; + } + } + if (!any_value) { + continue; + } + nlohmann::json values = nlohmann::json::array(); + for (const auto& row : rows) { + if (c < row.size() && row[c].value.has_value()) { + // Push the native alternative: nlohmann stores int64/uint64 natively, so a + // large count or a nanosecond timestamp survives the round-trip exactly. + std::visit([&values](auto v) { values.push_back(v); }, *row[c].value); + } else { + values.push_back(nlohmann::json::value_t::null); + } + } + columns[std::to_string(c)] = std::move(values); + } + if (columns.empty()) { + e.erase("column_values"); + } else { + e["column_values"] = std::move(columns); + } + return *this; + } + + /// Draw the sort arrow on `column` WITHOUT enabling Qt's built-in sorting. + /// For tables that sort themselves via onHeaderClicked: Qt paints the indicator + /// only when its own sorting is on, so a plugin-sorted table shows no arrow + /// unless it declares one here. Re-send whenever the sort state changes. + WidgetData& setTableSortIndicator(std::string_view name, int column, bool ascending) { + auto& e = entry(name); + e["sort_indicator"] = {{"col", column}, {"asc", ascending}}; return *this; } diff --git a/pj_plugins/dialog_protocol/tests/widget_data_test.cpp b/pj_plugins/dialog_protocol/tests/widget_data_test.cpp index 21d9c03..f78f8e5 100644 --- a/pj_plugins/dialog_protocol/tests/widget_data_test.cpp +++ b/pj_plugins/dialog_protocol/tests/widget_data_test.cpp @@ -3,8 +3,12 @@ #include +#include +#include #include #include +#include +#include using PJ::WidgetData; using json = nlohmann::json; @@ -129,6 +133,171 @@ TEST(WidgetDataTest, SetTableRows) { EXPECT_EQ(j["data_table"]["rows"][1][2], "10"); } +// --- TableWidget: typed rows (TableItem) --- + +// The string overload is the compatibility baseline: it must stay a pure-text +// emission, or an old host would start seeing a key it cannot interpret. +TEST(WidgetDataTest, SetTableRowsStringOverloadEmitsNoColumnValues) { + WidgetData wd; + std::vector> rows = {{"imu", "100"}, {"gps", "10"}}; + wd.setTableRows("data_table", rows); + auto j = parse(wd); + EXPECT_FALSE(j["data_table"].contains("column_values")); + EXPECT_EQ(j["data_table"], json::parse(R"({"rows": [["imu", "100"], ["gps", "10"]]})")); +} + +// Non-finite keys cannot cross the wire: nlohmann's dump() serializes NaN and +// ±infinity as null, so the host sees a keyless cell (text rank). Pinned here so +// the degradation stays deliberate and documented rather than accidental. +TEST(WidgetDataTest, SetTableRowsTypedNonFiniteKeysSerializeAsNull) { + WidgetData wd; + wd.setTableRows( + "t", {{PJ::TableItem(std::numeric_limits::infinity(), "inf")}, + {PJ::TableItem(-std::numeric_limits::infinity(), "-inf")}, + {PJ::TableItem(std::numeric_limits::quiet_NaN(), "nan")}}); + auto j = parse(wd); + const auto& col = j["t"]["column_values"]["0"]; + ASSERT_EQ(col.size(), 3u); + EXPECT_TRUE(col[0].is_null()); + EXPECT_TRUE(col[1].is_null()); + EXPECT_TRUE(col[2].is_null()); +} + +// A typed delivery followed by a string delivery on the SAME widget must drop the +// stale keys: with a matching row count the host would otherwise pair the new +// rows with the old keys and silently sort them wrong. +TEST(WidgetDataTest, SetTableRowsStringOverloadClearsStaleColumnValues) { + WidgetData wd; + std::vector> typed = { + {PJ::TableItem("a"), PJ::TableItem(std::uint64_t{2})}, {PJ::TableItem("b"), PJ::TableItem(std::uint64_t{1})}}; + wd.setTableRows("t", typed); + ASSERT_TRUE(parse(wd)["t"].contains("column_values")); + std::vector> plain = {{"c", "20"}, {"d", "10"}}; + wd.setTableRows("t", plain); + EXPECT_FALSE(parse(wd)["t"].contains("column_values")); +} + +// `rows` is what an old host reads, so the typed overload must produce byte-identical +// display text — the sort keys ride alongside, never inside it. +TEST(WidgetDataTest, SetTableRowsTypedEmitsSameDisplayTextAsStringOverload) { + WidgetData typed; + typed.setTableRows( + "t", {{PJ::TableItem("imu"), PJ::TableItem(std::uint64_t{100})}, + {PJ::TableItem("gps"), PJ::TableItem(std::uint64_t{10})}}); + + WidgetData text; + std::vector> rows = {{"imu", "100"}, {"gps", "10"}}; + text.setTableRows("t", rows); + + EXPECT_EQ(parse(typed)["t"]["rows"], parse(text)["t"]["rows"]); +} + +// Only columns that carry a key appear: a text column must not pay for the feature, +// and its absence is how the host knows to sort that column by text. +TEST(WidgetDataTest, SetTableRowsTypedColumnValuesAreSparse) { + WidgetData wd; + wd.setTableRows( + "t2", {{PJ::TableItem("chan_a"), PJ::TableItem("mcap"), PJ::TableItem(std::uint64_t{1234})}, + {PJ::TableItem("chan_b"), PJ::TableItem("mcap"), PJ::TableItem(std::uint64_t{720})}}); + wd.setTableSortIndicator("t2", 2, false); + + EXPECT_EQ(parse(wd), json::parse(R"({"t2": { + "column_values": {"2": [1234, 720]}, + "rows": [["chan_a", "mcap", "1234"], ["chan_b", "mcap", "720"]], + "sort_indicator": {"asc": false, "col": 2} + }})")); +} + +TEST(WidgetDataTest, SetTableRowsTypedWithNoValuesOmitsColumnValues) { + WidgetData wd; + wd.setTableRows("t", {{PJ::TableItem("a"), PJ::TableItem("b")}, {PJ::TableItem("c"), PJ::TableItem("d")}}); + auto j = parse(wd); + EXPECT_FALSE(j["t"].contains("column_values")); + ASSERT_EQ(j["t"]["rows"].size(), 2u); + EXPECT_EQ(j["t"]["rows"][1][0], "c"); +} + +// The reason the key exists. An int64 nanosecond timestamp and a uint64 byte count +// both exceed 2^53, so a sort key routed through a double would silently round — +// producing ties between distinct rows and a wrong answer to "which is largest". +TEST(WidgetDataTest, SetTableRowsTypedInt64AndUint64SurviveExactly) { + constexpr std::int64_t kNanos = 1780000000000000123; + constexpr std::uint64_t kMax = std::numeric_limits::max(); + + WidgetData wd; + wd.setTableRows("t", {{PJ::TableItem(kNanos)}, {PJ::TableItem(kMax)}}); + auto j = parse(wd); + + const auto& col = j["t"]["column_values"]["0"]; + ASSERT_EQ(col.size(), 2u); + EXPECT_EQ(col[0].get(), kNanos); + EXPECT_EQ(col[1].get(), kMax); + // Exact, not merely close: the same timestamp forced through a double lands on + // ...0000000000000000. Anything that reintroduces a double here is a regression. + EXPECT_NE(static_cast(static_cast(kNanos)), kNanos); +} + +TEST(WidgetDataTest, SetTableRowsTypedDoublesSurviveExactly) { + constexpr double kUlogParam = 560535533.0; + constexpr double kMax = std::numeric_limits::max(); + + WidgetData wd; + // The lossy "%g" rendering next to the exact value is the point of TableItem: + // text and value are different information, not duplicated information. + wd.setTableRows("t", {{PJ::TableItem(kUlogParam, "5.60536e+08")}, {PJ::TableItem(kMax, "1.79769e+308")}}); + auto j = parse(wd); + + const auto& col = j["t"]["column_values"]["0"]; + ASSERT_EQ(col.size(), 2u); + EXPECT_EQ(col[0].get(), kUlogParam); + EXPECT_EQ(col[1].get(), kMax); + EXPECT_EQ(j["t"]["rows"][0][0], "5.60536e+08"); +} + +TEST(WidgetDataTest, SetTableRowsTypedFloatSurvivesExactly) { + WidgetData wd; + wd.setTableRows("t", {{PJ::TableItem(1.5f)}, {PJ::TableItem(-0.25f)}}); + auto j = parse(wd); + const auto& col = j["t"]["column_values"]["0"]; + ASSERT_EQ(col.size(), 2u); + EXPECT_EQ(col[0].get(), 1.5); + EXPECT_EQ(col[1].get(), -0.25); +} + +// A hidden key: what the user reads and what the column orders by need not match. +TEST(WidgetDataTest, SetTableRowsTypedHiddenSortKey) { + constexpr std::int64_t kNanos = 1784283780000000000; + + WidgetData wd; + wd.setTableRows("t", {{PJ::TableItem(kNanos, "2026-07-17 10:23")}}); + auto j = parse(wd); + EXPECT_EQ(j["t"]["rows"][0][0], "2026-07-17 10:23"); + EXPECT_EQ(j["t"]["column_values"]["0"][0].get(), kNanos); +} + +// ulog mixes real numbers with "N/A" in one column. The keyless cell must hold its +// slot as null: shifting it would re-key every row below it. +TEST(WidgetDataTest, SetTableRowsTypedValuelessCellIsNull) { + WidgetData wd; + wd.setTableRows("t", {{PJ::TableItem(3.5)}, {PJ::TableItem("N/A")}, {PJ::TableItem(1.5)}}); + auto j = parse(wd); + + const auto& col = j["t"]["column_values"]["0"]; + ASSERT_EQ(col.size(), 3u); + EXPECT_EQ(col[0].get(), 3.5); + EXPECT_TRUE(col[1].is_null()); + EXPECT_EQ(col[2].get(), 1.5); + EXPECT_EQ(j["t"]["rows"][1][0], "N/A"); +} + +TEST(WidgetDataTest, SetTableSortIndicator) { + WidgetData wd; + wd.setTableSortIndicator("t", 3, true); + auto j = parse(wd); + EXPECT_EQ(j["t"]["sort_indicator"]["col"], 3); + EXPECT_EQ(j["t"]["sort_indicator"]["asc"], true); +} + // --- Label / Button --- TEST(WidgetDataTest, SetLabel) { diff --git a/pj_plugins/dialog_protocol/tests/widget_data_view_test.cpp b/pj_plugins/dialog_protocol/tests/widget_data_view_test.cpp index a5453f8..7fa41b2 100644 --- a/pj_plugins/dialog_protocol/tests/widget_data_view_test.cpp +++ b/pj_plugins/dialog_protocol/tests/widget_data_view_test.cpp @@ -3,8 +3,12 @@ #include +#include +#include #include #include +#include +#include // --- QLineEdit --- @@ -116,6 +120,122 @@ TEST(WidgetDataViewTest, TableRows) { EXPECT_EQ((*rows)[1][1], "2"); } +// A cell the reader cannot render as text still owns a column. Skipping it would +// pull every later cell one column left and mis-align the row against its headers. +TEST(WidgetDataViewTest, TableRowsNonStringCellKeepsRowShape) { + PJ::WidgetDataView v(R"({"tbl": {"rows": [["a", 1234, "z"], ["b", "2", "y"]]}})"); + auto rows = v.tableRows("tbl"); + ASSERT_TRUE(rows.has_value()); + ASSERT_EQ(rows->size(), 2u); + ASSERT_EQ((*rows)[0].size(), 3u); + EXPECT_EQ((*rows)[0][0], "a"); + EXPECT_EQ((*rows)[0][1], ""); + EXPECT_EQ((*rows)[0][2], "z"); +} + +// --- QTableWidget: typed sort keys --- + +TEST(WidgetDataViewTest, TableColumnValuesRoundTrip) { + constexpr std::int64_t kNanos = 1780000000000000123; + constexpr std::uint64_t kMax = std::numeric_limits::max(); + + PJ::WidgetData wd; + wd.setTableRows( + "tbl", {{PJ::TableItem("a"), PJ::TableItem(kNanos, "2026-07-17 10:23"), PJ::TableItem(1.5)}, + {PJ::TableItem("b"), PJ::TableItem(kMax, "lots"), PJ::TableItem(-2.5)}}); + PJ::WidgetDataView v(wd.toJson()); + + auto cols = v.tableColumnValues("tbl"); + ASSERT_EQ(cols.size(), 2u); // column 0 is text-only and carries no key + EXPECT_EQ(cols.count(0), 0u); + + const auto& keys = cols.at(1); + ASSERT_EQ(keys.size(), 2u); + ASSERT_TRUE(keys[0].has_value()); + ASSERT_TRUE(keys[1].has_value()); + EXPECT_EQ(std::get(*keys[0]), static_cast(kNanos)); + EXPECT_EQ(std::get(*keys[1]), kMax); + + const auto& floats = cols.at(2); + ASSERT_EQ(floats.size(), 2u); + EXPECT_EQ(std::get(*floats[0]), 1.5); + EXPECT_EQ(std::get(*floats[1]), -2.5); +} + +// JSON has one integer syntax, so the *sign of the value* — not the plugin's C++ +// type — decides which alternative survives the wire. A column that straddles zero +// therefore arrives as a mix of int64 and uint64 and any consumer comparing them +// must handle that, not assume one alternative per column. +TEST(WidgetDataViewTest, TableColumnValuesSignednessFollowsTheValue) { + PJ::WidgetData wd; + wd.setTableRows("tbl", {{PJ::TableItem(std::int32_t{-5})}, {PJ::TableItem(std::int32_t{10})}}); + PJ::WidgetDataView v(wd.toJson()); + + const auto cols = v.tableColumnValues("tbl"); + const auto& keys = cols.at(0); + ASSERT_EQ(keys.size(), 2u); + EXPECT_EQ(std::get(*keys[0]), -5); + EXPECT_EQ(std::get(*keys[1]), 10u); +} + +TEST(WidgetDataViewTest, TableColumnValuesNullEntryIsNullopt) { + PJ::WidgetDataView v(R"({"tbl": {"rows": [["3.5"], ["N/A"]], "column_values": {"0": [3.5, null]}}})"); + const auto cols = v.tableColumnValues("tbl"); + const auto& keys = cols.at(0); + ASSERT_EQ(keys.size(), 2u); + EXPECT_TRUE(keys[0].has_value()); + EXPECT_FALSE(keys[1].has_value()); +} + +TEST(WidgetDataViewTest, TableColumnValuesAbsentIsEmpty) { + PJ::WidgetDataView v(R"({"tbl": {"rows": [["a", "1"], ["b", "2"]]}})"); + EXPECT_TRUE(v.tableColumnValues("tbl").empty()); + EXPECT_TRUE(v.tableColumnValues("missing").empty()); +} + +// A short column cannot be zipped against the rows, and guessing an alignment would +// sort some rows by number and the rest by text. +TEST(WidgetDataViewTest, TableColumnValuesCountMismatchIsDropped) { + PJ::WidgetDataView v( + R"({"tbl": {"rows": [["a", "1"], ["b", "2"], ["c", "3"]], + "column_values": {"0": [1, 2, 3], "1": [1, 2]}}})"); + auto cols = v.tableColumnValues("tbl"); + EXPECT_EQ(cols.size(), 1u); + EXPECT_EQ(cols.count(1), 0u); + EXPECT_EQ(cols.at(0).size(), 3u); +} + +TEST(WidgetDataViewTest, TableColumnValuesBadKeyIsDropped) { + PJ::WidgetDataView v(R"({"tbl": {"rows": [["a"]], "column_values": {"abc": [1], "-1": [2], "1.5": [3], "0": [4]}}})"); + auto cols = v.tableColumnValues("tbl"); + ASSERT_EQ(cols.size(), 1u); + EXPECT_EQ(std::get(*cols.at(0)[0]), 4u); +} + +TEST(WidgetDataViewTest, TableSortIndicatorRoundTrip) { + PJ::WidgetData wd; + wd.setTableSortIndicator("tbl", 2, false); + PJ::WidgetDataView v(wd.toJson()); + ASSERT_TRUE(v.tableSortIndicator("tbl").has_value()); + EXPECT_EQ(*v.tableSortIndicator("tbl"), std::make_pair(2, false)); +} + +TEST(WidgetDataViewTest, TableSortIndicatorAbsent) { + PJ::WidgetDataView v(R"({"tbl": {"rows": [["a"]]}})"); + EXPECT_FALSE(v.tableSortIndicator("tbl").has_value()); + EXPECT_FALSE(v.tableSortIndicator("missing").has_value()); +} + +TEST(WidgetDataViewTest, TableSortIndicatorMalformed) { + EXPECT_FALSE(PJ::WidgetDataView(R"({"tbl": {"sort_indicator": {"asc": true}}})").tableSortIndicator("tbl")); + EXPECT_FALSE(PJ::WidgetDataView(R"({"tbl": {"sort_indicator": {"col": 1}}})").tableSortIndicator("tbl")); + EXPECT_FALSE( + PJ::WidgetDataView(R"({"tbl": {"sort_indicator": {"col": 1, "asc": "yes"}}})").tableSortIndicator("tbl")); + EXPECT_FALSE( + PJ::WidgetDataView(R"({"tbl": {"sort_indicator": {"col": "1", "asc": true}}})").tableSortIndicator("tbl")); + EXPECT_FALSE(PJ::WidgetDataView(R"({"tbl": {"sort_indicator": 2}})").tableSortIndicator("tbl")); +} + // --- QLabel --- TEST(WidgetDataViewTest, Label) { diff --git a/pj_plugins/docs/dialog-plugin-guide.md b/pj_plugins/docs/dialog-plugin-guide.md index 89478a2..e3672e3 100644 --- a/pj_plugins/docs/dialog-plugin-guide.md +++ b/pj_plugins/docs/dialog-plugin-guide.md @@ -257,12 +257,15 @@ bool onTextChanged(std::string_view name, std::string_view text) override { All handlers default to returning `false`. Override only the ones you need. -> **Column sorting.** The host does not enable `QTableWidget`'s built-in sort -> (it would reorder items and desync the index-based `setVisibleRows` / -> `setSelectedRows` model). Instead, header clicks arrive as -> `onHeaderClicked(name, section)`; a plugin that wants sortable columns -> re-orders its own row model (numeric for numeric columns) and re-emits the -> rows. The host shows a sort indicator on the clicked column. +> **Column sorting.** Two mechanisms, and they must not be mixed on one table — +> see [Sortable tables](#sortable-tables). Either give the cells sort keys +> (`setTableRows` with `TableItem`) and let the host sort, or take +> `onHeaderClicked(name, section)` and re-order your own row model. Never both. +> Also keep item drag/drop (`dragEnabled`, `InternalMove`) OFF on any sortable +> table: Qt rebuilds dropped cells from serialized display roles, which strips +> the typed sort key — the column ends up mixing keyed and keyless cells, and a +> row moved under the host scrambles the plugin-row mapping every index-keyed +> aspect (selection, visibility) relies on. ### 5. Export the plugin @@ -358,7 +361,7 @@ work like polling a server for available topics. | QPushButton (folder picker) | `setFolderPicker` | `onFolderSelected(name, path)` | | QLabel | `setLabel` | (none — display only) | | QListWidget | `setListItems`, `setSelectedItems` | `onSelectionChanged(name, items)`, `onItemDoubleClicked(name, index)` | -| QTableWidget | `setTableHeaders`, `setTableRows`, `setSelectedRows`, `setVisibleRows`, `setRowColor`, `setCellTooltip` | `onSelectionChanged(name, items)`, `onHeaderClicked(name, section)` | +| QTableWidget | `setTableHeaders`, `setTableRows` (strings, or `TableItem` for sortable columns), `setTableSortIndicator`, `setSelectedRows`, `setVisibleRows`, `setRowColor`, `setCellTooltip` | `onSelectionChanged(name, items)`, `onHeaderClicked(name, section)` | | QPlainTextEdit | `setPlainText`, `setCodeContent`, `setCodeLanguage`, `setCodeCursor`, `setCodeCaretTracking` | `onCodeChanged(name, code)`, or `onCodeChangedWithCursor(name, code, cursor)` when the editor opts into caret tracking | | QFrame (chart container) | `setChartSeries`, `clearChart`, `setChartZoomEnabled` | `onChartViewChanged(name, x_min, x_max, y_min, y_max)` | | QDateTimeEdit (incl. QDateEdit/QTimeEdit) | `setDateTime`, `setDateTimeRange` | `onDateTimeChanged(name, iso8601)` | @@ -372,12 +375,11 @@ All widgets also support `setEnabled(name, bool)`, `setVisible(name, bool)`, inline valid/invalid indicator the plugin drives). Drop targets receive `onItemsDropped(name, items)`. -`onHeaderClicked(name, section)` reports the clicked column index for plugins -that drive their own sorting. The `QTableWidget` styling setters layer over the -row data: `setVisibleRows` live-filters by index (an empty set hides every row; -to re-show all rows pass the full index list — clearing the field makes *no* -change), `setRowColor` tints a row (`"#rrggbb"`, or `""` to clear), and -`setCellTooltip` annotates a single cell. +The `QTableWidget` styling setters layer over the row data: `setVisibleRows` +live-filters by index (an empty set hides every row; to re-show all rows pass the +full index list — clearing the field makes *no* change), `setRowColor` tints a row +(`"#rrggbb"`, or `""` to clear), and `setCellTooltip` annotates a single cell. +Sorting has its own section below. For `DateRangePicker`, the `from_iso` / `to_iso` strings are ISO-8601 datetimes and are empty when that side of the range is unbounded. @@ -391,6 +393,120 @@ and are empty when that side of the range is unbounded. > (promote a placeholder `QWidget` to the class name); the host binds them by > object name exactly like the stock widgets above. +## Sortable Tables + +The guiding rule: **never sort on presentation strings; sort on the value and +format for display.** A cell reaches the host as text, so without a sort key the +host can only compare what it renders — and text compares lexicographically: +`"9"` lands *after* `"10"`, `"720"` after `"7"`. The column looks sorted and is +wrong. + +### Give the cells a sort key (preferred) + +`PJ::TableItem` carries both halves of a cell — what it displays and what it +orders by: + +```cpp +struct TableItem { + std::string text; // display — you own the formatting + std::optional value; // ordering truth; unset ⇒ sort by text +}; +``` + +Pass a `vector>` to `setTableRows` (an overload beside the +plain-string one) and the host sorts on the numbers. Rows are heterogeneous, as +real tables are — strings and numbers side by side: + +> One overload-resolution edge: a braced literal such as +> `setTableRows("t", {{"a", "b"}})` is ambiguous between the string and +> `TableItem` overloads (a compile error, never a silent behavior change). Name +> the vector — `std::vector> rows = …` — or wrap the +> cells in `PJ::TableItem(...)` to pick a side. Already-compiled plugins are +> unaffected. + +```cpp +std::string widget_data() override { + PJ::WidgetData wd; + std::vector> rows; + for (const auto& ch : channels_) { + rows.push_back({ + PJ::TableItem(ch.topic), // text: sorts by text + PJ::TableItem(ch.encoding), // text: sorts by text + PJ::TableItem(ch.message_count), // uint64: sorts numerically + PJ::TableItem(ch.rate_hz, formatRate(ch.rate_hz)), // shows "12.5 Hz", sorts on the double + }); + } + wd.setTableRows("tableWidget", rows); + return wd.toJson(); +} +``` + +Notes on the constructors above: + +- `TableItem("text")` / `TableItem(std::string)` leaves `value` unset — the cell + sorts by its text. That is the right answer for a genuinely textual column. +- `TableItem(v)` for any arithmetic `v` renders it with `std::to_string` and sorts + on it. +- `TableItem(v, "display")` sorts on `v` and shows `display` — use it whenever the + rendering is lossy or decorated (`"5.60536e+08"`, `"1.2 MB"`, `"12 Hz"`). +- **Pass the native type.** `NumericValue` keeps each width exactly, so a `uint64` + count or an `int64` nanosecond timestamp is never squeezed through a `double` + (exact only to 2⁵³ — beyond it, distinct values collapse into ties and "which is + largest" gets a wrong answer). Don't pre-cast to `double`. +- A cell with no value in an otherwise numeric column (ulog's `"N/A"`) is fine: + it keeps its slot and the host orders it deterministically against the numbers. + +### Hidden sort keys + +`text` and `value` need not resemble each other. A column can display a date and +sort on nanoseconds: + +```cpp +PJ::TableItem(entry.max_ts_ns, formatDate(entry.max_ts_ns)) // shows "2026-07-17 10:23" +``` + +This is why one mechanism covers every column in practice — the key does not have +to be visible. + +### On the wire + +The SDK derives both keys from the one `TableItem` matrix, so display and value +**cannot** desync: + +```json +"rows": [["chan_a","mcap","1234"], ["chan_b","mcap","720"]], +"column_values": {"2": [1234, 720]} +``` + +`column_values` is **sparse** — only columns where some cell has a value appear +(text-only columns cost nothing), and a valueless cell serializes as `null` in its +column's array. A host that predates the overload never looks for the key and +reads `rows` exactly as before, so adopting `TableItem` cannot break an old host. + +### Or sort it yourself + +If the ordering isn't numeric — or the sort has side effects — override +`onHeaderClicked(name, section)`, re-order your own row model, and re-emit the +rows. Then tell the host which arrow to paint: + +```cpp +wd.setTableSortIndicator("seqTable", sort_column_, sort_ascending_); +``` + +`setTableSortIndicator` exists because Qt paints a header arrow **only when its +own sorting is enabled**. A table you sort yourself would otherwise show no +indicator at all — the rows reorder and the header never says why. It is purely +cosmetic: it never reorders anything. Re-send it whenever the sort state changes. + +> **Trap: never combine `sortingEnabled=true` with `onHeaderClicked`.** A table +> that sets `sortingEnabled` in its `.ui` XML gets Qt's built-in sorting (that +> property is raw Qt reaching the widget through `QUiLoader`, not an SDK feature). +> Add `onHeaderClicked` on top and both sides act on the same click: Qt re-sorts +> the *view* by rendered text while your handler re-orders the *model*, and your +> order is the one that gets clobbered. Pick one mechanism per table. If you want +> Qt's sorting to be correct, don't intercept the header — supply `TableItem` +> values and let it sort on those. + ## Optional Features ### onTick — periodic background work