Fix camera config UI responsiveness issues#58
Open
C-Achard wants to merge 13 commits intocy/pre-release-fixes-2.0from
Open
Fix camera config UI responsiveness issues#58C-Achard wants to merge 13 commits intocy/pre-release-fixes-2.0from
C-Achard wants to merge 13 commits intocy/pre-release-fixes-2.0from
Conversation
Add showEvent to reset the cleanup guard and rebuild the working settings from the latest accepted settings, then repopulate the dialog. Allow removing/moving active cameras even while a scan is running and adjust preview button enablement logic. Auto-select the first active camera when populating the list and ensure multi_camera_settings is updated from the working copy before emitting settings_changed on apply.
Add two end-to-end GUI tests for camera configuration dialog to prevent regressions: - test_remove_active_camera_works_while_scan_running: Verifies that removing the active camera still works while a discovery scan is running. The test slows CameraFactory.detect_cameras via monkeypatch to keep the scan running, ensures the remove button is enabled during scan, removes the selected camera, and cleans up by cancelling the scan. - test_ok_updates_internal_multicamera_settings: Ensures that after adding a second camera and accepting the dialog (OK), the settings_changed signal emits the updated MultiCameraSettings and the dialog's internal _multi_camera_settings is updated to match the accepted settings. These tests guard against regressions where scan-running state blocked structure edits (remove/move) and where dialog acceptance did not update the dialog's internal settings.
C-Achard
added
bug
There was a problem hiding this comment.
Pull request overview
Improves the robustness of the camera configuration dialog when reopening it and while camera discovery scanning is in progress, and adds GUI E2E regression tests to prevent those issues from recurring.
Changes:
- Reinitialize the dialog’s working state on show, and keep the internal “accepted settings” source-of-truth in sync when OK is pressed.
- Allow active-camera structural edits (remove/move) while a discovery scan is running, and auto-select the first active camera after repopulating.
- Add E2E regression tests covering remove-during-scan and OK syncing internal settings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
dlclivegui/gui/camera_config/camera_config_dialog.py |
Resets dialog editing state on show, enables remove/move during scanning, selects first active camera on repopulate, and syncs _multi_camera_settings on OK. |
tests/gui/camera_config/test_cam_dialog_e2e.py |
Adds GUI E2E regression tests for remove-while-scanning and OK updating internal multicamera settings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Introduce a helper to detect live previews and replace scattered direct PreviewState checks with it. Add validation for selected detected-camera items (show a warning if invalid). Normalize camera backend id casing when formatting labels. Remove a redundant try/except around cleanup guard reset and always reset _cleanup_done on show. Drop an extra in-place settings assignment in the apply flow and remove a now-unneeded call to _populate_from_settings on show. These changes tidy preview control, improve robustness for camera selection, and unify camera identity handling.
Clean up and shorten comments in tests/gui/camera_config/test_cam_dialog_e2e.py: remove parenthetical notes about expected failures tied to implementation details and replace them with concise, direct expectations. No functional test logic changed.
Refine CameraConfigDialog behavior to prevent races and invalid states: only clean up the scan worker when it's not running; disable the Add Camera button unless the selected item is a DetectedCamera; improve probe worker cancellation and waiting to avoid concurrent probes; emit the multi-camera model copy instead of a deepcopy when settings change. Add _enabled_count_with and enforce MAX_CAMERAS when enabling a camera and before closing the dialog. Clamp crop coordinates and only apply crop when the rectangle is valid to avoid invalid-frame cropping. Small comment/organization tweaks included.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Ensure selection state is synchronized after initialization by calling _populate_from_settings and a new _post_init_sync_selection to set _current_edit_index. Install event filters on camera numeric spinboxes and their lineEdits, and improve Enter-key handling so interpretText() is invoked on the correct spinbox (whether the event comes from the spinbox or its lineEdit) before applying camera settings. Introduce _selected_detected_camera helper to centralize retrieval of the selected detected camera and use it to control the Add button enablement; simplify scan-related UI enable/disable logic and remove duplicated state handling. Miscellaneous cleanup and minor refactors in camera_config_dialog.py.
Normalize dlc_camera_id values (split on ':' and lowercase backend part, fallback to lowercasing whole string) and refresh camera labels accordingly. Improve camera discovery shutdown: set scan state to CANCELING when stopping, avoid immediate cleanup if the scan thread is still running, and route worker finished to a new handler (_on_scan_thread_finished) that performs cleanup and sets state to IDLE. Also stop calling _populate_from_settings() when opening the camera dialog to avoid overwriting unsaved changes; let the dialog refresh itself when shown.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Introduce caching for camera discovery to avoid unnecessary rescans by adding _last_scan_backend and _has_scan_results and a _maybe_refresh_available_cameras(force=False) helper that only calls _refresh_available_cameras when backend or cache state requires it. Refactor population logic: add _populate_active_list_from_working to fill the active cameras list (preserving selection optionally) and simplify _populate_from_settings to use it. Add public set_settings(...) to update the dialog with new MultiCameraSettings and optional dlc_camera_id without destroying unsaved UI state. Improve scan worker cleanup logic and add a debug message to clarify deferred cleanup. Update backend-change handling to invalidate cache, and update scan result handlers to set cache flags appropriately. Minor UI tweaks: remove a stray currentRow() call, ensure button states are updated, and switch main window to call set_settings(...) when reusing the dialog so the dialog manages its own refresh behavior.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace a bare exception handler when installing event filters with specific (AttributeError, RuntimeError) handling and log a warning so failures are not silently swallowed. Enhance _on_scan_thread_finished to ignore finished signals from stale workers, attempt deleteLater() on old senders, and only transition the active worker to IDLE to avoid race conditions affecting scan state.
Introduce explicit cleanup flags and tighten cleanup logic to avoid races and double-run teardown. Add _cleanp_requested and _cleanup_completed, replace uses of the old _cleanup_done flag, and only mark cleanup completed when no scan/preview/probe worker is active. Also always transition the scan state to IDLE when cleaning up. Update the e2e test to wait for scans to finish earlier and reduce related timeouts to reflect the more deterministic teardown behavior.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
Fixes a few camera config UI issues that could arise when reopening the dialog or adding new cameras, such as :
Also adds e2e tests to avoid regression.
Improves the usability and reliability of the camera configuration dialog in the GUI, addressing key issues around camera management during scanning and ensuring internal state consistency after accepting changes. The changes also add regression tests to prevent future breakage of these behaviors.
Camera management improvements:
Internal state consistency:
_multi_camera_settings) is updated to match the accepted settings, ensuring that subsequent reads of the dialog's state reflect the user's choices.Regression test coverage: