fix: startup tweaks (#41)

* fix: startup tweaks * fix: Add a few safety checks
2026-01-18 23:57:14 -08:00
parent 610654ae38
commit eb2f6fba2a
7 changed files with 196 additions and 20 deletions
@@ -1027,6 +1027,12 @@ If you put auto-start logic in `Init.py`, it will only run when the user manuall
 2. Use `QTimer.singleShot()` to defer execution until GUI is fully ready
 3. The `Initialize()` method is too late - it only runs when workbench is selected

+**Note on GuiWaiter vs QTimer.singleShot:**
+
+- `GuiWaiter` works well when called from `Init.py` (workbench selection) or `startup_bridge.py`
+- However, `GuiWaiter` has timing issues when used from `InitGui.py` module-level code
+- For `InitGui.py`, use `QTimer.singleShot()` with a sufficient delay (e.g., 3 seconds)
+
 **Example pattern in InitGui.py:**

 ```python
@@ -1036,12 +1042,14 @@ try:
 except ImportError:
    from PySide6 import QtCore

-def _deferred_auto_start() -> None:
+def _auto_start_bridge() -> None:
    """Auto-start bridge after GUI is fully ready."""
    # ... auto-start logic here ...

 # Schedule auto-start after GUI initializes (3 second delay)
-QtCore.QTimer.singleShot(3000, _deferred_auto_start)
+from preferences import get_auto_start
+if get_auto_start():
+    QtCore.QTimer.singleShot(3000, _auto_start_bridge)
 ```

 **This is different from regular Python packages** where `__init__.py` runs on import. FreeCAD workbenches have special loading behavior.
@@ -1081,28 +1089,32 @@ When FreeCAD starts in GUI mode, there's a timing window where:
 - Integration tests pass initial connection, then crash on first document operation
 - Thread check shows `is_main_thread: False` with `thread_name: 'MCP-QueueProcessor'` even when `gui_up: True`

-**The Fix (in `InitGui.py` or `startup_bridge.py`):**
+**The Fix:**

-Use `QTimer.singleShot()` to defer bridge startup until after GUI is ready:
+The approach depends on where the code runs:
+
+**For `InitGui.py` module-level code:** Use `QTimer.singleShot()` with a delay:

 ```python
 # WRONG - starts bridge immediately, GUI may not be ready
 _auto_start_bridge()

-# CORRECT - wait for GUI to be ready (use sufficient delay)
-QtCore.QTimer.singleShot(3000, _deferred_auto_start)
+# CORRECT - defer with sufficient delay for GUI to stabilize
+QtCore.QTimer.singleShot(3000, _auto_start_bridge)
 ```

-For `startup_bridge.py`, use `GuiWaiter` to poll for `FreeCAD.GuiUp`:
+**For `startup_bridge.py` or `Init.py`:** Use `GuiWaiter` to poll for `FreeCAD.GuiUp`:

 ```python
-# Wait for GuiUp to become True before starting
-from bridge_utils import GuiWaiter
+# CORRECT - poll for GuiUp to be True, then start
+from freecad_mcp_bridge.bridge_utils import GuiWaiter
 _gui_waiter = GuiWaiter(callback=_start_bridge, log_prefix="Startup Bridge")
 _gui_waiter.start()
 ```

-**Note:** This QTimer wait pattern is **only for GUI startup scenarios**. In headless mode (`freecadcmd`), the bridge starts directly without waiting because there is no Qt event loop to wait for. The four startup paths are:
+**Note:** `GuiWaiter` has timing issues when used from `InitGui.py` module-level code due to how FreeCAD loads workbenches. Use `QTimer.singleShot()` for `InitGui.py` instead.
+
+**Note:** These wait patterns are **only for GUI startup scenarios**. In headless mode (`freecadcmd`), the bridge starts directly without waiting because there is no Qt event loop to wait for. The four startup paths are:

 1. **GUI already up** (`FreeCAD.GuiUp = True`): Start bridge immediately with Qt timer
 2. **True headless** (`QCoreApplication` exists but is NOT a `QApplication`): Start directly with background thread
@@ -37,6 +37,27 @@ def _auto_start_bridge() -> None:
    This function is called via a deferred timer (GUI mode) or directly
    (headless mode) after FreeCAD finishes loading. It starts the bridge
    without requiring the workbench to be selected.
+
+    Args:
+        None.
+
+    Returns:
+        None. Early returns if auto-start is disabled or bridge is already running.
+
+    Raises:
+        Exception: Any exception during bridge startup is caught, logged to
+            FreeCAD.Console.PrintError with full traceback, and suppressed.
+
+    Side Effects:
+        - Imports and checks auto-start preference from preferences module
+        - Creates and starts a FreecadMCPPlugin instance if not already running
+        - Registers the plugin with the workbench commands module
+        - Prints status messages to FreeCAD.Console
+
+    Example:
+        This function is typically called via QTimer or GuiWaiter callback::
+
+            QtCore.QTimer.singleShot(1000, _auto_start_bridge)
    """
    try:
        from preferences import get_auto_start
@@ -183,9 +183,42 @@ try:
    # This runs at FreeCAD GUI startup (InitGui.py module-level code)
    # Note: Init.py does NOT run at startup for workbench addons, so auto-start
    # must be triggered from here instead.
-    def _deferred_auto_start() -> None:
-        """Auto-start bridge after GUI is fully ready."""
+    #
+    # We use GuiWaiter to poll FreeCAD.GuiUp instead of a fixed timer delay.
+    # This ensures we wait for the GUI to actually be ready, rather than
+    # hoping a fixed delay is long enough.
+    def _auto_start_bridge() -> None:
+        """Auto-start bridge after GUI is confirmed ready.
+
+        This is the callback invoked by GuiWaiter once FreeCAD.GuiUp is True
+        and a defer period has elapsed. At this point, it's safe to start
+        the MCP bridge with Qt timer-based queue processing.
+
+        Args:
+            None.
+
+        Returns:
+            None. Early returns if auto-start disabled or bridge already running.
+
+        Raises:
+            Exception: Any exception during bridge startup is caught, logged
+                to FreeCAD.Console.PrintError with full traceback, and suppressed.
+
+        Side Effects:
+            - Creates and starts a FreecadMCPPlugin instance
+            - Registers the plugin with the workbench commands module
+            - Syncs the status bar widget with bridge state
+        """
        try:
+            # Safety check: verify GUI is actually ready before starting
+            # If not ready, reschedule for another attempt
+            if not FreeCAD.GuiUp:
+                FreeCAD.Console.PrintMessage(
+                    "Robust MCP Bridge: GUI not ready, rescheduling auto-start...\n"
+                )
+                QtCore.QTimer.singleShot(500, _auto_start_bridge)
+                return
+
            from preferences import get_auto_start

            if not get_auto_start():
@@ -238,12 +271,23 @@ try:

            FreeCAD.Console.PrintError(traceback.format_exc())

-    # Schedule auto-start after a short delay to ensure GUI is fully ready
-    # Use a longer delay than status bar sync to avoid race conditions
-    QtCore.QTimer.singleShot(3000, _deferred_auto_start)
+    # Check if auto-start is enabled before scheduling
+    from preferences import get_auto_start
+
+    if get_auto_start():
+        # Schedule auto-start after a delay to ensure GUI is fully ready.
+        # InitGui.py module-level code runs early in FreeCAD startup, so we
+        # need to defer the bridge start to avoid race conditions.
+        # Note: We use a simple QTimer.singleShot() here because GuiWaiter
+        # has timing issues when used from module-level code during startup.
+        QtCore.QTimer.singleShot(3000, _auto_start_bridge)
        FreeCAD.Console.PrintMessage(
            "Robust MCP Bridge: Auto-start scheduled from InitGui (3s)\n"
        )
+    else:
+        FreeCAD.Console.PrintMessage(
+            "Robust MCP Bridge: Auto-start disabled in preferences\n"
+        )
 except Exception as e:
    FreeCAD.Console.PrintWarning(
        f"Robust MCP Bridge: Could not schedule status bar sync: {e}\n"
@@ -17,7 +17,7 @@ This release fixes some auto-start issues and improves the overall startup exper

 - **Auto-start not working at FreeCAD startup**: Fixed bug where auto-start only worked when manually selecting the workbench. For FreeCAD workbench addons, `Init.py` does NOT run at startup - only `InitGui.py` module-level code runs. Auto-start logic has been moved to `InitGui.py`.
 - **Status bar not appearing after auto-start**: The status bar widget now syncs immediately after the bridge starts, instead of on a timer that ran before the bridge was ready.
- **Integration test crashes**: Fixed race condition where the bridge could start before `FreeCAD.GuiUp` was `True`, causing Qt operations to run on a background thread and crash FreeCAD.
+- **Integration test crashes**: Fixed race condition where the bridge could start before `FreeCAD.GuiUp` was `True`, causing Qt operations to run on a background thread and crash FreeCAD. Auto-start is now deferred with `QTimer.singleShot()` to allow the GUI to stabilize.

 ### Note

@@ -269,8 +269,8 @@ class FreecadMCPPlugin:
        if FREECAD_AVAILABLE:
            FreeCAD.Console.PrintMessage(
                f"MCP Bridge started (Instance ID: {self._instance_id}):\n"
-                f"  - JSON-RPC: {self._host}:{self._port}\n"
            )
+            FreeCAD.Console.PrintMessage(f"  - JSON-RPC: {self._host}:{self._port}\n")
            if self._enable_xmlrpc:
                FreeCAD.Console.PrintMessage(
                    f"  - XML-RPC: {self._host}:{self._xmlrpc_port}\n"
@@ -59,6 +59,12 @@ def _start_bridge() -> None:
    environment variables and registers the plugin with the workbench commands
    module for visibility to other components.

+    Args:
+        None.
+
+    Returns:
+        None. Early returns if bridge is already running.
+
    Environment Variables:
        FREECAD_XMLRPC_PORT: XML-RPC port (default: 9875)
        FREECAD_SOCKET_PORT: JSON-RPC socket port (default: 9876)
@@ -73,6 +79,12 @@ def _start_bridge() -> None:
        - Creates and starts a FreecadMCPPlugin instance
        - Registers the plugin with the workbench commands module
        - Prints status messages to FreeCAD.Console
+
+    Example:
+        This function is typically called via GuiWaiter callback or directly::
+
+            _gui_waiter = GuiWaiter(callback=_start_bridge)
+            _gui_waiter.start()
    """
    # Check if bridge is already running (from auto-start in Init.py)
    from bridge_utils import get_running_plugin
@@ -278,6 +278,93 @@ class TestGuiWaiterUsage:
        assert "did not become ready" in bridge_utils_code


+class TestInitGuiAutoStart:
+    """Tests for auto-start logic in InitGui.py.
+
+    InitGui.py is the primary entry point for auto-start at FreeCAD GUI startup.
+    Init.py does NOT run at startup for workbench addons, so InitGui.py must
+    handle auto-start using QTimer.singleShot() to defer the bridge start.
+    """
+
+    @pytest.fixture
+    def initgui_code(self) -> str:
+        """Load InitGui.py content."""
+        return (ADDON_DIR / "InitGui.py").read_text()
+
+    def test_uses_single_shot_timer_for_auto_start(self, initgui_code: str) -> None:
+        """InitGui.py should use QTimer.singleShot for deferred auto-start."""
+        # Should use singleShot with a delay
+        assert "QTimer.singleShot" in initgui_code
+        assert "_auto_start_bridge" in initgui_code
+
+    def test_checks_auto_start_preference(self, initgui_code: str) -> None:
+        """InitGui.py should check auto-start preference before scheduling.
+
+        Uses AST-based analysis to verify there's an if-condition that calls
+        get_auto_start(), rather than relying on brittle string matching.
+        """
+        tree = ast.parse(initgui_code)
+
+        def is_get_auto_start_call(node: ast.AST) -> bool:
+            """Check if node is a call to get_auto_start()."""
+            if not isinstance(node, ast.Call):
+                return False
+            func = node.func
+            # Handle direct name: get_auto_start()
+            if isinstance(func, ast.Name) and func.id == "get_auto_start":
+                return True
+            # Handle attribute access: module.get_auto_start()
+            return isinstance(func, ast.Attribute) and func.attr == "get_auto_start"
+
+        def contains_get_auto_start_call(node: ast.AST) -> bool:
+            """Check if node or any children contain get_auto_start call."""
+            return any(is_get_auto_start_call(child) for child in ast.walk(node))
+
+        # Find any ast.If node whose test contains a get_auto_start call
+        for node in ast.walk(tree):
+            if isinstance(node, ast.If) and contains_get_auto_start_call(node.test):
+                return  # Found an if-condition invoking get_auto_start()
+
+        pytest.fail(
+            "InitGui.py should have an if-condition that calls get_auto_start() "
+            "to check whether auto-start is enabled before scheduling"
+        )
+
+    def test_auto_start_bridge_function_exists(self, initgui_code: str) -> None:
+        """InitGui.py should have _auto_start_bridge callback function."""
+        assert "def _auto_start_bridge" in initgui_code
+
+    def test_auto_start_bridge_checks_if_running(self, initgui_code: str) -> None:
+        """_auto_start_bridge should check if bridge is already running."""
+        # Parse the function and check for is_bridge_running check
+        tree = ast.parse(initgui_code)
+        for node in ast.walk(tree):
+            if isinstance(node, ast.FunctionDef) and node.name == "_auto_start_bridge":
+                func_code = ast.unparse(node)
+                assert "is_bridge_running" in func_code
+                return
+        pytest.fail("_auto_start_bridge function not found")
+
+    def test_auto_start_syncs_status_bar(self, initgui_code: str) -> None:
+        """_auto_start_bridge should sync status bar after starting bridge."""
+        # Parse the function and check for sync_status_with_bridge call
+        tree = ast.parse(initgui_code)
+        for node in ast.walk(tree):
+            if isinstance(node, ast.FunctionDef) and node.name == "_auto_start_bridge":
+                func_code = ast.unparse(node)
+                assert "sync_status_with_bridge" in func_code
+                return
+        pytest.fail("_auto_start_bridge function not found")
+
+    def test_logs_auto_start_scheduled(self, initgui_code: str) -> None:
+        """InitGui.py should log when auto-start is scheduled."""
+        assert "Auto-start scheduled" in initgui_code
+
+    def test_logs_auto_start_disabled(self, initgui_code: str) -> None:
+        """InitGui.py should log when auto-start is disabled."""
+        assert "Auto-start disabled" in initgui_code
+
+
 class TestAutoStartCodePaths:
    """Tests to verify all auto-start code paths are properly handled."""