GNOME Bugzilla – Bug 594192
Save as -> Replace doesn't replace files
Last modified: 2009-12-29 20:25:24 UTC
If you download something and select Save as and then select a file to be replaced, the downloaded file will never be moved to the final destination.
Created attachment 142517 [details] [review] [PATCH] Implement Save as -> Replace functionality Bug 594192 - Save as -> Replace doesn't replace files --- embed/ephy-embed.c | 15 ++++++++++++++- 1 files changed, 14 insertions(+), 1 deletions(-)
(In reply to comment #1) > Created an attachment (id=142517) [details] > [PATCH] Implement Save as -> Replace functionality > > > Bug 594192 - Save as -> Replace doesn't replace files > --- > embed/ephy-embed.c | 15 ++++++++++++++- > 1 files changed, 14 insertions(+), 1 deletions(-) This is wrong. What needs to happen is for the old file to be replaced with the new one in an atomic operation when the new one has finished downloading.
In such case we need to listen for the error signal of WebKitDownload, how does webkit handle the download? Does it block on the error callback before 'dropping' the file when the destination is already taken? I couldn't get a clear idea from Webkit source, it seems like setting the destination uri already does this and emits an error if it's an existant path. I see two possible options: it blocks on error callback when /finishing/ downloading or it blocks on error callback when /setting the uri/. The first case would be easier, the second one would need us to catch the error on set, give an alternative tmp path and on /finish/ move the files. Sounds right?
(In reply to comment #3) > In such case we need to listen for the error signal of WebKitDownload, how does > webkit handle the download? Does it block on the error callback before > 'dropping' the file when the destination is already taken? I couldn't get a > clear idea from Webkit source, it seems like setting the destination uri > already does this and emits an error if it's an existant path. > > I see two possible options: it blocks on error callback when /finishing/ > downloading or it blocks on error callback when /setting the uri/. The first > case would be easier, the second one would need us to catch the error on set, > give an alternative tmp path and on /finish/ move the files. Sounds right? IIRC epiphany starts to download the file anyway, and waits until the user has selected a destination in the filechooser to set the URI. So you need to check if the file already exists in the response callback for the filechooser and keep downloading in the temporary location until the end, then swap them.
Created attachment 149682 [details] [review] WIP: downloads replace and unwritable dest Ok. I kind of got this working but I have a problem handling the replace. I was doing it on download_finished but if you take too long picking the destination download-finished can be triggered with a NULL user-destination, which obviously fails plus when you finally select a destination you won't get to see your file being moved to the chosen destination. So the problem now seems to be *when* to trigger the replace, and *how*. I was thinking that the conditional should be that we have already set a destination, hence the filechooser being away. Then the problem would be if the download already finished, in such case I guess I could use a flag to be read by the dialog response callback that if it's FALSE the callback would handle the move itself (in opposition to the notify::status callback, see below).
Created attachment 149744 [details] [review] Implement downloads replace action I came up with a much simpler solution, I have delayed download moving until the user has selected a destination (auto downloads and clicking Open still work as expected). All is handled with thaw and freeze calls to control when download_status_changed_cb() is allowed to be called. I like this approach, hopefully it's not flawed :).
With Christian's change in http://bugs.webkit.org/show_bug.cgi?id=32359 we can now delay the download start all we want and start it by ourselves with webkit_download_start(). This would solve part of our original problem. However I think it is better to keep our current behaviour of starting the download as soon as the user clicks on it and moving the file to its final destination at finish time. So I'm still proposing this patch as is. Comments welcome.
Review of attachment 149744 [details] [review]: Looks good to me. We should revisit download, and make it simpler sometime, after regressions are down =P ::: embed/ephy-embed.c @@ +692,3 @@ gtk_widget_show (button); + gtk_dialog_add_action_widget (GTK_DIALOG (dialog), + button, DOWNLOAD_ACTION_DOWNLOAD); I think these style changes should go in a separate commit. @@ +736,3 @@ + * Remember that we are blocking this callback until the user has picked a + * destination and that is only possible if he chooses to Save as or Download. + */ it should not be the case in Download - we need to fix that, so I would recommend removing this from the comment.
Created attachment 150525 [details] [review] Minor style fixes in downloads code. Bug #594192
Created attachment 150526 [details] [review] downloader-view: fix wrong return type Bug #594192
Created attachment 150527 [details] [review] Implement replace action for downloads Always download to a temporary location until, only move the file to the final destination after it has been downloaded completely. Bug #594192
Review of attachment 150525 [details] [review]: looks good
Review of attachment 150526 [details] [review]: ++ =)
Review of attachment 150527 [details] [review]: OK
Attachment 150525 [details] pushed as abeb7b1 - Minor style fixes in downloads code. Attachment 150526 [details] pushed as 915203e - downloader-view: fix wrong return type Attachment 150527 [details] pushed as 03ad276 - Implement replace action for downloads