fix(plugins): add acquisitions to the group chosen when Acquire is clicked

datalab/gui/plugins/instrument.py

@@ -4,6 +4,7 @@
 
 from __future__ import annotations
 
+import dataclasses
 import logging
 import queue
 import threading
@@ -34,7 +35,12 @@
         PluginInstrument,
     )
 
-__all__ = ["InstrumentWindow", "SerialWorker", "wait_instruments_closed"]
+__all__ = [
+    "AcquisitionTarget",
+    "InstrumentWindow",
+    "SerialWorker",
+    "wait_instruments_closed",
+]
 
 
 def wait_instruments_closed(
@@ -90,12 +96,27 @@ def _run(self) -> None:
                     future.set_exception(exc)
 
 
+@dataclasses.dataclass(frozen=True)
+class AcquisitionTarget:
+    """Destination of an acquisition, taken when it is requested.
+
+    Args:
+        new_group: True to add the acquired objects to a new group
+        group_ids: current group of each panel (``"signal"``, ``"image"``), if
+         any: the panel of the objects is only known once they are acquired
+    """
+
+    new_group: bool
+    group_ids: dict[str, str | None]
+
+
 class InstrumentWindow(QW.QDialog):
     """Live view of a plugin instrument, next to its settings.
 
     The view is refreshed when settings change, and periodically in live mode.
     Each acquisition is added to the workspace in a new group or, when the
-    **New group** option is off, in the current group of the panel. The
+    **New group** option is off, in the current group of the panel, as they are
+    when the acquisition is requested. The
     instrument is opened with the window and closed with it; if opening it
     fails, the reason is shown, and it is opened again before the next call. Its
     methods are called one at a time in a worker thread: a request made while a
@@ -121,7 +142,7 @@ class InstrumentWindow(QW.QDialog):
     #: Maximum time DataLab waits for an instrument to close (s)
     CLOSE_TIMEOUT = 5.0
 
-    SIG_CALL_DONE = QC.Signal(str, object, object)
+    SIG_CALL_DONE = QC.Signal(str, object, object, object)
 
     def __init__(
         self,
@@ -148,6 +169,8 @@ def __init__(
         self._busy = False
         self._opened = False
         self._pending: str | None = None
+        # Destination of the requested acquisition, until the instrument is called
+        self._acquire_target: AcquisitionTarget | None = None
         self._close_future: Future | None = None
         self.SIG_CALL_DONE.connect(self._call_done)
 
@@ -238,8 +261,13 @@ def __init__(
         self._pending = "preview"
         self._executor.submit(self._call, "open")
 
-    def _call(self, method: str) -> None:
-        """Call an instrument method (worker thread)."""
+    def _call(self, method: str, target: AcquisitionTarget | None = None) -> None:
+        """Call an instrument method (worker thread).
+
+        Args:
+            method: name of the method
+            target: destination of an acquisition, returned with the result
+        """
         result, error = None, None
         try:
             result = getattr(self.instrument, method)()
@@ -249,7 +277,7 @@ def _call(self, method: str) -> None:
             if method == "close":
                 logging.getLogger(__name__).exception("Closing instrument failed")
         try:
-            self.SIG_CALL_DONE.emit(method, result, error)
+            self.SIG_CALL_DONE.emit(method, result, error, target)
         except RuntimeError:
             pass  # Window deleted after being closed
 
@@ -270,13 +298,22 @@ def _request(self, method: str) -> None:
             self._pending = method
             self._executor.submit(self._call, "open")
             return
+        target = None
         if method == "acquire":
             self._live_timer.stop()
             self.acquire_button.setEnabled(False)
+            # Requests made during the acquisition do not change its destination
+            target, self._acquire_target = self._acquire_target, None
         self._busy = True
-        self._executor.submit(self._call, method)
+        self._executor.submit(self._call, method, target)
 
-    def _call_done(self, method: str, result, error: Exception | None) -> None:
+    def _call_done(
+        self,
+        method: str,
+        result,
+        error: Exception | None,
+        target: AcquisitionTarget | None,
+    ) -> None:
         """Show the result of an instrument call, then make the pending one."""
         if method == "close":
             return
@@ -300,7 +337,7 @@ def _call_done(self, method: str, result, error: Exception | None) -> None:
         elif method == "preview":
             self._show_frame(result)
         elif method == "acquire":
-            self._add_acquired(result)
+            self._add_acquired(result, target)
         pending, self._pending = self._pending, None
         if pending is not None:
             self._request(pending)
@@ -479,13 +516,30 @@ def render(self, frame: InstrumentFrame) -> None:
 
     def acquire(self) -> None:
         """Add an acquisition to the workspace, in a new group or in the current
-        group of the panel, depending on the **New group** option."""
+        group of the panel, depending on the **New group** option.
+
+        The option and the current group are taken now: changing them while the
+        acquisition is running does not change its destination."""
+        self._acquire_target = self.acquisition_target()
         self._request("acquire")
 
-    def _add_acquired(self, acquisition: InstrumentAcquisition) -> None:
+    def acquisition_target(self) -> AcquisitionTarget:
+        """Return the destination of an acquisition requested now: state of the
+        **New group** option and current group of each panel."""
+        return AcquisitionTarget(
+            self.new_group_box.isChecked(),
+            {
+                "signal": self.main.signalpanel.objview.get_current_group_id(),
+                "image": self.main.imagepanel.objview.get_current_group_id(),
+            },
+        )
+
+    def _add_acquired(
+        self, acquisition: InstrumentAcquisition, target: AcquisitionTarget | None
+    ) -> None:
         """Add the objects acquired by the instrument to the workspace."""
         try:
-            group_title = self.add_acquisition(acquisition)
+            group_title = self.add_acquisition(acquisition, target)
         except Exception as exc:  # pylint: disable=broad-except
             self.status_label.setText(str(exc))
             return
@@ -494,25 +548,39 @@ def _add_acquired(self, acquisition: InstrumentAcquisition) -> None:
             % (len(acquisition.objects), group_title)
         )
 
