Repository: square/retrofit
Issue: Change Fetcher API To Report Progress as Float

From @swankjesse on #62:

> It would be more conventional for the progress listener to just take a float between 0 and 1. It's sort of weird that a UI decision (progress update granularity) is being decided here.
> 
> In slow transfers (say 1MiB over 3G) using percent is lame because multiple reads may yield the same percentage of progress. (10KiB and 19 KiB are both reported as 1% done).


Issue comments:
`Fetcher` removed.


Pull request review comments:
`1f *` is way cuter than (float) . . . fun.

Nit: it isn't 'percent' anymore, it's 'fraction'

Say `Guarded by 'this'` rather than `Synchronized on 'this'.` ?


Patch that resolved the issue:
diff --git a/http/src/main/java/retrofit/http/Fetcher.java b/http/src/main/java/retrofit/http/Fetcher.java
index 8f4d1f66e8..37cc0df1c3 100644
--- a/http/src/main/java/retrofit/http/Fetcher.java
+++ b/http/src/main/java/retrofit/http/Fetcher.java
@@ -71,9 +71,6 @@ public void run() {
   }
 
   static class DownloadHandler extends CallbackResponseHandler<Void> {
-    /** Throttles how often we send progress updates. Specified in %. */
-    private static final int PROGRESS_GRANULARITY = 5;
-
     private final ByteSink.Factory destination;
     private final ProgressListener progressListener;
     private final MainThread mainThread;
@@ -93,8 +90,8 @@ static class DownloadHandler extends CallbackResponseHandler<Void> {
         InputStream in = entity.getContent();
         try {
           ByteSink out = destination.newSink();
-          final int total = (int) entity.getContentLength();
-          int totalRead = 0;
+          final long total = entity.getContentLength();
+          long totalRead = 0;
           try {
             byte[] buffer = new byte[4096];
             int read;
@@ -102,10 +99,12 @@ static class DownloadHandler extends CallbackResponseHandler<Void> {
               out.write(buffer, read);
               if (progressListener != null) {
                 totalRead += read;
-                int percent = totalRead * 100 / total;
-                if (percent - progressUpdate.percent > PROGRESS_GRANULARITY) {
-                  progressUpdate.percent = percent;
-                  mainThread.execute(progressUpdate);
+                progressUpdate.percent = 1f * totalRead / total;
+                synchronized (progressUpdate) {
+                  if (progressUpdate.run) {
+                    progressUpdate.run = false;
+                    mainThread.execute(progressUpdate);
+                  }
                 }
               }
             }
@@ -123,9 +122,17 @@ static class DownloadHandler extends CallbackResponseHandler<Void> {
 
     /** Invokes ProgressListener in UI thread. */
     private class ProgressUpdate implements Runnable {
-      private volatile int percent;
+      /** Amount to report when run. */
+      volatile float percent;
+
+      /** Whether or not this update has been run. Synchronized on 'this'. */
+      boolean run = true;
+
       public void run() {
         progressListener.hearProgress(percent);
+        synchronized (this) {
+          run = true;
+        }
       }
     }
   }
diff --git a/http/src/main/java/retrofit/http/ProgressListener.java b/http/src/main/java/retrofit/http/ProgressListener.java
index 7427ca45b2..2e485c8b2c 100644
--- a/http/src/main/java/retrofit/http/ProgressListener.java
+++ b/http/src/main/java/retrofit/http/ProgressListener.java
@@ -1,4 +1,4 @@
-// Copyright 2010 Square, Inc.
+// Copyright 2012 Square, Inc.
 package retrofit.http;
 
 /**
@@ -11,7 +11,7 @@ public interface ProgressListener {
   /**
    * Hears a progress update.
    *
-   * @param percent 0-100
+   * @param percent Float bounded by (0..1].
    */
-  void hearProgress(int percent);
+  void hearProgress(float percent);
 }


Test patch:
diff --git a/http/src/test/java/retrofit/http/FetcherTest.java b/http/src/test/java/retrofit/http/FetcherTest.java
index cbddcff439..589ccc9b84 100644
--- a/http/src/test/java/retrofit/http/FetcherTest.java
+++ b/http/src/test/java/retrofit/http/FetcherTest.java
@@ -17,7 +17,7 @@
 import java.util.Arrays;
 import java.util.concurrent.Executor;
 
-import static org.easymock.EasyMock.anyInt;
+import static org.easymock.EasyMock.anyFloat;
 import static org.easymock.EasyMock.createMock;
 import static org.easymock.EasyMock.expect;
 import static org.easymock.EasyMock.expectLastCall;
@@ -40,8 +40,7 @@ public class FetcherTest {
   private HttpResponse response = createMock(HttpResponse.class);
   @SuppressWarnings("unchecked")
   private Callback<Void> callback = createMock(Callback.class);
-  private ProgressListener progressListener
-      = createMock(ProgressListener.class);
+  private ProgressListener progressListener = createMock(ProgressListener.class);
 
   @Test public void testSuccessfulFetch() throws Exception {
     DummyInputStream in = new DummyInputStream();
@@ -56,7 +55,8 @@ public class FetcherTest {
     expect(entity.getContentLength()).andReturn(3L);
     expect(entity.getContent()).andReturn(in);
     expect(sinkFactory.newSink()).andReturn(sink);
-    progressListener.hearProgress(anyInt()); expectLastCall().atLeastOnce();
+    progressListener.hearProgress(anyFloat());
+    expectLastCall().atLeastOnce();
     callback.call(null);
 
     replayAll();