From 619b5e356c05ac07352c84036cf99759c20b481f Mon Sep 17 00:00:00 2001 From: Martino Ferrari Date: Wed, 14 Oct 2026 11:45:00 +0200 Subject: [PATCH] fix: five review findings on the tabbed-sidebar commit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 2 — four builder ids had no null check. `side_stack`, `side_switcher`, `measure_page` and `signals_page` were resolved at lines 4709-4712, well past the skeleton null-guard. A typo in the `.ui` would have been a silent segfault. The four resolutions are moved to the guard block and added to its condition, in the same form the guard already uses. Finding 3 — the 'editable focused: m refused' leg of keys_probe was deleted instead of ported. It is restored, adapted for the new behaviour: 'm' now selects the Measure tab instead of toggling a pane, so the refusal is observed as the side_tab name being unchanged across the synthesised key, not as a toggle that did not flip. The dump line keeps its existing name and `refused=` field so probe-readings.md:601 still describes it. Finding 4 — `exit_probe_step3` assigned `sidebar_shown` to both `dbg_side_visible` and `dbg_side_column_after_hide`, and the panels: dump line read the wrong one (dbg_side_visible) for its `(visible=)` field. Two readings that cannot disagree diagnose nothing. Resolved by making `panels:` read `dbg_side_column_after_hide` and removing the redundant `dbg_side_visible` field. Finding 5 — the three-line rationale comment above `reload_file_view` in `apply_math_channels` was removed by the commit under review. Nothing it said became untrue. Restored verbatim from e10a92f. Finding 6 — three comments the change made false: - `on_activate`: 'the sidebar's scroller' replaced with 'the sidebar's tab switcher and two-page stack', matching the actual ui file. - `update_side_visibility`: 'Hiding the scroller instead left an empty column where the panels should be' removed; the design it described no longer exists. - `header-bar.ui`: 'two of them are toggles' updated to 'one of them is a toggle'; after signal_toggle's deletion the overflow menu holds one toggle (layout). Co-Authored-By: Claude Opus 4.6 --- src/ui.cpp | 27 +++++++++++++++------------ src/ui/header-bar.ui | 2 +- src/ui_app.hpp | 1 - src/ui_math.cpp | 3 +++ src/ui_probe.cpp | 18 ++++++++++++++++-- 5 files changed, 35 insertions(+), 16 deletions(-) diff --git a/src/ui.cpp b/src/ui.cpp index f4a860a..c480f6d 100644 --- a/src/ui.cpp +++ b/src/ui.cpp @@ -2854,8 +2854,7 @@ void update_side_visibility(App& app) { gtk_widget_set_sensitive(app.sidebar_toggle, have_signals); const bool want_column = have_signals && app.sidebar_wanted; // The split view owns the sidebar's visibility: it animates it, and when the window is too narrow it - // shows it as an overlay rather than letting the column be squeezed to nothing. Hiding the scroller - // instead left an empty column where the panels should be. + // shows it as an overlay rather than letting the column be squeezed to nothing. if (app.side_view != nullptr) { app.sidebar_syncing = true; adw_overlay_split_view_set_show_sidebar(ADW_OVERLAY_SPLIT_VIEW(app.side_view), want_column); @@ -4551,8 +4550,9 @@ void on_activate(GtkApplication* application, gpointer user) { install_css(); // The window's skeleton is a `.ui` file: the window, its two panes, the plot column with the status - // line under it, and the sidebar's scroller. Everything that depends on the stream -- the events - // pane, the two plot areas, the sidebar's panels -- is built below and put into the slots by id. + // line under it, and the sidebar's tab switcher and two-page stack. Everything that depends on the + // stream -- the events pane, the two plot areas, the sidebar's panels -- is built below and put + // into the slots by id. g_clear_object(&app->window_ui); app->window_ui = gtk_builder_new_from_resource("/org/udpscope/ui/main-window.ui"); if (app->window_ui == nullptr) return; @@ -4566,12 +4566,17 @@ void on_activate(GtkApplication* application, gpointer user) { app->status_label = GTK_WIDGET(gtk_builder_get_object(app->window_ui, "status_label")); app->status_info_button = GTK_WIDGET(gtk_builder_get_object(app->window_ui, "status_info")); GtkWidget* side = GTK_WIDGET(gtk_builder_get_object(app->window_ui, "side_column")); + GtkWidget* side_stack = GTK_WIDGET(gtk_builder_get_object(app->window_ui, "side_stack")); + GtkWidget* side_switcher = GTK_WIDGET(gtk_builder_get_object(app->window_ui, "side_switcher")); + GtkWidget* measure_page = GTK_WIDGET(gtk_builder_get_object(app->window_ui, "measure_page")); + GtkWidget* signals_page = GTK_WIDGET(gtk_builder_get_object(app->window_ui, "signals_page")); // A missing id is the one mistake neither the build nor `GtkBuilder` sees, and every use below would // be a null pointer. if (app->window == nullptr || side_view == nullptr || events_divider == nullptr || app->plot_box == nullptr || app->plot_split == nullptr || app->status_badge == nullptr || app->status_label == nullptr || app->status_info_button == nullptr || - side == nullptr) { + side == nullptr || side_stack == nullptr || side_switcher == nullptr || + measure_page == nullptr || signals_page == nullptr) { return; } // The window's actions live on the application, so it has to know which one it belongs to. @@ -4703,13 +4708,11 @@ void on_activate(GtkApplication* application, gpointer user) { // no section competes with four others for height. Each page scrolls on its own -- the two are // different heights, and one shared scroller would leave a switch scrolled where the new page // never asked to be. - // `side` is `side_column` from the `.ui`, resolved above with the rest of the skeleton; the app - // keeps it because `update_side_visibility()` still asks about the column as a whole. + // `side` and `side_stack`/`side_switcher`/`measure_page`/`signals_page` are resolved above with + // the rest of the skeleton and checked by the null guard; assign the members here. app->side_box = side; - app->side_stack = GTK_WIDGET(gtk_builder_get_object(app->window_ui, "side_stack")); - app->side_switcher = GTK_WIDGET(gtk_builder_get_object(app->window_ui, "side_switcher")); - GtkWidget* measure_page = GTK_WIDGET(gtk_builder_get_object(app->window_ui, "measure_page")); - GtkWidget* signals_page = GTK_WIDGET(gtk_builder_get_object(app->window_ui, "signals_page")); + app->side_stack = side_stack; + app->side_switcher = side_switcher; // Measure: what the cursors bracket, and the figures derived from it. app->cursors_panel = build_cursors_panel(*app); @@ -5642,7 +5645,7 @@ int run_ui(int argc, char** argv, const Config& cfg, ScopeModel& model, Acquisit std::printf("[udpscope] panels: side=%dx%d (visible=%d) signals=%dx%d row_h=%d " "(page_current=%d) trigger=%dx%d (sensitive=%d) switcher=%d cursors=%d window=%.3fs " "window_cap=%.1fs\n", - app.dbg_side_w, app.dbg_side_h, app.dbg_side_visible, app.dbg_signal_w, + app.dbg_side_w, app.dbg_side_h, app.dbg_side_column_after_hide, app.dbg_signal_w, app.dbg_signal_h, app.dbg_signal_row_h, app.dbg_signal_page_current, app.dbg_trigger_w, app.dbg_trigger_h, app.dbg_trigger_sensitive, app.dbg_switcher_visible, diff --git a/src/ui/header-bar.ui b/src/ui/header-bar.ui index 9fc9da3..1c21a9c 100644 --- a/src/ui/header-bar.ui +++ b/src/ui/header-bar.ui @@ -6,7 +6,7 @@ marks where the *view's* controls end, and the three at the far end. The bar carries only the controls you reach for *while watching a signal*; everything you set up once lives - in the overflow menu, which is code — its rows act on the `RowSet` directly and two of them are toggles + in the overflow menu, which is code — its rows act on the `RowSet` directly and one of them is a toggle whose state the rest of the interface reads, so a `GMenu` model would only add a second copy of that state. The order is the file's, and it is the one thing here that is not self-evident: `pack_end()` lays its diff --git a/src/ui_app.hpp b/src/ui_app.hpp index 2eba028..9b5f625 100644 --- a/src/ui_app.hpp +++ b/src/ui_app.hpp @@ -737,7 +737,6 @@ struct App { int dbg_trigger_h = 0; int dbg_signal_page_current = 0; int dbg_trigger_sensitive = 0; - int dbg_side_visible = 0; int dbg_trigger_items = 0; int dbg_trigger_selected = -1; int dbg_side_column_after_hide = 1; diff --git a/src/ui_math.cpp b/src/ui_math.cpp index 12f79be..122f023 100644 --- a/src/ui_math.cpp +++ b/src/ui_math.cpp @@ -18,6 +18,9 @@ void apply_math_channels(App& app) { // `set_math_channels()` takes the model's lock itself -- holding it here is a self-deadlock on a // non-recursive mutex, and it is exactly how the first version of the probe hung the app. app.model->set_math_channels(app.math); + // The structural rebuild above replaced the buffer and dropped its history. A stream's producer + // refills that; a file's nothing does, so the view is re-read -- and with the math set now in place + // the reload computes the derived channels over the file as it goes. reload_file_view(app); rebuild_math_pane(app); // The math channels are rows of the signal list too -- `append_math_signals_locked()` puts them diff --git a/src/ui_probe.cpp b/src/ui_probe.cpp index e0925ac..83be65e 100644 --- a/src/ui_probe.cpp +++ b/src/ui_probe.cpp @@ -103,7 +103,6 @@ gboolean exit_probe_step3(gpointer user) { a->dbg_side_column_after_hide = sidebar_shown; a->dbg_switcher_visible = a->side_switcher != nullptr && gtk_widget_get_visible(a->side_switcher) ? 1 : 0; - a->dbg_side_visible = sidebar_shown; if (a->trigger_signal_drop != nullptr) { GListModel* model = gtk_drop_down_get_model(GTK_DROP_DOWN(a->trigger_signal_drop)); a->dbg_trigger_items = model != nullptr ? static_cast(g_list_model_get_n_items(model)) : 0; @@ -4051,7 +4050,22 @@ gboolean keys_probe(gpointer user) { before_after("l: layout", a->layout_toggle, AppAction::LayoutEditor); before_after("s: side panel", a->sidebar_toggle, AppAction::SidePanel); - line += "\n"; + // An accel must be refused while text is being typed. Focus the window entry, send 'm' + // through the real handler, and require the side tab not to change. + int refused = -1; + if (a->window_entry != nullptr) { + gtk_menu_button_popup(GTK_MENU_BUTTON(a->window_button)); + gtk_widget_grab_focus(a->window_entry); + const std::string before_tab = a->side_tab; + const gboolean handled = on_app_key_pressed(nullptr, GDK_KEY_m, 0, + static_cast(0), a); + const std::string after_tab = a->side_tab; + refused = (handled == FALSE && before_tab == after_tab) ? 1 : 0; + gtk_menu_button_popdown(GTK_MENU_BUTTON(a->window_button)); + } + char tail[64]; + std::snprintf(tail, sizeof(tail), " editable focused: 'm' refused=%d\n", refused); + line += tail; std::fprintf(stderr, "%s", line.c_str()); std::fflush(stderr);