fix(ViewRange): prevent duplicate windows, close crash, and invalid values#3240
fix(ViewRange): prevent duplicate windows, close crash, and invalid values#3240
Conversation
|
@jmcouffin big day for you, can I get some copilot eyes here, Tested on Revit 2026 and 2025 and it works great. |
There was a problem hiding this comment.
PR Summary:
- Fixes Revit crash when the ViewRange window is closed (DC3D server race condition)
- Prevents duplicate windows via envvar guard +
script.exit()on repeated clicks - Changes engine config from
clean: true→clean: falseto prevent IronPython engine disposal crashes - Pre-validates view range before
SetViewRangeand surfaces errors via per-field highlighting and a styled warning banner in XAML
Review Summary:
Overall this is a solid fix for a real crash and duplicate-window problem. The DC3D server swap pattern (remove → set meshes → re-add) is correct and the deferred close cleanup via execute_in_revit_context is the right approach. The new warning banner and field error highlighting are clean additions.
A few issues found:
Critical pattern error in transaction handling: The SetViewRange exception is caught inside the with revit.Transaction(...) block, so Transaction.__exit__ sees no exception, calls Commit(), and adds a ghost no-op entry to the Undo stack. The exception must propagate out of the with block for the rollback to occur.
Envvar race condition: VIEWRANGE_WINDOW_KEY is cleared synchronously in window_closed before the deferred _on_close_cleanup ExternalEvent fires — the exact race window this PR is trying to close. The key should only be cleared inside _on_close_cleanup.
Dual validators disagree on units: _validate_view_range_order computes elevations in display units with zero tolerance, while _find_elevation_violations works in internal feet with a 0.001 ft tolerance — they can produce contradictory results for the same input.
clear_warning() leaks foreground color: Does not reset warning_fg, so after a success banner the next clear_warning() leaves _warning_fg at green until the next error call overrides it.
Black formatting (repo guideline): The PR checklist explicitly notes Black was not run. The new class-level SolidColorBrush constant block (lines 683–706) and the redundant double-parentheses show_error((…)) pattern throughout are non-compliant with Black formatting, which is a repo requirement.
Suggestions
- Run
pipenv run black script.pyon the file to fix the Black formatting violations flagged in the checklist. Apply - Consolidate
_validate_view_range_orderand_find_elevation_violationsinto a single validation method using internal Revit units throughout. Apply
⚡ Apply all quick fixes (4 quick fixes)
...RevitTools.extension/pyRevit.tab/Toggles.panel/toggles3.stack/ViewRange.pushbutton/script.py
Outdated
Show resolved
Hide resolved
...RevitTools.extension/pyRevit.tab/Toggles.panel/toggles3.stack/ViewRange.pushbutton/script.py
Outdated
Show resolved
Hide resolved
...RevitTools.extension/pyRevit.tab/Toggles.panel/toggles3.stack/ViewRange.pushbutton/script.py
Show resolved
Hide resolved
...RevitTools.extension/pyRevit.tab/Toggles.panel/toggles3.stack/ViewRange.pushbutton/script.py
Show resolved
Hide resolved
...RevitTools.extension/pyRevit.tab/Toggles.panel/toggles3.stack/ViewRange.pushbutton/script.py
Outdated
Show resolved
Hide resolved
…ange errors (#3205) - Add early envvar guard to prevent duplicate DC3D servers - Change engine config to clean:false to prevent engine disposal crashes - Remove server before clearing meshes (prevents GetBoundingBox null crash) - Re-add server only after meshes are populated - Defer all close cleanup to Revit API context via ExternalEvent - Pre-validate view range with single validator using internal units - Add warning banner with field-level error highlighting in XAML - Address devloai review: fix transaction rollback pattern, envvar race, consolidate validators, fix clear_warning fg leak, remove constant aliases
925b381 to
95361ed
Compare
|
@devloai , your job is to review the latest commit after resolving your concerns |
There was a problem hiding this comment.
All 5 concerns from the previous review have been fully addressed in this latest commit:
| # | Issue | Status |
|---|---|---|
| 1 | Ghost transaction on SetViewRange failure |
✅ Fixed — try/except now wraps the entire with revit.Transaction(...) block externally, so exceptions reach __exit__ and trigger proper rollback |
| 2 | Race condition: VIEWRANGE_WINDOW_KEY cleared before deferred cleanup |
✅ Fixed — envvar cleared as last step in _on_close_cleanup, blocking re-entry until the server is fully removed |
| 3 | Dual validators with inconsistent unit bases | ✅ Fixed — _validate_view_range_order removed entirely; single _find_elevation_violations operates on the assembled view_range in internal units |
| 4 | clear_warning() leaking _warning_fg state after show_success() |
✅ Fixed — clear_warning() now explicitly resets _warning_fg = self._ERROR_BANNER_FG and fires the binding |
| 5 | Redundant constant aliases | ✅ Fixed — single definition at lines 11–12, used consistently |
The overall approach is solid: pre-validation in memory before the transaction, deferred cleanup via ExternalEvent, server remove/add bracketing the mesh swap, and the clean: false engine config to prevent IronPython engine disposal crashes. LGTM 🎉
|
📦 New work-in-progress (wip) builds are available for 6.2.0.26091+2016-wip |
|
📦 New public release are available for 6.2.0.26090+1754 |
fix(ViewRange): prevent duplicate windows, close crash, and invalid range errors (#3205)
Description
Fixes the Show View Range tool crashing Revit when the window is closed, and prevents duplicate windows from being opened when the button is clicked multiple times. The crash was caused by the DirectContext3D server's GetBoundingBox() returning null to .NET when meshes were cleared while the server was still registered — a race condition on the Draw Thread. The fix ensures the server is always unregistered before meshes are cleared and only re-registered after new meshes are populated. Additionally, the engine config is changed from clean: true to clean: false to prevent IronPython engine disposal from corrupting shared CLR state on repeated clicks. Invalid view range inputs are now caught before the transaction and surfaced to the user through per-field error highlighting and a warning banner instead of producing unhandled transaction rollback errors in the log.
Checklist
Before submitting your pull request, ensure the following requirements are met:
pipenv run black {source_file_or_directory}Related Issues
If applicable, link the issues resolved by this pull request:
Thank you for contributing to pyRevit! 🎉