Improve camera config loading UX and improve Basler backend #45
+1,737
−438
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Refactor and harden BaslerCameraBackend: add a basler namespace view/ensure helpers, stable device_id (serial) handling with legacy fallbacks, and persistence of device identity. Introduce fast_start probe mode to avoid starting acquisition/creating converter, and an apply_transforms option to perform rotation/crop in read(). Add testable device enumeration helpers ( _enumerate_devices_cls, discover_devices, get_device_count, quick_ping ) and utility operations (rebind_settings, sanitize_for_probe) to improve discovery, rebinding, and fast probing. Improve open()/read()/close() with safer node access, better logging, explicit handling of exposure/gain/fps, resolution snapping to node increments/ranges, read-back of actual telemetry, and clearer errors when operating in fast-start probe mode. Maintain backward compatibility with legacy properties (serial, resolution) while preferring namespaced options.
Move camera probe options out of commented code and explicitly disable fast_start and apply_transforms during preview probing to avoid double transforms (Basler) and ensure a full open when probing. Replace _needs_preview_reopen with _should_restart_preview and implement a backend-agnostic policy: restart preview only for camera-side capture parameter changes (width, height, fps, exposure, gain) and do not restart for rotation/crop to provide a faster UX. Improve _apply_camera_settings by preventing applies while a loader is active, computing/logging a pre-apply diff, persisting the validated model, and then deciding whether to restart the preview; use a short QTimer delay for driver stability when restarting. Add debug/info logging around preview start/stop, loader success, backend close, and more robust error logging for loader failures. Small cleanup of exception handling and safer restart decision behavior.
Remove the apply_transforms flag and associated transform logic from dlclivegui/cameras/backends/basler_backend.py. This deletes the _apply_crop helper, the attribute initialization for _apply_transforms, and the code that applied rotation and cropping to captured frames, leaving frames unmodified by the backend. Simplifies the Basler backend by delegating any image transforms to upstream/post-processing.
Config: tighten CameraSettings crop handling — treat all-zeros as "no crop", allow x1/y1 == 0 to mean "to edge", and only enforce x1>x0 / y1>y0 when x1/y1 are explicitly >0. GUI (camera_config_dialog): remove backend apply_transforms tweak in probe worker; add robust auto-commit behavior to prevent losing/accepting invalid edits when switching selection, adding/removing/reordering cameras or closing the dialog. Introduce _commit_pending_edits() which attempts to apply pending changes (and shows a warning on failure), make _apply_camera_settings() return success as a boolean, and block UI actions when validation fails. Also auto-applies pending settings on OK and ensures previews are stopped appropriately. Main window: ensure camera.properties is a dict and set backend-namespaced properties; set fast_start in backend namespace (and initialize properties safely) instead of the old top-level quick default. These changes improve UX by validating/committing edits proactively and make crop semantics clearer and more flexible.
Add a visual "dirty" state for the Apply Settings button to make unapplied camera config edits more visible. Introduces _set_apply_dirty to toggle button text, icon and tooltip, and a _mark_dirty handler that is connected to various input signals (valueChanged, currentIndexChanged, stateChanged). Ensure the dirty/state is cleared when populating settings and after a successful apply, and unify/replace previous ad-hoc enable calls so the button reliably reflects pending edits.
Basler backend: simplify and harden exposure/gain configuration by only applying positive values, turning off auto modes when available, and logging failures as warnings. Harden stream lifecycle for previews by stopping grabbing if needed, creating the ImageFormatConverter before StartGrabbing, forcing MaxNumBuffer, starting grabbing reliably and logging grab state; also wrap StopGrabbing on shutdown to ignore errors. Camera config UI: refine property merge and UI behavior — avoid reloading the form after merge, safely refresh camera labels by guarding null lists and blocking signals, ignore redundant selection events, and add explicit preview restart/start helpers (_restart_preview_for_camera and _start_preview_with_camera). Ensure previews never use fast_start mode and improve logging around selection and preview startup.
C-Achard
added
enhancement
Avoid unwanted UI-driven side-effects when programmatically updating or selecting cameras. Add suppression flags to skip selection handlers/ actions during updates and preview startup, and wrap preview start/stop in a try/finally to restore suppression. Block widget signals while loading camera settings to prevent spurious change events, add exposure/gain to the form field groups, and skip probing when a preview is active. Also add a few diagnostic logs to help trace selection/preview behavior.
Improve camera dialog test fixture to show the dialog, yield it to tests, and perform a robust teardown: stop preview, reject/close the dialog, and wait for loader/scan worker threads and preview state to clear. Also add an autouse fixture that monkeypatches QMessageBox methods to raise on any unexpected modal dialog, preventing tests from hanging due to blocking message boxes. Small formatting/inline cleanup of MultiCameraSettings initialization included.
Ensure the probe worker is cleanly stopped when shutting down: call request_cancel(), wait (up to 1500ms), and clear the _probe_worker reference to avoid dangling threads. Also make a copy of the QImage created from the frame buffer (QImage(...).copy()) so the Qt image doesn't reference the underlying frame memory, preventing use-after-free crashes or visual corruption.
Camera dialog: tighten preview lifecycle and disabled-control handling. Introduces a preview-starting flag, commits pending edits before starting preview, consolidates loader/start/stop flow, and treats exposure/gain as 0 when their controls are disabled so they won't trigger unnecessary restarts. Also fixes various preview UI/loader state transitions and button states during async start/stop. Tests: large refactor of test fixtures to provide deterministic fake backends and DLCLive doubles. Adds reusable test backend helpers (make_backend_class, temp_backend, register_fake_backend_session), a stable fake_backend_factory, and simplifies GUI autouse patches. Expands and reorganizes unit and end-to-end camera-config tests to cover preview start/stop, restart/no-restart semantics, scan cancellation, duplicate/max-camera guards, commit-on-select/OK behavior, crop validation, and backend capability handling. Updates gui conftest to use the new fake backend and DLCLive test doubles.
Stabilize GUI tests by making backend discovery and capabilities deterministic. Add _select_backend_for_active_cam helper to ensure combo/backend coherence during tests. Replace global fake/opencv mixing with a patch_detect_cameras fixture (staticmethod) and use a stable 'fake' backend in E2E fixtures. Inline CountingBackend in relevant tests and monkeypatch CameraFactory.create as staticmethod to avoid hardware access. In unit tests, patch backend_capabilities for predictable enable/disable state and switch assertions to FPS (supported) instead of gain. Misc: minor test cleanups, docstring tweaks, and staticmethod adjustments for slow scan/loader helpers.
C-Achard
changed the title
There was a problem hiding this comment.
Pull request overview
Improves camera configuration UX and robustness by auto-committing pending edits before context changes, tightening validation, and refining preview restart behavior; also enhances the Basler backend’s identity/discovery handling and stabilizes GUI testing.
Changes:
- Added auto-commit/validation guards for camera edits and refined when preview restarts.
- Expanded GUI test infrastructure (fake backend registration, deterministic dialog fixtures, modal-messagebox fail-fast).
- Improved Basler backend device discovery, stable identity persistence, and configuration handling.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/gui/conftest.py | Simplifies factory patching and adds an autouse guard to prevent modal QMessageBox hangs in GUI tests. |
| tests/gui/camera_config/test_cam_dialog_unit.py | Adds deterministic unit tests for dirty-state, auto-commit, validation blocking, and Enter-to-apply behavior. |
| tests/gui/camera_config/test_cam_dialog_e2e.py | Adds end-to-end coverage for async scan, preview start/stop, restart policy, duplicate/max-camera guards, and cancel-loading. |
| tests/conftest.py | Introduces reusable fake backend registration/helpers and consolidates test configuration building. |
| dlclivegui/gui/main_window.py | Changes per-camera fast_start handling in preview startup to use backend namespaces and disable fast-start for preview. |
| dlclivegui/gui/camera_config_dialog.py | Implements dirty UI state, pending-edit auto-commit, preview restart policy, selection-update suppression, and improved teardown/cancellation. |
| dlclivegui/config.py | Updates crop validation semantics to allow x1/y1=0 to mean “to edge” with clearer errors. |
| dlclivegui/cameras/backends/basler_backend.py | Adds stable identity (serial) namespacing, richer discovery, rebind/sanitize helpers, and more robust open/config behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1599
to
+1605
| if self._preview_active and isinstance(old_settings, CameraSettings): | ||
| restart = self._should_restart_preview(old_settings, new_model) | ||
| # If the preview is starting but not fully active yet, | ||
| # we can skip the restart since the new settings will be picked up on start anyway | ||
| if self._preview_active and not getattr(self, "._preview_starting", False): | ||
| if restart: | ||
| QTimer.singleShot(0, lambda cam=new_model: self._restart_preview_for_camera(cam)) |
There was a problem hiding this comment.
The “preview starting” guard is broken (getattr(self, "._preview_starting", False) looks up an attribute name with a leading dot), and the restart is scheduled twice (once in the restart decision block and again unconditionally in the if self._preview_active: block). This can cause duplicate preview restarts and flaky timing. Use a single, correctly named flag (e.g., self._preview_starting) and schedule the restart in exactly one place.
Suggested change
| if self._preview_active and isinstance(old_settings, CameraSettings): | |
| restart = self._should_restart_preview(old_settings, new_model) | |
| # If the preview is starting but not fully active yet, | |
| # we can skip the restart since the new settings will be picked up on start anyway | |
| if self._preview_active and not getattr(self, "._preview_starting", False): | |
| if restart: | |
| QTimer.singleShot(0, lambda cam=new_model: self._restart_preview_for_camera(cam)) | |
| # Only consider restarting the preview if it's active, we have valid settings, | |
| # and a preview start isn't already in progress. | |
| should_consider_restart = ( | |
| self._preview_active | |
| and isinstance(old_settings, CameraSettings) | |
| and not getattr(self, "_preview_starting", False) | |
| ) | |
| if should_consider_restart: | |
| restart = self._should_restart_preview(old_settings, new_model) |
Comment on lines
1615
to
+1618
| if self._preview_active: | ||
| if must_reopen: | ||
| self._stop_preview() | ||
| self._start_preview() | ||
| if restart: | ||
| self._append_status("[Apply] Restarting preview to apply camera settings…") | ||
| QTimer.singleShot(0, lambda cam=new_model: self._restart_preview_for_camera(cam)) |
There was a problem hiding this comment.
The “preview starting” guard is broken (getattr(self, "._preview_starting", False) looks up an attribute name with a leading dot), and the restart is scheduled twice (once in the restart decision block and again unconditionally in the if self._preview_active: block). This can cause duplicate preview restarts and flaky timing. Use a single, correctly named flag (e.g., self._preview_starting) and schedule the restart in exactly one place.
| return | ||
| if self._preview_active or self._loading_active: | ||
| return | ||
| self.starting_preview = True |
There was a problem hiding this comment.
The dialog defines self._preview_starting in __init__, but _start_preview sets/clears self.starting_preview (a different attribute). This makes any logic that relies on _preview_starting ineffective and introduces state inconsistencies. Replace self.starting_preview with self._preview_starting (and update any checks to use the same flag).
Suggested change
| self.starting_preview = True | |
| self._preview_starting = True |
Comment on lines
+1818
to
+1819
| finally: | ||
| self.starting_preview = False |
There was a problem hiding this comment.
The dialog defines self._preview_starting in __init__, but _start_preview sets/clears self.starting_preview (a different attribute). This makes any logic that relies on _preview_starting ineffective and introduces state inconsistencies. Replace self.starting_preview with self._preview_starting (and update any checks to use the same flag).
| cam.backend, | ||
| cam.index, | ||
| ) | ||
| self._suppress_selection_actions = True |
There was a problem hiding this comment.
There are two suppression flags with different names (_suppress_selection_actions vs _suppress_selection_change) used in different places. As written, _restart_preview_for_camera toggles _suppress_selection_actions, but _on_active_camera_selected only checks _suppress_selection_change, so the suppression intent won’t apply consistently. Consolidate to a single flag name and use it everywhere.
| # Start preview without relying on selection state | ||
| self._start_preview_with_camera(cam) | ||
| finally: | ||
| self._suppress_selection_actions = False |
There was a problem hiding this comment.
There are two suppression flags with different names (_suppress_selection_actions vs _suppress_selection_change) used in different places. As written, _restart_preview_for_camera toggles _suppress_selection_actions, but _on_active_camera_selected only checks _suppress_selection_change, so the suppression intent won’t apply consistently. Consolidate to a single flag name and use it everywhere.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
UI/UX improvements
This pull request introduces significant improvements to the camera configuration dialog, focusing on robust handling of pending edits, improved validation, and a more intuitive user experience when editing camera settings. The changes ensure that any unsaved or invalid edits are automatically validated and committed before users perform actions that would change context, such as switching cameras, adding/removing/reordering cameras, or closing the dialog. Additionally, the logic for when to restart the camera preview has been clarified and simplified, and the UI now provides clearer feedback about pending changes.
Most important changes:
Robust Edit Commitment and Validation
_commit_pending_editsmethod that auto-applies and validates edits before context-changing actions (e.g., switching selection, adding/removing/reordering cameras, closing the dialog). If validation fails, the action is blocked and the user receives a clear warning. This mechanism is now called at all relevant points in the dialog. [1] [2] [3] [4] [5] [6] [7]Preview Restart Logic and Change Tracking
_needs_preview_reopenmethod with_should_restart_preview, which now restarts the preview only for changes to camera-side capture parameters (resolution, fps, exposure, gain), and not for rotation/crop/enabled, providing a smoother user experience. Also, added detailed logging of changes and preview restart decisions. [1] [2]UI Feedback and Dirty State Management
_set_apply_dirtyto visually indicate when there are unapplied changes, updating the Apply button's text, icon, and tooltip. All relevant input fields now mark the settings as dirty and enable the Apply button upon change. [1] [2] [3]Crop Rectangle Validation
x1/y1of zero to mean "to edge", and clarified error messages for invalid crop rectangles, making cropping more flexible and user-friendly.UI and Usability Enhancements
These changes collectively make the camera configuration dialog more robust, user-friendly, and less error-prone, especially when dealing with multiple edits and camera management actions.
Testing infrastructure overhaul
This pull request also significantly refactors and improves the test infrastructure for camera backends and application configuration in the codebase. The main focus is on reducing duplication, improving maintainability, and making test fixtures more flexible and reusable.
Additionally, there are targeted improvements to camera cropping validation logic and camera property handling.
Key changes include:
Test Infrastructure Refactoring
make_backend_class,_temp_backend) to create lightweight, configurable camera backend classes for tests, removing duplication of fake backend logic. [1] [2]register_fake_backend_session,fake_backend_cls,fake_backend_factory,temp_backend), enabling deterministic and isolated test setups.make_app_config, simplifying the creation of test configs with multiple cameras and custom options.Camera Cropping and Property Handling
dlclivegui/config.pyto allowx1/y1values of 0 to mean "to edge", and clarified error handling for invalid crop rectangles.fast_starttoFalsein the appropriate context.These changes improve test reliability, reduce boilerplate, and make the codebase easier to extend for new camera backends and configurations.