Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Delete functionality for Peek #35418

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

daverayment
Copy link
Contributor

@daverayment daverayment commented Oct 13, 2024

Summary of the Pull Request

This PR adds the ability for users to delete the file currently being previewed in Peek.

PR Checklist

Detailed Description of the Pull Request / Additional comments

There are several new and changed elements in this PR:

  • Navigation has been updated to track forward and backward movement through files. This is necessary so deleting a file always shows the next item in the natural sequence, i.e. the previous item if the user was moving backwards. This replicates Windows Photo and other media viewers.
  • The indexes of deleted items are recorded rather than altering the Items collection (which could be very large). There are now DisplayIndex and DisplayItemCount properties, which reflect the calculated position and total. The old CurrentIndex still points to the actual index of the current item in the collection, but this is now internal only.
  • The MainWindow ViewModel includes quite a bit of navigation-related code. We may wish to split this out into a separate model or provider in the future.
  • When previewing a selection of items (where the number and total are shown in the title bar), these counts will always be shown. Previously, this info would not show if the count was 1. (This was likely because opening Peek while a single item is selected expands the Items collection to cover the entire folder and turns off the "(position/total)" display.)
  • A text message has been added for when there are no more files remaining, i.e. the user has deleted the last one from their selection (or from the current folder if Peek wasn't opened via multi-selection). It may be worthwhile to add a simple icon by the side here - perhaps an image placeholder graphic? - but I've gone with the simple approach for now.
  • Changes were made to the title bar code to ensure that the last filename and "open in application" details were not shown when there are no more files to preview. This was because there was an (entirely reasonable!) assumption in the code that there would always be one or more files being previewed.
  • The delete operation sends the item to the Recycle Bin only. It was considered whether pressing Shift + Delete could do a permanent delete, and this is certainly possible in the future, but this release errs on the side of caution, especially as it's a new feature and involves deleting things.
  • The Delete function itself relies upon a shell file operation, reflecting the fact that we're previewing shell items. This required adding some code to NativeMethods.cs for the API call, struct, flags, and error messages.
  • Only file deletion is allowed. I considered allowing for empty folder deletes, too, but, again, this first release is deliberately conservative.
  • Updated Peek documentation.

Screenshots

No More Files message

Screenshot 2024-10-13 230651 (Small)

File Count For Single Remaining File

image

Validation Steps Performed

(Manual tests only.)

Tested:

  • Single and multi-selection methods of opening Peek.
  • Deleting from beginning, end and middle of a sequence (after opening Peek via multi-selection).
  • Deleting until no files remained.
  • Previewing a folder containing a mix of files and folders.
  • Previewing a folder containing a mix of files which can be previewed and unsupported file types.
  • Attempting to delete folders via Peek.
  • Deleting a file in the selected list via Explorer before it could be previewed.

Removed package updates, as these are not relevant to the new functionality.
…rce text. Also altered the text style to be consistent with the FailedFallbackPreviewControl error page.
src/modules/peek/Peek.FilePreviewer/FilePreview.xaml Outdated Show resolved Hide resolved
src/modules/peek/Peek.UI/MainWindowViewModel.cs Outdated Show resolved Hide resolved
{
wFunc = FO_DELETE,
pFrom = path + "\0\0",
fFlags = (ushort)(FOF_NOCONFIRMATION | (permanent ? 0 : FOF_ALLOWUNDO)),
Copy link
Collaborator

@htcfreek htcfreek Oct 14, 2024

Choose a reason for hiding this comment

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

Does this override the recycle bin property? (It is possible to configure the delete confirmation in the recycle bin properties.)

How does Fotos app handle it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This replicates how the Photos application handles deletes. Pressing Delete immediately sends the file to the Recycle Bin without popping up a message, and then it moves on to the next file.

The permanent argument is there for possible future use, but is always false for now.

src/modules/peek/Peek.UI/MainWindowViewModel.cs Outdated Show resolved Hide resolved
@htcfreek
Copy link
Collaborator

@daverayment
Please fix the merge conflicts. (This is the reason for the failing test.)

This comment has been minimized.

@daverayment
Copy link
Contributor Author

@htcfreek

Thanks for your comments and suggestions.

I've updated the code with an error InfoBar, which is shown when a delete fails:

image

I've added user-friendly messages for the most common failures. Others result in a generic message being shown on the InfoBar.

The failure is also logged, with the code and a more technical error message (taken from winerror.h).

If the file still exists after the deletion attempt, it is kept in the items collection for previewing; otherwise it is removed.

An aside: Windows Photo handles files being renamed or deleted differently to Peek, in that it seems to constantly monitor the folder. (Try changing the filename of a photo you're previewing and you can see it update in the UI after a second or two.) We may wish to replicate this functionality in the future, as it would be increase robustness, but that it outside of the scope of this issue.

@crutkas crutkas added the Needs-Review This Pull Request awaits the review of a maintainer. label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Review This Pull Request awaits the review of a maintainer.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants