fix(launcher): avoid orphan widgets detection and logging

2026-06-05 12:58:40 +02:00 · 2026-06-02 10:22:26 +02:00
parent 3796984182
commit 9f94ca7748
4 changed files with 151 additions and 77 deletions
@@ -207,6 +207,7 @@ class LaunchWindow(BECMainWindow):

        self.app = QApplication.instance()
        self.tiles: dict[str, LaunchTile] = {}
+        self._logged_unparented_connections: set[str] = set()
        # Track the smallest main‑label font size chosen so far
        self._min_main_label_pt: int | None = None

@@ -655,37 +656,51 @@ class LaunchWindow(BECMainWindow):
        super().showEvent(event)
        self.setFixedSize(self.size())

-    def _launcher_is_last_widget(self, connections: dict) -> bool:
+    def _has_external_window(self, connections: dict) -> bool:
        """
-        Check if the launcher is the last widget in the application.
+        Check if any registered non-launcher connection owns a top-level Qt window.
        """
-
        for connection in connections.values():
-            if not self._connection_belongs_to_launcher(connection):
-                return False
-        return True
+            if self._connection_belongs_to_launcher(connection):
+                continue
+            if isinstance(connection, QWidget) and connection.isWindow():
+                return True
+        return False
+
+    def _log_unparented_connections(self, connections: dict) -> None:
+        """
+        Log non-launcher RPC connections that remain without an active top-level window.
+        """
+        for connection in connections.values():
+            if self._connection_belongs_to_launcher(connection):
+                continue
+            if isinstance(connection, QWidget) and connection.isWindow():
+                continue
+
+            connection_description = (
+                f"type={type(connection).__name__} objectName={connection.objectName()!r} "
+                f"gui_id={connection.gui_id!r}"
+            )
+            if connection_description in self._logged_unparented_connections:
+                continue
+            self._logged_unparented_connections.add(connection_description)
+            logger.warning(
+                "Registered non-launcher RPC connection has no active top-level window: "
+                f"{connection_description}"
+            )

    def _connection_belongs_to_launcher(self, connection: QObject) -> bool:
        """
        Check whether a registered connection is the launcher itself or part of its Qt hierarchy.
-
-        Registered top-level windows such as BECMainWindowNoRPC are expected when another GUI is
-        open. They are not launcher children, but they are also not an error condition.
        """
-        try:
-            if connection is self or getattr(connection, "gui_id", None) == self.gui_id:
-                return True
-            if connection.objectName() == self.objectName():
-                return True
+        if connection is self or connection.gui_id == self.gui_id:
+            return True

-            parent = connection.parent()
-            while parent is not None:
-                if parent is self:
-                    return True
-                parent = parent.parent()
-        except Exception as e:
-            logger.error(f"Error checking launcher ownership of connection: {e}")
-            return False
+        parent = connection.parent()
+        while parent is not None:
+            if parent is self:
+                return True
+            parent = parent.parent()

        return False

@@ -694,29 +709,30 @@ class LaunchWindow(BECMainWindow):
        If there is only one connection remaining, it is the launcher, so we show it.
        Once the launcher is closed as the last window, we quit the application.
        """
-        if self._launcher_is_last_widget(connections):
-            self.show()
-            self.activateWindow()
-            self.raise_()
+        if self._has_external_window(connections):
+            self.hide()
            if self.app:
-                self.app.setQuitOnLastWindowClosed(True)  # type: ignore
+                self.app.setQuitOnLastWindowClosed(False)  # type: ignore
            return

-        self.hide()
+        self._log_unparented_connections(connections)
+        self.show()
+        self.activateWindow()
+        self.raise_()
        if self.app:
-            self.app.setQuitOnLastWindowClosed(False)  # type: ignore
+            self.app.setQuitOnLastWindowClosed(True)  # type: ignore

    def closeEvent(self, event):
        """
        Close the launcher window.
        """
        connections = self.register.list_all_connections()
-        if self._launcher_is_last_widget(connections):
-            event.accept()
+        if self._has_external_window(connections):
+            event.ignore()
+            self.hide()
            return

-        event.ignore()
-        self.hide()
+        event.accept()


 if __name__ == "__main__":  # pragma: no cover
@@ -331,32 +331,34 @@ class BECWidget(BECConnector):
            # All widgets need to call super().cleanup() in their cleanup method
            logger.info(f"Registry cleanup for widget {self.__class__.__name__}")
            self.rpc_register.remove_rpc(self)
-        children = self.findChildren(BECWidget)
-        for child in children:
-            if not shiboken6.isValid(child):
-                # If the child is not valid, it means it has already been deleted
-                continue
-            child.close()
-            child.deleteLater()
+            children = self.findChildren(BECWidget)
+            for child in children:
+                if not shiboken6.isValid(child):
+                    # If the child is not valid, it means it has already been deleted
+                    continue
+                child.close()
+                child.deleteLater()

-        # Tear down busy overlay explicitly to stop spinner and remove filters
-        overlay = getattr(self, "_busy_overlay", None)
-        if overlay is not None and shiboken6.isValid(overlay):
-            try:
-                overlay.hide()
-                filt = getattr(overlay, "_filter", None)
-                if filt is not None and shiboken6.isValid(filt):
-                    try:
-                        self.removeEventFilter(filt)
-                    except Exception as exc:
-                        logger.warning(f"Failed to remove event filter from busy overlay: {exc}")
+            # Tear down busy overlay explicitly to stop spinner and remove filters
+            overlay = getattr(self, "_busy_overlay", None)
+            if overlay is not None and shiboken6.isValid(overlay):
+                try:
+                    overlay.hide()
+                    filt = getattr(overlay, "_filter", None)
+                    if filt is not None and shiboken6.isValid(filt):
+                        try:
+                            self.removeEventFilter(filt)
+                        except Exception as exc:
+                            logger.warning(
+                                f"Failed to remove event filter from busy overlay: {exc}"
+                            )

-                # Cleanup the overlay widget. This will call cleanup on the custom widget if present.
+                    # Cleanup the overlay widget. This will call cleanup on the custom widget if present.

-                overlay.cleanup()
-                overlay.deleteLater()
-            except Exception as exc:
-                logger.warning(f"Failed to delete busy overlay: {exc}")
+                    overlay.cleanup()
+                    overlay.deleteLater()
+                except Exception as exc:
+                    logger.warning(f"Failed to delete busy overlay: {exc}")

    def closeEvent(self, event):
        """Wrap the close even to ensure the rpc_register is cleaned up."""
@@ -6,6 +6,7 @@ from qtpy.QtCore import QObject
 from qtpy.QtWidgets import QApplication, QWidget

 from bec_widgets.utils.bec_connector import BECConnector
+from bec_widgets.utils.bec_widget import BECWidget
 from bec_widgets.utils.error_popups import SafeProperty
 from bec_widgets.utils.error_popups import SafeSlot as Slot

@@ -15,6 +16,9 @@ from .client_mocks import mocked_client
 class BECConnectorQObject(BECConnector, QObject): ...


+class _CleanupBroadcastWidget(BECWidget, QWidget): ...
+
+
@pytest.fixture
 def bec_connector(mocked_client):
    connector = BECConnectorQObject(client=mocked_client)
@@ -146,6 +150,28 @@ def test_bec_connector_change_object_name(bec_connector):
    assert not any(obj.objectName() == previous_name for obj in all_objects)


+def test_bec_widget_cleanup_broadcasts_after_children_are_unregistered(mocked_client, qtbot):
+    parent = _CleanupBroadcastWidget(client=mocked_client, object_name="cleanup_parent")
+    child = _CleanupBroadcastWidget(
+        parent=parent, client=mocked_client, object_name="cleanup_child"
+    )
+    qtbot.addWidget(parent)
+
+    observed_connections = []
+    parent.rpc_register.callbacks.append(
+        lambda connections: observed_connections.append(set(connections))
+    )
+
+    parent.close()
+
+    assert parent._destroyed is True
+    assert child.gui_id not in parent.rpc_register.list_all_connections()
+    assert all(
+        parent.gui_id in snapshot or child.gui_id not in snapshot
+        for snapshot in observed_connections
+    )
+
+
 def test_bec_connector_export_settings():

    class MyWidget(BECConnector, QWidget):
@@ -4,7 +4,9 @@ import os
 from unittest import mock

 import pytest
+from qtpy.QtCore import QObject
 from qtpy.QtGui import QFontMetrics
+from qtpy.QtWidgets import QWidget

 import bec_widgets
 from bec_widgets.applications.launch_window import START_EMPTY_PROFILE_OPTION, LaunchWindow
@@ -16,6 +18,28 @@ from .client_mocks import mocked_client
 base_path = os.path.dirname(bec_widgets.__file__)


+def _launcher_child_connection(launcher: LaunchWindow, name: str) -> QObject:
+    connection = QObject(parent=launcher)
+    connection.gui_id = f"{launcher.gui_id}:{name}"
+    connection.setObjectName(name)
+    return connection
+
+
+def _top_level_connection(qtbot, name: str) -> QWidget:
+    connection = QWidget()
+    connection.gui_id = name
+    connection.setObjectName(name)
+    qtbot.addWidget(connection)
+    return connection
+
+
+def _unparented_connection(name: str) -> QObject:
+    connection = QObject()
+    connection.gui_id = name
+    connection.setObjectName(name)
+    return connection
+
+
@pytest.fixture
 def bec_launch_window(qtbot, mocked_client):
    widget = LaunchWindow(client=mocked_client)
@@ -117,20 +141,20 @@ def test_open_dock_area_with_start_empty_option_calls_launch(bec_launch_window):
        (["launcher", "dock_area", "scan_progress_simple", "scan_progress_full"], False),
        (
            ["launcher", "dock_area", "scan_progress_simple", "scan_progress_full", "hover_widget"],
-            True,
+            False,
        ),
+        (["launcher", "external_window"], True),
    ],
 )
-def test_gui_server_turns_off_the_lights(bec_launch_window, connection_names, hide):
+def test_gui_server_turns_off_the_lights(bec_launch_window, qtbot, connection_names, hide):
    connections = {}
    for name in connection_names:
-        conn = mock.MagicMock()
        if name == "hover_widget":
-            conn.parent.return_value = None
-            conn.objectName.return_value = "HoverWidget"
+            conn = _unparented_connection("HoverWidget")
+        elif name == "external_window":
+            conn = _top_level_connection(qtbot, "external_window")
        else:
-            conn.parent.return_value = mock.MagicMock()
-            conn.objectName.return_value = bec_launch_window.objectName()
+            conn = _launcher_child_connection(bec_launch_window, name)
        connections[name] = conn
    with (
        mock.patch.object(bec_launch_window, "show") as mock_show,
@@ -153,15 +177,21 @@ def test_gui_server_turns_off_the_lights(bec_launch_window, connection_names, hi
            mock_set_quit_on_last_window_closed.assert_called_once_with(True)


-def test_launcher_detects_external_main_window_without_info_log(bec_launch_window):
-    connection = mock.MagicMock()
-    connection.parent.return_value = None
-    connection.objectName.return_value = "BECMainWindowNoRPC"
+def test_launcher_detects_external_main_window(bec_launch_window, qtbot):
+    connection = _top_level_connection(qtbot, "BECMainWindowNoRPC")

-    with mock.patch("bec_widgets.applications.launch_window.logger.info") as mock_info:
-        assert not bec_launch_window._launcher_is_last_widget({"window": connection})
+    assert bec_launch_window._has_external_window({"window": connection})

-    mock_info.assert_not_called()
+
+def test_launcher_logs_unparented_non_window_connection_once(bec_launch_window):
+    connection = _unparented_connection("HoverWidget")
+
+    with mock.patch("bec_widgets.applications.launch_window.logger.warning") as mock_warning:
+        bec_launch_window._turn_off_the_lights({"window": connection})
+        bec_launch_window._turn_off_the_lights({"window": connection})
+
+    mock_warning.assert_called_once()
+    assert "HoverWidget" in mock_warning.call_args.args[0]


@pytest.mark.parametrize(
@@ -174,11 +204,12 @@ def test_launcher_detects_external_main_window_without_info_log(bec_launch_windo
        (["launcher", "dock_area", "scan_progress_simple", "scan_progress_full"], True),
        (
            ["launcher", "dock_area", "scan_progress_simple", "scan_progress_full", "hover_widget"],
-            False,
+            True,
        ),
+        (["launcher", "external_window"], False),
    ],
 )
-def test_launch_window_closes(bec_launch_window, connection_names, close_called):
+def test_launch_window_closes(bec_launch_window, qtbot, connection_names, close_called):
    """
    Test that the close event is handled correctly based on the connections.
    If there are no connections or only the launcher connection, the window should close.
@@ -186,13 +217,12 @@ def test_launch_window_closes(bec_launch_window, connection_names, close_called)
    """
    connections = {}
    for name in connection_names:
-        conn = mock.MagicMock()
        if name == "hover_widget":
-            conn.parent.return_value = None
-            conn.objectName.return_value = "HoverWidget"
+            conn = _unparented_connection("HoverWidget")
+        elif name == "external_window":
+            conn = _top_level_connection(qtbot, "external_window")
        else:
-            conn.parent.return_value = mock.MagicMock()
-            conn.objectName.return_value = bec_launch_window.objectName()
+            conn = _launcher_child_connection(bec_launch_window, name)
        connections[name] = conn
    close_event = mock.MagicMock()
    with mock.patch.object(