Closed Bug 1745624 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Downloads Panel, enhancement)

Firefox 97
Desktop
All
enhancement
Points:
5

Tracking

()

VERIFIED FIXED
98 Branch
Tracking Status
relnote-firefox --- 98+
firefox98 --- verified

People

(Reporter: alice0775, Assigned: aminomancer)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [fidefe-mr11-downloads])

Attachments

(1 file, 1 obsolete file)

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.

Severity: -- → N/A
Points: --- → 3
OS: Windows 10 → All
Whiteboard: [fidefe-mr11-downloads]

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

Blocks: 1744297
No longer blocks: 1733587
Points: 3 → 5

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

(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.

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.

(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.

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

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.

Assignee: nobody → shmediaproductions
Status: NEW → ASSIGNED
Attachment #9257846 - Attachment is obsolete: true
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e5bddc0306d0 Add "Delete" menuitem to downloads panel context menu. r=Gijs,fluent-reviewers
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch
Depends on: 1750484
Depends on: 1750533

Should we mention that feature in our release notes? Thanks

Flags: needinfo?(gijskruitbosch+bugs)

(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 11 is private: false

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

relnote-firefox: --- → ?
Flags: needinfo?(gijskruitbosch+bugs)

Note added to Nightly 98 release notes, thanks.

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

Flags: qe-verify+
Flags: in-qa-testsuite?(adrian.florinescu)

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1755729
Regressions: 1755729
Flags: in-qa-testsuite?(adrian.florinescu) → in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: