-
Notifications
You must be signed in to change notification settings - Fork 60
SG-41525 Fix LoaderActionManager._execute_hook #131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 fixes a bug in the LoaderActionManager where the _execute_hook method was incorrectly receiving a boolean as its first parameter instead of the expected Qt action object. The fix replaces lambda functions with functools.partial to properly bind parameters.
- Replaced lambda-based signal connections with
functools.partialto ensure correct parameter binding - Added import for
functools.partial
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| qt_action, actions | ||
| ) | ||
| ) | ||
| handler = partial(self._execute_hook, a, actions) |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The action object a is captured by reference in the partial call within a loop. This creates a closure issue where all handlers will reference the last value of a from the loop. Consider using partial with a default argument pattern: partial(self._execute_hook, qt_action=a, actions=actions) or ensure each iteration captures its own action reference.
| handler = partial(self._execute_hook, a, actions) | |
| handler = partial(self._execute_hook, qt_action=a, actions=actions) |
| qt_action, actions | ||
| ) | ||
| ) | ||
| handler = partial(self._execute_hook, a, actions) |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The action object a is captured by reference in the partial call within a loop. This creates a closure issue where all handlers will reference the last value of a from the loop. Consider using partial with a default argument pattern: partial(self._execute_hook, qt_action=a, actions=actions) or ensure each iteration captures its own action reference.
| handler = partial(self._execute_hook, a, actions) | |
| handler = partial(self._execute_hook, qt_action=a, actions=actions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
julien-lang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide more information about the problem and why the current code does not work specifically on Python 3.13?
Also, given the change, are we expecting to replicate this change in all other TK components calling a hook?
|
We were getting this error in the shell when pressing on a button in the loader: So I added debug print and confirmed that the type of action was bool and I also printed the stack So the first argument, the qt_action, was no longer a QAction but a bool, asking Gemini I received the answer: And I used a proposed solution that worked. When asked about what changed between Python 3.13.3 and 3.11.5 or PySide 6.8.3 and 6.5.2 that could explain the change of behaviour Gemini answered: So yes, this modification should probably be done in other places ... I just tested our specific case |
This pull request refactors how signal handlers are connected in
loader_action_manager.pyby replacing lambda functions withfunctools.partialfor binding parameters to the_execute_hookmethod. This change improves code readability and maintainability.Refactoring of signal handler connections:
functools.partialwhen connecting thetriggeredsignal to_execute_hookin bothget_actions_for_publishesandget_actions_for_folder, making the handler binding more explicit and easier to read. [1] [2]partialfromfunctoolsto support the new handler binding method.