Skip to content

GUI: Fix redundant signal calls, streamline path selections - #3395

Merged
deruyter92 merged 6 commits into
mainfrom
cy/analyze-vids-gui-fixes
Jul 20, 2026
Merged

GUI: Fix redundant signal calls, streamline path selections#3395
deruyter92 merged 6 commits into
mainfrom
cy/analyze-vids-gui-fixes

Conversation

@C-Achard

@C-Achard C-Achard commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Scope

Improve GUI file selection and signal blocking, while preserving path types and workflows.

Changes

  • Added binding-safe QFileDialog helpers with consistent Path/string conversion
  • Correctly handles single-file and multi-file dialog return values
  • Normalized selected video, snapshot, and conditions paths
  • Prevented feedback loops when synchronizing video type and shuffle widgets via signal blocking
  • Made GUI handling robust to temporarily unavailable pose configuration
  • Improved widget typing, editor lifetime, and noisy state-change logging

Goal

File selection now works more reliably, selected paths remain consistent throughout the GUI, and synchronized controls no longer trigger repeated clearing, slow updates, or state sync exceptions

@C-Achard C-Achard self-assigned this Jul 14, 2026
@C-Achard C-Achard added enhancement New feature or request GUI issues relating to GUI bug fix! fix for a real buggy one... labels Jul 14, 2026
@C-Achard
C-Achard requested a review from Copilot July 15, 2026 10:17
Copilot AI reviewed Jul 15, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves DeepLabCut’s Qt GUI reliability around file selection, path normalization, and synchronized widget state updates, reducing redundant signals and avoiding feedback loops during UI state synchronization.

Changes:

  • Added binding-safe QFileDialog helper wrappers and normalized Path/string handling for selected files.
  • Prevented unnecessary state churn by blocking signals during widget synchronization (e.g., shuffle/videotype updates) and by early-returning on no-op updates.
  • Made snapshot-selection UI logic more robust when pose configuration is temporarily unavailable.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
deeplabcut/gui/window.py Lowers verbosity for frequent GUI state-change logs (video type / clearing files).
deeplabcut/gui/tabs/train_network.py Guards against missing/None pose config when determining whether to show detector snapshot selection.
deeplabcut/gui/components.py Introduces QFileDialog helpers, normalizes selected paths, and tightens signal-blocking to prevent sync feedback loops.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@C-Achard
C-Achard marked this pull request as ready for review July 15, 2026 11:15
@C-Achard
C-Achard requested a review from deruyter92 July 15, 2026 11:16
deruyter92
deruyter92 approved these changes Jul 15, 2026
View reviewed changes

@deruyter92 deruyter92 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid, well-scoped PR, thanks!

I've added one commit to change str() conversion to os.fspath which is a bit more type-safe and consistent with the other places.

LGTM

Base automatically changed from jaap/pathlib_path_policy to main July 15, 2026 15:53
@C-Achard
C-Achard marked this pull request as draft July 16, 2026 08:02
@C-Achard

C-Achard commented Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Caution

Requires rebasing as the base was squashed

@C-Achard

C-Achard commented Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Solid, well-scoped PR, thanks!

I've added one commit to change str() conversion to os.fspath which is a bit more type-safe and consistent with the other places.

LGTM

Sorry, wouldn't Path usage make more sense given recent efforts in e.g. #3350 ? Should I adapt this file?

C-Achard and others added 4 commits July 16, 2026 10:12
@C-Achard
Lower GUI video logs to debug
5de072f 
Change two MainWindow log calls in `deeplabcut/gui/window.py` from info to debug when setting the video type and clearing video files. This reduces routine GUI noise in normal logs while keeping the messages available for troubleshooting.
@C-Achard
Guard Top-Down method check in training tab
659a0b6 
Hardened the GUI logic that toggles detector snapshot selection by safely handling missing `pose_cfg` or `method` values. The check now defaults to an empty dict/string before lowercasing, preventing runtime errors and preserving the intended Top-Down (`td`) behavior.
@C-Achard
Fix file selection and state sync
5c25f1a 
- Call QFileDialog positionally for PySide6 compatibility
- Normalize Path values before passing them to Qt
- Handle single-file and multi-file dialog results correctly
- Preserve Path objects in GUI state
- Prevent video type and shuffle signal feedback loops casing slow and redundant updates
- Retain ConfigEditor instances after opening
@deruyter92 @C-Achard
use os.fspath instead of str
4c84692
@C-Achard
C-Achard force-pushed the cy/analyze-vids-gui-fixes branch from ef05ba3 to 4c84692 Compare July 16, 2026 08:12
@C-Achard
C-Achard marked this pull request as ready for review July 16, 2026 08:12
@deruyter92
deruyter92 self-requested a review July 17, 2026 09:06
@deruyter92 deruyter92 self-assigned this Jul 17, 2026
@deruyter92

deruyter92 commented Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Sorry, wouldn't Path usage make more sense [...]?

QFileDialog expects a string, so at this boundary I think the conversion is warranted (which you did correctly).

Regarding os.fspath() vs str() for the conversion: the first one is theoretically more robust for edge case conversions and fails for None instead of converting to a string "None" . However, these benefits are purely theoretical and in practice I think in hindsight, actually conversion via str() is clearer. Let's try to consistently use that everywhere.

I'm reverting my commit.

@deruyter92
revert use os.fspath (d795bab)
193258e 
str() conversions are clearer
deruyter92
deruyter92 approved these changes Jul 17, 2026
View reviewed changes

@deruyter92 deruyter92 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Ready to merge I think!

@deruyter92
deruyter92 requested review from AlexEMG and MMathisLab July 17, 2026 11:27
@deruyter92
deruyter92 merged commit f10664e into main Jul 20, 2026
31 checks passed
@deruyter92
deruyter92 deleted the cy/analyze-vids-gui-fixes branch July 20, 2026 11:40
@deruyter92 deruyter92 mentioned this pull request Jul 20, 2026
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@AlexEMG AlexEMG AlexEMG approved these changes
@MMathisLab MMathisLab MMathisLab approved these changes
@deruyter92 deruyter92 deruyter92 approved these changes

Assignees

@MMathisLab MMathisLab

@deruyter92 deruyter92

@C-Achard C-Achard

Labels

bug fix! fix for a real buggy one... enhancement New feature or request GUI issues relating to GUI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@C-Achard @deruyter92 @AlexEMG @MMathisLab