Fix BarWidget Python-side sync and button query scoping

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();
 }

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

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