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

Send files to OS trash mechanism on delete #1968

Merged
merged 2 commits into from Oct 20, 2017

Conversation

Projects
None yet
5 participants
@takluyver
Copy link
Member

takluyver commented Dec 12, 2016

This adds a config option, defaulting to use trash, and adds a dependency on the pure-Python 'send2trash' package. Linux, Mac and Windows should all be supported.

Alternatively, we could make it default to hard delete (the current behaviour), and let users opt in to trash behaviour. Then Send2Trash could be a soft dependency.

This doesn't touch the UI yet, so you still get a confirmation dialog which inaccurately says it will permanently delete' the file. If we want to do this, we'll need some way for the contents manager to pass
the UI a hint about whether deleting is permanent or not.

Closes gh-165

Send files to OS trash mechanism on delete
This adds a config option, defaulting to use trash, and adds a
dependency on the pure-Python 'send2trash' package. Linux, Mac and
Windows should all be supported.

Alternatively, we could make it default to hard delete (the current
behaviour), and let users opt in to trash behaviour. Then Send2Trash
could be a soft dependency.

This doesn't touch the UI yet, so you still get a confirmation dialog
which inaccurately says it will 'permanently delete' the file. If we
want to do this, we'll need some way for the contents manager to pass
the UI a hint about whether deleting is permanent or not.

Closes gh-165

@gnestor gnestor added this to the 4.3.1 milestone Dec 13, 2016

@minrk minrk modified the milestones: 5.0, 4.3.1 Dec 14, 2016

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Jan 4, 2017

I think a lot of people would be happy about this, so I would say that it makes sense as a default. One case where this might not be sufficient is a hosted service where the notebook server is the only window on the system (e.g. JupyterHub, cloud services). In these cases, without a read API for the trash, you get something in the middle: lost access to files, but the data isn't actually cleaned up. But then again, trash is really something along the lines of a ~/.trash folder, so it's possible to dig around and find what you lost.

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Jan 19, 2017

This still seems like there's enough to work out that I'm bumping it to 5.1.

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Jul 3, 2017

5.1 is approaching, and there's still quite a bit of work to do. Bumping to backlog.

@takluyver takluyver modified the milestones: Backlog, 5.1 Jul 3, 2017

@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Jul 27, 2017

@takluyver It would be great to ship this! What more needs to be done?

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Jul 27, 2017

To enable it by default, we'd need a way to get files back from the trash, I think. The major platforms have different trash mechanisms, so that's not straightforward.

To make it an off-by-default option, which offers much less benefit, we'd need a way for the frontend to know whether 'delete' is hard or soft, so that the UI can display appropriate information.

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Jul 27, 2017

Looking at some of the issues open on send2trash (hsoft/send2trash#9, hsoft/send2trash#2), and thinking a bit about the complexity of the operation in general, I suspect that this would be an ongoing source of problems which we have little ability to fix. I'm not sure whether that outweighs the benefits.

One option would be to define our own trash mechanism separate from the system recycle bin. But I suspect that there's a bunch of complexity we haven't thought about, with things like network filesystems, folders shared with VMs/containers, and folders synced by things like Dropbox. It would also be annoying for people who actually want to delete things if those things were neither deleted nor placed in the normal trash.

@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Jul 27, 2017

I hear ya. It sounds like an elegant solution for the common user (because they can always recover the notebook from the trash) but I'm sue there are many edge cases where this fails.

@minrk

minrk approved these changes Jul 31, 2017

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Jul 31, 2017

👍 on trying this out in 5.1, for me.

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Jul 31, 2017

Do you think we should try to do UI changes for 5.1, or leave the UI as is, with no mention of trash?

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Jul 31, 2017

Since there's a server-side flag and we have no UI of our own for recovering things from the trash, I think it's okay as-is for now.

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Jul 31, 2017

Travis failures are due to a bug in Send2Trash, fixed by hsoft/send2trash#12 . I've asked for a new release there.

@gnestor gnestor referenced this pull request Jul 31, 2017

Closed

Release 5.1 #2708

11 of 11 tasks complete

@takluyver takluyver modified the milestones: 5.1, Backlog Jul 31, 2017

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Jul 31, 2017

Optimistically tagged as 5.1 - we're waiting on a new release of send2trash before we can do this. If we're ready to release first, bump this to 5.2.

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Jul 31, 2017

There's a new release of send2trash - re-triggering tests.

@takluyver takluyver closed this Jul 31, 2017

@takluyver takluyver reopened this Jul 31, 2017

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Jul 31, 2017

There's another test failure with a unicode character. I can try to make a fix in send2trash, but I'm going to bed soon, so it won't be today.

@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Jul 31, 2017

Ok no prob, let's just mark as 5.2 for now...

@gnestor gnestor modified the milestones: 5.2, 5.1 Jul 31, 2017

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Aug 1, 2017

AOK with 5.2. Let's try to get this in master soon after shipping 5.1, so we start facing the bugs in daily usage.

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Aug 8, 2017

Re-running tests with send2trash 1.4.1.

@takluyver takluyver closed this Aug 8, 2017

@takluyver takluyver reopened this Aug 8, 2017

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Aug 8, 2017

Tests are passing, but I think it's still best to merge this after the release, so we can test it in master for a while.

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Sep 15, 2017

5.1 is now out, if we want to come back to this. As a reminder, this PR currently:

  • Makes Send2Trash a hard dependency.
  • Adds a config option FileContentsManager.delete_to_trash.
    • False is the current behaviour (hard delete)
    • True (default) sends files to the platform trash location when you delete them through the contents API
  • Does not provide any way to restore trashed files or permanently delete them. Users will have to use their system UI for this.
@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Sep 16, 2017

I just tested and it works for me on macOS 10.12.6. I think this is heaps better than permanently deleting files be default. I imagine it would be really painful to accidentally delete some important notebooks and files and not be able to recover them. It's much less painful to be required to go to the trash to permanently delete files.

I vote to merge!

@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Oct 9, 2017

Would love to get this in 5.2, but I think it still requires a little more review/testing. Marking as 5.3 for now...

@gnestor gnestor modified the milestones: 5.2, 5.3 Oct 9, 2017

@dsblank

This comment has been minimized.

Copy link
Member

dsblank commented Oct 9, 2017

Imagine having a class of new users... please give us a countdown so that we can celebrate the coming of this feature :)

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Oct 10, 2017

I think the plan was to merge it just after 5.1 so it was in master for a while. We missed doing that, and we're releasing 5.2 sooner than expected, so I guess the new plan is to merge it soon after 5.2 so that it's in master for a while before 5.3.

@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Oct 10, 2017

Roger that @takluyver!

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Oct 20, 2017

Ping @gnestor @minrk - still happy to try putting this in for 5.3?

@minrk minrk merged commit aa461d9 into jupyter:master Oct 20, 2017

4 checks passed

codecov/patch 100% of diff hit (target 0%)
Details
codecov/project Absolute coverage decreased by -0.1% but relative coverage increased by +20.23% compared to decb30d
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@minrk

This comment has been minimized.

Copy link
Member

minrk commented Oct 20, 2017

Let's do it!

@takluyver takluyver deleted the takluyver:delete-to-trash branch Oct 20, 2017

@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Oct 20, 2017

👏👏👏

@willingc

This comment has been minimized.

Copy link
Member

willingc commented Oct 21, 2017

Nice addition @takluyver. Small issue with the merged PR: it broke the RTD build :(

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Oct 21, 2017

Thanks Carol. I think #2964 should fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.