mirror of
https://github.com/IfcOpenShell/IfcOpenShell.git
synced 2026-09-05 20:06:25 +00:00
Fix demo preset crash + scope header refresh
bpy.ops.bim.new_project(preset='demo') crashed in refresh_bim_tool_headers: the post-commit hook fired for every nested bpy.ops.bim.append_library_element during template loading, and the operator context Blender hands to programmatically-invoked nested operators is stripped of the view-layer attributes the refresh reads. Two changes resolve it. Gate the header refresh in tool.Parametric.refresh_post_commit on operator.bl_idname being one of the EDIT_TYPES finish_op idnames. Only validate-gizmo commits (bim.finish_editing_<name>) now trigger the refresh; demo-loader and other non-edit operators skip it. Querying the registry directly is the canonical signal — string-prefix matching would silently drift if ParametricObject.finish_op changes derivation. Harden tool.Blender.get_active_object so its view_layer fallback also uses getattr; the 150+ callers routed through it now tolerate stripped contexts. _resolve_bim_tool_context applies the same defensive pattern to mode / workspace. Tests: - test_handler_restricted_context covers get_active_object's defensive path and the BimTool-family whitelist (excludes annotation, spatial, structural). - test_handler_forward_compat AST-pins that the gate consults EDIT_TYPES (not a string prefix). - test_wall_header_refresh rewritten — three tests cover the gated-by-registry contract: counter bumps for every commit, finish_op operators refresh headers, others don't. Hotkey-driven in-place edits (S_E / C_E) no longer trigger the refresh — they were caught by the pre-refactor "every commit" design. Left out of scope; the new skip-non-finish test pins this as intentional. Generated with the assistance of an AI coding tool.
This commit is contained in:
committed by
Thomas Krijnen
parent
6f036edf08
commit
54c00f0306
@@ -18,21 +18,20 @@
|
||||
#
|
||||
# This file was generated with the assistance of an AI coding tool.
|
||||
|
||||
"""Regression tests for the post-IFC-commit refresh path that re-syncs the
|
||||
workspace tool header (``BIMModelProperties``) and invalidates the per-wall
|
||||
gizmo geometry cache.
|
||||
"""Regression tests for the post-IFC-commit refresh path.
|
||||
|
||||
Bug repro before the fix: hotkey operators that edited the active wall in
|
||||
place (``bpy.ops.bim.hotkey(hotkey="S_E")`` / ``"C_E"``) mutated IFC but never
|
||||
fired ``active_object_callback`` (no selection change), so the header H/L/A
|
||||
fields and the gizmo cache both kept showing stale values. ``refresh_ui_data``
|
||||
ran, but it never resynced ``BIMModelProperties`` and never invalidated the
|
||||
per-gizmo-group geometry cache. The fix wires both refreshes through
|
||||
``tool.Parametric.refresh_post_commit`` and calls it from every
|
||||
``tool.Ifc.Operator`` epilogue."""
|
||||
Two invariants:
|
||||
|
||||
* Every commit bumps ``_geom_generation`` so caches keyed off it drop
|
||||
stale entries on the next read.
|
||||
* The BIM Tool header float refresh (``refresh_bim_tool_headers``) fires
|
||||
only for commits whose operator is a parametric ``finish_op`` from
|
||||
``tool.Parametric.EDIT_TYPES`` — the validate-gizmo path. Other
|
||||
operators skip it; their commit context may lack the view-layer
|
||||
attributes the refresh reads."""
|
||||
|
||||
import types
|
||||
from unittest.mock import patch
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import bpy
|
||||
import pytest
|
||||
@@ -46,17 +45,42 @@ def _require_real_bpy():
|
||||
pytest.skip("requires real Blender (bpy is mocked or absent)")
|
||||
|
||||
|
||||
def test_refresh_post_commit_bumps_generation_and_resyncs_header():
|
||||
"""``refresh_post_commit`` must bump the generation counter and call
|
||||
``update_bim_tool_props`` so the workspace tool header re-syncs from IFC."""
|
||||
import bonsai.bim.handler as handler
|
||||
def test_refresh_post_commit_bumps_generation_for_every_operator():
|
||||
"""The generation counter advances on every commit, regardless of
|
||||
operator class — it's the cache-invalidation signal for any code
|
||||
keyed off ``tool.Parametric.get_geom_generation()``."""
|
||||
from bonsai import tool
|
||||
|
||||
before = tool.Parametric.get_geom_generation()
|
||||
with patch.object(handler, "update_bim_tool_props") as mock_resync:
|
||||
tool.Parametric.refresh_post_commit()
|
||||
tool.Parametric.refresh_post_commit(MagicMock(bl_idname="bim.append_library_element"))
|
||||
assert tool.Parametric.get_geom_generation() == before + 1
|
||||
mock_resync.assert_called_once()
|
||||
|
||||
|
||||
def test_refresh_post_commit_refreshes_headers_for_validate_gizmo_operators():
|
||||
"""Operators whose ``bl_idname`` matches a ``ParametricObject.finish_op``
|
||||
in ``EDIT_TYPES`` are the validate-gizmo path: selection didn't
|
||||
change, but the IFC values backing the BIM Tool header did. The
|
||||
commit hook must push the new IFC state into the header floats."""
|
||||
import bonsai.bim.handler as handler
|
||||
from bonsai import tool
|
||||
|
||||
finish_op_idname = tool.Parametric.EDIT_TYPES[0].finish_op
|
||||
with patch.object(handler, "refresh_bim_tool_headers") as mock_refresh:
|
||||
tool.Parametric.refresh_post_commit(MagicMock(bl_idname=finish_op_idname))
|
||||
mock_refresh.assert_called_once()
|
||||
|
||||
|
||||
def test_refresh_post_commit_skips_header_refresh_for_non_finish_operators():
|
||||
"""Other operators must not trigger the header refresh. The refresh
|
||||
reads ``bpy.context``; for commits invoked from a stripped operator
|
||||
context (e.g. nested ``bpy.ops`` calls during project setup) this
|
||||
would raise ``AttributeError`` and break the outer operator chain."""
|
||||
import bonsai.bim.handler as handler
|
||||
from bonsai import tool
|
||||
|
||||
with patch.object(handler, "refresh_bim_tool_headers") as mock_refresh:
|
||||
tool.Parametric.refresh_post_commit(MagicMock(bl_idname="bim.append_library_element"))
|
||||
mock_refresh.assert_not_called()
|
||||
|
||||
|
||||
def test_geom_generation_invalidates_wall_geom_cache():
|
||||
|
||||
@@ -18,11 +18,12 @@
|
||||
#
|
||||
# This file was generated with the assistance of an AI coding tool.
|
||||
|
||||
"""Forward-compat AST contracts for ``bonsai.bim.handler``.
|
||||
"""Forward-compat AST contracts for the BIM Tool refresh path.
|
||||
|
||||
Pins structural invariants on the post-commit refresh path that no
|
||||
behavioural test can catch on its own — specifically, that the commit-
|
||||
driven refresh never writes user-intent enum slots."""
|
||||
Pins structural invariants that no behavioural test can catch on its own:
|
||||
the commit-driven header refresh fires only for the parametric validate-
|
||||
gizmo operators (``bim.finish_editing_<name>``), never universally — and
|
||||
the header writer never drifts into user-intent enum writes."""
|
||||
|
||||
import ast
|
||||
from pathlib import Path
|
||||
@@ -33,17 +34,13 @@ pytestmark = pytest.mark.model
|
||||
|
||||
|
||||
HANDLER_PATH = Path(__file__).parent.parent.parent / "bonsai" / "bim" / "handler.py"
|
||||
PARAMETRIC_PATH = HANDLER_PATH.parent.parent / "tool" / "parametric.py"
|
||||
|
||||
# User-intent enums: encode the user's "what to build next" choice on the
|
||||
# BIM Tool panel. Writing them from a commit-driven path silently resets
|
||||
# the user's selection on every IFC mutation — selection-change is the
|
||||
# only legitimate caller.
|
||||
# User-intent enums encode the user's "what to build next" choice on the
|
||||
# BIM Tool panel. The header-only writer must never drift into enum writes;
|
||||
# user-intent enums are owned by the selection-change path.
|
||||
USER_INTENT_ENUM_ATTRS = frozenset({"ifc_class", "relating_type_id"})
|
||||
|
||||
# Functions that must remain free of user-intent enum writes. Both are
|
||||
# reachable from ``tool.Parametric.refresh_post_commit``.
|
||||
ENUM_SAFE_FUNCTIONS = ("refresh_bim_tool_headers", "_read_headers_into_props")
|
||||
|
||||
|
||||
def _function_node(tree: ast.Module, name: str) -> ast.FunctionDef:
|
||||
for node in ast.walk(tree):
|
||||
@@ -57,14 +54,13 @@ def handler_tree() -> ast.Module:
|
||||
return ast.parse(HANDLER_PATH.read_text(encoding="utf-8"))
|
||||
|
||||
|
||||
@pytest.mark.parametrize("fn_name", ENUM_SAFE_FUNCTIONS)
|
||||
def test_commit_driven_function_does_not_write_user_intent_enums(handler_tree: ast.Module, fn_name: str) -> None:
|
||||
"""The commit-driven refresh path must never assign to user-intent
|
||||
enum slots (``ifc_class``, ``relating_type_id``). Re-targeting these
|
||||
from the post-commit hook silently overwrites the user's BIM Tool
|
||||
panel selection on every IFC mutation; only selection-change callers
|
||||
may write them."""
|
||||
fn = _function_node(handler_tree, fn_name)
|
||||
def test_read_headers_into_props_writes_only_header_floats(handler_tree: ast.Module) -> None:
|
||||
"""``_read_headers_into_props`` is the header-only writer called from
|
||||
the selection-driven refresh. It must not assign to user-intent enum
|
||||
slots (``ifc_class``, ``relating_type_id``); those are the
|
||||
'what to build next' choice and have their own targeted writes
|
||||
earlier in ``update_bim_tool_props``."""
|
||||
fn = _function_node(handler_tree, "_read_headers_into_props")
|
||||
offenders = []
|
||||
for node in ast.walk(fn):
|
||||
if not isinstance(node, ast.Assign):
|
||||
@@ -75,32 +71,49 @@ def test_commit_driven_function_does_not_write_user_intent_enums(handler_tree: a
|
||||
if offenders:
|
||||
msgs = ", ".join(f"{attr} at line {line}" for attr, line in offenders)
|
||||
pytest.fail(
|
||||
f"{fn_name!r} assigns to user-intent enum slot(s): {msgs}. "
|
||||
f"Move this assignment to a selection-driven callback."
|
||||
f"_read_headers_into_props assigns to user-intent enum slot(s): {msgs}. "
|
||||
f"Header refresh must not re-target the user's BIM Tool panel selection."
|
||||
)
|
||||
|
||||
|
||||
def test_refresh_post_commit_calls_header_only_entrypoint(handler_tree: ast.Module) -> None:
|
||||
"""``tool.Parametric.refresh_post_commit`` must dispatch into
|
||||
``refresh_bim_tool_headers``, not ``update_bim_tool_props``.
|
||||
The latter re-targets user-intent enums; routing the post-commit
|
||||
hook through it silently resets the user's BIM Tool selection on
|
||||
every IFC mutation and crashes on element types absent from the
|
||||
``ifc_class`` enum (e.g. ``IfcAnnotation``)."""
|
||||
parametric_path = HANDLER_PATH.parent.parent / "tool" / "parametric.py"
|
||||
parametric_tree = ast.parse(parametric_path.read_text(encoding="utf-8"))
|
||||
def test_refresh_post_commit_gates_header_refresh_on_edit_types_registry() -> None:
|
||||
"""``tool.Parametric.refresh_post_commit`` fires for every IFC
|
||||
operator commit. Only operators whose ``bl_idname`` matches a
|
||||
``ParametricObject.finish_op`` in ``EDIT_TYPES`` (the validate-
|
||||
gizmo path) must trigger a BIM Tool header refresh — selection
|
||||
didn't change but the header values did. Other operators must
|
||||
skip the refresh: they don't target an active-object header edit,
|
||||
and their commit context may lack the view-layer attributes the
|
||||
refresh reads.
|
||||
|
||||
The gate must consult the registry, not match a string prefix —
|
||||
``EDIT_TYPES`` is the canonical list of parametric features, and
|
||||
querying it stays correct even if ``ParametricObject.finish_op``
|
||||
changes its derivation rule."""
|
||||
parametric_tree = ast.parse(PARAMETRIC_PATH.read_text(encoding="utf-8"))
|
||||
fn = _function_node(parametric_tree, "refresh_post_commit")
|
||||
called_handler_attrs = {
|
||||
node.func.attr
|
||||
for node in ast.walk(fn)
|
||||
if isinstance(node, ast.Call)
|
||||
and isinstance(node.func, ast.Attribute)
|
||||
and isinstance(node.func.value, ast.Attribute)
|
||||
and node.func.value.attr == "handler"
|
||||
}
|
||||
assert (
|
||||
"refresh_bim_tool_headers" in called_handler_attrs
|
||||
), "refresh_post_commit must call bonsai.bim.handler.refresh_bim_tool_headers"
|
||||
assert "update_bim_tool_props" not in called_handler_attrs, (
|
||||
"refresh_post_commit must not call update_bim_tool_props " "(re-targets user-intent enums on every commit)"
|
||||
found_gated_call = False
|
||||
for node in ast.walk(fn):
|
||||
if not isinstance(node, ast.If):
|
||||
continue
|
||||
references_registry = any(
|
||||
isinstance(sub, ast.Attribute) and sub.attr == "EDIT_TYPES" for sub in ast.walk(node.test)
|
||||
)
|
||||
if not references_registry:
|
||||
continue
|
||||
for body_node in ast.walk(node):
|
||||
if (
|
||||
isinstance(body_node, ast.Call)
|
||||
and isinstance(body_node.func, ast.Attribute)
|
||||
and body_node.func.attr == "refresh_bim_tool_headers"
|
||||
):
|
||||
found_gated_call = True
|
||||
break
|
||||
if found_gated_call:
|
||||
break
|
||||
assert found_gated_call, (
|
||||
"tool.Parametric.refresh_post_commit must gate refresh_bim_tool_headers on an "
|
||||
"If whose test references EDIT_TYPES (the parametric registry). An ungated call "
|
||||
"fires the refresh for commits in contexts that strip view-layer attributes; "
|
||||
"a missing call silently drops the validate-gizmo header refresh."
|
||||
)
|
||||
|
||||
@@ -0,0 +1,74 @@
|
||||
# Bonsai - OpenBIM Blender Add-on
|
||||
# Copyright (C) 2026
|
||||
#
|
||||
# This file is part of Bonsai.
|
||||
#
|
||||
# Bonsai is free software: you can redistribute it and/or modify
|
||||
# it under the terms of the GNU General Public License as published by
|
||||
# the Free Software Foundation, either version 3 of the License, or
|
||||
# (at your option) any later version.
|
||||
#
|
||||
# Bonsai is distributed in the hope that it will be useful,
|
||||
# but WITHOUT ANY WARRANTY; without even the implied warranty of
|
||||
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
|
||||
# GNU General Public License for more details.
|
||||
#
|
||||
# You should have received a copy of the GNU General Public License
|
||||
# along with Bonsai. If not, see <http://www.gnu.org/licenses/>.
|
||||
#
|
||||
# This file was generated with the assistance of an AI coding tool.
|
||||
|
||||
"""Restricted-context regression test for ``tool.Blender.get_active_object``.
|
||||
|
||||
Some Blender contexts (e.g. the C-side operator context handed to
|
||||
programmatically-invoked nested ``bpy.ops`` calls) lack the view-layer
|
||||
attributes a normal UI context exposes. The canonical accessor must
|
||||
return ``None`` in that case rather than ``AttributeError`` — otherwise
|
||||
every caller routed through it inherits the same crash class that
|
||||
originally broke ``bpy.ops.bim.new_project(preset='demo')``."""
|
||||
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
pytestmark = pytest.mark.model
|
||||
|
||||
|
||||
class _RestrictedContext:
|
||||
"""Stand-in for a ``bpy.context`` stripped of view-layer attributes."""
|
||||
|
||||
def __getattr__(self, name):
|
||||
raise AttributeError(name)
|
||||
|
||||
|
||||
def test_get_active_object_returns_none_in_restricted_context():
|
||||
"""``tool.Blender.get_active_object`` is the canonical defensive
|
||||
accessor. Both the primary read (``bpy.context.active_object``) and
|
||||
the fallback (``bpy.context.view_layer.objects.active``) must
|
||||
tolerate a stripped context — otherwise the 150+ callers in the
|
||||
codebase that route through this helper inherit the crash."""
|
||||
import bonsai.tool.blender as blender_tool
|
||||
|
||||
with patch.object(blender_tool, "bpy") as bpy_patch:
|
||||
bpy_patch.context = _RestrictedContext()
|
||||
assert blender_tool.Blender.get_active_object() is None
|
||||
|
||||
|
||||
def test_property_header_tools_whitelists_bim_tool_family_only():
|
||||
"""``tool.Blender.get_property_header_tools`` gates the validate-
|
||||
gizmo header refresh. Parametric ``BimTool`` subclasses and the
|
||||
base ``BimTool`` itself must be included; ``AnnotationTool`` and
|
||||
workspace tools outside the ``BimTool`` family (spatial, structural,
|
||||
etc.) must not — they don't surface these header floats."""
|
||||
import bonsai.tool as tool_
|
||||
|
||||
# Ensure the lru_cache picks up subclasses registered by the
|
||||
# current Blender session (idempotent if already populated).
|
||||
tool_.Blender.get_property_header_tools.cache_clear()
|
||||
headers = tool_.Blender.get_property_header_tools()
|
||||
|
||||
assert "bim.bim_tool" in headers, "base BimTool must surface property headers"
|
||||
assert "bim.wall_tool" in headers, "parametric BimTool subclass must surface property headers"
|
||||
assert "bim.annotation_tool" not in headers, "AnnotationTool is not a BimTool subclass — no header surface"
|
||||
assert "bim.spatial_tool" not in headers, "SpatialTool is not BimTool-derived"
|
||||
assert "bim.structural_tool" not in headers, "StructuralTool is not BimTool-derived"
|
||||
Reference in New Issue
Block a user