From ccc71ed86a9f2eaf5b11862f8a5f8d61b2f9f52d Mon Sep 17 00:00:00 2001 From: Carson Date: Wed, 28 Oct 2026 16:40:42 -0500 Subject: [PATCH] Fix BarWidget Python-side sync and button query scoping Fixes #42, fixes #43. BarWidget's frontend now: - seeds its collections from the model's existing data at render time, instead of discarding constructor-provided data and immediately overwriting it with all-y_min bins (#42) - registers change: handlers for data, n_bins, y_min, and y_max, so Python-side updates take effect without a re-render (#42). The change:data handler skips the echo of its own model.set() in updateDataOut() (which fires change synchronously) to avoid a loop - scopes its collection-button queries to its own container instead of the whole document, so render no longer throws when the view isn't attached to the document yet, the initial .click() can't hit another widget's button, and multiple BarWidgets on a page no longer switch each other's active collection (#43) width/height/collection_names changes still require a re-render. Tests: static regression checks on the bundle (same style as tests/test_scatter_widget_init_order.py), plus manual verification in a live Shiny for Python app (via shinywidgets): constructor seeding, Python-side data=/n_bins= pushes, and two BarWidgets coexisting without cross-contamination all work; no render-time console errors. --- drawdata/static/bar_widget.js | 44 +++++++++++++++++-- js/bar_widget.js | 63 +++++++++++++++++++++++++--- tests/test_bar_widget_python_sync.py | 38 +++++++++++++++++ 3 files changed, 137 insertions(+), 8 deletions(-) create mode 100644 tests/test_bar_widget_python_sync.py diff --git a/drawdata/static/bar_widget.js b/drawdata/static/bar_widget.js index 3b47cf5..587ea3a 100644 --- a/drawdata/static/bar_widget.js +++ b/drawdata/static/bar_widget.js @@ -9554,6 +9554,19 @@ function render({ model, el }) { let minY = model.get("y_min"); let maxY = model.get("y_max"); let data; + let syncingDataOut = false; + function seedFromModel() { + Object.keys(collections).forEach((key) => { + collections[key].data = new Array(getBins()).fill(model.get("y_min")); + }); + (model.get("data") || []).forEach((d) => { + const collection = collections[d.collection]; + if (collection && d.bin >= 0 && d.bin < collection.data.length) { + collection.data[d.bin] = Math.max(minY, Math.min(maxY, d.value)); + } + }); + } + seedFromModel(); updateDataOut(); getBins(); let container = document.createElement("div"); @@ -9654,10 +9667,35 @@ function render({ model, el }) { data.push({ collection: key, value, bin: j, color: collections[key].color }); }); }); + syncingDataOut = true; model.set("data", data); model.save_changes(); + syncingDataOut = false; } ; + model.on("change:data", function() { + if (syncingDataOut) + return; + seedFromModel(); + updateChart(); + }); + model.on("change:n_bins", function() { + Object.keys(collections).forEach((key) => { + const old = collections[key].data; + collections[key].data = d3.range(getBins()).map( + (i) => i < old.length ? old[i] : model.get("y_min") + ); + }); + updateChart(); + }); + model.on("change:y_min", function() { + minY = model.get("y_min"); + updateChart(); + }); + model.on("change:y_max", function() { + maxY = model.get("y_max"); + updateChart(); + }); const chartArea = svg.append("rect").attr("width", width).attr("height", height).attr("fill", "none").attr("pointer-events", "all"); function updateValue(event) { const [xPos, yPos] = d3.pointer(event); @@ -9683,9 +9721,9 @@ function render({ model, el }) { chartArea.on("mouseleave", () => { isDrawing = false; }); - document.querySelectorAll("button.control").forEach((button) => { + container.querySelectorAll("button.control").forEach((button) => { button.addEventListener("click", () => { - document.querySelectorAll("button.control").forEach((b) => b.classList.remove("active")); + container.querySelectorAll("button.control").forEach((b) => b.classList.remove("active")); button.classList.add("active"); activeCollection = button.querySelector("span").textContent; }); @@ -9697,7 +9735,7 @@ function render({ model, el }) { updateChart(); }); if (Object.keys(collections).length > 1) { - document.querySelector("button.control").click(); + container.querySelector("button.control").click(); } updateChart(); } diff --git a/js/bar_widget.js b/js/bar_widget.js index 43d4008..3d1b2f7 100644 --- a/js/bar_widget.js +++ b/js/bar_widget.js @@ -24,7 +24,28 @@ function render({ model, el }) { let isDrawing = false; let minY = model.get("y_min"); let maxY = model.get("y_max"); - let data; + let data; + // True while updateDataOut() is syncing data to the model, so the + // change:data handler below can tell its own echo apart from a real + // Python-side update (model.set() fires change synchronously) + let syncingDataOut = false; + + // (Re)fill each collection's bins from the model's data, so data passed + // to the constructor (or set later from Python) is reflected in the + // chart. Bins the data doesn't cover fall back to y_min, and values are + // clamped to the [y_min, y_max] range like drawn values are. + function seedFromModel() { + Object.keys(collections).forEach(key => { + collections[key].data = new Array(getBins()).fill(model.get("y_min")); + }); + (model.get("data") || []).forEach(d => { + const collection = collections[d.collection]; + if (collection && d.bin >= 0 && d.bin < collection.data.length) { + collection.data[d.bin] = Math.max(minY, Math.min(maxY, d.value)); + } + }); + } + seedFromModel(); updateDataOut(); // Create SVG @@ -207,10 +228,39 @@ function render({ model, el }) { data.push({collection: key, value: value, bin: j, color: collections[key].color}); }) }); + syncingDataOut = true; model.set("data", data); model.save_changes(); + syncingDataOut = false; }; + // React to Python-side updates (ScatterWidget already does this via its + // own change: handlers) + model.on("change:data", function() { + if (syncingDataOut) return; + seedFromModel(); + updateChart(); + }); + model.on("change:n_bins", function() { + // Re-fit each collection to the new bin count, keeping the bins + // that still exist + Object.keys(collections).forEach(key => { + const old = collections[key].data; + collections[key].data = d3.range(getBins()).map(i => + i < old.length ? old[i] : model.get("y_min") + ); + }); + updateChart(); + }); + model.on("change:y_min", function() { + minY = model.get("y_min"); + updateChart(); + }); + model.on("change:y_max", function() { + maxY = model.get("y_max"); + updateChart(); + }); + // Handle mouse events const chartArea = svg.append("rect") .attr("width", width) @@ -248,10 +298,13 @@ function render({ model, el }) { isDrawing = false; }); - // Handle collection selection - document.querySelectorAll('button.control').forEach(button => { + // Handle collection selection. Scope the queries to this widget's own + // container: document-level queries throw when the view isn't attached + // to the document yet (some hosts render before attaching), and make + // multiple BarWidgets on a page switch each other's active collection. + container.querySelectorAll('button.control').forEach(button => { button.addEventListener('click', () => { - document.querySelectorAll('button.control').forEach(b => b.classList.remove('active')); + container.querySelectorAll('button.control').forEach(b => b.classList.remove('active')); button.classList.add('active'); activeCollection = button.querySelector('span').textContent; }); @@ -266,7 +319,7 @@ function render({ model, el }) { }); if (Object.keys(collections).length > 1) { - document.querySelector('button.control').click(); + container.querySelector('button.control').click(); } // Initialize chart diff --git a/tests/test_bar_widget_python_sync.py b/tests/test_bar_widget_python_sync.py new file mode 100644 index 0000000..03c203a --- /dev/null +++ b/tests/test_bar_widget_python_sync.py @@ -0,0 +1,38 @@ +from pathlib import Path + + +BUNDLE = Path("drawdata/static/bar_widget.js").read_text(encoding="utf-8") + + +def test_bar_widget_seeds_from_model_data(): + # render() must read the model's existing data into the collections + # (so constructor-provided data survives the first render) + assert "seedFromModel" in BUNDLE + assert 'model.get("data")' in BUNDLE + + +def test_bar_widget_listens_for_python_side_updates(): + # ScatterWidget already has change: handlers; BarWidget needs them too, + # otherwise Python-side updates only apply on a re-render + assert 'model.on("change:data"' in BUNDLE + assert 'model.on("change:n_bins"' in BUNDLE + assert 'model.on("change:y_min"' in BUNDLE + assert 'model.on("change:y_max"' in BUNDLE + + +def test_bar_widget_change_data_handler_ignores_own_echo(): + # updateDataOut() calls model.set("data", ...), which fires change:data + # synchronously; without a guard the handler would loop + assert "syncingDataOut" in BUNDLE + + +def test_bar_widget_scopes_button_queries_to_its_container(): + # document.querySelector('button.control') throws when the view isn't + # attached to the document yet, and cross-contaminates multiple + # BarWidgets on a page; queries must go through the widget's container + assert 'document.querySelector(\'button.control\')' not in BUNDLE + assert 'document.querySelectorAll(\'button.control\')' not in BUNDLE + assert 'document.querySelector("button.control")' not in BUNDLE + assert 'document.querySelectorAll("button.control")' not in BUNDLE + assert 'container.querySelector("button.control")' in BUNDLE + assert 'container.querySelectorAll("button.control")' in BUNDLE