Feature combobox button bundle #2930
Conversation
- added initial combobox bundle to test case
- Add COMBOBOX_POSTFIX to extension directory hash calculation - Implement ComboBox creation and member handling in uimaker.py - Add _PyRevitRibbonComboBox wrapper class in ribbon.py - Add ComboBoxGroup component class with members parsing
- the combobox loads and tirggers update
- because of event handling and initialization, we had to match the design pattern used with the smartbutton - using delayed script loading
- Added comprehensive ComboBox property support (Name, ToolTip, Image, ItemText, Current, etc.) - Implemented all ComboBox methods (add_item, add_items, add_separator, get_items, etc.) - Fixed member properties by preserving full member dictionaries in components.py (was converting to tuples) - Fixed member icon/tooltip/group properties by setting on ComboBoxMember object after AddItem - Added encoding declaration to uimaker.py to fix non-ASCII character error - Enhanced logging for ComboBox creation and member property setting - Updated add_item() to return ComboBoxMember for property setting - Full support for ComboBoxMemberData properties (icon, tooltip, group, tooltip_ext, tooltip_image)
- Added comprehensive ComboBox property support (Name, ToolTip, Image, ItemText, Current, etc.) - Implemented all ComboBox methods (add_item, add_items, add_separator, get_items, etc.) - Fixed member properties by preserving full member dictionaries in components.py (was converting to tuples) - Fixed member icon/tooltip/group properties by setting on ComboBoxMember object after AddItem - Added encoding declaration to uimaker.py to fix non-ASCII character error - Updated add_item() to return ComboBoxMember for property setting - Full support for ComboBoxMemberData properties (icon, tooltip, group, tooltip_ext, tooltip_image) - Removed all debugging logging statements
- Formatted components.py, ribbon.py, and uimaker.py with Black - Ensures code follows PEP 8 style guidelines
|
Unable to perform a code review. You have run out of credits 😔 |
jmcouffin
added
New Feature
|
@romangolev this may touch your actual refactoring work |
|
This is great stuff @dnenov I'll take a look as soon as I can. In the meantime, if it is not too much to ask, could make a sample/test in the pyrevit dev extensions in a new PR. That would be helpful. |
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive ComboBox support for pyRevit extensions, exposing all properties and methods from the Autodesk.Revit.UI ComboBox API. The implementation follows the SmartButton pattern for deferred initialization using the __selfinit__ function.
Key Changes:
- Complete ComboBox API wrapper with all properties and methods (name, tooltip, image, current, events, etc.)
- Rich member metadata support (icons, tooltips, groups, extended tooltips, tooltip images)
- SmartButton-style deferred initialization pattern for event handler setup
- Bug fixes for member property setting and icon path resolution
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
pyrevitlib/pyrevit/loader/uimaker.py |
Added _produce_ui_combobox function implementing full ComboBox creation with member configuration and __selfinit__ pattern support |
pyrevitlib/pyrevit/extensions/components.py |
Added ComboBoxGroup class to handle ComboBox metadata and member preservation |
pyrevitlib/pyrevit/extensions/__init__.py |
Added COMBOBOX_POSTFIX constant for ComboBox component identification |
pyrevitlib/pyrevit/coreutils/ribbon.py |
Added _PyRevitRibbonComboBox wrapper class with complete API methods, properties, and event handler support |
Comments suppressed due to low confidence (1)
pyrevitlib/pyrevit/loader/uimaker.py:1023
- Except block directly handles BaseException.
except:
| # Log panel items before adding slideout (for debugging order issues) | ||
| try: | ||
| if hasattr(parent_ui_item, "get_rvtapi_object"): | ||
| existing_items = parent_ui_item.get_rvtapi_object().GetItems() | ||
| mlogger.warning( | ||
| "SLIDEOUT: Panel has %d items before adding slideout", | ||
| len(existing_items), | ||
| ) | ||
| for idx, item in enumerate(existing_items): | ||
| mlogger.warning( | ||
| "SLIDEOUT: Existing item %d: %s (type: %s)", | ||
| idx, | ||
| getattr(item, "Name", "unknown"), | ||
| type(item).__name__, | ||
| ) | ||
| except Exception as log_err: | ||
| mlogger.debug("SLIDEOUT: Could not log existing items: %s", log_err) | ||
|
|
||
| mlogger.warning("SLIDEOUT: Adding slide out to: %s", parent_ui_item) | ||
| try: | ||
| parent_ui_item.add_slideout() | ||
| mlogger.warning("SLIDEOUT: Slideout added successfully") |
There was a problem hiding this comment.
Debug logging code left in production. These warning-level logs (lines 181-191, 195, 198) appear to be temporary debugging code based on the comment 'for debugging order issues'. Warning-level logs should be reserved for actual warnings, not diagnostic information. Either remove this debug code or change to mlogger.debug() level if the diagnostic information is needed long-term.
There was a problem hiding this comment.
Indeed, use mlogget.debug
| # Diagnostic logging to track panel placement issues | ||
| parent_name = getattr(ui_maker_params.parent_ui, "name", None) | ||
| if not parent_name: | ||
| try: | ||
| parent_name = str(type(ui_maker_params.parent_ui)) | ||
| except: | ||
| parent_name = "unknown" | ||
| mlogger.warning( | ||
| "BUILDING COMPONENT: %s parent: %s", sub_cmp.name, parent_name | ||
| ) |
There was a problem hiding this comment.
Bare except clause catches all exceptions including system exits. Replace except: on line 1023 with except Exception: to avoid catching SystemExit, KeyboardInterrupt, and other system-level exceptions that should not be suppressed.
| parent_name = str(type(ui_maker_params.parent_ui)) | ||
| except: | ||
| parent_name = "unknown" | ||
| mlogger.warning( |
There was a problem hiding this comment.
Debug logging code left in production. This warning-level log (lines 1025-1027) with comment 'Diagnostic logging to track panel placement issues' appears to be temporary debugging code. Warning-level logs should be reserved for actual warnings. Either remove this debug code or change to mlogger.debug() level if the diagnostic information is needed long-term.
| mlogger.warning( | |
| mlogger.debug( |
| # Add member to ComboBox first (returns ComboBoxMember object) | ||
| try: | ||
| member = combobox_ui.add_item(member_data) | ||
| if not member: | ||
| mlogger.warning("AddItem returned None for: %s", member_text) | ||
| continue |
There was a problem hiding this comment.
Variable name 'member' is reused and shadows the loop variable from line 572. The returned ComboBoxMember object should use a different name such as 'member_obj' or 'added_member' to avoid confusion and shadowing.
| # Enable verbose logging to see WARNING messages (can be removed later) | ||
| # Uncomment the line below to see all log messages: | ||
| # mlogger.set_verbose_mode() |
There was a problem hiding this comment.
Remove or clean up commented-out debug configuration. The comment 'can be removed later' on line 24 suggests this is temporary code. Either remove these lines or if verbose mode configuration is intended to stay, document when and why users would enable it.
| # Enable verbose logging to see WARNING messages (can be removed later) | |
| # Uncomment the line below to see all log messages: | |
| # mlogger.set_verbose_mode() |
| member_data (UI.ComboBoxMemberData): Member data to add | ||
| Returns: | ||
| (UI.ComboBoxMember): The created ComboBoxMember object, or None |
There was a problem hiding this comment.
Method documentation should explain when None is returned. The docstring states the return value can be None but doesn't document under what conditions this occurs (e.g., when in itemdata_mode or on error).
| (UI.ComboBoxMember): The created ComboBoxMember object, or None | |
| (UI.ComboBoxMember): The created ComboBoxMember object, or None. | |
| None is returned if the ComboBox is in itemdata_mode (i.e., when the underlying Revit API object is not available and items cannot be added directly). |
| """Add multiple items to the ComboBox. | ||
| Args: | ||
| member_data_list (list): List of UI.ComboBoxMemberData objects |
There was a problem hiding this comment.
Method missing return value documentation. The docstring should document what this method returns (appears to return None based on the implementation).
| member_data_list (list): List of UI.ComboBoxMemberData objects | |
| member_data_list (list): List of UI.ComboBoxMemberData objects | |
| Returns: | |
| None |
| ) | ||
|
|
||
| def add_separator(self): | ||
| """Add a separator to the ComboBox dropdown list.""" |
There was a problem hiding this comment.
Method missing return value and exception documentation. The docstring should document what this method returns and that it may raise PyRevitUIError on failure.
| """Add a separator to the ComboBox dropdown list.""" | |
| """Add a separator to the ComboBox dropdown list. | |
| Returns: | |
| None | |
| Raises: | |
| PyRevitUIError: If adding the separator fails. | |
| """ |
|
@sanzoghenzo I'd like your opinionated review 😸 |
Cool stuff! @jmcouffin would be nice to implement this functionality in new loader as well. Do you think we could confirm that that it works, confirm that my PR works as well and then proceed with this? |
100% Both PR are quite extensive, so expect turbulences. |
|
I will do my best to get a test/example in the dev extension - I wasn't aware of it! As you guys can see, the actual, in-code implementation of the combobox is a bit complicated, but that's to be expected. It will be of great value to have that tested the way you want, so I'll jump on it asap! Re-splitting this one up - it's a singular new feature :/ I struggle to find a way to have it broken down, as each piece works together with the rest. If you guys, @romangolev @jmcouffin have a better idea how to get it split up, ... yes, up for it too. Essentially, it's a wrapper around the native Revit combobox ui element, much like the other ui elements we have. |
sanzoghenzo
left a comment
sanzoghenzo
left a comment
There was a problem hiding this comment.
HI, here's my review.
I didn't test any of this, just looked at the code style 😉
| self.members = [] | ||
|
|
||
| # Read members from metadata | ||
| if self.meta: |
There was a problem hiding this comment.
Invert the if condition and exit early to reduce code indentation
| if self.meta: | |
| if not self.meta: | |
| return |
| if adwindows_obj: | ||
| exToolTip = self.get_rvtapi_object().ToolTip | ||
| if not isinstance(exToolTip, str): | ||
| exToolTip = None | ||
| adwindows_obj.ToolTip = AdWindows.RibbonToolTip() | ||
| adwindows_obj.ToolTip.Title = self.ui_title | ||
| adwindows_obj.ToolTip.Content = exToolTip | ||
| _StackPanel = System.Windows.Controls.StackPanel() | ||
| _video = System.Windows.Controls.MediaElement() | ||
| _video.Source = Uri(tooltip_video) | ||
| _video.LoadedBehavior = System.Windows.Controls.MediaState.Manual | ||
| _video.UnloadedBehavior = System.Windows.Controls.MediaState.Manual | ||
|
|
||
| def on_media_ended(sender, args): | ||
| sender.Position = System.TimeSpan.Zero | ||
| sender.Play() | ||
|
|
||
| _video.MediaEnded += on_media_ended | ||
|
|
||
| def on_loaded(sender, args): | ||
| sender.Play() | ||
|
|
||
| _video.Loaded += on_loaded | ||
| _StackPanel.Children.Add(_video) | ||
| adwindows_obj.ToolTip.ExpandedContent = _StackPanel | ||
| adwindows_obj.ResolveToolTip() | ||
| else: | ||
| self.tooltip_video = tooltip_video |
| """ | ||
| try: | ||
| adwindows_obj = self.get_adwindows_object() | ||
| if adwindows_obj: |
| """ | ||
| if self.itemdata_mode: | ||
| return self.ui_title | ||
| else: |
There was a problem hiding this comment.
no need for an else here
| else: | ||
| if hasattr(self._rvtapi_object, "ItemText"): | ||
| self._rvtapi_object.ItemText = self.ui_title = ui_title | ||
| self._dirty = True |
There was a problem hiding this comment.
| else: | |
| if hasattr(self._rvtapi_object, "ItemText"): | |
| self._rvtapi_object.ItemText = self.ui_title = ui_title | |
| self._dirty = True | |
| elif hasattr(self._rvtapi_object, "ItemText"): | |
| self._rvtapi_object.ItemText = self.ui_title = ui_title | |
| self._dirty = True |
| # Log panel items before adding slideout (for debugging order issues) | ||
| try: | ||
| if hasattr(parent_ui_item, "get_rvtapi_object"): | ||
| existing_items = parent_ui_item.get_rvtapi_object().GetItems() | ||
| mlogger.warning( | ||
| "SLIDEOUT: Panel has %d items before adding slideout", | ||
| len(existing_items), | ||
| ) | ||
| for idx, item in enumerate(existing_items): | ||
| mlogger.warning( | ||
| "SLIDEOUT: Existing item %d: %s (type: %s)", | ||
| idx, | ||
| getattr(item, "Name", "unknown"), | ||
| type(item).__name__, | ||
| ) | ||
| except Exception as log_err: | ||
| mlogger.debug("SLIDEOUT: Could not log existing items: %s", log_err) | ||
|
|
||
| mlogger.warning("SLIDEOUT: Adding slide out to: %s", parent_ui_item) | ||
| try: | ||
| parent_ui_item.add_slideout() | ||
| mlogger.warning("SLIDEOUT: Slideout added successfully") |
There was a problem hiding this comment.
Indeed, use mlogget.debug
| mlogger.error("Error calling create_combobox: %s", create_err) | ||
| import traceback | ||
|
|
||
| mlogger.error("Traceback: %s", traceback.format_exc()) |
There was a problem hiding this comment.
just use
| mlogger.error("Error calling create_combobox: %s", create_err) | |
| import traceback | |
| mlogger.error("Traceback: %s", traceback.format_exc()) | |
| mlogger.exception("Error calling create_combobox: %s", create_err) |
it will add the exception info automatically
| return None | ||
|
|
||
|
|
||
| def _produce_ui_combobox(ui_maker_params): |
There was a problem hiding this comment.
This function is really long, is there a way to split it into logical chunks?
Also, is this code that error prone that you need to catch an exception on each line? (I didn't try it so this is just by reading the code).
If this is necessary, can you reduce the scope of the exceptions to only catch those expected?
|
|
||
| # IMPORTANT: only bail here if it is still ComboBoxData | ||
| if isinstance(combobox_obj, UI.ComboBoxData): | ||
| return combobox_ui # <- ONLY in this branch |
There was a problem hiding this comment.
Not sure what the comment means
| except PyRevitException as err: | ||
| mlogger.error("UI error creating ComboBox: %s", err.msg) | ||
| import traceback | ||
|
|
||
| mlogger.error("Traceback: %s", traceback.format_exc()) | ||
| return None | ||
| except Exception as err: | ||
| mlogger.error("Error creating ComboBox: %s", err) | ||
| import traceback | ||
|
|
||
| mlogger.error("Full traceback: %s", traceback.format_exc()) |
There was a problem hiding this comment.
You basically already catched each and every exception line by line, and you didn't (re)raised anything, is this still necessary?
|
@devloai please review |
![devloai[bot]](https://avatars.githubusercontent.com/in/886113?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary:
This PR implements comprehensive ComboBox support for pyRevit extensions, exposing all properties and methods from the Autodesk.Revit.UI ComboBox API. Key features include:
- Full property support (Name, ToolTip, Image, ItemText, Enabled, Visible, Current, etc.)
- Complete method implementation (add_item, add_items, add_separator, get_items, etc.)
- Rich member metadata support (icons, tooltips, groups, extended tooltips)
- Event handling via
__selfinit__pattern (following SmartButton architecture) - Proper member property assignment on ComboBoxMember objects
Review Summary:
The implementation is well-structured and follows existing pyRevit patterns correctly. The code properly:
- Uses the SmartButton pattern for deferred initialization via
__selfinit__ - Handles member property extraction and assignment correctly (initial concern about variable shadowing was unfounded - the code cleverly extracts properties before reassigning the variable)
- Follows the existing pattern where
create_*methods don't return values (consistent withcreate_pulldown_button,create_split_button, etc.) - Implements comprehensive error handling with appropriate logging levels
- Preserves full member dictionaries to maintain rich metadata (icons, tooltips, groups)
The architecture aligns with the SmartButton pattern documented in the knowledge base, allowing scripts to access fully constructed UI objects and set up event handlers after UI creation.
No critical issues found. The implementation is solid and ready to merge. 🎉
Follow-up suggestions:
4 participants
Complete ComboBox Implementation with Full API Support
Description
This PR implements comprehensive ComboBox support for pyRevit extensions, exposing all properties and methods from the Autodesk.Revit.UI ComboBox API. The implementation enables full control over ComboBox creation, member management, and property configuration through bundle.yaml metadata.
The ComboBox implementation follows the same SmartButton pattern for deferred initialization, using the
__selfinit__function inscript.pyto allow scripts to access the fully constructed UI object and set up event handlers after the UI is created.Combobox UI Element
Key Features
__selfinit__pattern (following SmartButton architecture)__selfinit__function, consistent with SmartButton implementationTechnical Changes
_PyRevitRibbonComboBoxclass with all API methods and properties__selfinit__pattern (SmartButton pattern) for deferred script initializationBug Fixes
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:
Additional Notes
Files Modified
pyrevitlib/pyrevit/extensions/components.py- Fixed member dictionary preservationpyrevitlib/pyrevit/coreutils/ribbon.py- Added complete ComboBox wrapper classpyrevitlib/pyrevit/loader/uimaker.py- Enhanced ComboBox creation and member configurationArchitecture Pattern
The ComboBox implementation follows the SmartButton pattern for deferred initialization:
bundle.yamlmetadatascript.pyis loaded as a module__selfinit__function exists, it's called with(component, ui_item, uiapp)parameters__selfinit__returnsFalse, the ComboBox is deactivatedThis pattern allows scripts to:
Usage Example
ComboBoxes can now be created with full metadata support in
bundle.yaml:And in
script.py, use the__selfinit__pattern to wire ComboBox items to commands:Key Points:
idfield frombundle.yamlmembers becomes theNameproperty of theComboBoxMembertextfield becomes theItemTextpropertysender.Current.Nameto get the member ID for routing to commandssender.Current.ItemTextto get the display textFUNCTION_MAP) to connect member IDs to your command functionsAPI Methods Available
All ComboBox properties and methods from Autodesk.Revit.UI are now accessible:
name,current,visible,enabled,get_title(), etc.set_icon(),set_tooltip(),add_item(),get_items(), etc.CurrentChanged,DropDownOpened,DropDownClosed(viaadd_*_handler()methods)Gotchas
Thread Context: Do not use
print()or equivalent logging inside the__selfinit__initialization phase, as the thread context will break. Useloggerfrompyrevit.scriptinstead:Member Icons: Using
image(oricon) for ComboBox member items can yield strange UI behavior in Revit's ComboBox dropdown. The items may appear with extra padding/margin, pushing text to the right. This is a limitation of the Revit API's ComboBox rendering and is not controllable via the Ribbon API. Consider using icons sparingly or testing thoroughly if visual alignment is critical.Testing
Tested with Spectrum extension ComboBox implementation. All member icons, tooltips, and properties are working correctly. Event handlers work as expected using the
__selfinit__pattern.Reviewers
FYI
Thank you for contributing to pyRevit! 🎉