After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 726254 - Downloader does not report progress for non http(s) files
Downloader does not report progress for non http(s) files
Status: RESOLVED FIXED
Product: gnome-boxes
Classification: Applications
Component: general
3.11.x
Other Linux
: Normal normal
: 3.22
Assigned To: GNOME Boxes maintainer(s)
GNOME Boxes maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-03-13 15:55 UTC by Timm Bäder
Modified: 2016-03-31 13:22 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
downloader: Also report progress for non-http(s) (2.35 KB, patch)
2014-03-13 15:55 UTC, Timm Bäder 		needs-work Details | Review
downloader: Also report progress for non-http(s) (2.32 KB, patch)
2014-03-13 17:48 UTC, Timm Bäder 		committed Details | Review

Description Timm Bäder 2014-03-13 15:55:23 UTC 
Created attachment 271741 [details] [review]
downloader: Also report progress for non-http(s)

The code in Downloader.download currently contains a FIXME about the missing progress report when the remote_file has a uri scheme != http(s).
The attached patch fixes that by just using the ActivityProgress from Downloader.download and updating that in GLib.File.copy_async's FileProgressCallback.
Comment 1 Zeeshan Ali 2014-03-13 16:29:06 UTC 
Review of attachment 271741 [details] [review]:

Looks good apart from these nitpicks. Some issues & nitpicks about commit log:

* "Downloader.download previously just used Boxes.copy_file" That makes it sound like its not the case anymore.
* no http(s) -> non-http(s)
* The last sentence is redundant, you don't need to describe every detail of your patch.
* No need to put every sentence in a new line. Feel free to make use of proper paragraphs though.
* No bug link in the commit log? You didn't use git-bz?

::: src/util-app.vala
@@ +368,3 @@
+                                 File dest_file,
+                                 ActivityProgress progress,
+                                 Cancellable? cancellable = null) throws GLib.Error {

while args themselves are correctly aligned, you also want to align the identifier names:

    File             src_file,
    File             dest_file,
    ActivityProgress progress,

@@ +371,3 @@
         try {
             debug ("Copying '%s' to '%s'..", src_file.get_path (), dest_file.get_path ());
+            yield src_file.copy_async (dest_file, 0, Priority.DEFAULT, cancellable, (cur, total) => {

'current' isn't very long so butter use full word here.
Comment 2 Timm Bäder 2014-03-13 17:48:49 UTC 
Created attachment 271773 [details] [review]
downloader: Also report progress for non-http(s)

That should fix all the problems from the patch before. Now *I* don't like the commit message but I didn't know what to change without causing another patch ping-pong. :)
Comment 3 Zeeshan Ali 2014-03-13 19:31:32 UTC 
Review of attachment 271773 [details] [review]:

looks good