Add QML Actions callback system#3123
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a callback registry system to Meshroom, enabling conditional hooks such as prompting users to save files as standard projects or templates when the MR_ASK_TEMPLATE_BEFORE_SAVING environment variable is set. Key feedback includes resolving a state persistence bug in the save file dialog, fixing a potential QML layout binding loop, removing redundant checks and adding exception handling in the Python-to-QML slots, renaming the decorator to adhere to PEP 8 naming conventions, and centralizing the environment variable definition in meshroom/env.py.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| _callback = callback | ||
| open() | ||
| } |
There was a problem hiding this comment.
The _saveAsTemplate property is persistent on the saveFileDialog instance. If the dialog was previously used to save a template (setting _saveAsTemplate to true), any subsequent call to prompt() (e.g., from the unsaved changes dialog) will incorrectly inherit this state, causing standard projects to be saved as templates. Reset _saveAsTemplate to false inside prompt() to prevent this.
_saveAsTemplate = false
_callback = callback
open()
}
| ColumnLayout { | ||
| anchors.fill: parent | ||
| spacing: 12 |
There was a problem hiding this comment.
Using anchors.fill: parent inside a Dialog without an explicit height can cause a binding loop and layout issues, as the dialog's height depends on its content's implicit height, while the content is trying to fill the dialog's height. Instead, anchor only the left and right sides to the parent to respect the dialog's width while letting the height be determined implicitly by the layout's children.
ColumnLayout {
anchors.left: parent.left
anchors.right: parent.right
spacing: 12
| @Slot(str, result="QVariant") | ||
| def getCallbackResult(self, name): | ||
| """ | ||
| Trigger a registered callback by name (no arguments) and return the result. | ||
| Returns None (undefined in QML) if no callback is registered. | ||
| """ | ||
| cb = getRegisteredCallback(name) | ||
| if cb is None: | ||
| return None | ||
| return triggerCallback(name) |
There was a problem hiding this comment.
The getRegisteredCallback check is redundant because triggerCallback already performs the dictionary lookup and safely returns None if the callback is not registered. Additionally, wrapping the callback execution in a try...except block prevents unhandled exceptions from propagating to QML and potentially crashing the application.
| @Slot(str, result="QVariant") | |
| def getCallbackResult(self, name): | |
| """ | |
| Trigger a registered callback by name (no arguments) and return the result. | |
| Returns None (undefined in QML) if no callback is registered. | |
| """ | |
| cb = getRegisteredCallback(name) | |
| if cb is None: | |
| return None | |
| return triggerCallback(name) | |
| @Slot(str, result="QVariant") | |
| def getCallbackResult(self, name): | |
| """ | |
| Trigger a registered callback by name (no arguments) and return the result. | |
| Returns None (undefined in QML) if no callback is registered. | |
| """ | |
| try: | |
| return triggerCallback(name) | |
| except Exception: | |
| return None |
| @Slot(str, "QVariantList", result="QVariant") | ||
| def triggerCallbackWithArgs(self, name, args): | ||
| """ | ||
| Trigger a registered callback by name with arguments and return the result. | ||
| Args: | ||
| name (str): The callback name. | ||
| args (list): List of arguments to pass to the callback. | ||
| Returns: | ||
| The result of the callback, or None if not registered. | ||
| """ | ||
| cb = getRegisteredCallback(name) | ||
| if cb is None: | ||
| return None | ||
| return triggerCallback(name, *args) |
There was a problem hiding this comment.
Similar to getCallbackResult, the getRegisteredCallback check is redundant here as well. Wrapping the call in a try...except block ensures that any exceptions raised during callback execution are caught and do not crash the UI.
| @Slot(str, "QVariantList", result="QVariant") | |
| def triggerCallbackWithArgs(self, name, args): | |
| """ | |
| Trigger a registered callback by name with arguments and return the result. | |
| Args: | |
| name (str): The callback name. | |
| args (list): List of arguments to pass to the callback. | |
| Returns: | |
| The result of the callback, or None if not registered. | |
| """ | |
| cb = getRegisteredCallback(name) | |
| if cb is None: | |
| return None | |
| return triggerCallback(name, *args) | |
| @Slot(str, "QVariantList", result="QVariant") | |
| def triggerCallbackWithArgs(self, name, args): | |
| """ | |
| Trigger a registered callback by name with arguments and return the result. | |
| Args: | |
| name (str): The callback name. | |
| args (list): List of arguments to pass to the callback. | |
| Returns: | |
| The result of the callback, or None if not registered. | |
| """ | |
| try: | |
| return triggerCallback(name, *args) | |
| except Exception: | |
| return None |
| def REGISTER_CALLBACK(name): | ||
| """ | ||
| Decorator to register a function as a callback. | ||
|
|
||
| Usage: | ||
| @REGISTER_CALLBACK(name="save") | ||
| def my_save_callback(*args, **kwargs): | ||
| ... | ||
| """ | ||
| def decorator(func): | ||
| registerCallback(name, func) | ||
| return func | ||
| return decorator |
There was a problem hiding this comment.
Naming the decorator function in ALL_CAPS (REGISTER_CALLBACK) violates PEP 8 naming conventions (which reserves ALL_CAPS for constants) and is inconsistent with the camelCase style used throughout the rest of the codebase. Renaming it to registerCallbackDecorator improves readability and style consistency.
| def REGISTER_CALLBACK(name): | |
| """ | |
| Decorator to register a function as a callback. | |
| Usage: | |
| @REGISTER_CALLBACK(name="save") | |
| def my_save_callback(*args, **kwargs): | |
| ... | |
| """ | |
| def decorator(func): | |
| registerCallback(name, func) | |
| return func | |
| return decorator | |
| def registerCallbackDecorator(name): | |
| """ | |
| Decorator to register a function as a callback. | |
| Usage: | |
| @registerCallbackDecorator(name="save") | |
| def my_save_callback(*args, **kwargs): | |
| ... | |
| """ | |
| def decorator(func): | |
| registerCallback(name, func) | |
| return func | |
| return decorator |
| from meshroom.core.callbacks import ( | ||
| registerCallback, | ||
| unregisterCallback, | ||
| getRegisteredCallback, | ||
| triggerCallback, | ||
| clearCallbacks, | ||
| REGISTER_CALLBACK, | ||
| ) |
There was a problem hiding this comment.
Update the imports to use the renamed registerCallbackDecorator instead of REGISTER_CALLBACK.
| from meshroom.core.callbacks import ( | |
| registerCallback, | |
| unregisterCallback, | |
| getRegisteredCallback, | |
| triggerCallback, | |
| clearCallbacks, | |
| REGISTER_CALLBACK, | |
| ) | |
| from meshroom.core.callbacks import ( | |
| registerCallback, | |
| unregisterCallback, | |
| getRegisteredCallback, | |
| triggerCallback, | |
| clearCallbacks, | |
| registerCallbackDecorator, | |
| ) |
| def test_register_callback_decorator(): | ||
| """Test the REGISTER_CALLBACK decorator.""" | ||
| @REGISTER_CALLBACK(name="decorated") | ||
| def my_decorated_cb(x): | ||
| return x * 2 | ||
|
|
||
| assert getRegisteredCallback("decorated") is my_decorated_cb | ||
| assert triggerCallback("decorated", 5) == 10 |
There was a problem hiding this comment.
Update the test case to use the renamed registerCallbackDecorator.
| def test_register_callback_decorator(): | |
| """Test the REGISTER_CALLBACK decorator.""" | |
| @REGISTER_CALLBACK(name="decorated") | |
| def my_decorated_cb(x): | |
| return x * 2 | |
| assert getRegisteredCallback("decorated") is my_decorated_cb | |
| assert triggerCallback("decorated", 5) == 10 | |
| def test_register_callback_decorator(): | |
| """Test the registerCallbackDecorator decorator.""" | |
| @registerCallbackDecorator(name="decorated") | |
| def my_decorated_cb(x): | |
| return x * 2 | |
| assert getRegisteredCallback("decorated") is my_decorated_cb | |
| assert triggerCallback("decorated", 5) == 10 |
| if os.environ.get("MR_ASK_TEMPLATE_BEFORE_SAVING", "0") == "1": | ||
| registerCallback("save", _saveCallback) | ||
| logger.info("Save callback registered (MR_ASK_TEMPLATE_BEFORE_SAVING=1).") | ||
| else: | ||
| logger.debug("Save callback not registered (MR_ASK_TEMPLATE_BEFORE_SAVING is not set to 1).") |
There was a problem hiding this comment.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3123 +/- ##
===========================================
+ Coverage 85.33% 85.45% +0.12%
===========================================
Files 73 75 +2
Lines 11414 11516 +102
===========================================
+ Hits 9740 9841 +101
- Misses 1674 1675 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Seems to work pretty well already. Warning If we don't need it for other things than the save, then maybe do something more specific ? record-2026-06-05_15.30.12.mp4 |
- Add meshroom/core/callbacks.py with register, unregister, get, trigger, and decorator APIs - Add meshroom/core/save_callback.py that conditionally registers a "save" callback when MR_ASK_TEMPLATE_BEFORE_SAVING=1 - Expose callback system in MeshroomApp via QML-accessible Slots (hasRegisteredCallback, getCallbackResult, triggerCallbackWithArgs) - Merge saveFileDialog and saveTemplateDialog into a single unified dialog - Add saveChoiceDialog for template-vs-project choice when callback is active - Modify save/saveAs/saveAsTemplate actions to use the callback system - Add comprehensive tests in tests/test_callbacks.py
4c72e74 to
3a5d54f
Compare
Description
Propose a new system to add callbacks on specific actions.
For now we would add an optional callback to propose to save as template or save as project before saving.
This way this would avoid people saving templates as projects and therefore avoid compatibility node issues.