Ensure bitmaps (even assets) are loaded on a background thread.

[hwrdprkns]

When that is done we can call postInvalidate to signal from any thread that this view needs to be redrawn.

[hwrdprkns]

@moagrius If this looks good it'd be cool if it made it into a .12 release sometime soon!

Thanks for the awesome project!

[moagrius]

Thanks for contributing. In general it looks good, but can we reuse that Runnable? e.g.,

private Runnable runDecode = new Runnable(){
  @Override
  public void run() {
    bitmap = decoder.decode( fileName, getContext() );
    postInvalidate();
  }
}
...
AsyncTask.execute(runDecode);

As shown in the example, we'd need to make fileName final or an instance member, but I'm not married to a specific implementation, whatever works.

[hwrdprkns]

@moagrius Had to use a private class, but I think this takes care of the issue.

[moagrius]

Thanks for making those changes. A couple things:

First, what are you thoughts about a little more direct approach? Based on your example, but a little leaner:

private String lastFileName;  // already exists, included just for reference
private String currentFileName;

private Runnable decodeRunnable = new Runnable(){
  @Override
  public void run(){
    bitmap = decoder.decode( currentFileName, getContext() );
    postInvalidate();
  }
}

public void update() {
  DetailLevel detailLevel = detailManager.getCurrentDetailLevel();
  if( detailLevel != null ) {
    currentFileName = detailLevel.getDownsample();
    if( currentFileName != null ) {
      if( !currentFileName.equals( lastFileName ) ) {
        AsyncTask.execute( decodeRunnable );
      }
    }
    lastFileName = currentFileName;
  }    
}

Next, if we're multi-threading here (which I agree is appropriate for decoding bitmaps, especially if that decode could potentially include a network request), there should be some kind of cancellation (and therefor, resumption) available, e.g., during the containing Activity's onPause. Currently, this is routed through the TileView instance - users are encouraged to call TileView.pause in the Activity's onPause, and TileView.resume in the Activity's onResume. As you can see here https://github.com/moagrius/TileView/blob/master/src/com/qozix/tileview/TileView.java#L791, the TileView instance then notifies any member instances that need to know, in order to cancel network requests, close open threads, close database, recycle bitmaps, whatever. Obviously this makes everything a little more cumbersome (we might need a more standard implementation of AsyncTask instead of the brief-but-limited static execute sugar), which is probably why I never got around to this personally, but I think it's necessary.

[hwrdprkns]

Re: approach, I don't mind either way — I can make the change you suggested.

I don't think specifically implementing AsyncTask.onCancelled is enough here unless we're going to add a cancel method to the BitmapDecoder interface and implementations; that's overshooting the intent of this PR. Regarding the stale bitmap reference, I suppose that could be achieved by the following?

public class SampleManager extends View implements ... {

... other fields ...

protected boolean isAttached;

@Override
    public void onAttachedToWindow() {
        super.onAttachedToWindow();
        isAttached = true;
    }

    @Override
    public void onDetachedFromWindow() {
        isAttached = false;
        super.onDetachedFromWindow();
    }

private class BitmapRunnable implements Runnable {
        private String fileName;

        public void setFileName(String file) {
            fileName = file;
        }

        @Override
        public void run() {
            Bitmap sample = decoder.decode(fileName, getContext());
            if (isAttached) {
                bitmap = sample;
          // I actually think postInvalidate has an internal 
         // check to see whether it's attached, but let's just put it in the guard for posterity
                postInvalidate();
            }
        }
    }
} 

[moagrius]

I agree there's not much to do on a cancel directive if we get one, but AsyncTask.cancel(true) will send an interrupt signal to the background thread, which I think is important to do during activity pause. I'm less concerned about the bitmap, since if we get one back it should be assigned regardless of state (or nulled, but that's probably for another conversation).

I'd probably create a pause method that cancels the AsyncTask (or tries to interrupt the thread or stop processing if you're using threading outside of AsyncTask, which is fine) - this may require setting lastFileName to null, to indicate that next update may need to fetch the image again, since it may have been interrupted. Having a pause means there should probably be a resume method as well, but that could probably just call or alias update.

Then TileView would have to be updated to have it's pause and resume methods (clear, or whatever) updated to call pause and resume on the SampleManager member instance, just like it does with the others (e.g., TileManager).

If this is more work than you're available for, I totally understand - I think the patch you're describing is important and I appreciate your willingness to pitch in, but if it's too much I can get to it in the future. That said, help is always welcome and it'd be great if you wanted to take this home.

[moagrius]

You said

Re: approach, I don't mind either way — I can make the change you suggested.

Great, thanks. I prefer the leaner version.

[hwrdprkns]

It'd be nice if we could do this without changing the public API for SampleManager.

Maybe let's make the BitmapDecodeTask a little beefier?

diff --git a/src/com/qozix/tileview/samples/SampleManager.java b/src/com/qozix/tileview/samples/SampleManager.java
index 4d2845e..ce9a200 100644
--- a/src/com/qozix/tileview/samples/SampleManager.java
+++ b/src/com/qozix/tileview/samples/SampleManager.java
@@ -6,6 +6,7 @@ import android.graphics.Canvas;
 import android.graphics.Rect;
 import android.view.View;

+import com.qozix.os.AsyncTask;
 import com.qozix.tileview.detail.DetailLevel;
 import com.qozix.tileview.detail.DetailLevelEventListener;
 import com.qozix.tileview.detail.DetailManager;
@@ -21,6 +22,7 @@ public class SampleManager extends View implements DetailLevelEventListener {

    private Bitmap bitmap;
    private String lastFileName;
+   private String currentFileName;

    public SampleManager( Context context, DetailManager dm ) {

@@ -40,17 +42,62 @@ public class SampleManager extends View implements DetailLevelEventListener {
    public void clear(){
        bitmap = null;
        lastFileName = null;
+       decodeTask.cancel(true);
    }

+   private class BitmapDecodeTask implements RunnableFuture {
+        private AtomicBoolean wasCancelled = new AtomicBoolean();
+        private AtomicBoolean isFinished = new AtomicBoolean();
+
+        @Override
+        public void run() {
+            if (wasCancelled.compareAndSet(false, false)) {
+                isFinished.set(false);
+                bitmap = decoder.decode(currentFileName, getContext());
+                postInvalidate();
+                isFinished.set(true);
+            }
+        }
+
+        @Override
+        public boolean cancel(boolean mayInterruptIfRunning) {
+            wasCancelled.set(true);
+            return true;
+        }
+
+        @Override
+        public boolean isCancelled() {
+            return wasCancelled.get();
+        }
+
+        @Override
+        public boolean isDone() {
+            return isFinished.get();
+        }
+
+        @Override
+        public Object get() throws InterruptedException, ExecutionException {
+            throw new UnsupportedOperationException();
+        }
+
+        @Override
+        public Object get(long timeout, TimeUnit unit) throws
+                InterruptedException,
+                ExecutionException,
+                TimeoutException {
+            throw new UnsupportedOperationException();
+        }
+    }
+
+   private BitmapDecodeTask decodeTask = new BitmapDecodeTask();
+
    public void update() {
        DetailLevel detailLevel = detailManager.getCurrentDetailLevel();
        if( detailLevel != null ) {
            String fileName = detailLevel.getDownsample();
-           if( fileName != null ) {
-               if( !fileName.equals( lastFileName ) ) {
-                   bitmap = decoder.decode( fileName, getContext() );
-                   invalidate();
-               }
+           if( fileName != null && !fileName.equals( lastFileName )) {
+               currentFileName = fileName;
+               AsyncTask.execute(decodeTask);
            }
            lastFileName = fileName;
        }       

I still think this is susceptible to a race condition given that decoder and currentFileName are fields instead of arguments to the class, which is why I opted out of that approach in the beginning. Do you think that will be an issue given the changes above?

[moagrius]

I don't mind adding public methods to the SampleManager, as long as we don't break existing contracts (remove, rename, modify existing public methods). That said, what you've posted is interesting... I've got one foot out the door but I'll check back as soon as I get some time, tonight or tomorrow morning.

The one that jumps out is that I don't think decodeTask.cancel(true) will actually send the interrupt to the thread - I think that's the responsibility of the implementation.

Thanks again!

[hwrdprkns]

It may, but it will act as a scheduled noop since wasCancelled has been set.

[moagrius]

So I don't think a race condition is a concern here since we're pretty solidly in control of interactions with the SampleManager instance, and I can't think of a scenario where multiple threads are touching it. Worst case, we could synchronize update, (where fileName is updated, outside the AsyncTask's thread), but again I don't think it's a concern at all at this point.

Next, if we're not interrupting the thread, I don't see any reason to cancel. Maybe this is something to look at further down the road.

For now, why don't we go with what was settled in #152 (comment)

Then I should be able to push out minor 13 this weekend.

Thanks

[hwrdprkns]

@moagrius Sounds good, thanks!

[moagrius]

Cool - you've tested this thoroughly? It looks straightforward enough but want to make sure it's passing... I'll merge if so.

[hwrdprkns]

I have — I can verify that resources are never loaded on the main thread after this change. Before, if the decoder was of Http type, Android would throw a NetworkOnMainThreadException.

[moagrius]

And it correctly updates between changes to different samples for different zoom levels? Never misses a change?

[hwrdprkns]

Yep, the zoom levels populate properly. Though I only tested those with the http decoder (don't have an app with hella zoom level assets!); there shouldn't be any difference with the asset decoder.

[moagrius]

Awesome, thanks