Skip to content

feat: Add confirmation dialogs for folder changes and marked deletions#58

Merged
duartebarbosadev merged 2 commits intomainfrom
fix-losing-marked-deleted-when-switching-folders
Nov 6, 2025
Merged

feat: Add confirmation dialogs for folder changes and marked deletions#58
duartebarbosadev merged 2 commits intomainfrom
fix-losing-marked-deleted-when-switching-folders

Conversation

@duartebarbosadev
Copy link
Copy Markdown
Owner

This pull request introduces an improved and reusable confirmation dialog for handling uncommitted file deletions when closing the app or switching folders, along with some UI/UX enhancements and a minor documentation link update. The most significant changes are the refactoring of dialog logic for consistency, the addition of a new confirmation dialog for folder changes, and visual improvements to the dialog.

Dialogs and Confirmation Handling:

  • Refactored the confirmation dialog for uncommitted deletions into a reusable private method (_show_marked_files_confirmation_dialog), which supports both closing the app and switching folders with customizable titles, messages, and button labels. (src/ui/dialog_manager.py [1] [2]
  • Added a new show_folder_change_confirmation_dialog method to prompt the user when they attempt to switch folders with uncommitted deletions, giving options to commit, ignore, or cancel. (src/ui/dialog_manager.py [1] src/ui/app_controller.py [2]
  • Updated the folder loading logic to invoke the new folder change confirmation dialog and handle user choices appropriately (commit, ignore, or cancel). (src/ui/app_controller.py src/ui/app_controller.pyR235-R260)

UI/UX Improvements:

  • Increased dialog and thumbnail sizes, improved spacing, and adjusted alignment for a more user-friendly and visually appealing confirmation dialog. (src/ui/dialog_manager.py [1] [2] [3]
  • Made dialog button labels and object names context-specific and more descriptive, improving clarity and accessibility. (src/ui/dialog_manager.py [1] [2]

Documentation:

  • Updated the documentation link in the best shot models missing dialog to point to the new section in the README. (src/ui/dialog_manager.py src/ui/dialog_manager.pyL1744-R1787)

Copilot AI review requested due to automatic review settings November 6, 2025 10:49
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 refactors dialog code to handle uncommitted deletions in multiple scenarios and updates a GitHub URL. The primary change is extracting the close confirmation dialog logic into a reusable helper method and adding a new folder change confirmation dialog.

Key changes:

  • Extracted common dialog logic into _show_marked_files_confirmation_dialog() helper method
  • Added new show_folder_change_confirmation_dialog() to handle uncommitted deletions when switching folders
  • Updated the Best Shot models documentation URL to reflect current GitHub structure

Reviewed Changes

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

File Description
src/ui/dialog_manager.py Refactored close confirmation dialog into reusable helper method, added folder change confirmation dialog, updated documentation URL
src/ui/app_controller.py Added logic to check for uncommitted deletions before loading a new folder and prompt user with confirmation dialog

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

Comment on lines +254 to +255
self.app_state.clear_all_deletion_marks()
self.main_window._refresh_visible_items_icons()
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

Inconsistent use of app_state methods compared to the existing closeEvent implementation in main_window.py. In main_window.py:1529, the pattern uses self.app_state.marked_for_deletion.clear() followed by _refresh_visible_items_icons(), but the new code here uses self.app_state.clear_all_deletion_marks(). Additionally, main_window.py has a _clear_all_deletion_marks() method (line 4004) that uses the deletion_controller for proper model updates, which should be used instead to ensure consistent behavior across the codebase.

Suggested change
self.app_state.clear_all_deletion_marks()
self.main_window._refresh_visible_items_icons()
self.main_window._clear_all_deletion_marks()

Copilot uses AI. Check for mistakes.

def show_folder_change_confirmation_dialog(self, marked_files: List[str]) -> str:
"""
Shows a confirmation dialog when changing folders with marked files.
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

The docstring is incomplete. It should include Args and Returns sections matching the pattern used in show_close_confirmation_dialog() (lines 1643-1651) for consistency.

Suggested change
Shows a confirmation dialog when changing folders with marked files.
Shows a confirmation dialog when changing folders with marked files.
Args:
marked_files: The list of files marked for deletion.
Returns:
A string indicating the user's choice: "commit", "ignore", or "cancel".

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

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


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

@duartebarbosadev duartebarbosadev merged commit b16369d into main Nov 6, 2025
9 checks passed
@duartebarbosadev duartebarbosadev deleted the fix-losing-marked-deleted-when-switching-folders branch November 6, 2025 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants