Decouple extension UI layout from folder structure#3386
Conversation
When using custom layouts stored in AppData, adding a tool that wasn't in the original layout caused "Revit cannot run available command" errors. The assembly DLL only contained command types for layout-referenced tools, and changes to custom layout files outside the extension directory didn't invalidate the hash-based cache. Both C# and Python parsers now track which tools from the tool index are referenced by the layout and compile unreferenced tools as assembly-only components. The layout controls ribbon UI placement only — all discovered tools have compiled command types available in the cached assembly.
Visual Studio's WPF designer generates *_wpftmp.csproj files during build that should never be committed. Remove the accidentally-tracked file and add a gitignore rule.
Reformats the 7 fully-new files introduced by this feature so they comply with the project's recommended black style. Existing files modified by this branch are intentionally left untouched: upstream develop is not black-formatted, and reformatting them here would churn hundreds of unrelated lines.
Replace hardcoded '.extension', '.tab', '.panel', '.stack', and 'tools' string literals with the existing constants in pyrevit.extensions (UI_EXTENSION_POSTFIX, TAB_POSTFIX, PANEL_POSTFIX, STACK_BUTTON_POSTFIX, TOOLS_DIR_NAME).
Replace the ~80-line hand-rolled YAML serializer in layout_cli.py (serialize_layout_yaml, _yaml_dump, _yaml_scalar, _write_panel_yaml) with the existing pyrevit.coreutils.yaml.dump_dict wrapper around YamlDotNet. The Layout Builder pushbutton script is updated to call dump_dict directly instead of importing the removed helper. Also normalizes the 3 existing extension_layout.yaml files (Core, Tools, Templates) to a canonical unquoted-scalar block style, since they were previously hand-authored with always-quoted strings. Final output style is verified at runtime against YamlDotNet; minor touch-ups may be needed after first in-Revit regeneration.
- layout_parser.py: drop unused `coreutils`, `GenericUIContainer`, `LayoutItem` imports. - layout_cli.py: drop unused `os` import; remove the `stack_buffer` list in `_build_panel_layout` along with its flush blocks (the buffer was initialized and cleared but never appended to). - Layout Builder/script.py: drop unused function-local `Windows` import; drop top-level `codecs` import (no longer needed after the yaml refactor). - Test Layout Parsing/script.py: drop unused `Tab`, `Panel`, `GenericStack` imports.
Replace the cross-module write to the private _assembly_only_commands attribute from layout_parser with a public setter on Extension. Keeps the underscore-prefixed storage internal to the class while giving the layout parser a clean entry point.
- parser.get_parsed_extension: remove the try/except that hid ImportError from the layout_parser/toolindex imports. The imports are unconditional and any failure was masquerading as "use legacy mode", which would silently disable the new feature for everyone. - layout_parser.get_layout_file: log the swallowed exception at debug level instead of pass. - Layout Builder _get_custom_layout_path: same; add a module-level mlogger via script.get_logger().
Move the function-local imports of Tab, Panel, and GenericStack from inside _create_tab, _create_panel, and _create_stack to the module header. components.py does not import from layout_parser/toolindex/ layout_cli, so there is no circular-import risk. The user_config import inside get_layout_file is intentionally left function-local since user_config performs config-file I/O on import.
- toolindex.py: collapse the Args/Returns docstrings on the two trivial helpers (_get_extension_name, _get_dir_extension) into a single line. - layout_parser.py: collapse the Args/Returns block on _create_tab into a single line. - Layout Builder/script.py: remove the four inner banner comments (# -- properties exposed to WPF binding --, etc.) that only restate what the method group does. The outer section banners are kept for navigation in this 700-line file. - userconfig.py: add docstring on disable_custom_layouts setter. - PyRevitConfig.cs: drop the noisy extra blank line introduced by the branch (matches upstream spacing). Project convention per CLAUDE.md is to favor WHY-comments over WHAT-comments; the C# XML doc comments on private helpers are kept because they match the surrounding C# style.
The method's accessibility is `internal`, not `public`, so its name was misleading. Updates both call sites in LayoutParser.cs.
Black cleanup after removing the hand-rolled YAML serializer left two trailing blank lines at end of file.
…re-0x5KJ Claude/pr checklist new feature 0x5 kj
…r an extension. plus cleanup and documentation for PR
…sley/pyRevit-fork into feature/decouple-layout
…ypgdQ Add dark mode icon for Layout Builder tool
- layout_parser.py / LayoutParser.cs: match separator (---) and slideout (>>>) by equality instead of substring so tool names containing those tokens are not misclassified. - parser.py: always build tool index so hybrid mode (layout YAML + legacy .tab/.panel tools) sees the legacy tree. - components.py: filter set_assembly_only_commands to GenericUICommand so non-command components are not handed to the assembly builder. - LayoutParser.cs CreateStack: derive stack uniqueId from a per-panel counter to prevent collisions when concatenated child names match (e.g. ["A","BC"] vs ["AB","C"]). - ExtensionParser.cs ParseSingleComponent: emit the InheritIcon, LargeIcon, and HelpFile fields the duplicated logic was missing. - PyRevitConfig.cs DisableCustomLayouts: parse with TryParseConfigBool to accept the same boolean variants as LoadBeta and friends. - Settings.smartbutton import_layout_for_ext: stage to a temp dir and swap so a mid-copy failure cannot leave the user without a layout but with a config still pointing at one. - Settings.smartbutton: drop redundant PYREVIT_APP_DIR redefinition and reuse the shared pyrevit constant. - Layout Builder + dev tool scripts: discover extensions via user_config.get_ext_root_dirs() instead of walking dirname(__file__), so user-installed extensions are visible.
refactor: collapse ParseComponents duplication into shared ParseSingleBundle helper, fixing bundle helpurl template substitution along the way
Claude/review pr feedback 6d jz9
There was a problem hiding this comment.
PR Summary:
Introduces an optional YAML-based layout system that decouples ribbon UI structure from the on-disk folder hierarchy. Extensions can declare tabs/panels/tools in extension_layout.yaml while tools live in a flat tools/ directory. Fully backward-compatible — legacy extensions are untouched. Includes a WPF Layout Builder tool, per-extension custom layout import/export in Settings, C# loader parity via LayoutParser.cs, and migrations of core bundled extensions.
Review Summary:
The overall architecture is well-thought-out and the backward-compatibility story is solid. The Python-side layout parser and tool indexer are clean and well-documented. The C# LayoutParser correctly mirrors the Python logic with appropriate logging.
Two higher-severity issues were found: (1) the Layout Builder script runs its entire entry-point (extension discovery, SelectFromList dialog, build_tool_index, window creation) at module level — a known anti-pattern in pyRevit's IronPython engine where module-level code persists between executions; (2) the layout cache directory is computed differently in the Layout Builder (%APPDATA%\pyRevit\Layouts\) vs the Settings window (PYREVIT_APP_DIR\Layouts\), which will silently diverge on non-standard installations, causing the builder to write layouts to a location Settings never reads. A latent circular import between toolindex.py and parser.py was also flagged — currently safe due to lazy import ordering but fragile. On the C# side, HasLayoutFile and GetLayoutFilePath duplicate their config-lookup logic independently, and ParseLayout has a null-coalescing fallback to a hard-coded filename after the tool index is already built. Exception handling in the Settings import callback re-raises without a user-visible alert.
Suggestions
- Consolidate the layout cache directory path into a single shared utility function (e.g. in
layout_parser.py) imported by bothLayout BuilderandSettingsscripts to eliminate the divergent path computation. Apply - Move the module-level entry-point logic in
Layout Builder.pushbutton/script.py(lines 687–738) into amain()function guarded byif __name__ == '__main__':to follow the pyRevit IronPython anti-stale-global pattern. Apply
| # Discover every UI extension across all configured pyRevit extension | ||
| # roots (shipped + user-installed) so the picker isn't limited to the | ||
| # folder this script happens to live in. | ||
| from pyrevit.userconfig import user_config |
There was a problem hiding this comment.
Module-level code and stale user_config reference at module level
Lines 687–738 execute at module level (outside any function or class). In pyRevit's IronPython engine, module-level code persists between script executions within the same session. This means:
-
user_configis captured at module level (from pyrevit.userconfig import user_configat line 687), which is fine as a reference, but the entire extension-discovery loop,forms.SelectFromList.show,build_tool_index, andload_layout_treecalls at lines 689–738 also run at import time on every re-execution — including when Revit re-uses the cached module. The WPF dialog will pop up again on subsequent executions, but theext_dirs/seenvariables are re-initialized (since they're re-assigned), so the dialog flow still works. However this is an anti-pattern. -
More critically:
from pyrevit.userconfig import user_configat line 687 is module-level global state per the IronPython patterns checklist — ifuser_configis replaced or reloaded between executions, the cached reference becomes stale.
Move the extension-discovery and dialog logic into the __main__ guard or a dedicated main() function:
def main():
from pyrevit.userconfig import user_config
ext_dirs = []
seen = set()
for root_dir in user_config.get_ext_root_dirs():
...
# rest of the script
window = LayoutBuilderWindow(selected_dir, selected_name, tool_index, roots)
window.show_dialog()
if __name__ == '__main__':
main()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.
| # Custom layout storage | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| APPDATA_DIR = os.environ.get("APPDATA", op.expanduser("~")) |
There was a problem hiding this comment.
Cache-dir path is computed differently here vs Settings.smartbutton
_get_layout_cache_dir builds the path as:
op.join(APPDATA_DIR, "pyRevit", "Layouts", extension_name)where APPDATA_DIR = os.environ.get("APPDATA", op.expanduser("~")).
LayoutExtensionItem._get_cache_dir in Settings.smartbutton/script.py (line 106) builds it as:
op.join(PYREVIT_APP_DIR, 'Layouts', self.Name)where PYREVIT_APP_DIR comes from pyrevit.PYREVIT_APP_DIR.
These may diverge if PYREVIT_APP_DIR is not %APPDATA%\pyRevit (e.g., on non-standard installs). Use the same canonical source for both. Prefer the PYREVIT_APP_DIR constant (imported from pyrevit) rather than rolling your own APPDATA + "/pyRevit" construct.
from pyrevit import PYREVIT_APP_DIR
def _get_layout_cache_dir(extension_name):
return op.join(PYREVIT_APP_DIR, "Layouts", extension_name)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.
| return false; | ||
|
|
||
| // Check custom layout path in config first | ||
| var extName = Path.GetFileNameWithoutExtension(extensionDir); |
There was a problem hiding this comment.
HasLayoutFile returns true for a configured custom path even if the file doesn't exist
var customPath = config?.GetCustomLayoutPath(extName);
if (!string.IsNullOrEmpty(customPath))
return true;GetCustomLayoutPath already validates File.Exists(value) and returns null if the file is absent — so this is currently safe only because of that guard. But HasLayoutFile does a second config lookup independently of GetLayoutFilePath, which also calls GetCustomLayoutPath. If someone calls HasLayoutFile without calling GetLayoutFilePath (e.g., in a future path), and the custom-path validation changes, this logic could return true while GetLayoutFilePath returns null (the bundled path), silently enabling layout mode with the wrong file.
Prefer calling GetLayoutFilePath internally:
public static bool HasLayoutFile(string extensionDir)
{
return GetLayoutFilePath(extensionDir) != null;
}This also eliminates the duplicated config-access logic between the two methods.
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.
| slideout.name = exts.SLIDEOUT_IDENTIFIER | ||
| parent.add_component(slideout) | ||
| return | ||
|
|
There was a problem hiding this comment.
Double blank line between guard clauses inside function — Black formatting violation
Lines 363–365 contain two blank lines between the slideout return and the tool-lookup if block. Black only permits double blank lines between top-level definitions, not inside functions. Running black will collapse this to a single blank line.
if tool_name == exts.SLIDEOUT_IDENTIFIER:
slideout = GenericUIComponent()
slideout.type_id = exts.SLIDEOUT_IDENTIFIER
slideout.name = exts.SLIDEOUT_IDENTIFIER
parent.add_component(slideout)
return
if tool_name in tool_index: # single blank line onlyactions
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.
| Dictionary<string, string> inheritedTemplates, | ||
| int revitYear) | ||
| { | ||
| // Resolve layout file (custom path > bundled) |
There was a problem hiding this comment.
ParseLayout falls back to a hard-coded LayoutFileName constant when GetLayoutFilePath returns null
var layoutPath = GetLayoutFilePath(extensionDir)
?? Path.Combine(extensionDir, LayoutFileName);GetLayoutFilePath already returns the bundled extension_layout.yaml path if it exists, and null if neither the custom path nor the bundled file exists. The null-coalescing fallback here means ParseLayout will try to LoadYaml a non-existent file and silently return an empty LayoutParseResult — but only after it has already built the (potentially large) tool index.
This is a latent issue: ParseLayout is only called from ParseExtension after HasLayoutFile returns true, so in practice the file exists. But the logic is fragile. The ?? Path.Combine(...) fallback should either be removed (trust that the caller checked HasLayoutFile) or guard-failed early:
var layoutPath = GetLayoutFilePath(extensionDir);
if (layoutPath == null)
{
LogWarning($"Layout: No layout file found for {extensionDir}");
return new LayoutParseResult();
}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.
| if op.isdir(cache_dir): | ||
| shutil.rmtree(cache_dir) | ||
| shutil.move(tmp_dir, cache_dir) | ||
| except Exception: |
There was a problem hiding this comment.
Silent re-raise inside import_layout_for_ext swallows the error from the user's perspective
except Exception:
shutil.rmtree(tmp_dir, ignore_errors=True)
raiseThe bare except Exception: ... raise is fine for cleanup, but there is no outer handler in this event callback — the raise will propagate out of the WPF event handler with no user-visible message. The user will see the dialog freeze or silently fail.
Wrap the outer call with a user-friendly alert:
except Exception as ex:
shutil.rmtree(tmp_dir, ignore_errors=True)
mlogger.error("Failed to import layout: %s", ex)
forms.alert("Failed to import layout files:\n{}".format(ex))
returnThis also requires changing except Exception: → except Exception as ex:.
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.
| from pyrevit import coreutils | ||
| from pyrevit.coreutils.logger import get_logger | ||
| import pyrevit.extensions as exts | ||
| from pyrevit.extensions.parser import ( |
There was a problem hiding this comment.
Circular import: toolindex.py imports private functions from parser.py, which itself imports from toolindex.py (via layout_parser.py)
Import chain:
parser.py→layout_parser.py(lazy, insideget_parsed_extension) →toolindex.pytoolindex.py→parser.py(_parse_for_components,_create_subcomponents,_get_subcomponents_classes) ← at module level
The import in toolindex.py at lines 14–18 is a top-level import of three private (_-prefixed) functions from parser.py. Since parser.py itself lazy-imports toolindex inside a function, the circular dependency is not triggered at startup — but this is fragile and relies on the lazy-import order being preserved. Any future refactoring that hoists the import in parser.py to module level will cause an import error.
Consider either:
- Moving
_parse_for_components/_get_subcomponents_classesto a shared_parseutils.pymodule that neitherparser.pynortoolindex.pyimports from each other. - Keeping the import of
parserinside_index_tool(lazily), consistent with howlayout_parserhandles 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.
Description
This PR introduces an optional YAML-based layout system that separates an extension's UI layout (tabs, panels, button arrangement) from its on-disk folder structure. Extensions can now declare their ribbon layout in
extension_layout.yamlinstead of encoding it in nested.tab/.panel/.stack/directories. Tool bundles can live in a flattools/directory, and admins or end users can override the bundled layout with a custom one — without forking the extension.The system is fully backward-compatible: extensions with no
extension_layout.yamlcontinue to load via the existing folder-walking parser, unchanged. Extensions that opt in can migrate incrementally —tools/and legacy.tab/.panel/directories are both scanned, so tools can be moved over piecemeal.Example layout
Included as optional presets in .extension/tools/

Layout Builder
What's in this PR
Python-side parser (
pyrevitlib/pyrevit/extensions/)layout_parser.py— readsextension_layout.yaml/*.panel.yaml, resolves tool references against a tool index, and builds the same component treeuimaker.pyalready consumes.toolindex.py— recursive scanner fortools/(and the legacy hierarchy) that produces a name → component dictionary. Plain subfolders intools/are treated as organizational only.layout_cli.py— programmatic API forgenerate_layout()(emit YAML mirroring an existing legacy structure) andlist_tools()(diagnostic listing).parser.py— dispatches to layout-based or legacy parsing based on whether a layout file is present.components.py—Extensionnow carries an_assembly_only_commandslist so unreferenced tools still get compiled into the DLL (see "assembly-only commands" below).userconfig.py— adds a globaldisable_custom_layoutstoggle.C# loader parser (
dev/pyRevitLoader/pyRevitExtensionParser/)LayoutParser.cs— mirrors the Python parser for the new-loader code path. Handles tool indexing, custom-layout-path resolution from the INI config, panel layout files, stacks, separators, and slideouts.ExtensionParser.cs— dispatches toLayoutParser.ParseLayoutwhen a layout file is detected; exposes a newParseSingleComponententry point so the layout path can reuse all existing bundle/script/icon parsing.ParsedExtension.cs— carriesAssemblyOnlyComponents; adds.extensionandtoolsto the directory-hash inputs so cache invalidation is correct under the new structure.PyRevitConfig.cs— readsdisable_custom_layouts(global) andcustom_layout_path(per extension, in[<name>.extension]).PyRevitConsts.cs— adds the matching config key constant.Layout YAML format
Tools are referenced by their folder name without the postfix (
MyTool.pushbutton→"MyTool"). Lookups are case-insensitive. Panel layouts can be inline or split out into<Name>.panel.yamlfiles alongsideextension_layout.yaml.Custom user layouts
%APPDATA%\pyRevit\Layouts\<ExtensionName>\.custom_layout_pathin the user INI; fall back to the bundled layout if absent or if the globaldisable_custom_layoutsis set.Custom/Defaultstatus column, and a global "Disable all custom layouts" checkbox.Layout Builder (new Core tool)
extensions/pyRevitCore.extension/tools/Layout Builder.pushbutton/— WPF visual editor. Two-pane UI: left side lists available tools with search and a "Show All / Hide Placed" toggle; right side is the tab/panel/stack/tool tree with add / move-up/down / remove buttons. Saves to the user's custom-layout cache directory and reloads pyRevit. Optional developer-shipped presets (<ext>/layouts/*.layout.yaml) appear via a "Load Preset" button when present.Dev tools (
extensions/pyRevitDevTools.extension/)Generate Layout.pushbutton— generates anextension_layout.yamlfrom any existing legacy extension as a starting point for migration. Shift-click splits panels into separate files.List Tools.pushbutton— diagnostic listing of all tools discoverable in an extension.Test Layout Parsing.pushbutton— exercises the parser for sanity checks during development.Migrated bundled extensions
pyRevitCore.extension— fully migrated: tools moved frompyRevit.tab/pyRevit.panel/*.stack/...to a flattools/directory, structural folders removed, layout declared inextension_layout.yaml. Adds the new "Layout Builder" button. The Settings smartbutton picks up new XAML for the Extension Layout section.pyRevitTools.extension— opts in withextension_layout.yamlonly, demonstrating the hybrid case where tools remain in the legacy folder structure but the ribbon is now layout-driven.pyRevitTemplates.extension— minimal example layout plusextension-layout-guide.mdandmigration-instructions.mdfor extension authors.Assembly-only commands
When layout mode is active, any tool found on disk but not referenced by the layout is still compiled into the extension assembly — it just doesn't appear in the ribbon. This was a deliberate design choice so that user-facing layout edits (via the Layout Builder or by hand) don't trigger a full assembly rebuild on the next reload: the command type is already available, the layout just decides whether to surface it. Python side:
Extension.set_assembly_only_commands()registers the unreferenced tools, andExtension.get_all_commands()merges them with UI commands for assembly compilation. C# side:ParsedExtension.AssemblyOnlyComponents, also folded intoCollectCommandComponents().Backward compatibility & safety
extension_layout.yamluse the existing parser unchanged — no behavioral change..tab/.panel/tree; both locations are scanned and merged into the tool index (tools/wins on duplicate name).<extensionname>_<toolname>, flat) vs. legacy path-based IDs. Cache directory hashing inputs were updated (.extensionandtoolsadded) so stale caches don't mask the new structure. Users with leftover compiled DLLs from older builds may need to clear%APPDATA%\pyRevit\<RevitYear>\*.dllonce — this is documented inmigration-instructions.md.Notes for reviewers
namefield onbundle.yaml, but the implementation instead derives the name from the folder name in all cases. I could go either way here. Open to feedback.extensions/pyRevitTemplates.extension/extension-layout-guide.mdandmigration-instructions.mdare the authoritative end-user docs.Layout Builderbutton is placed in the corepyRevittab rather than under Dev Tools because it's user-facing, not developer-only.Checklist
pipenv run black {source_file_or_directory}Related Issues
Additional Notes
bin/were rebuilt against the newLayoutParser.cs/ExtensionParser.csso the new loader can parse layout-mode extensions out of the box.ParsedExtension.cs(adding.extensionandtools) is required for layout-mode extensions but is also a strict improvement for legacy extensions — the previous hash inputs missed changes inside the extension root folder itself.