-    def add_acquisition(self, acquisition: InstrumentAcquisition) -> str:
+    def add_acquisition(
+        self,
+        acquisition: InstrumentAcquisition,
+        target: AcquisitionTarget | None = None,
+    ) -> str:
         """Add acquired objects to their panel.
 
         The objects are added to the current group of the panel if the **New
         group** option is off and the panel has a current group, otherwise to a
-        new group.
+        new group. A new group is also created if the current group was removed
+        since the acquisition was requested.
+
+        Args:
+            acquisition: acquired objects
+            target: destination taken when the acquisition was requested
+             (default: current state of the option and of the panel)
 
         Returns:
             Title of the group where the objects were added
         """
+        if target is None:
+            target = self.acquisition_target()
         objects = acquisition.objects
         panel_name = "signal" if isinstance(objects[0], SignalObj) else "image"
         panel = (
             self.main.signalpanel if panel_name == "signal" else self.main.imagepanel
         )
         with self.main.context_no_refresh():
-            group_id = None
-            if not self.new_group_box.isChecked():
-                group_id = panel.objview.get_current_group_id()
+            group_id = None if target.new_group else target.group_ids.get(panel_name)
+            existing = {get_uuid(group) for group in panel.objmodel.get_groups()}
+            if group_id not in existing:
+                # No current group, or group removed since the request
+                group_id = None
             if group_id is None:
                 group = panel.add_group(acquisition.group_title)
                 try:

datalab/tests/features/plugins/instrument_window_unit_test.py

@@ -21,6 +21,7 @@
 import threading
 import time
 from concurrent.futures import Future
+from types import SimpleNamespace
 
 import guidata.dataset as gds
 import numpy as np
@@ -149,11 +150,22 @@ def test_reopened_window_waits_for_closing() -> None:
         main.deleteLater()
 
 
+class MainWindowDouble(QW.QWidget):
+    """Main window double, with the panels whose current group is taken when an
+    acquisition is requested (adding acquired objects to them fails, which the
+    window reports)."""
+
+    def __init__(self) -> None:
+        super().__init__()
+        objview = SimpleNamespace(get_current_group_id=lambda: None)
+        self.signalpanel = self.imagepanel = SimpleNamespace(objview=objview)
+
+
 def test_live_mode_after_acquisition() -> None:
     """Live mode is resumed after an acquisition, unless it was switched off
     while the acquisition was running."""
     with qt_app_context():
-        main = QW.QWidget()
+        main = MainWindowDouble()
         instrument = SlowInstrument()
         instrument.release.set()
         window = InstrumentWindow(main, instrument, "Slow instrument")

datalab/tests/features/plugins/plugin_tools_app_test.py

@@ -7,6 +7,8 @@
 from __future__ import annotations
 
 import textwrap
+import threading
+import time
 
 import numpy as np
 import pytest
@@ -328,3 +330,83 @@ def acquire(window: InstrumentWindow) -> None:
             assert not window.new_group_box.isChecked()
             assert key not in Conf.instrument_new_group.get()
             win.close_plugin_instruments()
