Conversation
Added T3LabLite extension with details including author, description, and URLs.
…t window Fixes #1729 The pyRevit output window hosts a WebBrowser control inside a WPF MetroWindow via WindowsFormsHost. Revit's message loop intercepts Ctrl+C via IOleInPlaceActiveObject.TranslateAccelerator before WPF, WinForms, IMessageFilter, or JavaScript events can see the keystroke. This adds a Win32 low-level keyboard hook (WH_KEYBOARD_LL) that intercepts Ctrl+C and Ctrl+A at the OS level, before Revit's accelerator table processes them. The hook only activates when the ScriptConsole is the active window with renderer focus, and forwards the keystrokes to HtmlDocument.ExecCommand for native browser copy. The hook is installed on window creation and disposed on close.
Add Ctrl+C and Ctrl+A keyboard shortcuts to ScriptConsole output window
Use core.set_option() instead of set_thirdparty_ext_root_dirs() when registering extension paths. This avoids the existence check in set_thirdparty_ext_root_dirs() which would fail for temporarily-offline network shares, defeating the purpose of reading the raw list. Fixes the issue identified in review: set_thirdparty_ext_root_dirs() raises PyRevitException for any path that doesn't currently exist on disk.
…egistered - Move CONSTS import outside try block so it's available for both the read and write paths regardless of whether an exception occurs. - Replace set_thirdparty_ext_root_dirs() with core.set_option() to avoid the existence-check rejection of temporarily-offline network shares when writing back the updated directory list. - Capture the read exception and log it at debug level before falling back to the helper, replacing the silent bare 'except Exception' swallow.
…ause-by-Copilot-Auto-Fix Fix Regressions cause by Copilot Auto Fix
SeedEnvironmentDictionary() was called before _pyRevitRoot was resolved,
so EnvDictionarySeeder.Seed() received an empty string for pyRevitRoot.
Both ReadPyRevitVersion() and ReadIPYVersion() returned "Unknown" because
the repo root path was empty, skipping the version file and IronPython.dll
lookup entirely.
Fix: eagerly resolve _pyRevitRoot from _binDir inside InitializeScriptExecutor(),
which runs immediately before SeedEnvironmentDictionary() in LoadSession().
The executing assembly lives at {pyRevitRoot}/bin/, so FindPyRevitRoot(_binDir)
walks up one level and finds pyrevitlib/. If it fails (non-standard layout),
_pyRevitRoot stays null and BuildSearchPaths() retries later with the extension
directory as an additional hint.
Log whether pyRevit root resolution succeeded or fell back to null, making it easier to diagnose version-display issues in the output window.
…3260) Add PyRevitRootResolutionTests to verify that FindPyRevitRoot correctly resolves the repo root from a bin/ subdirectory (via pyRevitfile marker or pyrevitlib/ directory), and that ReadPyRevitVersion/ReadIPYVersion return actual values (not 'Unknown') when given a valid root. End-to-end tests replicate the exact bug scenario: without early resolution of _pyRevitRoot in InitializeScriptExecutor(), the SeedEnvironmentDictionary() call receives an empty string, causing both version reads to short-circuit to 'Unknown'. Changes: - SessionManagerService.FindPyRevitRoot: private → internal (testable) - EnvDictionarySeeder.ReadPyRevitVersion: private → internal (testable) - New PyRevitRootResolutionTests.cs with 11 test cases - Updated .csproj to include the new test file
…-output fix: resolve pyRevit/IPY version showing Unknown in output window
…uard as a init has run check
ReadIPYVersion() had three bugs:
1. Searched wrong paths (bin/IPY342/) instead of the actual layout
(bin/{netcore|netfx}/engines/{IPY342|IPY2712PR}/)
2. Only checked for "IronPython.dll" — missing the IPY2712PR variant
named "pyRevitLabs.IronPython.dll"
3. Checked the executing assembly directory last instead of first,
reporting the wrong engine version when multiple engines are present
Fix: check the active engine directory first (Assembly.GetExecutingAssembly
sits alongside the active IronPython DLL), use correct bin/ layout for
fallback paths, check both DLL name variants, and add Trace.WriteLine
to the catch block for diagnosability.
…nknown fix: resolve IronPython version showing Unknown in output window
Bugfixes for sectionbox navigator
Update extensions.json - Add T3LabLite extension details
There was a problem hiding this comment.
PR Summary:
- Adds a
pushtrigger tomain.ymlso that signing secrets (unavailable for fork PRs) are available on the resulting push after merge - Adds a pre-signing step to validate all Azure Trusted Signing secrets are present
- Fixes
EnvDictionarySeeder.ReadIPYVersionto prioritize the active engine directory and support both IronPython DLL name variants - Adds regression tests for the pyRevit root resolution ordering bug (PR #3260)
- Section Box Navigator: switches from
Level.ElevationtoLevel.ProjectElevationand introducesupdate_grids_and_levels()helper - Extensions manager: reads raw config list to preserve offline network paths
Review Summary:
The core intent of the main.yml fix is sound — fork PRs don't have signing secrets, so a push trigger ensures the merge commit is signed. However, there are two workflow correctness issues: the new push trigger will fire a full duplicate pipeline run on every same-repo merge (once for pull_request: closed, once for push), and the actual signing steps lack the WipRun/ReleaseRun guard that the new secrets-validation step has, meaning every routine push to develop/master will consume Azure signing quota.
The Python changes are clean and follow IronPython patterns. The EnvDictionarySeeder improvements and new regression tests are well-structured. One fragile pattern in the Section Box Navigator (update_grids_and_levels capturing the module-level doc global in an event-driven context) and a misleading hasattr guard are flagged.
Suggestions
- Add a dedup guard to the
buildjob (e.g., skippushevents when apull_requestevent for the same commit already ran) to avoid double builds on internal merges. Apply - Apply the
WipRun/ReleaseRun/workflow_dispatchcondition to both signing steps so routinepushcommits don't consume Azure signing quota. Apply
⚡ Apply all quick fixes (3 quick fixes)
| if: > | ||
| github.repository == env.MainRepo && | ||
| (github.event_name != 'pull_request' || | ||
| github.event.pull_request.head.repo.full_name == github.repository) |
There was a problem hiding this comment.
The signing steps for DLLs/EXEs and installers are gated only on the repo identity and PR-from-fork check, but not on WipRun/ReleaseRun. This means every direct push to develop or master (even routine commits without a release intent) will attempt to sign artifacts — consuming Azure signing quota unnecessarily.
The upstream "Require Trusted Signing secrets" step already has the WipRun/ReleaseRun/push/workflow_dispatch guard, but the actual signing steps do not. Consider aligning with the same condition (minus the bare push clause, since signing every push is what we're trying to avoid):
if: >
github.repository == env.MainRepo &&
(github.event_name != 'pull_request' ||
github.event.pull_request.head.repo.full_name == github.repository) &&
(env.WipRun == 'true' || env.ReleaseRun == 'true' ||
github.event_name == 'workflow_dispatch')Same issue applies to the installer signing step at line 190.
actions
Quick Fix: Apply a small focused fix for this specific issue (takes < 1min).
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
| - 'release/**' | ||
| - 'site-packages/**' | ||
| # After merges (especially from forks), secrets are available on push but not on pull_request. | ||
| push: |
There was a problem hiding this comment.
Adding a push trigger on develop/master alongside the existing pull_request trigger creates duplicate CI runs for internal PRs (non-forks): the PR merge generates both a pull_request (type: closed) event and a push event, causing the entire build pipeline to run twice.
The build job condition github.event_name != 'pull_request' || github.event.action != 'closed' || github.event.pull_request.merged already passes for the push event unconditionally — so there's no filtering that avoids the duplication.
A common mitigation is to skip the push event when it was triggered by a PR merge commit, e.g.:
jobs:
build:
if: >
github.event_name == 'workflow_dispatch' ||
(github.event_name == 'push' && github.event.head_commit.author.name != 'GitHub Actions') ||
(github.event_name == 'pull_request' && (github.event.action != 'closed' || github.event.pull_request.merged))Or alternatively, only use push for fork-originating commits (detect via github.event.head_commit not having a pull_request origin) and skip when a matching pull_request event already handled it.
actions
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
|
|
||
| def update_grids_and_levels(self): | ||
| """Updates grids and levels""" | ||
| self.all_levels = get_all_levels(doc, self.chkIncludeLinks.IsChecked) |
There was a problem hiding this comment.
update_grids_and_levels() captures the module-level doc variable at call time. When this method is invoked from the on_view_or_doc_changed event handler after initialize_globals() has refreshed doc to the new document, this is fine. But if it's ever called from a Dispatcher-posted callback on a different thread before the module-level doc is updated (e.g., race between the event handler and the Dispatcher), the stale doc reference will be used.
More concretely: on_view_or_doc_changed first calls initialize_globals() (updates doc), then posts update_grids_and_levels via Dispatcher.Invoke. Since Invoke is synchronous, this is safe today. However, the pattern is fragile — update_grids_and_levels should explicitly receive the document it operates on rather than capturing the global:
def update_grids_and_levels(self, target_doc=None):
"""Updates grids and levels"""
target_doc = target_doc or doc
self.all_levels = get_all_levels(target_doc, self.chkIncludeLinks.IsChecked)
self.all_grids = get_all_grids(target_doc, self.chkIncludeLinks.IsChecked)This matches the pyRevit IronPython guideline: avoid relying on module-level document references in callbacks.
actions
Quick Fix: Apply a small focused fix for this specific issue (takes < 1min).
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
| try: | ||
| # I assume the event handler is sometimes faster than the init, causing a initial error message | ||
| # therefore safeguard with: | ||
| if not hasattr(self, "current_view"): |
There was a problem hiding this comment.
The hasattr(self, "current_view") guard is unnecessary and misleading. The event handler on_view_or_doc_changed already gates on if sb_form (line 177), and sb_form is only assigned after SectionBoxNavigatorForm.__init__() fully completes (line 1984). So update_info() can never be invoked during __init__ via the event pathway.
Furthermore, self.current_view is unconditionally set at line 204 — before update_grids_and_levels() at line 208 — so it is always present by the time the form is shown.
A more meaningful early-exit guard would check the attributes that are actually None-initialized during construction:
def update_info(self):
"""Update the information display."""
try:
if self.all_levels is None or self.all_grids is None:
return # Not yet fully initialized
last_view = self.current_view.Idactions
Quick Fix: Apply a small focused fix for this specific issue (takes < 1min).
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
|
📦 New public release are available for 6.3.0.26095+0830 |
No description provided.