Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,18 @@ private void UpdateExistingChildren(
existingBtn.Enabled = true;
existingBtn.Visible = true;

// 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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
var shouldActivate = _smartButtonScriptInitializer.ExecuteSelfInit(sub, existingBtn);
if (!shouldActivate)
{
existingBtn.Enabled = false;
Logger.Debug($"SmartButton '{sub.DisplayName}' in pulldown deactivated by __selfinit__ during update.");
}
}

Logger.Debug($"Updated existing child button '{sub.DisplayName}' in pulldown '{component.DisplayName}'. New text: '{buttonText}'");
}
catch (Exception ex)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,18 @@ private void UpdateExistingChildren(
if (!wasVisible && IsRibbonItemVisible(existingBtn, fallbackVisible: false))
newlyVisibleNames.Add(sub.DisplayName);

// 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)
{
var shouldActivate = _smartButtonScriptInitializer.ExecuteSelfInit(sub, existingBtn);
if (!shouldActivate)
{
existingBtn.Enabled = false;
Logger.Debug($"SmartButton '{sub.DisplayName}' in split button deactivated by __selfinit__ during update.");
}
}

Logger.Debug($"Updated existing child button '{sub.DisplayName}' in split button '{component.DisplayName}'. New text: '{buttonText}'");
}
catch (Exception ex)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,18 @@ def __selfinit__(script_cmp, ui_button_cmp, __rvt__):
# do not load the tool if user should not update
if not user_config.user_can_update:
return False
# otherwise set icon depending on if updates are available
# set icon if updates available; a failed availability check must not
# deactivate the tool that performs the update
try:
has_update_icon = script_cmp.get_bundle_file('icon_hasupdates.png')
if user_config.check_updates \
and not user_config.auto_update \
and updater.check_for_updates():
has_update_icon = script_cmp.get_bundle_file('icon_hasupdates.png')
ui_button_cmp.set_icon(has_update_icon,
icon_size=ribbon.ICON_LARGE)
return True
except Exception as ex:
logger.error('Error in checking updates: {}'.format(ex))
return False
logger.error('Error in checking updates: %s', ex)
return True


if __name__ == '__main__':
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
title: >-
Test Reload

Icon Toggle
tooltip: >-
SmartButton that mirrors Update.smartbutton: click to toggle a state
stored in the user config ini; __selfinit__ applies the orange dot icon
when the state is on and leaves the default bundle icon when off. After a
reload the icon must match the stored state - default shown while on
means __selfinit__ was not re-run in this pulldown, orange shown while
off means the loader failed to restore the bundle icon.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
"""Test SmartButton icon persistence across reloads inside pulldowns.

Mirrors the Update.smartbutton pattern: __selfinit__ applies the orange dot
icon only when the stored state is on, and leaves the loader-applied default
icon otherwise. Click the button to toggle the state; it is stored in the
user config ini so it can be inspected while troubleshooting.

After a reload the icon must match the stored state: orange when on, the
default bundle icon when off. A mismatch means __selfinit__ was not re-run
(default shown while state is on) or the loader failed to restore the bundle
icon (orange shown while state is off).
"""
#pylint: disable=C0103,E0401
from pyrevit import script
from pyrevit.coreutils.ribbon import ICON_MEDIUM

CONFIG_SECTION = 'devtools_reload_tests'
STATE_OPTION = 'pulldown_state'


def __selfinit__(script_cmp, ui_button_cmp, __rvt__):
# apply the stored state; the loader is responsible for restoring the
# default bundle icon when the state is off
config = script.get_config(CONFIG_SECTION)
if config.get_option(STATE_OPTION, default_value=False):
icon = script_cmp.get_bundle_file('on.png')
ui_button_cmp.set_icon(icon, icon_size=ICON_MEDIUM)
return True


if __name__ == '__main__':
from pyrevit.userconfig import user_config

config = script.get_config(CONFIG_SECTION)
state = not config.get_option(STATE_OPTION, default_value=False)
config.set_option(STATE_OPTION, state)
script.save_config()
# the default bundle icon stands in for the off state so the click
# result matches what a reload would show
script.toggle_icon(state, off_icon_path=script.get_bundle_file('icon.png'))

print('Config file: {}'.format(user_config.config_file))
print('[{}] {} = {}'.format(CONFIG_SECTION, STATE_OPTION, state))
print('Icon should now be {} and must stay that way after a reload.'
.format('the orange dot' if state else 'the default bundle icon'))
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ layout:
- Test pyRevit Bundle
- Test pyRevit Button
- Test Smart Button
- Test Reload Icon Toggle
- Test C# Script
- Test Direct Invoke
- Test Direct Invoke (ExecParams)
Expand Down