GNOME Bugzilla – Bug 674291
Save As suggests long complicated filename
Last modified: 2014-01-13 23:33:29 UTC
To see the problem: 1. Send yourself a message in Gmail and attach a file foo.txt. 2. In Epiphany, read the message containing the attachment. It will have links "View" and "Download". 3. Click the Download link. Epiphany will download the file and will name it "foo.txt". 4. Now right click the Download link and choose Save Link As. Epiphany will open a file chooser dialog. In that dialog, the proposed filename will look something like this: ?ui=2&ik=a01b8d774a&view=att&th=136c280a34d3496b&attid=0.1&disp=safe&realattid=f_h15jqkke0&zw Instead, Epiphany should propose the same name "foo.txt" that it used when you downloaded directly in step 3 above.
This also occurs in Epiphany 3.0.4, so this is not new.
Even when querying WebKitDownload for the suggested filename, the name is still incorrect. This being a WebKit problem, it seems WebKitDownload needs to receive a network response to properly set up the suggested filename.
(In reply to comment #2) > Even when querying WebKitDownload for the suggested filename, the name is still > incorrect. This being a WebKit problem, it seems WebKitDownload needs to > receive a network response to properly set up the suggested filename. So here's what I think is going on For 'Save as' the Download API is not used, we are just using the base name for the resource. We need to add a get_suggested_filename API in WebKitNetworkResponse that uses the Content-Disposition data of the already received data. That would fix doing 'Save as' in the menu for an already loaded page (think; attachments in bugzilla). This is in window-commands.c. For right-click 'Save Link As' we crate a new download and then use the basename of that download as the suggested filename. This is broken. Using the network response API would not help, because there's been none already. So I'm not sure what to do here, my impression is that the initial suggestion is just going to suck in some cases? Maybe we can start downloading it in the background for a second before showing the suggested filename? The code for this is in popup-commands.c
(In reply to comment #3) > (In reply to comment #2) > > Even when querying WebKitDownload for the suggested filename, the name is still > > incorrect. This being a WebKit problem, it seems WebKitDownload needs to > > receive a network response to properly set up the suggested filename. > > So here's what I think is going on > > For 'Save as' the Download API is not used, we are just using the base name for > the resource. We need to add a get_suggested_filename API in > WebKitNetworkResponse that uses the Content-Disposition data of the already > received data. That would fix doing 'Save as' in the menu for an already loaded > page (think; attachments in bugzilla). This is in window-commands.c. The patch for this is already in wk bugzilla, waiting to land: https://bugs.webkit.org/show_bug.cgi?id=92878 I'll cook a patch once it lands. > > For right-click 'Save Link As' we crate a new download and then use the > basename of that download as the suggested filename. This is broken. Using the > network response API would not help, because there's been none already. So I'm > not sure what to do here, my impression is that the initial suggestion is just > going to suck in some cases? Maybe we can start downloading it in the > background for a second before showing the suggested filename? The code for > this is in popup-commands.c I think this is what other browsers do..
I pushed http://git.gnome.org/browse/epiphany/commit/?id=8a38c7a9ba0238092edc4e398ca7635bb4392c5f for the 'Save As' case. Leaving this open for the 'Save Link As' bug.
This will break the build with WK2. The API for WK2 has not landed yet and it's slightly different, see https://bugs.webkit.org/show_bug.cgi?id=92967 . Also, these methods can return NULL, so I'm not sure your patch will handle that case ok.
(In reply to comment #6) > This will break the build with WK2. The API for WK2 has not landed yet and it's > slightly different, see https://bugs.webkit.org/show_bug.cgi?id=92967 . Ah right, just need to branch for that. > > Also, these methods can return NULL, so I'm not sure your patch will handle > that case ok. I suppose we should fallback to the base path in that case. I can do that now.
This is still not working even in the Save As case. For example, visit bug 697812 and scroll down to the attachments section near the end of the page. Right click on the attachment "first log file" and choose Save As. Epiphany will suggest the filename "attachment.cgi?id=241264". In this situation Firefox will propose the filename "Xorg.blank.log", which was the attachment's original filename and which is correct in this case.
Created attachment 248352 [details] [review] Rework how downloads are managed In WebKit2 the "download-started" signal is emitted before the EphyDownload is created and as such the "ephy-download-set" object data will never be set so a new EphyDownload will be created. This results in two EphyDownloads being created per WebKitDownload. Instead set the EphyDownload as object data on the WebKitDownload and return it if it is set. Also connect to the EphyEmbedShell's "download-removed" signals and remove the download from the UI in the callback.
Created attachment 248353 [details] [review] Bug 674291 - Use the suggested name for Save As
Garrett, thanks for the fix! This bug has been a major annoyance for a long time so I'd love to see this committed - can one of the Epiphany developers review?
Review of attachment 248352 [details] [review]: I think it would be best for the review if you could split your rework in a series of minor patches, whenever possible, and explain each change. That would make review much easier.
Review of attachment 248353 [details] [review]: Is this independent from the previous patch?
(In reply to comment #12) > Review of attachment 248352 [details] [review]: > > I think it would be best for the review if you could split your rework in a > series of minor patches, whenever possible, and explain each change. That would > make review much easier. How should I split this? I had to completely rework how downloads were managed because it was broken, which is all this patch fixes as explained in the commit message. The second patch was split because it was for something else. (In reply to comment #13) > Review of attachment 248353 [details] [review]: > > Is this independent from the previous patch? No, without the other the file is downloaded twice.
Ping? This bug is a constant annoyance, and I'd love to see a fix committed for 3.10.
Sorry, I'll try to review this before 3.10.
Review of attachment 248353 [details] [review]: Looks good in general, except the comments below. Nonetheless, you need to write a more descriptive comment in the log. ::: src/popup-commands.c @@ +236,3 @@ + ephy_download = ephy_download_new_for_download (download, GTK_WINDOW (window)); + + switch (gtk_dialog_run (GTK_DIALOG (dialog))) Please don't use gtk_dialog_run(). @@ +283,3 @@ char *base; + base = ephy_sanitize_filename (g_path_get_basename (location)); This change should definitely go in a separate patch (even a separate bug), explaining why it is necessary, even if it is obvious and a small change, as it is unrelated to the bug you're fixing with this patch.
Review of attachment 248352 [details] [review]: I have a hard time understanding what you're trying to fix with this patch, and therefore I can't get the solution you're proposing. Could you elaborate in the exact problem first so that we can discuss what would be the best way to fix whatever is broken? Also, please note that we've removed WebKit1 support from epiphany, so if it makes your patches cleaner, feel free to remoe the left-over bits from WebKit1 in a zero patch. ::: embed/ephy-download.c @@ +959,3 @@ + } + + return ephy_download; There is something wrong in our direction if a method that is called ephy_download_new_for_download() already has a download and returns the existing one. We need to make sure this doesn't happen instead of working around the case when the method is called a second time. ::: src/ephy-window.c @@ +3379,3 @@ + children = gtk_container_get_children (GTK_CONTAINER (window->priv->downloads_box)); + if (g_list_length (children) == 1) + ephy_window_set_downloads_box_visibility (window, FALSE); I don't understand why we need to do UI related changes in this patch. Could you please elaborate? ::: src/window-commands.c @@ +630,3 @@ + /* We do not want this download to show up in the UI */ + ephy_download = ephy_download_new_for_download (download, NULL); + ephy_embed_shell_remove_download (ephy_embed_shell_get_default (), ephy_download); I think we need to rethink the way this is done. I don't think creating a download and immediately removing it is good. If the API is forcing us to do this, then we need to rework the API.
This is still not working in git master. I believe that Garrett is no longer working on this. I'd love to see someone pick this back up.
I had a go at this. I'll submit it as a series of three small patches.
Created attachment 261277 [details] [review] Add filename-suggested signal to EphyDownload to propagate WebKit's decide-destination signal.
Created attachment 261278 [details] [review] Add ephy_file_move_uri to ephy-file-helpers.
Created attachment 261279 [details] [review] Fix Save As to use suggested filename
Daniel, thanks for your work on this. To try it out, I just built Epiphany from git master with your patches applied. First, I think you should generate patches using 'git format-patch' rather than 'git apply' - that way your name/email and a description of each change can be included in the git history. WIth your changes, if I right click a link (e.g. one of the "Created an attachment" links above) and choose Save As, the Save Link As dialog shows the correct filename ("filename-suggested-signal.diff"). That's great. However, I also see at the bottom of the window that the file is being downloaded to a filename such as ephy-download-MHU36W, which appears in my Downloads directory. That file apparently gets copied to filename-suggested-signal.diff at the location I choose. It seems a bit ugly that this temporary filename is exposed to the user. Is that necessary? Could the file be downloaded using its actual name ("filename-suggested-signal.diff")? Could it be moved rather than copied to the destination so there won't be an extra copy lying around afterward?
Adam, you bring up some good points. As for moving vs copying, it _should_ already be a move operation, not a copy. It uses g_file_move. Anyways, I'll see what I can do to address the other points.
Created attachment 263211 [details] [review] Add filename-suggested signal to EphyDownload
Created attachment 263212 [details] [review] Add ephy_file_move_uri to ephy-file-helpers.
Created attachment 263213 [details] [review] Fix Save As to use suggested filename
The new patches should provide a better user experience. I made two changes: 1) Hide the download widget until the user decides on a destination so they won't see the ugly temporary filename. 2) Delete temporary files if a download is canceled.
I built with these latest patches. This certainly seems much better from a user standpoint - thanks. I'm curious about one more thing. If I click on a large download without choosing "Save As", then the file is downloaded directly to my Downloads directory with its final name. If I open a Nautilus window on that directory, I can see the file increasing in size as the download progresses. That seems nice. But even with this patch, if I choose "Save As" and choose a directory and a filename, Epiphany first downloads to the Downloads directory with a temporary filename, and only afterward moves the file to its destination. Why is that necessary? Could we not just download directly to the destination? It seems like that would be simpler and more efficient (no file move necessary). I can only surmise that the goal is to begin the download even while the user is in the Save As dialog; since we don't yet know the destination filename, we have to download to Downloads instead. (If that is the intention, I'm a little skeptical that saving a few seconds of download time is worth potentially having to do a large copy across filesystems later on, though I don't feel strongly about this.) Can one of the Epiphany developers comment on this last point, and review these patches? It would be nice to land this if possible.
>I can only surmise that the goal is to begin the download even while the user is in the Save As dialog This is essentially correct. The download must be started to receive the suggested filename from the server. So by the time the Save As dialog is shown, the connection is already open. We can't really rely on a TCP connection staying open forever. So, it downloads to a temporary filename because the user may take any amount of time to specify a destination via the Save As dialog. (The way Chrome seems to handle this is it starts downloading to a temporary after the Save As dialog is open for about 2 seconds.) It would be nice to move and continue the download as soon as the location is decided by the user. Unfortunately, looking at the WebKit docs, I don't see any way to relocate a download while it is in progress. Chrome (and I'll assume Chromium) does manage to do this but I believe it does not use WebKit for downloads. Anyways, I guess we'll see what the devs have to say.
Epiphany developers, could you possibly look at this and land it if appropriate? It would be really nice to have this fix for 3.12.
Review of attachment 263213 [details] [review]: I have a few comments, but I think Carlos should review this too. ::: src/popup-commands.c @@ +241,3 @@ + if (completed && dest_uri) { + const char *source = ephy_download_get_destination_uri (download); + ephy_file_move_uri (source, dest_uri); This can happen much latter than the user selects the destination. A file could be created in the location of "save-as-uri", and this move here will then overwrite it. This is not safe. @@ +337,3 @@ + + download = ephy_download_new_for_uri (location, + GTK_WINDOW (window)); Indentation. @@ +339,3 @@ + GTK_WINDOW (window)); + + gtk_widget_hide (ephy_download_get_widget (download)); Is the widget visible before this at all? If it is, then this will look odd. @@ +342,3 @@ + + dest_dir = ephy_file_get_downloads_dir (); + dest_name = ephy_file_tmp_filename ("ephy-download-XXXXXX", NULL); How about also making the file hidden? @@ +358,3 @@ + G_CALLBACK (save_as_completed_cb), NULL); + g_signal_connect (download, "error", + G_CALLBACK (save_as_error_cb), NULL); Indentation in these four calls.
Review of attachment 263211 [details] [review]: OK
Review of attachment 263212 [details] [review]: OK
Created attachment 264721 [details] [review] Add filename-suggested signal to EphyDownload
Created attachment 264722 [details] [review] Add ephy_file_move_uri to ephy-file-helpers
Created attachment 264723 [details] [review] Fix Save As to use suggested filename
Created attachment 264724 [details] [review] Make temporary downloads hidden files
*** Bug 720881 has been marked as a duplicate of this bug. ***
Forgot about this for a bit. So, where do we stand on this now?
Ping. Server was malfunctioning when I commented last.
Comment on attachment 264722 [details] [review] Add ephy_file_move_uri to ephy-file-helpers transferring status of last patch to this one
Comment on attachment 264721 [details] [review] Add filename-suggested signal to EphyDownload adding last review status to new patch
Attachment 264721 [details] pushed as 4897f86 - Add filename-suggested signal to EphyDownload Attachment 264722 [details] pushed as 5409a66 - Add ephy_file_move_uri to ephy-file-helpers
It's nice to see some activity on this bug. Jon, it looks like you've pushed two of the patches that Daniel posted above but not the third, which would be required to fix this. As Daniel asked above, where do we now stand? In comment 33 above Claudio posted some feedback on the patch "Fix Save As to use suggested filename", but said "Carlos should review this too". So are we now waiting for Carlos's review comments?
Review of attachment 264724 [details] [review]: Instead of using a hidden file, I would have appended .part to the filename (as bittorrent clients usually do) or even added a new extension (Chrome uses .crdownload for example). This makes it easier to identify unfinished downloads in the Downloads directory, and avoids large amounts of space being used and hidden from the user.
(In reply to comment #47) > Review of attachment 264724 [details] [review]: > > Instead of using a hidden file, I would have appended .part to the filename (as > bittorrent clients usually do) or even added a new extension (Chrome uses > .crdownload for example). > This makes it easier to identify unfinished downloads in the Downloads > directory, and avoids large amounts of space being used and hidden from the > user. But see Bug #721726
(In reply to comment #48) > (In reply to comment #47) > > Review of attachment 264724 [details] [review] [details]: > > > > Instead of using a hidden file, I would have appended .part to the filename (as > > bittorrent clients usually do) or even added a new extension (Chrome uses > > .crdownload for example). > > This makes it easier to identify unfinished downloads in the Downloads > > directory, and avoids large amounts of space being used and hidden from the > > user. > > But see Bug #721726 I don't see how this bug matters in our case. If epiphany crashes, or the download is still on-going, I'd rather see the file with a .part suffix than nothing at all.
(In reply to comment #47) > Review of attachment 264724 [details] [review]: > > Instead of using a hidden file, I would have appended .part to the filename (as > bittorrent clients usually do) or even added a new extension (Chrome uses > .crdownload for example). > This makes it easier to identify unfinished downloads in the Downloads > directory, and avoids large amounts of space being used and hidden from the > user. Exactly, but that should be done by webkit, I'm currently working on a patch for webkit to do it, using .wkdownload suffix.
(In reply to comment #49) > (In reply to comment #48) > > (In reply to comment #47) > > > Review of attachment 264724 [details] [review] [details] [details]: > > > > > > Instead of using a hidden file, I would have appended .part to the filename (as > > > bittorrent clients usually do) or even added a new extension (Chrome uses > > > .crdownload for example). > > > This makes it easier to identify unfinished downloads in the Downloads > > > directory, and avoids large amounts of space being used and hidden from the > > > user. > > > > But see Bug #721726 > > I don't see how this bug matters in our case. If epiphany crashes, or the > download is still on-going, I'd rather see the file with a .part suffix than > nothing at all. Bug #721726 is about cancelled downloads, not ongoing or partial file left after a crash. The partial file should be removed when the download is cancelled, but again it's not something ephy should care about. I'm fixing it in webkit too as part of the intermediate file patch.
Created attachment 265833 [details] [review] Do not duplicate the destination in EphyDownload
Created attachment 265834 [details] [review] Simplify save_property_url
Created attachment 265835 [details] [review] Do not show downloads in the UI until a destination has been created
Created attachment 265837 [details] [review] Use the filename suggested by the server when asking the user
(In reply to comment #54) > Created an attachment (id=265835) [details] [review] > Do not show downloads in the UI until a destination has been created This is wrong, and leaks the downloads cancelled before being shown, because for some reason is the download widget the one taking ownership of the download. Working on a new patch.
Created attachment 265846 [details] [review] Do not add automatically the downloads to the UI when they are created
Created attachment 265847 [details] [review] Use the filename suggested by the server when asking the user
Review of attachment 265833 [details] [review]: Looks good.
Review of attachment 265834 [details] [review]: Looks good. Were we leaking a string here?
Review of attachment 265846 [details] [review]: Looks good.
Review of attachment 265847 [details] [review]: Looks good. ::: src/popup-commands.c @@ +155,2 @@ { + ephy_download_cancel (download); Why does this need to go to an idle?
(In reply to comment #60) > Review of attachment 265834 [details] [review]: > > Looks good. Were we leaking a string here? Yes, the GValue was leaked.
(In reply to comment #62) > Review of attachment 265847 [details] [review]: > > Looks good. > > ::: src/popup-commands.c > @@ +155,2 @@ > { > + ephy_download_cancel (download); > > Why does this need to go to an idle? Because cancel finishes the webkit download, and the finished callbacks destroys the ephy download and we are in the decide-filename callback of the webkit download. When we return from filename-suggested callback, the ephy downloads is expected to be alive, but it can be destroyed, so it's better to cancel the download after the decide destination has finished.
*** Bug 678658 has been marked as a duplicate of this bug. ***