[C# loader] Layout allowlist + F1 help parity with Python loader#3111
Conversation
|
@jmcouffin rather than creating a new issue for the yaml parsing logic, I'm creating a PR for the "missing" logic I have encountered in 6.1.0. I hope this is not just redoing something @romangolev has worked on already. The logic for adding bundles not listed in [layout] to UI can be discussed, as the change in the new loader might have been intentional by roman/others. The old loader did not add bundles that where not listed in layout to UI, which might be used by some develppers to hide bundles they are working on or just want to implement later (at least thats what we do, as they are nive to have as "archived" pushbuttons). It can be solved on the extension side rather than changing the parsing logic, users just have to remove/rename the bundles to not be added in UI. |
There was a problem hiding this comment.
Pull request overview
This PR brings the C# extension parser (6.x) into parity with the Python-based loader by implementing two key behavioral changes: making the layout field a strict allowlist (only items in layout are shown, unlisted items are no longer appended), and adding F1 help functionality including auto-discovery of help.html files, relative path resolution, and template substitution for bundle help_url scalars.
Changes:
- Layout behavior changed from appending unlisted children to strict allowlist matching Python loader's
__iter__behavior - Added help file auto-discovery for files matching
.*help\..+pattern when no help_url is specified - Added template substitution for bundle scalar help_url (e.g.,
{{docpath}}/help.html) - Added relative path resolution that converts local help files to file:// URLs for Revit's contextual help
| if (string.IsNullOrEmpty(fileName) || fileName.IndexOf("help.", StringComparison.OrdinalIgnoreCase) < 0) | ||
| continue; | ||
| var ext = Path.GetExtension(fileName); | ||
| if (string.IsNullOrEmpty(ext) || ext.Length < 2) | ||
| continue; | ||
| // Must be "help.<something>" (e.g. help.html) | ||
| var nameWithoutExt = Path.GetFileNameWithoutExtension(fileName); | ||
| if (nameWithoutExt.EndsWith("help", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
The help file pattern matching doesn't correctly implement the Python regex pattern .*help\..+. The current logic requires "help." to be present in the filename (line 1381) and then checks if the name without extension ends with "help" (line 1388). This would incorrectly reject files like "myhelp.html" which should match the pattern (anything ending with "help" before the extension dot). Consider using a regex pattern match or revising the logic to: check if nameWithoutExt ends with "help" (case-insensitive) AND ensure there's a valid extension.
| catch | ||
| { |
There was a problem hiding this comment.
The exception handling here silently swallows all exceptions without logging. This could hide issues with file path processing. Consider logging the exception with LogWarning to help with debugging, similar to the pattern used in DiscoverHelpFileInBundleDirectory (line 1396).
| catch | |
| { | |
| catch (Exception ex) | |
| { | |
| LogWarning($"Error converting path to file URI for '{absolutePath}': {ex.Message}"); |
| // Determine final help URL: bundle helpurl takes precedence over script helpurl. | ||
| // Apply template substitution to bundle scalar help_url (parity with Python loader). | ||
| var bundleHelpUrlRaw = bundleInComponent?.HelpUrl; | ||
| var bundleHelpUrlSubstituted = SubstituteTemplates(bundleHelpUrlRaw, mergedTemplates); | ||
| string finalHelpUrl = !string.IsNullOrEmpty(bundleHelpUrlSubstituted) | ||
| ? bundleHelpUrlSubstituted | ||
| : scriptHelpUrl; | ||
| // Auto-discover help file in bundle directory when no help_url set (parity with Python loader). | ||
| if (string.IsNullOrEmpty(finalHelpUrl)) | ||
| { | ||
| finalHelpUrl = DiscoverHelpFileInBundleDirectory(dir); | ||
| } | ||
| // Resolve relative help_url (e.g. "help.html") against bundle directory to file:// URL. | ||
| if (!string.IsNullOrEmpty(finalHelpUrl)) | ||
| { | ||
| finalHelpUrl = ResolveHelpUrlIfRelative(finalHelpUrl, dir); | ||
| } |
There was a problem hiding this comment.
The new help file auto-discovery feature (DiscoverHelpFileInBundleDirectory) and relative path resolution (ResolveHelpUrlIfRelative) lack test coverage. Consider adding tests to verify: 1) help.html is discovered in bundle directory, 2) relative paths like "help.html" are resolved to file:// URLs, 3) template substitution works for bundle help_url (e.g., "{{docpath}}/help.html"), and 4) the pattern matching correctly identifies help files matching .*help..+ regex.
| /// <summary> | ||
| /// Discovers a help file in the bundle directory (parity with Python loader). | ||
| /// Matches pattern .*help\..+ (e.g. help.html, help.pdf, myhelp.html). |
There was a problem hiding this comment.
The documentation comment states this matches "myhelp.html" as an example, but the implementation at line 1381 would reject this file because it requires "help." (with a dot immediately after "help") to be present in the filename. The file "myhelp.html" contains "help" but not "help." so it would be skipped. This documentation should be corrected to match the actual behavior, or the implementation should be fixed to match the Python regex pattern .*help..+
|
@jmcouffin was the edits in ExtensionParser.cs in this PR merged to develop branch? It seems to me they are missing? (It might be my mistake, but looking in the files here on GH, I cannot see the changes) |
C# loader: parity with Python loader — layout allowlist and F1 help
Description
This PR brings the C# extension parser (6.x) in line with the Python-based loader for two behaviors:
Layout as strict allowlist
layoutinbundle.yamlis now treated as an allowlist: only items listed inlayoutare shown. Children not in the list are no longer appended at the end. This matches the Python loader (genericcomps.py), where__iter__only yields layout-listed components.F1 help (help.html and relative help_url)
help_urlis run throughSubstituteTemplates, so values like{{docpath}}/help.htmlwork when templates are defined.help_urlis set in YAML or script, the bundle directory is scanned for a help file matching.*help\..+(e.g.help.html). If found, itsfile://URL is used for F1.help_urlis a relative path (e.g.help.html), it is resolved against the bundle directory and converted to afile://URL so F1 opens the local file.Files changed:
dev/pyRevitLoader/pyRevitExtensionParser/ExtensionParser.csReorderByLayout: removed appending of unlisted children.file://.DiscoverHelpFileInBundleDirectory,PathToFileUri,ResolveHelpUrlIfRelative.Checklist
Before submitting your pull request, ensure the following requirements are met:
(
dotnet test dev\pyRevitLoader\pyRevitExtensionParserTester\pyRevitExtensionParserTest.csproj— 179 tests pass; one existing failure is unrelated (Dynamo custom script name).)Related Issues
If applicable, link the issues resolved by this pull request:
(Add the issue number when you open or link the issue from
docs/ISSUE_LOADER_BEHAVIOR.md.)Additional Notes
docs/ISSUE_LOADER_BEHAVIOR.md(layout filter, F1 help, bundle filename, scalar help_url substitution, etc.). This PR implements the two main parity fixes (layout + F1 help).*.bundle.yamland/or run scalarhelp_urlthrough templates in additional code paths if desired.Thank you for contributing to pyRevit!