Refactor bim/parametric_lifecycle — drift triad + Cancel polish

Three changes to the shared Enable/Finish/Cancel mixins:

1. Always-on drift triad on ParametricEditMixinBase. The base now
   provides ``_handle_drift_on_enable`` / ``_handle_drift_on_finish``
   / ``_handle_drift_on_cancel`` classmethods, called from the
   per-mixin ``_enable_one`` / ``_finish_one`` / ``_cancel_one``.
   Pre-edit Blender-side translations commit to IFC on Enable
   (apply_scale=False — only translation/rotation, not the user's
   accidental scale), in-edit drag commits on Finish (apply_scale=True),
   and Cancel restores the committed IFC placement via
   ``restore_or_rebaseline_placement``. Prevents the
   "uncommitted drag disappears on Finish" and "preview snaps back
   on Cancel" UX bugs.

2. ``_ParametricEditMixinBase`` renamed to ``ParametricEditMixinBase``
   (public). Per-feature mixins that need to subclass directly
   (e.g., when neither FeatureModifier nor PathPreserving fits)
   can do so without reaching into a private name.

3. ``_update_modifier_bmesh`` (PathPreserving) renamed to
   ``_restore_viewport_after_cancel``. The old name was inaccurate
   for subclasses that load a different IFC representation on
   Cancel rather than rebuilding a bmesh preview from props.

Plus two polish changes:

* ``_mark_type_thumbnail_dirty`` helper on the base centralises the
  ``ifcopenshell.util.element.get_type`` + thumbnail-mark pattern
  that both mixins repeated inline.
* ``FeatureModifierEditMixin._cancel_one`` and
  ``PathPreservingEditMixin._cancel_one`` wrap the restore in
  ``try/finally`` so ``props.is_editing = False`` flips even on
  partial restore failure. Without this, a Cancel that raised
  mid-restore would leave the user locked out of the edit lifecycle.
* ``PathPreservingEditMixin._finish_one`` / ``_cancel_one`` skip the
  pset commit + viewport rebuild when the draft equals the stored
  pset (no-op Enable→Finish round-trip should not pollute the
  representation list or burn an undo entry).

``FeatureModifierEditMixin._finish_one`` now routes the pset commit
through ``tool.Pset.write_bbim_data`` instead of inlining the
``createIfcText(json.dumps(...))`` + ``ifcopenshell.api.pset.edit_pset``
dance. Two test assertions updated to match.

Generated with the assistance of an AI coding tool.

src/bonsai/bonsai/bim/parametric_lifecycle.py

@@ -18,29 +18,54 @@
 #
 # This file was generated with the assistance of an AI coding tool.
 
-"""Shared Enable / Finish / Cancel lifecycle mixins for parametric-edit operators.
+"""Shared operator mixins for parametric-edit operators.
 
-Two mixins fit the parametric-edit triads in ``bim/module/model/``:
+Edit-lifecycle mixins (Enable / Finish / Cancel):
+    `FeatureModifierEditMixin` — door, window (BBIM_<Type> pset; nested
+        lining/panel properties; Finish + Cancel route through
+        ``ifcopenshell.api.feature``).
+    `PathPreservingEditMixin` — railing, roof (path_data preserved across
+        edit; only general kwargs are user-editable).
 
-`FeatureModifierEditMixin`
+Pattern selection (which approach a new feature should adopt):
-    Door, Window — BBIM_<Type> pset with nested ``lining_properties`` /
+    Every parametric edit lifecycle commits to one of three patterns. Pick by
-    ``panel_properties``; Finish calls ``update_<type>_modifier_representation``
+    answering "does the feature share the Enable→Finish→Cancel shape that
-    via ``ifcopenshell.api.feature``; Cancel restores via ``switch_representation``.
+    one of the existing mixins already encodes?":
 
-`PathPreservingEditMixin`
+    A. Inherit one of the shared mixins below and route through
-    Railing, Roof — BBIM_<Type> pset whose ``path_data`` is preserved through
+       `tool.Parametric.build_edit_lifecycle`:
-    edit (only general kwargs are user-editable); Finish calls
-    ``update_<type>_modifier_bmesh`` / ``update_<type>_modifier_ifc_data``;
-    Cancel re-reads the pset and rebuilds the bmesh preview.
 
-Stair and Wall stay standalone — their lifecycles diverge in ways that don't
+       - `FeatureModifierEditMixin` when the feature stores its pset as
-fit either mixin without optional escape hatches (Stair has a unique
+         `{general fields} + {lining_properties: {...}} + {panel_properties: {...}}`
-``update_ifc_stair_props`` post-Finish step + a separate ``get_props_kwargs_for_ifc_export``;
+         and Finish must call a per-type `update_<type>_modifier_representation`.
-Wall is validation-first, snapshot-driven, no preview regen in operators).
 
-This module sits separately from `bonsai.tool.Parametric` (the registry +
+       - `PathPreservingEditMixin` when the feature's pset carries a
-auto-commit) because it imports ``bonsai.tool`` freely, while the registry
+         `path_data` field that survives general-kwarg edits untouched, with
-itself must stay light — ``tool/blender.py`` consumes the registry at module load."""
+         a separate Enable/Finish/Cancel lifecycle for path editing itself.
+
+    B. Write a per-feature mixin that subclasses `ParametricEditMixinBase`
+       and provides `_enable_targets` / `_finish_targets` / `_cancel_targets`,
+       then route through `build_edit_lifecycle`. Pick this when the
+       feature's pset roundtrip or representation handling diverges from the
+       shared mixins but the Enable→Finish→Cancel shape still fits.
+
+    C. Declare standalone Enable/Finish/Cancel Operator subclasses (no
+       factory) when the feature's parameter-change logic is sufficiently
+       unique that even a per-feature mixin would force optional hooks or
+       dead branches. Such operators MUST call the matrix_world drift
+       helpers (`tool.Geometry.commit_placement_if_moved` on Enable/Finish,
+       `tool.Geometry.restore_or_rebaseline_placement` on Cancel) — the
+       drift contract is enforced uniformly regardless of which pattern the
+       operators adopt.
+
+    The authoritative list of registered parametric types — and which use
+    `build_edit_lifecycle` vs. standalone operators — lives in
+    `tool/parametric.py`'s `EDIT_TYPES` and is enforced by the registry
+    contract tests in `test/bim/test_parametric_registry.py`.
+
+This module hosts operator-side mixins that import ``bonsai.tool`` freely.
+The lightweight parametric registry consumed at addon-enable time must stay
+free of such imports and lives separately in ``tool/parametric.py``."""
 
 from __future__ import annotations
 
@@ -48,9 +73,7 @@ import json
 from typing import TYPE_CHECKING, ClassVar
 
 import bpy
-import ifcopenshell.api.pset
 import ifcopenshell.util.element
-import ifcopenshell.util.representation
 
 import bonsai.core.geometry
 import bonsai.tool as tool
@@ -59,8 +82,8 @@ if TYPE_CHECKING:
     from ifcopenshell import entity_instance
 
 
-class _ParametricEditMixinBase:
+class ParametricEditMixinBase:
-    """Common scaffolding for parametric edit-triad mixins.
+    """Common scaffolding for parametric edit-lifecycle mixins.
 
     Each per-type subclass provides four hooks:
 
@@ -69,6 +92,11 @@ class _ParametricEditMixinBase:
         ``_get_props(obj)``: PropertyGroup accessor
         ``_iter_targets(context)``: list of objects to act on (default: ``[active_object]``)
 
+    Drift handling is built in: pre-edit matrix_world drift commits to IFC on
+    Enable, in-edit drag commits on Finish, and Cancel restores the committed
+    IFC placement. This prevents an uncommitted drag from disappearing on
+    Finish or snapping back on Cancel.
+
     Operator subclasses call one of ``_enable_targets`` / ``_finish_targets`` /
     ``_cancel_targets`` from their ``_execute`` method."""
 
@@ -99,8 +127,29 @@ class _ParametricEditMixinBase:
             return None
         return element, cls._get_props(obj)
 
+    @classmethod
+    def _handle_drift_on_enable(cls, obj: bpy.types.Object) -> None:
+        tool.Geometry.commit_placement_if_moved(obj, apply_scale=False)
 
-class FeatureModifierEditMixin(_ParametricEditMixinBase):
+    @classmethod
+    def _handle_drift_on_finish(cls, obj: bpy.types.Object) -> None:
+        tool.Geometry.commit_placement_if_moved(obj)
+
+    @classmethod
+    def _handle_drift_on_cancel(cls, obj: bpy.types.Object, element: entity_instance) -> None:
+        tool.Geometry.restore_or_rebaseline_placement(obj, element)
+
+    @classmethod
+    def _mark_type_thumbnail_dirty(cls, element: entity_instance) -> None:
+        """Mark the element's type's preview thumbnail for refresh so the
+        property-panel preview reflects post-edit geometry. No-op for
+        occurrences without a backing type."""
+        element_type = ifcopenshell.util.element.get_type(element)
+        if element_type:
+            tool.Model.mark_thumbnail_for_update(element_type)
+
+
+class FeatureModifierEditMixin(ParametricEditMixinBase):
     """Lifecycle for door- and window-style parametric modifier operators.
 
     Enable:
@@ -121,10 +170,7 @@ class FeatureModifierEditMixin(_ParametricEditMixinBase):
 
     @classmethod
     def _update_modifier_representation(cls, obj: bpy.types.Object, context: bpy.types.Context) -> None:
-        """Hook: call the per-type ``update_<type>_modifier_representation``.
+        """Hook: call the per-type ``update_<type>_modifier_representation``."""
-
-        Door's helper takes ``obj``; window's takes ``context``. The hook lets
-        each subclass forward to its existing helper without unifying signatures."""
         raise NotImplementedError
 
     @classmethod
@@ -133,6 +179,7 @@ class FeatureModifierEditMixin(_ParametricEditMixinBase):
         if resolved is None:
             return
         element, props = resolved
+        cls._handle_drift_on_enable(obj)
         data = json.loads(ifcopenshell.util.element.get_pset(element, cls.pset_name, "Data"))
         data.update(data.pop("lining_properties"))
         data.update(data.pop("panel_properties"))
@@ -152,12 +199,9 @@ class FeatureModifierEditMixin(_ParametricEditMixinBase):
         data["lining_properties"] = props.get_lining_kwargs(convert_to_project_units=True)
         data["panel_properties"] = props.get_panel_kwargs(convert_to_project_units=True)
         cls._update_modifier_representation(obj, context)
-        element_type = ifcopenshell.util.element.get_type(element)
+        cls._mark_type_thumbnail_dirty(element)
-        if element_type:
+        tool.Pset.write_bbim_data(element, cls.pset_name, data)
-            tool.Model.mark_thumbnail_for_update(element_type)
+        cls._handle_drift_on_finish(obj)
-        pset = tool.Pset.get_element_pset(element, cls.pset_name)
-        data_text = tool.Ifc.get().createIfcText(json.dumps(data, default=list))
-        ifcopenshell.api.pset.edit_pset(tool.Ifc.get(), pset=pset, properties={"Data": data_text})
         # Set only on success: if any IFC op above raised, the user's draft survives for retry.
         props.is_editing = False
 
@@ -167,13 +211,21 @@ class FeatureModifierEditMixin(_ParametricEditMixinBase):
         if resolved is None:
             return
         element, props = resolved
-        data = json.loads(ifcopenshell.util.element.get_pset(element, cls.pset_name, "Data"))
+        # Cancel must always clear is_editing — leaving it True after a
-        data.update(data.pop("lining_properties"))
+        # restore-failure would block the user from re-entering edit mode and
-        data.update(data.pop("panel_properties"))
+        # the next save's stale-flag heal would silently roll back the
-        props.set_props_kwargs_from_ifc_data(data)
+        # cancellation. Wrap the restore in try/finally so the flag flips
-        body = ifcopenshell.util.representation.get_representation(element, "Model", "Body", "MODEL_VIEW")
+        # even on partial failure.
-        bonsai.core.geometry.switch_representation(tool.Ifc, tool.Geometry, obj=obj, representation=body)
+        try:
-        props.is_editing = False
+            data = json.loads(ifcopenshell.util.element.get_pset(element, cls.pset_name, "Data"))
+            data.update(data.pop("lining_properties"))
+            data.update(data.pop("panel_properties"))
+            props.set_props_kwargs_from_ifc_data(data)
+            body = tool.Geometry.get_body_representation(element)
+            bonsai.core.geometry.switch_representation(tool.Ifc, tool.Geometry, obj=obj, representation=body)
+            cls._handle_drift_on_cancel(obj, element)
+        finally:
+            props.is_editing = False
 
     def _enable_targets(self, context: bpy.types.Context) -> set[str]:
         for obj in self._iter_targets(context):
@@ -191,19 +243,20 @@ class FeatureModifierEditMixin(_ParametricEditMixinBase):
         return {"FINISHED"}
 
 
-class PathPreservingEditMixin(_ParametricEditMixinBase):
+class PathPreservingEditMixin(ParametricEditMixinBase):
     """Lifecycle for railing- and roof-style parametric modifier operators.
 
     Distinctive: ``path_data`` is part of the BBIM_<Type> pset but is **not**
-    user-editable through this triad — it survives the edit untouched, only
+    user-editable through this lifecycle — it survives the edit untouched, only
     general kwargs are diffed. (Path editing has its own separate operator
     pair, ``Enable/Finish/CancelEditing<Type>Path``, out of scope here.)
 
     Enable:
         Fetch pset data via ``tool.Model.get_modeling_bbim_pset_data`` → set
         draft props → ``is_editing = True``. The subclass post-load hook
-        lets railing JSON-serialise ``path_data`` for the PropertyGroup
+        can reshape the dict to fit the PropertyGroup's storage layout
-        string field.
+        (e.g., pre-serialise a structured pset value to JSON for a
+        ``StringProperty`` field).
 
     Finish:
         Read fresh pset → keep ``path_data`` → gather ``general`` kwargs
@@ -213,16 +266,18 @@ class PathPreservingEditMixin(_ParametricEditMixinBase):
 
     Cancel:
         Read fresh pset → restore draft props → call
-        ``_update_modifier_bmesh`` (per-type bmesh preview) →
+        ``_restore_viewport_after_cancel`` (per-type viewport restore — typically
-        ``is_editing = False``."""
+        rebuilds the bmesh preview, but subclasses may load a different
+        representation entirely) → ``is_editing = False``."""
 
     @classmethod
     def _post_load_data(cls, data: dict) -> dict:
         """Hook: optionally transform the pset data dict after loading and before
         passing to ``set_props_kwargs_from_ifc_data``. Default: pass-through.
 
-        Railing overrides to JSON-serialise ``path_data`` (its
+        Override when the PropertyGroup stores a structured pset field as a
-        BIMRailingProperties.path_data is a ``StringProperty`` holding JSON)."""
+        serialised primitive — e.g., a list/dict value mapped onto a
+        ``StringProperty`` requires JSON-encoding here."""
         return data
 
     @classmethod
@@ -238,9 +293,12 @@ class PathPreservingEditMixin(_ParametricEditMixinBase):
         raise NotImplementedError
 
     @classmethod
-    def _update_modifier_bmesh(cls, obj: bpy.types.Object, context: bpy.types.Context) -> None:
+    def _restore_viewport_after_cancel(cls, obj: bpy.types.Object, context: bpy.types.Context) -> None:
-        """Hook: per-type ``update_<type>_modifier_bmesh`` — rebuilds the
+        """Hook: restore the viewport mesh to match the just-restored draft props.
-        bmesh preview to match the current draft props (used by Cancel)."""
+
+        Most subclasses rebuild a bmesh preview from props. Subclasses whose
+        committed IFC representation diverges from the preview may switch
+        the mesh back to the committed representation instead."""
         raise NotImplementedError
 
     @classmethod
@@ -249,6 +307,7 @@ class PathPreservingEditMixin(_ParametricEditMixinBase):
         if resolved is None:
             return
         _element, props = resolved
+        cls._handle_drift_on_enable(obj)
         data = tool.Model.get_modeling_bbim_pset_data(obj, cls.pset_name)["data_dict"]
         data = cls._post_load_data(data)
         props.set_props_kwargs_from_ifc_data(data)
@@ -261,11 +320,18 @@ class PathPreservingEditMixin(_ParametricEditMixinBase):
             return
         element, props = resolved
         pset_data = tool.Model.get_modeling_bbim_pset_data(obj, cls.pset_name)
-        path_data = pset_data["data_dict"]["path_data"]
+        stored = pset_data["data_dict"]
         data = props.get_general_kwargs(convert_to_project_units=True)
