Skip to content

Conversation

@julien-lang
Copy link
Contributor

@julien-lang julien-lang commented Sep 16, 2025

This pull request refactors the Houdini menu XML generation logic to improve modularity and maintainability.
The main goal is to be able to use the class out of an engine context since we need to call it in startup.py starting with Houdini 21.0+

Based on #89

@julien-lang julien-lang requested a review from Copilot September 16, 2025 23:38
@julien-lang julien-lang changed the base branch from ticket/SG-40163-workaround-menu-h21.0 to ticket/SG-40304--remove-legacy-support September 16, 2025 23:39
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for Houdini version 21.0+ by refactoring menu creation and removing version-specific checks throughout the codebase. The main changes include creating a new engine-agnostic MenuBuilder class for menu XML generation and cleaning up legacy version compatibility code.

Key Changes:

  • Extracted menu creation logic into a dedicated MenuBuilder class for Houdini 21.0+ compatibility
  • Removed version-specific checks and legacy code for older Houdini versions
  • Updated import structure to expose the new MenuBuilder class
  • Removed unused Python packages (urllib2.py, httplib.py, LICENSE)

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
startup.py Added menu creation logic for Houdini 21.0+ using new MenuBuilder class
python/tk_houdini/ui_generation.py Refactored menu code into MenuBuilder class, removed legacy menu methods
python/tk_houdini/init.py Added MenuBuilder import to expose the class
python/packages/*.py Removed unused Python 2 compatibility packages
engine.py Removed version-specific checks and legacy compatibility code

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +134 to +136
# TODO - only for Houdini version 21.0+
enable_sg_menu = self.get_setting("enable_sg_menu", True)
if enable_sg_menu:
Copy link

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

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

The TODO comment indicates this is only for Houdini 21.0+, but there's no version check in the code. Either add the version check or update the comment to reflect the actual implementation.

Suggested change
# TODO - only for Houdini version 21.0+
enable_sg_menu = self.get_setting("enable_sg_menu", True)
if enable_sg_menu:
# Only enable SG menu for Houdini version 21.0+
enable_sg_menu = self.get_setting("enable_sg_menu", True)
if enable_sg_menu and self.version.version_tuple[0] >= 21:

Copilot uses AI. Check for mistakes.
# execution
main_dynamic_menu_script = ET.SubElement(main_dynamic_menu, "scriptCode")
main_dynamic_menu_script.text = (
"CDATA_START" + _g_dynamic_menu_script + "CDATA_END"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO replace that by inspect.getsource(test)

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Base automatically changed from ticket/SG-40304--remove-legacy-support to master October 9, 2025 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants