Fix camera config UI responsiveness issues

dlclivegui/gui/camera_config/camera_config_dialog.py

@@ -85,6 +85,15 @@ def dlc_camera_id(self, value: str | None) -> None:
         self._dlc_camera_id = value
         self._refresh_camera_labels()
 
+    def showEvent(self, event):
+        super().showEvent(event)
+        # Reset cleanup guard so close cleanup runs for each session
+        self._cleanup_done = False
+
+        # Rebuild the working copy from the latest “accepted” settings
+        self._working_settings = self._multi_camera_settings.model_copy(deep=True)
+        self._current_edit_index = None
+
     # Maintain overlay geometry when resizing
     def resizeEvent(self, event):
         super().resizeEvent(event)
@@ -173,7 +182,8 @@ def _on_close_cleanup(self) -> None:
             # Keep this short to reduce UI freeze
             sw.wait(300)
         self._set_scan_state(CameraScanState.IDLE)
-        self._cleanup_scan_worker()
+        if self._scan_worker and not self._scan_worker.isRunning():
+            self._cleanup_scan_worker()
 
         # Cancel probe worker
         pw = getattr(self, "_probe_worker", None)
@@ -288,6 +298,9 @@ def _mark_dirty(*_args):
     # -------------------------------
     # UI state updates
     # -------------------------------
+    def _is_preview_live(self) -> bool:
+        return self._preview.state in (PreviewState.ACTIVE, PreviewState.LOADING)
+
     def _set_apply_dirty(self, dirty: bool) -> None:
         """Visually mark Apply Settings button as 'dirty' (pending edits)."""
         if dirty:
@@ -304,13 +317,14 @@ def _update_button_states(self) -> None:
 
         active_row = self.active_cameras_list.currentRow()
         has_active_selection = active_row >= 0
-        allow_structure_edits = has_active_selection and not scan_running
 
-        self.remove_camera_btn.setEnabled(allow_structure_edits)
-        self.move_up_btn.setEnabled(allow_structure_edits and active_row > 0)
-        self.move_down_btn.setEnabled(allow_structure_edits and active_row < self.active_cameras_list.count() - 1)
-        # During loading, preview button becomes "Cancel Loading"
+        # Allow removing/moving active cameras even during scanning
+        self.remove_camera_btn.setEnabled(has_active_selection)
+        self.move_up_btn.setEnabled(has_active_selection and active_row > 0)
+        self.move_down_btn.setEnabled(has_active_selection and active_row < self.active_cameras_list.count() - 1)
+
         self.preview_btn.setEnabled(has_active_selection or self._preview.state == PreviewState.LOADING)
+
         available_row = self.available_cameras_list.currentRow()
         self.add_camera_btn.setEnabled(available_row >= 0 and not scan_running)
 
@@ -377,7 +391,7 @@ def _refresh_camera_labels(self) -> None:
 
     def _format_camera_label(self, cam: CameraSettings, index: int = -1) -> str:
         status = "✓" if cam.enabled else "○"
-        this_id = f"{cam.backend}:{cam.index}"
+        this_id = f"{(cam.backend or '').lower()}:{cam.index}"
         dlc_indicator = " [DLC]" if this_id == self._dlc_camera_id and cam.enabled else ""
         return f"{status} {cam.name} [{cam.backend}:{cam.index}]{dlc_indicator}"
 
@@ -630,7 +644,9 @@ def _on_available_camera_selected(self, row: int) -> None:
         if self._scan_worker and self._scan_worker.isRunning():
             self.add_camera_btn.setEnabled(False)
             return
-        self.add_camera_btn.setEnabled(row >= 0 and not self._is_scan_running())
+        item = self.available_cameras_list.item(row) if row >= 0 else None
+        detected = item.data(Qt.ItemDataRole.UserRole) if item else None
+        self.add_camera_btn.setEnabled(isinstance(detected, DetectedCamera))
 
     def _on_available_camera_double_clicked(self, item: QListWidgetItem) -> None:
         if self._is_scan_running():
@@ -676,7 +692,7 @@ def _on_active_camera_selected(self, row: int) -> None:
                 return
 
         # Stop any running preview when selection changes
