1745624 - Add a context menu on a downloaded file in download panel to delete the file from local file

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, enhancement)

Description

[Alice0775 White]

Now new download panel would not delete temporary file when quit Browser.
So, please add a context menu to manual delete the temporary file from Browser UI.

Steps to Reproduce:

1. Set PDF use Windows default application from about:preferences#general
2. Open a pdf from web site.(e.g. https://www.kenken.go.jp/becc/documents/building/Definitions/modelBulding5000_v1.pdf)
   --- PDF application(Adobe Acrobat) will open as expected
3. Close the PDF application.
4. Open Download panel
   --- There are no way to delete the temporary file from the Download panel
5. Quit Browser

Actual results:
There are no way to delete the temporary file from the Download pane.
And also, the temporary file would not be deleted automatically due to new download design behavior.

Expected results:
Add a context menu on a downloaded file in download panel to delete the temporary file from HDD.

Comment 1

[:Gijs (he/him)]

Based on conversations with product, we may add this but don't think we need this in order to ship the feature.

Comment 2

[Shane Hughes [:aminomancer]]

I can work on this. I also think we should add a "Move to..." menuitem that will basically give the sense of retroactively choosing "Save as..." in cases where the download system automatically saved the file to user's default directory. That way you can get some control back without requiring extra steps when you don't need to save in a particular directory. I'm not sure the safest way to implement this but I'll experiment with it and treat both menuitems as part of the same patch

Comment 3

[:Gijs (he/him)]

(In reply to aminomancer from comment #2)

I can work on this. I also think we should add a "Move to..." menuitem that will basically give the sense of retroactively choosing "Save as..." in cases where the download system automatically saved the file to user's default directory. That way you can get some control back without requiring extra steps when you don't need to save in a particular directory. I'm not sure the safest way to implement this but I'll experiment with it and treat both menuitems as part of the same patch

Please don't try to do more than 1 thing in one patch (and 1 bug). I am much less convinced that "move to..." is a useful menuitem, and we should at least have some product + UX conversations about it. If you want to help, try for a patch with the delete item on its own first. We should have a separate bug for the other item.

Comment 4

[Shane Hughes [:aminomancer]]

Hey @Gijs do you have any input on this? I'm trying to pick the safest delete method. In other methods, the downloads UI just calls new FileUtils.File(path) but they aren't using that to delete anything, so there's no risk of data loss anyway. With a delete menuitem, if we only use the path and the user overwrites the file at that path, we can end up deleting a file that doesn't genuinely correspond to the download entry. I guess I could do some kind of checksum but it seems a little janky to add all that for just a context menu item.

Then again, it would be inconsistent if we didn't delete the file in such a case, because every other file integration here only seems to care about path. Like if I delete a file, open the panel, the download is disabled/grayed out. Then if I make a new file in the same path with the same name, and open the panel again, the download item is restored to its former glory. And all the various commands work on it as you'd expect. So I guess it'd be a bit odd to do something different for a "delete file" command.

But if the concern about data loss is great enough, and there's an easy way to confirm the pedigree of the file, maybe we should implement it in all those methods that currently only check path. Such that making a new file with the same path would not re-enable the download item. That would make this a much bigger decision so I can't proceed much further on my own.

Comment 5

[:Gijs (he/him)]

(In reply to aminomancer from comment #4)

Hey @Gijs do you have any input on this? I'm trying to pick the safest delete method. In other methods, the downloads UI just calls new FileUtils.File(path) but they aren't using that to delete anything, so there's no risk of data loss anyway. With a delete menuitem, if we only use the path and the user overwrites the file at that path, we can end up deleting a file that doesn't genuinely correspond to the download entry. I guess I could do some kind of checksum but it seems a little janky to add all that for just a context menu item.

Then again, it would be inconsistent if we didn't delete the file in such a case, because every other file integration here only seems to care about path. Like if I delete a file, open the panel, the download is disabled/grayed out. Then if I make a new file in the same path with the same name, and open the panel again, the download item is restored to its former glory. And all the various commands work on it as you'd expect. So I guess it'd be a bit odd to do something different for a "delete file" command.

Right, I think this is enough of an edgecase that just deleting whatever exists at that path makes sense, also because...

But if the concern about data loss is great enough, and there's an easy way to confirm the pedigree of the file,

... there isn't, unfortunately, an easy way to do that. All the different OSes we support have different (sometimes filesystem-dependent) ways of tracking files, and we don't currently use them in downloads code (see also e.g. bug 1746386). For now, deleting what is at the path seems fine.

Comment 6

[Shane Hughes [:aminomancer]]

Alrighty I still haven't figured out mach so here's my patch if you wanna review it when you get back.

Comment 7

[Shane Hughes [:aminomancer]]

Attachment: Bug 1745624 - Add "Delete" menuitem to downloads panel context menu. r?Gijs

New "Delete" menuitem displays when the context menu is opened on a DownloadElement whose target file or partFile exists. On activation, it deletes the target file and clears any part or temporary data. Then, if it was the only download in the panel, it resets the download indicator's attention state.

Comment 8

[Shane Hughes [:aminomancer]]

Attachment: Bug 1745624 - Add "Delete" menuitem to downloads panel context menu. r?Gijs

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5bddc0306d0
Add "Delete" menuitem to downloads panel context menu. r=Gijs,fluent-reviewers

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/e5bddc0306d0

Comment 11

[Pascal Chevrel:pascalc]

Should we mention that feature in our release notes? Thanks

Comment 12

[:Gijs (he/him)]

(In reply to Pascal Chevrel:pascalc from comment #11)

Should we mention that feature in our release notes? Thanks

Maybe. We're tweaking the behaviour in bug 1750484 so I'll wait with requesting one until that's hashed out.

Comment 13

[:Gijs (he/him)]

Release Note Request (optional, but appreciated)
[Why is this notable]: New feature in 98 relating to the downloads changes launched in 97.
[Affects Firefox for Android]: No
[Suggested wording]: You can now delete downloaded files directly from the download panel and other download views using the context menu.
[Links (documentation, blog post, etc)]: n/a

Comment 14

[Pascal Chevrel:pascalc]

Note added to Nightly 98 release notes, thanks.

Comment 15

[Adrian Florinescu [:aflorinescu]]

Adding qe+ and reminder to add in testsuite for fx 98.

Comment 16

[Ardelean Oana, Desktop QA [:oardelean]]

This enhancement seems to be working on Firefox 98.0b4(20220213185901) as well as Nightly 99.0a1(20220213214259). There is now a "Delete" option implemented when right-clicking on the temporary file from the Downloads panel. Checked on macOS Big Sur 11, Linux(Ubuntu 20.04) and Win11.