+
+
+def test_instrument_acquisition_target() -> None:
+    """The destination of an acquisition is taken when **Acquire** is clicked:
+    selecting another group or toggling the **New group** option while the
+    acquisition is running does not change it, and a new group replaces a group
+    removed meanwhile."""
+    with (
+        Conf.plugins_enabled.context(True),
+        Conf.plugins_enabled_list.context(None),
+        Conf.instrument_new_group.context({}),
+        temporary_plugin_dir() as plugin_dir,
+    ):
+        with open(
+            f"{plugin_dir}/datalab_test_plugin_tools.py", "w", encoding="utf-8"
+        ) as handle:
+            handle.write(PLUGIN_SOURCE)
+        with datalab_test_app_context(console=False) as win:
+            plugin = PluginRegistry.get_plugin("org.example.tools-app")
+            instrument = plugin.get_instrument("generator")
+            running, release = threading.Event(), threading.Event()
+            acquire = instrument.acquire
+
+            def slow_acquire():
+                """Acquire once the test releases the acquisition."""
+                running.set()
+                assert release.wait(10.0)
+                return acquire()
+
+            instrument.acquire = slow_acquire
+            panel = win.signalpanel
+            window = plugin.launch_tool("generator")
+            assert window.wait_until_idle()
+
+            def start_acquisition() -> None:
+                """Click Acquire, and wait until the acquisition is running."""
+                running.clear()
+                release.clear()
+                window.acquire()
+                deadline = time.monotonic() + 10.0
+                while not running.is_set():
+                    assert time.monotonic() < deadline
+                    QW.QApplication.processEvents()
+                    time.sleep(0.005)
+
+            def finish_acquisition() -> None:
+                """Release the acquisition, and wait until it is added."""
+                release.set()
+                assert window.wait_until_idle()
+
+            # Option off and group A current when Acquire is clicked
+            window.new_group_box.setChecked(False)
+            group_a = panel.add_group("A", select=True)
+            start_acquisition()
+            group_b = panel.add_group("B", select=True)
+            window.new_group_box.setChecked(True)
+            finish_acquisition()
+            assert len(panel.objmodel.get_groups()) == 2
+            assert len(group_a) == 3 and len(group_b) == 0
+            assert "group 'A'" in window.status_label.text()
+
+            # Option on when Acquire is clicked
+            panel.objview.select_groups([group_b])
+            start_acquisition()
+            window.new_group_box.setChecked(False)
+            finish_acquisition()
+            groups = panel.objmodel.get_groups()
+            assert len(groups) == 3 and groups[-1].title == "Generator acquisition"
+            assert len(group_a) == 3 and len(group_b) == 0 and len(groups[-1]) == 3
+
+            # Current group removed while the acquisition is running
+            panel.objview.select_groups([group_b])
+            start_acquisition()
+            panel.remove_object(force=True)
+            assert group_b not in panel.objmodel.get_groups()
+            finish_acquisition()
+            groups = panel.objmodel.get_groups()
+            assert len(groups) == 3 and groups[-1].title == "Generator acquisition"
+            assert len(groups[-1]) == 3 and len(group_a) == 3
+            win.close_plugin_instruments()

doc/features/advanced/plugins.rst

@@ -790,7 +790,7 @@ A tool may name an ``instrument`` method instead of a ``launcher``. This method
 
 DataLab edits :attr:`~datalab.plugins.instruments.PluginInstrument.settings` in place, with the active state, groups and tabs of the guidata DataSet, then calls :meth:`~datalab.plugins.instruments.PluginInstrument.preview` after each change and, in live mode, every ``live_interval_ms`` milliseconds. An :class:`~datalab.plugins.instruments.InstrumentFrame` holds signals drawn together or a single image, a short summary shown below the view, and an optional fixed Y range (signals) or color range (image). :meth:`~datalab.plugins.instruments.PluginInstrument.acquire` returns an :class:`~datalab.plugins.instruments.InstrumentAcquisition`: DataLab adds its objects to a new group of the matching panel, titled after the acquisition, and selects them, so that a recipe can run on them at once. Both methods raise ``ValueError`` with a user-facing message when the settings cannot be used; DataLab shows the message in the window.
 
-When the user turns off the **New group** option of the window, DataLab adds the acquired objects to the current group of the panel instead: as they are selected, the next acquisitions go to the same group, until the user selects another group. A new group is still created when the panel has no current group, for example when it is empty. The :attr:`~datalab.plugins.instruments.PluginInstrument.new_group` class attribute sets the default state of the option (``True`` by default): an instrument whose acquisitions are usually single objects, such as a device attribute read on demand, may set it to ``False``. The choice of the user, once made, is remembered for each instrument and prevails over this default.
+When the user turns off the **New group** option of the window, DataLab adds the acquired objects to the current group of the panel instead: as they are selected, the next acquisitions go to the same group, until the user selects another group. The option and the current group are taken when **Acquire** is clicked: changing them while the acquisition is running does not change its destination, and a new group is created if the group was removed meanwhile. A new group is still created when the panel has no current group, for example when it is empty. The :attr:`~datalab.plugins.instruments.PluginInstrument.new_group` class attribute sets the default state of the option (``True`` by default): an instrument whose acquisitions are usually single objects, such as a device attribute read on demand, may set it to ``False``. The choice of the user, once made, is remembered for each instrument and prevails over this default.
 
 DataLab creates the instrument once per tool and keeps it until the plugins are reloaded, so the settings remain from one opening to the next.