-        if self._preview.state in (PreviewState.ACTIVE, PreviewState.LOADING):
+        if self._is_preview_live():
             self._stop_preview()
 
         self._current_edit_index = row
@@ -712,6 +728,9 @@ def _add_selected_camera(self) -> None:
             return
         item = self.available_cameras_list.item(row)
         detected = item.data(Qt.ItemDataRole.UserRole)
+        if not isinstance(detected, DetectedCamera):
+            QMessageBox.warning(self, "Invalid Selection", "Selected item is not a valid camera.")
+            return
         # make sure this is to lower for comparison against camera_identity_key
         backend = (self.backend_combo.currentData() or "opencv").lower()
 
@@ -751,6 +770,8 @@ def _add_selected_camera(self) -> None:
         self._start_probe_for_camera(new_cam)
 
     def _remove_selected_camera(self) -> None:
+        if self._is_preview_live():
+            self._stop_preview()
         if not self._commit_pending_edits(reason="before removing a camera"):
             return
         row = self.active_cameras_list.currentRow()
@@ -765,6 +786,8 @@ def _remove_selected_camera(self) -> None:
         self._update_button_states()
 
     def _move_camera_up(self) -> None:
+        if self._is_preview_live():
+            self._stop_preview()
         if not self._commit_pending_edits(reason="before reordering cameras"):
             return
         row = self.active_cameras_list.currentRow()
@@ -778,6 +801,8 @@ def _move_camera_up(self) -> None:
         self._refresh_camera_labels()
 
     def _move_camera_down(self) -> None:
+        if self._is_preview_live():
+            self._stop_preview()
         if not self._commit_pending_edits(reason="before reordering cameras"):
             return
         row = self.active_cameras_list.currentRow()
@@ -905,6 +930,14 @@ def _commit_pending_edits(self, *, reason: str = "") -> bool:
             )
             return False
 
