Fix/reload smartbutton#3476
Conversation
On reload, UpdateExistingChildren in PulldownButtonBuilder and SplitButtonBuilder re-applied bundle icons via ButtonPostProcessor.Process but never re-ran __selfinit__. SmartButtons nested in pulldowns and split buttons therefore lost any icon/state set by their init scripts, and icons set in a previous session could linger out of sync with current state (e.g. the Update button staying orange after updating). Top-level, slide-out, and stacked SmartButtons were unaffected — PushButtonBuilder and StackBuilder already re-ran __selfinit__ on their update paths. Note: this was originally reported as a stale IconManager BitmapCache, but the cache is not the cause — IconManager (and its cache) is recreated on every LoadSession, so no bitmaps survive a reload. Also: - Update.smartbutton: a failed update-availability check no longer deactivates the button (it only skips the icon swap and logs at debug); only the user_can_update gate returns False. Previously any transient git/network error during load disabled the one tool that performs the update. - Add "Test Reload Icon Toggle" SmartButton to pyRevitDevTools Bundle Tests pulldown: clicking toggles a state persisted in the user config ini; __selfinit__ re-applies the matching icon on load. After a reload the icon must match the stored state, distinguishing "__selfinit__ not re-run" from "bundle icon not restored".
![devloai[bot]](https://avatars.githubusercontent.com/in/886113?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary:
- Fixes SmartButtons nested in pulldown/split buttons losing
__selfinit__-driven icon/enabled state after a reload, by re-runningExecuteSelfInitinPulldownButtonBuilder.UpdateExistingChildrenandSplitButtonBuilder.UpdateExistingChildren(mirrors the existingPushButtonBuilder.UpdateExistingButtonpattern). - Hardens
Update.smartbutton's__selfinit__so a transient update-check failure no longer deactivates the button for the whole session — only theuser_can_updategate can now disable it. - Adds a new
Test Reload Icon ToggleDevTools SmartButton to manually verify reload behavior for nested SmartButtons.
Review Summary:
Traced the fix against the sibling implementations (PushButtonBuilder, StackBuilder, SmartButtonScriptInitializer.ExecuteSelfInit) and confirmed the new code faithfully mirrors the established create-path pattern (reset Enabled/Visible, re-run self-init, honor a false return) with no null-ref or ordering issues. The Update.smartbutton control-flow change was verified line-by-line against the diff and correctly always returns True after the try/except unless the permission gate short-circuits. The new DevTools bundle's config key naming is consistent between __selfinit__/__main__, and its icon fallback (missing on.dark.png) degrades safely via SmartButtonContext.get_bundle_file. Checked repo-wide guidelines (IPY2712 compatibility, PEP8, Black/style consistency) — no violations, and the unused off.png/off.dark.png assets match an existing convention already present elsewhere in the same extension, so not flagged as a new deviation. One code-smell comment was recorded: the "run self-init / disable on false" block is now duplicated across 6 call sites in 3 files, which is worth consolidating into a shared ButtonBuilderBase helper to avoid future drift (this duplication is arguably what caused the original bug).
Suggestions
|
|
||
| // Handle SmartButton-specific initialization on update; | ||
| // __selfinit__ must re-run after post-processing since it may override the bundle icon | ||
| if (sub.Type == CommandComponentType.SmartButton && _smartButtonScriptInitializer != null) |
There was a problem hiding this comment.
This "run ExecuteSelfInit, disable on false, log" block is now duplicated almost verbatim in 6 places: PushButtonBuilder.Build and UpdateExistingButton, PulldownButtonBuilder.AddSingleChildToPulldown (existing) and here, and SplitButtonBuilder.AddSingleChildToSplitButton (existing) and its UpdateExistingChildren counterpart. Since ButtonBuilderBase already centralizes similar cross-cutting helpers (UpdatePushButtonCommandBinding, DeactivateRibbonItem), consider factoring this into a shared helper there, e.g.:
protected void ExecuteSmartButtonSelfInitIfNeeded(
ParsedComponent sub,
PushButton btn,
SmartButtonScriptInitializer? initializer,
string context)
{
if (sub.Type != CommandComponentType.SmartButton || initializer == null)
return;
if (!initializer.ExecuteSelfInit(sub, btn))
{
btn.Enabled = false;
Logger.Debug($"SmartButton '{sub.DisplayName}' in {context} deactivated by __selfinit__.");
}
}This would prevent the two behaviors (create-path vs. update-path re-init) from drifting apart again in the future, which is effectively what caused the bug this PR fixes.
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.
There was a problem hiding this comment.
Pull request overview
This PR fixes a C# loader reload behavior where SmartButtons nested inside pulldown and split buttons were not re-running __selfinit__ during the “update existing UI” reload path, causing icon and enabled/disabled state to drift after reloads. It also hardens the Update SmartButton so transient update-check failures don’t deactivate the button, and adds a DevTools SmartButton to validate the behavior.
Changes:
- Re-run SmartButton
__selfinit__(viaSmartButtonScriptInitializer.ExecuteSelfInit) after post-processing when updating existing children in pulldown/split containers, disabling the button if__selfinit__returnsFalse. - Make
Update.smartbuttonlog update-check failures but remain active (onlyuser_can_updatecan deactivate it). - Add a DevTools “Test Reload Icon Toggle” SmartButton (plus menu entry) to verify icon persistence across reloads inside a pulldown.
Reviewed changes
Copilot reviewed 6 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| extensions/pyRevitDevTools.extension/pyRevitDev.tab/Debug.panel/Bundle Tests.pulldown/Test Reload Icon Toggle.smartbutton/script.py | New SmartButton that persists a toggle state in config and applies an on-icon in __selfinit__ to validate reload behavior. |
| extensions/pyRevitDevTools.extension/pyRevitDev.tab/Debug.panel/Bundle Tests.pulldown/Test Reload Icon Toggle.smartbutton/bundle.yaml | Adds title/tooltip metadata describing the reload test and expected icon behavior. |
| extensions/pyRevitDevTools.extension/pyRevitDev.tab/Debug.panel/Bundle Tests.pulldown/bundle.yaml | Adds the new test SmartButton to the pulldown layout. |
| extensions/pyRevitCore.extension/pyRevit.tab/pyRevit.panel/Update.smartbutton/script.py | Prevents transient update-check failures from disabling the Update tool; keeps the default icon unless an update is detected. |
| dev/pyRevitLoader/pyRevitAssemblyBuilder/UIManager/Buttons/SplitButtonBuilder.cs | Re-runs SmartButton __selfinit__ during reload updates for existing split-button children, after post-processing. |
| dev/pyRevitLoader/pyRevitAssemblyBuilder/UIManager/Buttons/PulldownButtonBuilder.cs | Re-runs SmartButton __selfinit__ during reload updates for existing pulldown children, after post-processing. |
That one is a pita to test.
|
|
i have not tried this PR because it's so annyoing to test |
|
Hmm. So maybe this PR does not address the update button at all... I think the changes I made to the However, I feel pretty confident that if the bug does exist, it is something within the Update script, not the UI builder. Other SmartButtons are working correctly. |
|
Ok, Claude to the rescue here. I was able to have it automate all the PITA parts of the test with a local repo 'helloworld' extension. The button state starts orange when an update is available and resets to the default state after a successful run without a Revit restart. I think this is as tested as we can get for this one.
|
Description
SmartButtons nested inside pulldown and split buttons lost their
__selfinit__icon/state changes on pyRevit reload under the C# loader. This PR re-runs__selfinit__on the reload update path for those two containers, hardensUpdate.smartbuttonagainst transient update-check failures, and adds a DevTools test button for verifying the behavior.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:
I was not able to reproduce the Update.smartbutton issue exactly.
Root cause
On reload,
UpdateExistingChildreninPulldownButtonBuilderandSplitButtonBuilderre-applied the bundle icon viaButtonPostProcessor.Process()but never re-ran__selfinit__. Any icon or enabled/disabled state set by the init script was lost, and an icon set in a previous session could linger out of sync with current state (e.g. the Update button staying orange after updating). The fix mirrors the pattern already used byPushButtonBuilder.UpdateExistingButtonandStackBuilder, which were unaffected.Notes
Stale
IconManagerBitmapCache(the original report's theory): not the cause -IconManagerand its cache are recreated on everyLoadSession, so no bitmaps survive a reload, andClearCache()in the reload sequence would be a no-op.Reviewer's can consider the updates to Update.SmartButton optional. I was not able to reproduce the stale icon issue with this button directly. I did create a test button similar to Test Reload Icon Toggle in a slideout - to reproduce the reload conditions for this button type - and the icon state changes correctly. If the Update.smartbutton issue is reproduceable, it is probably something deeper in the updater.
Changes
PulldownButtonBuilder.UpdateExistingChildren/SplitButtonBuilder.UpdateExistingChildren: after re-applyingpost-processing to an existing child, run
ExecuteSelfInitfor SmartButtons and honor aFalsereturn by disabling the button - matching the create path andPushButtonBuilder's update path.Update.smartbutton: a failed update-availability check no longer returnsFalsefrom__selfinit__(which deactivated the Update button for the whole session on any transient git/network error); it now logs the error and keeps the tool usable. Only theuser_can_updatepermission gate deactivates the button.Test Reload Icon ToggleSmartButton (pyRevitDevTools → Debug → Bundle Tests): clicking toggles a state persisted in the user config ini ([devtools_reload_tests]);__selfinit__re-applies the matching icon on every load. After a reload the icon must match the stored state, which distinguishes "__selfinit__not re-run" (default icon shown while state is on) from "bundle icon not restored" (orange shown while state is off).Testing
pyRevitLoader.sln) and reload pyRevit.__selfinit__never re-ran inside pulldowns).__selfinit__declines to set one — the scenario from the original report ("Update button stays orange after updating").