square/picasso — pull request #561 by sockeqwe (2014-06-18 13:07:51+00:00)
Title: ScrollListener to improve performance

Hi,
as promised I have spent some hours to work on a solution for the scroll performance issue. The result looks promising.

**Please note that I would like to discuss my solution and a some critical points with you** before merging this pull request.

So what I have did:
1. I have created a class called ``DispatchingQueue``and have integrated it into the already existing ``Dispatcher``class. I have added two methods to ``Dispatcher``:
* ``void interruptDispatching()``
* ``void continueDispatching()``
So when ``Dispatcher.dispatchComplete(BitmapHunter)`` will be called the dispatcher will check if the dispatching has been interrupted before (by calling ``interruptDispatching()``. If no, the "complete event" will dispatched directly. If the dispatching has been interrupted, then dispatching the "complete event" will be added / enqueued to the ``DispatchingQueue`` (internally stored in a class called ``DispatchJob``). So after continuing dispatching (by calling ``Dispatcher.continueDispatching()`` ) all waiting ``DispatchJobs`` will be executed. The same workflow will run for ``Dispatcher.dispatchFailed(BitmapHunter hunter)``.
**QUESTION 1:** Is it correct to do this only for this two methods or should ``Dispatcher.dispatchRetry()`` etc. run also with DispatchingQueue? I guess this two methods are enough right?
The big advantage by doing this in the ``Dispatcher`` is, that the BitmapHunter can do his work asynchronously and only the fading will be interrupted . Also setting placeholder or the image from memory cache works immediately.

2. Calling ``Dispatcher.performCancel(Action)`` while dispatching is interrupted (which means that its likely that there are DispatchJobs waiting in the DispatchingQueue) will also dequeue any waiting ``DispatchJob`` associated with this Action (to be more precise: with the associated BitmapHunter).
**QUESTION 2:** Is there another way a Action / BitmapHunter could be canceled that I have missed?
The ``DispatchingQueue`` will be cleaned up (clearing the queue) after ``Dispatcher.shutdown()``has been called.

3. I have written Unit Test for that (``DispatchingQueueTest``)

4. I have added two methods to ``Picasso`` class. So this two methods will be the public API for dispatching:
* ``Picasso.interruptDispatching()``: calls Dispatcher.interruptDispatching()
* ``Picasso.continueDispatching()``: calls call Dispatcher.continueDispatching()

5. I have added two default ``OnScrollListener`` implementations which can be found in the package ``com.squareup.picasso.scrolling``
* ``PicassoScrollListener``: interrupts / continues if and only if the scroll state is IDLE. So fling or simply scrolling will cause the dispatcher to be interrupted. I think that the most users who have gerneral performance issues while scrolling will also have performance issues while scrolling a few pixels (not flinging). Therefore I have decided that this one should be "the default" behaviour
* ``PicassoFlingScrollListener``: like the name alredy suggests this OnScrollListener will interrupt while flinging (so normal scrolling will not interrupt dispatching).

6. I have added to the samples two Actities just to demonstrate how to work with this two listeners. (well its more a ugly copy & past of already existing samples; I was a little bit lazy and didn't think about architecture, inheritance etc.)

**Last but not least there is one issue:**
As you see in the code ``DispatchingQueue.interruptDispatching()`` will simply increment a counter and ``DispatchingQueue.continueDispatching()``will decrement the counter. Dispatching is enabled if t ``counter == 0 `` I did that because I think there could be more than one ListViews displayed at the same time (like in a master / detail view on a tablet, or viewpager , etc. ). So far so good, but there is one **critical issue with this counter solution** (you can reproduce it also in the samples: for instance GridView with Scrolling). If dispatching has interrupted, for example by starting scrolling (the counter will be incremented to 1) and let the finger on the screen and rotate your device to landscape the PicassoScrollLister will not receive the IDLE Scroll State and the counter will not be decremented --> No further dispatching ...

**QUESTION 3:** Do you have any smart solution for this problem? My solution would be to use something like a ``WeakHashSet`` and changing interrupt / continue methods to something like this ``Dispatcher.interruptDispatching( Object interruptRequestor )`` and ``Dispatcher.continueDispatching( Object interruptRequestor )``. The interruptRequest could be the PicassoScrollListener. So the idea is to add the interruptRequest to the WeakHashSet and if the weakHashSet is empty that dispatching is enabled. So this should fix the problem described above. However there is no "WeakHashSet structure" in java. We could use WeakHashMap instead. However I can't see a way how to get notified if an entry has been removed from WeakHashMap, because after removing an entry we have to check if dispatching should be continued or not.
Any suggestion or even smarter solution is welcome.

The sample apk file can be found [here](https://db.tt/VUr5EKkM) and the picasso library jar [here](https://db.tt/3l01sQXJ)