Skip to content

Add support for Recycle Bin #840

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

Merged
merged 18 commits into from
May 31, 2020
Merged

Conversation

gave92
Copy link
Member

@gave92 gave92 commented May 25, 2020

Hello this pull request adds support for browsing the Recycle Bin!
"Support" here means that the recycle bin can be browsed just like any other folder in Files UWP, it's not just a "link" that opens the recycle bin in explorer.

image

How it works

As I'm not aware of any way to properly access the recycle bin folder from the UWP app, I'm making use of Files UWP full trust process. The fulltrust process enumerates the contents of the recycle bin using the Shell32 api and send the results to the UWP app. Communication between the fulltrust process and the UWP app is done through AppService.

NB: This requires to change the fulltrust process lifecyle: instead of starting a new instance every time to handle the commands to open CMD, toggle QuickLook, etc., a single instance keeps running until the UWP app is closed or suspended.

What works

  • New entry on the sidebar with localized name of the recycle bin
  • Enumerating recycle bin elements
  • Monitor filesystem changes and refresh items
  • Showing the thumbnails for deleted files
  • Properties dialog for deleted files
  • Deleting files from the recycle bin

What's missing

  • A button to empty recycle bin
  • A customized context menu with the proper options (e.g "restore")

Let me know what you think of this, if you approve of the "architectural" changes I can keep working to complete what's missing.

@gave92 gave92 marked this pull request as ready for review May 25, 2020 21:31
@ghost ghost added the needs - code review label May 25, 2020
@lukeblevins

This comment has been minimized.

@yaira2
Copy link
Member

yaira2 commented May 26, 2020

Support for recycle bin would be great, I am not sure if we want to keep it pinned by default. Can you add a feature flag that turns on/off this feature?

@gave92
Copy link
Member Author

gave92 commented May 26, 2020

@yaichenbaum yup I can do that, perhaps also add it to the New Tab page? Anyway is not the UI part that worries me :)

@duke7553 I've tried using the Shell32 api from the UWP side (meaning using the function SHGetFolderLocation and other IShellFolder related methods and for some reason I cannot enumerate the contents of the recycle bin. The same code works when executed in the fulltrust process.
Is this what you were suggesting by "accessing the recycling bin by it's GUID"?

@yaira2
Copy link
Member

yaira2 commented May 26, 2020

@gave92 I would add it to the new page, but it should be behind the feature flag as well.

@gave92
Copy link
Member Author

gave92 commented May 26, 2020

@yaichenbaum I'll do that. I'm gonna wait a bit though, until I get some feedback on proposed method for accessing the recycle bin (fulltrust process + appservice).
Perhaps there's a simpler way of doing that that I'm missing.

@yaira2 yaira2 requested review from lukeblevins and tsvietOK May 27, 2020 13:25
@tsvietOK
Copy link
Contributor

tsvietOK commented May 27, 2020

@gave92
First
Repro steps:

  1. Open recycle bin
  2. Navigate to folder inside it
  3. Press Up button on navigation panel
  4. Permission dialog displayed

Second
I think folders in recycle bin should not have ability to be opened, renamed, pinned to sidebar, shared, opened in new windows/tab.
Buttons like Add new item and Open in Terminal should be disabled

Third
"Recycle Bin" tab text untranslated
image

Fourth
Button "Copy path" causes app crash(try to copy path several times)

@tsvietOK
Copy link
Contributor

And last, i don't think it is a good idea to merge this PR without supporting restore and clear recycle bin functions.

@tsvietOK
Copy link
Contributor

Other way, mark this feature as Experimental

@yaira2
Copy link
Member

yaira2 commented May 27, 2020

And last, i don't think it is a good idea to merge this PR without supporting restore and clear recycle bin functions.

We can do this behind a feature flag, this way we are not making any commitment to the feature and the capabilities can be changed.

@gave92
Copy link
Member Author

gave92 commented May 27, 2020

Thanks for the feedbacks.

@tsvietOK

  1. Permission dialog displayed => will be fixed when disallowing navigation inside recycle bin folders (see 2)

  2. I think folders in recycle bin should not have ability to be opened, renamed, pinned to sidebar, shared, opened in new windows/tab => agreed, need to customize the context menu and buttons (e.g add an "Empty recycle bin" button and a "Restore" to context menu)

  3. "Recycle Bin" tab text untranslated => solved

  4. Button "Copy path" causes app crash => I cannot reproduce this, but it will go away when we change the buttons/context menu (see 2)

@yaira2
Copy link
Member

yaira2 commented May 28, 2020

  • By default, recycle bin should not be pinned to the sidebar.
  • The app crashes when navigating to recycle bin.

@gave92
Copy link
Member Author

gave92 commented May 28, 2020

@yaichenbaum Ok I'll change the default to not pinned.
I'm aware of the crash, I'm waiting for the owner of the Vanara library I'm now referencing to publicly release the bug fixed version (I have it locally but it's not on nuget yet).

@yaira2
Copy link
Member

yaira2 commented May 28, 2020

@gave92 Is there a way I can test this out in the meantime?

@gave92
Copy link
Member Author

gave92 commented May 28, 2020

@yaichenbaum Yes please try again with the latest commit.
Notes:

  • auto refresh of recycle bin contents is disabled until the release of the updated Vanara library
  • creating/pasting/... a file in the recycle bin crashes the app, I'm not fixing this as the solution will be to have a different context menu and buttons for the recycle bin

@yaira2
Copy link
Member

yaira2 commented May 28, 2020

@gave92 I can now open recycle bin but it appears to be empty.

@gave92
Copy link
Member Author

gave92 commented May 28, 2020

@yaichenbaum Can you check whether while you are in the recycle bin folder (in Files UWP) there's a process called FilesFullTrust.exe running in Task Manager?

@yaira2
Copy link
Member

yaira2 commented May 28, 2020

@gave92 I do not see FilesFullTrust.exe in Task Manager.

@gave92
Copy link
Member Author

gave92 commented May 28, 2020

Thanks for the feedback, the issue should now be fixed!

Copy link
Contributor

@lukeblevins lukeblevins left a comment

Choose a reason for hiding this comment

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

With the guarantee that this feature is behind a flag, and otherwise invisible, I'll approve this now.

gave92 added 3 commits May 29, 2020 20:10
Restore AppService after suspending
Enumerate recycle bin items
Use real (recycle bin) item path
Display recyclebin localized name
Fix up click button, test FileWatcher and Empty recyclebin
Treat zip as files, Add file size in bytes
Fix properties page, refresh on filesystem changes, move ShellFileItem to common project
Fix create file in recyclebin
Fix watchedItemsOperation null in recyclebin, fix EmptyTextState visibility on file delete
Fix crash when opening file in external app
Fix OpenShellCommandInExplorer
Add flag to pin recyclebin to sidebar
Properly watch for recycle bin updates
Recycle bin not pinned by default
Fix exception when deleting many files
Simplified method to get file size string
@lukeblevins
Copy link
Contributor

@gave92 Thanks for clarifying this. As long as the recycle bin location is only visible when a flag inside the Experimental Settings page is toggled from its default state, we can merge this when you've finished work on it.

@gave92
Copy link
Member Author

gave92 commented May 30, 2020

A little update on this:

  • Added customized context menu with "Empty recycle bin" action
  • Added customized context menu for recycle bin items with "Restore" action
  • Disabled menu bar "New file" action in recycle bin
  • Disabled renaming of items in recycle bin
  • Disabled navigation and opening of items in recycle bin
  • The option to enable recycle bin support is now in the Experimental settings page

What's missing:

  • Proper support for cutting from recycle bin: the pasted file will have a funny name like "$RIIJXS0.ext". I'm not sure if we can support this as cutting/pasting uses the Clipboard and I don't know of a way to specify a different destination name when pasting the file
  • Enable auto refresh when recycle bin contents change: I'm waiting for the release of an updated version of the Vanara library (used to invoke win32 api in a sane manner)

Let me know what you think!

@yaira2
Copy link
Member

yaira2 commented May 31, 2020

@duke7553 @gave92 I should have clarified the first time that when I said feature flag, I meant the experimental settings page.

@yaira2
Copy link
Member

yaira2 commented May 31, 2020

@gave92 The app doesn't crash but it still won't load any items I have in recycle bin.

@gave92
Copy link
Member Author

gave92 commented May 31, 2020

Curious. Please check the following:

  • That there is a "FilesFullTrust.exe" process running after you open the uwp app
  • That you can open files in normal folders when tapping them in FilesUWP
  • That no "FilesFullTrust.exe" process is running after you visit the bin from the uwp app

In the application data path ("%localappdata%\packages\FilesUWPDev_xxxxx\LocalState" there should be a "debug_fulltrust.txt" with the logged exception

@yaira2
Copy link
Member

yaira2 commented May 31, 2020

@gave92 FilesFullTrust.exe is running, I see the regular debug file but nothing for debug_fulltrust.exe.

@gave92
Copy link
Member Author

gave92 commented May 31, 2020

Ok so the fulltrust process is not crashing like before which is good... and now I'm confused.

  • Does opening files from the uwp work? Does the command "Open in terminal" work?
  • Would you be willing to run the app from this PR in debug mode in visual studio?

@yaira2
Copy link
Member

yaira2 commented May 31, 2020

I don't get a crash, it's just that the progress bar keeps loading in recycle bin and doesn't load any items that are there.

@yaira2
Copy link
Member

yaira2 commented May 31, 2020

@gave92 It finally loaded after a few minutes, I think we need to figure out a faster way to load recycle bin before shipping it.

@gave92
Copy link
Member Author

gave92 commented May 31, 2020

Ok just how much stuff is in your recycle bin? xD
I'll test with a lot of files as well. Thanks for finding out the issue.

Edit2: I can confirm this, with a 1000 files it starts to slow down. The solution might be to load incrementally (like a 100 at a time)
Edit3: Could you please check again with the latest commit? Let me know if it's noticeably faster or not (ignore that some info is missing like file size)

@yaira2
Copy link
Member

yaira2 commented May 31, 2020

@gave92 It is much faster now.

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels May 31, 2020
@lukeblevins lukeblevins self-requested a review May 31, 2020 19:03
Copy link
Contributor

@lukeblevins lukeblevins left a comment

Choose a reason for hiding this comment

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

Looks great! Maybe in the future we can use this AppService to load the extended properties of all StorageItems from the Shell, rather than using the Windows Runtime APIs.

@yaira2 yaira2 merged commit 768f6b7 into files-community:master May 31, 2020
@gave92 gave92 deleted the recyclebin_poc branch May 31, 2020 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants