Do not re-set container children that resolve to the same widgets

reacton/core.py

@@ -255,6 +255,34 @@ def _values_identical(a, b):
     return False
 
 
+_missing = object()
+
+
+def _widget_holds_values(widget: widgets.Widget, kwargs: Dict[str, Any]) -> bool:
+    """Does the widget already hold these (resolved) kwargs, so that setting them is a no-op?
+
+    Only for the same element as last render: its event listeners are the same callbacks,
+    and no argument was dropped. Compares by identity, where a list and a tuple with the
+    same items count as the same (a Tuple trait stores a list as a tuple).
+    """
+    for name, value in kwargs.items():
+        if name.startswith("on_") and not widget.has_trait(name):
+            continue
+        if not _value_held(value, getattr(widget, name, _missing)):
+            return False
+    return True
+
+
+def _value_held(value, held) -> bool:
+    if value is held:
+        return True
+    if isinstance(value, (list, tuple)):
+        return type(held) in (list, tuple) and len(value) == len(held) and all(_value_held(x, y) for x, y in zip(value, held))
+    if type(value) is dict:
+        return type(held) is dict and len(value) == len(held) and all(k in held and _value_held(v, held[k]) for k, v in value.items())
+    return False
+
+
 def _with_tracebacks(e, tracebacks):
     # copy it, and we need with_traceback for unknown reasons not to cause
     # an infinite loop
@@ -2798,15 +2826,17 @@ def _reconsolidate(self, el: Element, default_key: str, parent_key: str):
                     # update the existing widget in place
                     kwargs = self._visit_children_values(el.kwargs, key, parent_key, self._reconsolidate)
                     if not context.exceptions_children:
-                        if el is not el_prev or not _values_identical(kwargs, el.kwargs):
+                        # the same element whose kwargs hold no elements: nothing can have changed.
+                        # With elements (a container), the kwargs resolve to widgets, so compare
+                        # them with what the widget holds: equal means setting them is a no-op
+                        # (and a value changed from the frontend is still set back)
+                        if el is not el_prev or not (_values_identical(kwargs, el.kwargs) or _widget_holds_values(widget_previous, kwargs)):
                             try:
                                 el._update_widget(widget_previous, el_prev, kwargs)
                             except BaseException as e:
                                 context.exceptions_self.append(e)
                                 self._set_rerender_needed("Exception ocurred during reconciliation (updating widget)")
                                 _mark_needs_render_ancestors(context)
-                        # else: identical element and all children reconciled to the
-                        # same widgets, nothing can have changed
                     self._store_widget(context, el, key, widget_previous)
                 else:
                     assert el_prev is not None, "widget_previous is not None, but el_prev is"

reacton/core_test.py

@@ -3534,3 +3534,213 @@ def cleanup():
     finally:
         thread.join(5)
     assert not thread.is_alive()
+
+
+# Tests for the update path: what a state change re-renders and which widgets it touches.
+# Some of these pin properties of the fast renderer only (REACTON_FAST=1); the default
+# renderer walks and updates the whole tree on every render.
+fast_renderer_only = pytest.mark.skipif(core._render_context_class() is not core._RenderContextFast, reason="a property of the fast renderer (REACTON_FAST=1)")
+
+
+class UpdateSpy:
+    """Records the widgets that get a (re)assignment of their kwargs via Element._update_widget."""
+
+    def __init__(self):
+        self.updated: List[widgets.Widget] = []
+
+    def __enter__(self):
+        original = core.Element._update_widget
+        spy = self
+
+        def _update_widget(self, widget, el_prev, kwargs):
+            spy.updated.append(widget)
+            return original(self, widget, el_prev, kwargs)
+
+        self._patch = unittest.mock.patch.object(core.Element, "_update_widget", _update_widget)
+        self._patch.__enter__()
+        return self
+
+    def __exit__(self, *args):
+        self._patch.__exit__(*args)
+
+    def types(self):
+        return sorted(type(widget).__name__ for widget in self.updated)
+
+
+@fast_renderer_only
+def test_leaf_update_does_not_update_sibling_containers():
+    set_value = lambda x: None  # noqa
+
+    @react.component
+    def Row(i):
+        return w.HBox(children=[w.Button(description=f"button-{i}"), w.Label(value=f"label-{i}")])
+
+    @react.component
+    def Leaf():
+        nonlocal set_value
+        value, set_value = react.use_state(0)
+        return w.Button(description=f"leaf-{value}")
+
+    @react.component
+    def App():
+        # the HBox is a container next to the leaf, in the same (not re-rendered) component
+        return w.VBox(children=[w.HBox(children=[w.Label(value="sibling")]), Row(0), Row(1), Leaf()])
+
+    vbox, rc = react.render_fixed(App(), handle_error=False)
+    children_before = vbox.children
+    with UpdateSpy() as spy:
+        set_value(1)
+    assert rc.find(widgets.Button, description="leaf-1").widget is vbox.children[-1]
+    # only the leaf button gets new kwargs, the containers keep their children
+    assert spy.types() == ["Button"]
+    assert vbox.children == children_before
+    rc.close()
+
+
+def test_container_updates_when_child_widget_changes(Container):
+    # the component holding the containers does not re-render, but the root widget of
+    # a child component changes type: the container must get the new widget
+    setters = {}
+
+    @react.component
+    def Switch(name):
+        label, set_label = react.use_state(False)
+        setters[name] = set_label
+        if label:
+            return w.Label(value=name)
+        return w.Button(description=name)
+
+    @react.component
+    def App():
+        return w.VBox(children=[Container(children=[w.Button(description="sibling"), Switch("inner")]), Switch("outer")])
+
+    def describe(widget):
+        return (type(widget).__name__, widget.value if isinstance(widget, widgets.Label) else widget.description)
+
+    vbox, rc = react.render_fixed(App(), handle_error=False)
+    box = vbox.children[0]
+    assert isinstance(box, widgets.HBox)
+    assert [describe(child) for child in box.children] == [("Button", "sibling"), ("Button", "inner")]
+    assert describe(vbox.children[1]) == ("Button", "outer")
+
+    setters["outer"](True)
+    assert vbox.children[0] is box
+    assert describe(vbox.children[1]) == ("Label", "outer")
+
+    setters["inner"](True)
+    assert vbox.children[0] is box
+    assert [describe(child) for child in box.children] == [("Button", "sibling"), ("Label", "inner")]
+    label = box.children[1]
+
+    setters["inner"](False)
+    assert [describe(child) for child in box.children] == [("Button", "sibling"), ("Button", "inner")]
+    assert label.comm is None  # closed
+    setters["outer"](False)
+    assert describe(vbox.children[1]) == ("Button", "outer")
+    rc.close()
+
+
+def test_container_updates_when_fragment_child_changes():
+    # a child component returns a fragment: its widgets are spliced into the parent
+    # container, which must follow when the fragment changes
+    set_count = lambda x: None  # noqa
+
+    @react.component
+    def Items():
+        nonlocal set_count
+        count, set_count = react.use_state(1)
+        return reacton.Fragment(children=[w.Button(description=str(i)) for i in range(count)])
+
+    @react.component
+    def App():
+        return w.VBox(children=[w.Label(value="first"), Items(), w.Label(value="last")])
+
+    def describe(vbox):
+        return [child.value if isinstance(child, widgets.Label) else child.description for child in vbox.children]
+
+    vbox, rc = react.render_fixed(App(), handle_error=False)
+    assert describe(vbox) == ["first", "0", "last"]
+    set_count(3)
+    assert describe(vbox) == ["first", "0", "1", "2", "last"]
+    set_count(0)
+    assert describe(vbox) == ["first", "last"]
+    set_count(2)
+    assert describe(vbox) == ["first", "0", "1", "last"]
+    rc.close()
+
+
+def test_same_element_sets_back_changes_from_outside():
+    # the elements below are not re-created (App does not re-render), but a sibling update
+    # walks them: a value changed from the frontend (or by hand) is set back, like before
+    # the fast renderer skipped containers whose children resolve to the same widgets
+    set_value = lambda x: None  # noqa
+
+    @react.component
+    def Leaf():
+        nonlocal set_value
+        value, set_value = react.use_state(0)
+        return w.Button(description=f"leaf-{value}")
+
+    @react.component
+    def App():
+        return w.VBox(children=[w.HBox(children=[w.Label(value="child")]), w.IntSlider(value=5, layout=w.Layout(width="100px")), Leaf()])
+
+    vbox, rc = react.render_fixed(App(), handle_error=False)
+    hbox, slider = vbox.children[:2]
+    label = hbox.children[0]
+    hbox.children = ()
+    slider.value = 9
+    set_value(1)
+    assert hbox.children == (label,)
+    assert slider.value == 5
+    rc.close()
+
+
+def test_same_element_updates_when_slot_child_changes():
+    # an element nested in a list of dicts (like v_slots) that now resolves to a new widget
+    set_label = lambda x: None  # noqa
+
+    @react.component
+    def Activator():
+        nonlocal set_label
+        label, set_label = react.use_state(False)
+        return w.Label(value="label") if label else w.Button(description="button")
+
+    @react.component
+    def App():
+        return w.VBox(children=[v.Menu(v_slots=[{"name": "activator", "children": Activator()}])])
+
+    vbox, rc = react.render_fixed(App(), handle_error=False)
+    menu = vbox.children[0]
+    assert isinstance(menu.v_slots[0]["children"], widgets.Button)
+    set_label(True)
+    assert vbox.children[0] is menu
+    assert isinstance(menu.v_slots[0]["children"], widgets.Label)
+    set_label(False)
+    assert isinstance(menu.v_slots[0]["children"], widgets.Button)
+    rc.close()
+
+
+def test_container_updates_when_fragment_children_are_replaced():
+    # the fragment keeps its length, but its widgets change: the container must follow
+    set_offset = lambda x: None  # noqa
+
+    @react.component
+    def Items():
+        nonlocal set_offset
+        offset, set_offset = react.use_state(0)
+        return reacton.Fragment(children=[w.Button(description=str(i + offset)).key(f"item-{i + offset}") for i in range(2)])
+
+    @react.component
+    def App():
+        return w.VBox(children=[w.Label(value="first"), Items()])
+
+    vbox, rc = react.render_fixed(App(), handle_error=False)
+    first_button = vbox.children[1]
+    assert [child.description for child in vbox.children[1:]] == ["0", "1"]
+    set_offset(1)
+    assert [child.description for child in vbox.children[1:]] == ["1", "2"]
+    assert first_button not in vbox.children
+    set_offset(0)
+    assert [child.description for child in vbox.children[1:]] == ["0", "1"]
+    rc.close()