+    def _enabled_count_with(self, row: int, new_enabled: bool) -> int:
+        count = 0
+        for i, cam in enumerate(self._working_settings.cameras):
+            enabled = new_enabled if i == row else bool(cam.enabled)
+            if enabled:
+                count += 1
+        return count
+
     def _apply_camera_settings(self) -> bool:
         try:
             for sb in (
@@ -932,10 +965,15 @@ def _apply_camera_settings(self) -> bool:
             current_model = self._working_settings.cameras[row]
             new_model = self._build_model_from_form(current_model)
 
-            diff = CameraSettings.check_diff(current_model, new_model)
+            if bool(new_model.enabled):
+                if self._enabled_count_with(row, True) > self.MAX_CAMERAS:
+                    QMessageBox.warning(
+                        self, "Maximum Cameras", f"Maximum of {self.MAX_CAMERAS} active cameras allowed."
+                    )
+                    self.cam_enabled_checkbox.setChecked(bool(current_model.enabled))
+                    return False
 
-            self._working_settings.cameras[row] = new_model
-            self._update_active_list_item(row, new_model)
+            diff = CameraSettings.check_diff(current_model, new_model)
 
             LOGGER.debug(
                 "[Apply] backend=%s idx=%s changes=%s",
@@ -1014,6 +1052,9 @@ def _populate_from_settings(self) -> None:
                 item.setForeground(Qt.GlobalColor.gray)
             self.active_cameras_list.addItem(item)
 
+        if self.active_cameras_list.count() > 0:
+            self.active_cameras_list.setCurrentRow(0)
+
         self._refresh_available_cameras()
         self._update_button_states()
 
@@ -1026,7 +1067,7 @@ def _reset_selected_camera(self, *, clear_backend_cache: bool = False) -> None:
             return
 
         # Stop preview to avoid fighting an open capture
-        if self._preview.state in (PreviewState.ACTIVE, PreviewState.LOADING):
+        if self._is_preview_live():
             self._stop_preview()
 
         cam = self._working_settings.cameras[row]
@@ -1065,6 +1106,9 @@ def _on_ok_clicked(self) -> None:
         # Auto-apply pending edits before saving
         if not self._commit_pending_edits(reason="before going back to the main window"):
             return
+        if len(self._working_settings.get_active_cameras()) > self.MAX_CAMERAS:
+            QMessageBox.warning(self, "Maximum Cameras", f"Maximum of {self.MAX_CAMERAS} active cameras allowed.")
+            return
         try:
             if self.apply_settings_btn.isEnabled():
                 self._append_status("[OK button] Auto-applying pending settings before closing dialog.")
@@ -1076,13 +1120,14 @@ def _on_ok_clicked(self) -> None:
         if self._working_settings.cameras and not active:
             QMessageBox.warning(self, "No Active Cameras", "Please enable at least one camera or remove all cameras.")
             return
-        self.settings_changed.emit(copy.deepcopy(self._working_settings))
+        self._multi_camera_settings = self._working_settings.model_copy(deep=True)
+        self.settings_changed.emit(self._multi_camera_settings)
 
         self._on_close_cleanup()
         self.accept()
 
     # -------------------------------
-    # Probe (device telemetry) management
+    # Probe management
     # -------------------------------
 
     def _start_probe_for_camera(self, cam: CameraSettings, *, apply_to_requested: bool = False) -> None:
@@ -1092,9 +1137,18 @@ def _start_probe_for_camera(self, cam: CameraSettings, *, apply_to_requested: bo
         requested width/height/fps with detected device values.
         """
         # Don’t probe if preview is active/loading
-        if self._preview.state in (PreviewState.ACTIVE, PreviewState.LOADING):
+        if self._is_preview_live():
             return
 
+        pw = getattr(self, "_probe_worker", None)
+        if pw and pw.isRunning():
+            try:
+                pw.request_cancel()
+            except Exception:
+                pass
+            pw.wait(200)
+        self._probe_worker = None
+
         # Track probe intent
         self._probe_apply_to_requested = bool(apply_to_requested)
         self._probe_target_row = int(self._current_edit_index) if self._current_edit_index is not None else None
@@ -1339,7 +1393,7 @@ def _start_preview(self) -> None:
         """Start camera preview asynchronously (no UI freeze)."""
         if not self._commit_pending_edits(reason="before starting preview"):
             return
-        if self._preview.state in (PreviewState.ACTIVE, PreviewState.LOADING):
+        if self._is_preview_live():
             return
 
         row = self._current_edit_index
@@ -1587,13 +1641,21 @@ def _update_preview(self) -> None:
             rotation = self.cam_rotation.currentData()
             frame = apply_rotation(frame, rotation)
 
-            # Apply crop if set in the form (real-time from UI)
+            # Compute crop with clamping
             h, w = frame.shape[:2]
-            x0 = self.cam_crop_x0.value()
-            y0 = self.cam_crop_y0.value()
-            x1 = self.cam_crop_x1.value() or w
-            y1 = self.cam_crop_y1.value() or h
-            frame = apply_crop(frame, x0, y0, x1, y1)
+            x0 = max(0, min(self.cam_crop_x0.value(), w))
+            y0 = max(0, min(self.cam_crop_y0.value(), h))
+            x1_val = self.cam_crop_x1.value()
+            y1_val = self.cam_crop_y1.value()
+            x1 = max(0, min(x1_val if x1_val > 0 else w, w))
+            y1 = max(0, min(y1_val if y1_val > 0 else h, h))
+
+            # Only apply if valid rectangle; otherwise skip crop
+            if x1 > x0 and y1 > y0:
+                frame = apply_crop(frame, x0, y0, x1, y1)
+            else:
+                # Optional: show a status once, not every frame
+                pass
 
             # Resize to fit preview label
             frame = resize_to_fit(frame, max_w=400, max_h=300)

tests/gui/camera_config/test_cam_dialog_e2e.py

@@ -424,3 +424,97 @@ def slow_run(self):
 
     qtbot.waitUntil(lambda: dialog._preview.loader is None and dialog._preview.state == PreviewState.IDLE, timeout=2000)
     assert dialog._preview.backend is None
+
+
+@pytest.mark.gui
+def test_remove_active_camera_works_while_scan_running(dialog, qtbot, monkeypatch):
+    """
+    Regression test for:
+    - 'When coming back to camera config after choosing a camera, it cannot be removed'
+    Root cause: scan_running disabled structure edits (Remove/Move).
+    Expected: Remove works even while discovery scan is running.
+    """
+
+    # Slow down camera detection so scan stays RUNNING long enough for interaction
+    def slow_detect(backend, max_devices=10, should_cancel=None, progress_cb=None, **kwargs):
+        for i in range(50):
+            if should_cancel and should_cancel():
+                break
+            if progress_cb:
+                progress_cb(f"Scanning… {i}")
+            time.sleep(0.02)
+        return [
+            DetectedCamera(index=0, label=f"{backend}-X"),
+            DetectedCamera(index=1, label=f"{backend}-Y"),
+        ]
+
+    monkeypatch.setattr(CameraFactory, "detect_cameras", staticmethod(slow_detect))
+
+    # Ensure an active row is selected
+    dialog.active_cameras_list.setCurrentRow(0)
+    qtbot.waitUntil(lambda: dialog.active_cameras_list.currentRow() == 0, timeout=1000)
+
+    initial_active = dialog.active_cameras_list.count()
+    initial_model = len(dialog._working_settings.cameras)
+    assert initial_active == initial_model == 1
+
+    # Trigger scan; wait until scan controls indicate it's running
+    qtbot.mouseClick(dialog.refresh_btn, Qt.LeftButton)
+    qtbot.waitUntil(lambda: dialog._is_scan_running(), timeout=1000)
+    qtbot.waitUntil(lambda: dialog.scan_cancel_btn.isVisible(), timeout=1000)
+
+    # Remove button should be enabled even during scan
+    qtbot.waitUntil(lambda: dialog.remove_camera_btn.isEnabled(), timeout=1000)
+
+    # Remove the selected active camera during scan
+    qtbot.mouseClick(dialog.remove_camera_btn, Qt.LeftButton)
+
+    assert dialog.active_cameras_list.count() == initial_active - 1
+    assert len(dialog._working_settings.cameras) == initial_model - 1
+
+    # Clean up: cancel scan so teardown doesn't hang waiting for scan completion
+    if dialog.scan_cancel_btn.isVisible() and dialog.scan_cancel_btn.isEnabled():
+        qtbot.mouseClick(dialog.scan_cancel_btn, Qt.LeftButton)
+
+    qtbot.waitUntil(lambda: not dialog._is_scan_running(), timeout=3000)
+
+
+@pytest.mark.gui
+def test_ok_updates_internal_multicamera_settings(dialog, qtbot):
+    """
+    Regression test for:
+    - 'adding another camera and hitting OK does not add the new extra camera'
+    when caller reads dialog._multi_camera_settings after closing.
+
+    Expected:
+    - OK emits updated settings
+    - dialog._multi_camera_settings is updated to match accepted settings
+    """
+
+    # Ensure backend combo matches the active camera backend, so duplicate logic behaves consistently
+    _select_backend_for_active_cam(dialog, cam_row=0)
+
+    # Scan and add a non-duplicate camera (index 1)
+    _run_scan_and_wait(dialog, qtbot, timeout=2000)
+    dialog.available_cameras_list.setCurrentRow(1)
+    qtbot.mouseClick(dialog.add_camera_btn, Qt.LeftButton)
+
+    qtbot.waitUntil(lambda: dialog.active_cameras_list.count() == 2, timeout=1000)
+    assert len(dialog._working_settings.cameras) == 2
+
+    # Click OK and capture emitted settings
+    with qtbot.waitSignal(dialog.settings_changed, timeout=2000) as sig:
+        qtbot.mouseClick(dialog.ok_btn, Qt.LeftButton)
+
+    emitted = sig.args[0]
+    assert isinstance(emitted, MultiCameraSettings)
+    assert len(emitted.cameras) == 2
+
+    # Check: internal source-of-truth must match accepted state
+    assert dialog._multi_camera_settings is not None
+    assert len(dialog._multi_camera_settings.cameras) == 2
+
+    # Optional: ensure camera identities match (names/index/backend)
+    assert [(c.backend, int(c.index)) for c in dialog._multi_camera_settings.cameras] == [
+        (c.backend, int(c.index)) for c in emitted.cameras
+    ]