-        data["path_data"] = path_data
+        data["path_data"] = stored["path_data"]
-        cls._update_pset(element, data)
+        # Skip the pset commit when the draft is identical to the stored pset:
-        cls._update_modifier_ifc_data(obj, context)
+        # an Enable → Finish-without-changes cycle should not pollute the
+        # representation list or burn an undo entry. Drift commit still runs
+        # unconditionally — matrix_world drift is independent of pset content.
+        if data != stored:
+            cls._update_pset(element, data)
+            cls._update_modifier_ifc_data(obj, context)
+            cls._mark_type_thumbnail_dirty(element)
+        cls._handle_drift_on_finish(obj)
         # Set only on success: if any IFC op above raised, the user's draft survives for retry.
         props.is_editing = False
 
@@ -274,12 +340,26 @@ class PathPreservingEditMixin(_ParametricEditMixinBase):
         resolved = cls._resolve(obj)
         if resolved is None:
             return
-        _element, props = resolved
+        element, props = resolved
-        data = tool.Model.get_modeling_bbim_pset_data(obj, cls.pset_name)["data_dict"]
+        try:
-        data = cls._post_load_data(data)
+            pset_data = tool.Model.get_modeling_bbim_pset_data(obj, cls.pset_name)
-        props.set_props_kwargs_from_ifc_data(data)
+            stored = pset_data["data_dict"]
-        cls._update_modifier_bmesh(obj, context)
+            draft = props.get_general_kwargs(convert_to_project_units=True)
-        props.is_editing = False
+            draft["path_data"] = stored["path_data"]
+            nothing_changed = draft == stored
+            data = cls._post_load_data(stored)
+            props.set_props_kwargs_from_ifc_data(data)
+            # Skip the viewport rebuild on a no-op cancel: the mesh on screen is
+            # still the committed representation, and the per-type viewport-restore
+            # hook may be expensive (some subclasses reload a high-poly IFC
+            # representation rather than rebuild a preview mesh).
+            if not nothing_changed:
+                cls._restore_viewport_after_cancel(obj, context)
+            cls._handle_drift_on_cancel(obj, element)
+        finally:
+            # Always clear the flag — see ``FeatureModifierEditMixin._cancel_one``
+            # for the rationale.
+            props.is_editing = False
 
     def _enable_targets(self, context: bpy.types.Context) -> set[str]:
         for obj in self._iter_targets(context):

src/bonsai/test/bim/test_parametric_lifecycle.py

@@ -198,10 +198,12 @@ def test_feature_modifier_finish_one_clears_is_editing_and_writes_pset(patched_t
 
     assert props.is_editing is False
     assert obj in cls.representations_called
-    # edit_pset is called exactly once; properties key is "Data" wrapping JSON.
+    # tool.Pset.write_bbim_data is called exactly once with the merged dict.
-    patched_tool_and_ifc["ifc"].api.pset.edit_pset.assert_called_once()
+    patched_tool_and_ifc["tool"].Pset.write_bbim_data.assert_called_once()
-    kwargs = patched_tool_and_ifc["ifc"].api.pset.edit_pset.call_args.kwargs
+    call_args = patched_tool_and_ifc["tool"].Pset.write_bbim_data.call_args
-    assert "properties" in kwargs and "Data" in kwargs["properties"]
+    assert call_args.args[1] == "BBIM_Door"  # pset_name positional arg
+    written_data = call_args.args[2]
+    assert "lining_properties" in written_data and "panel_properties" in written_data
 
 
 def test_feature_modifier_finish_one_exception_leaves_draft_in_progress(patched_tool_and_ifc):
@@ -302,7 +304,7 @@ def _path_mixin_cls(match=True):
             cls.ifc_data_updates.append(obj)
 
         @classmethod
-        def _update_modifier_bmesh(cls, obj, context):
+        def _restore_viewport_after_cancel(cls, obj, context):
             cls.bmesh_updates.append(obj)
 
     return _TestPathMixin
@@ -344,7 +346,7 @@ def test_path_preserving_finish_one_preserves_path_data_and_clears_is_editing(pa
     assert obj in cls.ifc_data_updates
 
 
-def test_path_preserving_cancel_one_calls_update_modifier_bmesh(patched_tool_and_ifc):
+def test_path_preserving_cancel_one_calls_restore_viewport_after_cancel(patched_tool_and_ifc):
     props = _FakePathProps()
     props.is_editing = True
     obj = _make_obj(props)