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

feat: remove torrent immediately, but allow Undo #1788

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

dsernst
Copy link
Contributor

@dsernst dsernst commented Feb 6, 2020

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix
[x] New feature
[ ] Other, please explain:

What changes did you make? (Give an overview)

This is a continuation of the work started in PR #1087 to replace the Remove a Torrent UI modal with a Snackbar style alert + option to Undo.

Kapture 2020-02-05 at 17 26 10

The 3 other methods of removing a torrent:

  • Right click in list > Remove Data File
  • Transfers menu > Remove All From List
  • Transfers menu > Remove All Data Files

all continue to use the same modal as before (per discussion in #970).

Which issue (if any) does this pull request address?

#970, #1087 (abandoned PR)

👍 This is tested locally and works on my machine running OS X 10.14.6.

@dsernst
Copy link
Contributor Author

dsernst commented Feb 6, 2020

From #970 (comment):

image

This PR doesn't add support for a Ctrl-Z style Undo hotkey.

I wanted to get something working out the door first, & don't think the hotkey ought to be a blocker.

@dsernst
Copy link
Contributor Author

dsernst commented Feb 6, 2020

One important thing to note:

The Undo feature currently only works for magnet links, which doesn't include the Featured torrents on the WebTorrent Desktop homepage, so I don't know how to demo that properly.

✅ I confirmed that the Undo feature does work on a torrent with good magnet links.

  • If someone can point me to a good public demo torrent w/ magnet support, I can make a quick gif demo of that to show it resuming etc.

Even better would be to fix this up so the Undo works for torrents added via a .torrent file.

I can work on that, but would appreciate a little advice on how best to approach.

Currently, the locally stored ${appConfig}/Torrents/ files are still being deleted immediately.

  • So one solution is to simply delay deleting the .torrent files until after the Snackbar autohides itself after 5 seconds. That ought to work, but if the Application quits before the 5 second timer, the file may never get cleaned up, which doesn't seem great.

  • A different solution is to remember how the torrent was added in first place, such as via pasting in a link like https://archive.org/download/art_of_war_librivox/art_of_war_librivox_archive.torrent. Then the Undo call could go try to add that way again. But as far as I could tell, these links aren't being stored within torrentSummary right now, just the path to the new local .torrent file.

@subins2000
Copy link
Contributor

subins2000 commented May 12, 2020

I tested this locally and the Undo option also worked for the Featured torrents in WebTorrent.

System: Ubuntu 18.04

@dsernst
Copy link
Contributor Author

dsernst commented Jul 18, 2020

Fixed merge conflict

@feross feross self-assigned this Nov 19, 2020
@dsernst
Copy link
Contributor Author

dsernst commented Dec 4, 2020

Best path forward, per discussion in today's Webtorrent Maintainers call:

So one solution is to simply delay deleting the .torrent files until after the Snackbar autohides itself after 5 seconds. That ought to work, but if the Application quits before the 5 second timer, the file may never get cleaned up, which doesn't seem great.

We can add a flag like pendingDelete to the torrent. The UI will hide these items, so from the user's perspective it's been deleted. Then have a function like removePendingDeletes that goes through and actually deletes it from the state. That function can be called after the Undo's timer ends, as well as on app shutdown and app start (in case the app had crashed).

@github-actions
Copy link

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

@github-actions github-actions bot added the stale label Jul 22, 2021
@dsernst
Copy link
Contributor Author

dsernst commented Jul 22, 2021

Yes it’s still relevant, if we want to simplify the UI (replace two click modal w/ one click undoable).

Nothing’s changed since my last comment.

  • This works great for .torrent files.
  • Undo doesn’t work for magnet links yet. There is a relatively clear path forward.

I just haven’t taken the time to work on this lately, as I’ve been focused on other projects 😄

@github-actions github-actions bot removed the stale label Jul 23, 2021
@github-actions
Copy link

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

@github-actions github-actions bot added the stale label Sep 22, 2021
@alxhotel alxhotel added accepted and removed stale labels Sep 22, 2021
@DiegoRBaquero
Copy link
Member

@dsernst What's the path forward of with magnets, I'll take it on myself

@DiegoRBaquero DiegoRBaquero changed the title Remove torrent immediately, but allow Undo feat: remove torrent immediately, but allow Undo Oct 19, 2021
@dsernst
Copy link
Contributor Author

dsernst commented Oct 19, 2021

@dsernst What's the path forward of with magnets, I'll take it on myself

Awesome to hear. 🎉

Last I spoke about this w/ @feross, he liked this approach:

Best path forward, per discussion in today's Webtorrent Maintainers call:

So one solution is to simply delay deleting the .torrent files until after the Snackbar autohides itself after 5 seconds. That ought to work, but if the Application quits before the 5 second timer, the file may never get cleaned up, which doesn't seem great.

We can add a flag like pendingDelete to the torrent. The UI will hide these items, so from the user's perspective it's been deleted. Then have a function like removePendingDeletes that goes through and actually deletes it from the state. That function can be called after the Undo's timer ends, as well as on app shutdown and app start (in case the app had crashed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants