Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement request group and pause/resume support #665

Merged
merged 2 commits into from Sep 23, 2014

Conversation

@lucasr
Copy link
Contributor

lucasr commented Sep 22, 2014

Ok, here's an initial take on the whole pause/resume support for Picasso.

This PR adds a new API to RequestCreator that allows you set a group tag/marker to specific requests:

Picasso.with(context)
       .load("http://example.com/image.jpg")
       .group("mygroup")
       .into(someImageView)

By default, all requests are associated with the default group defined as Picasso.DEFAULT_GROUP. This tag can be used to cancel, pause, and resume requests in the same group. To do so, you can use the following new APIs in Picasso:

  • cancelRequestGroup(String group)
  • pauseRequestGroup(String group)
  • resumeRequestGroup(String group)

Maybe we should also have pauseRequest(Target|ImageView|RemoveView) and resumeRequest(Target|ImageView|RemoveView) for parity with the existing cancelRequest(Target|ImageView|RemoveView). Let me know what you think.

Here are a few general notes on how this new API behaves:

  1. When you pause a group, any new request for the paused group will not be attached to any BitmapHunter until the group gets resumed. In this case, the requests for a paused group will be queued in the Dispatcher (see pausedActions member).
  2. If a request group is paused while there are already on-going requests for it (i.e. BitmapHunter is either running or waiting for its turn), the requests in the now paused group will get detached from any pending BitmapHunter immediately. If the affected BitmapHunter ends up with no pending actions, it will get canceled (see performPauseGroup).
  3. If a request is canceled while its group is paused, it will be removed from the queue of paused requests accordingly (see performCancel). This might happen, for example, if a new request is initiated for the same target than a paused request.
  4. A BitmapHunter might contain Actions from multiple groups at any given time. Not likely to happen in practice, but it's something we have to handle. This is why there's no 1-to-1 mapping between BitmapHunter and requests groups.

I should probably add some more tests for the new APIs. But I wanted to get some early feedback before spending more time on them. I've added a super simple scroll listener based on the new API. Works like a charm ;-)

closes #561

@Override
public void onScrollStateChanged(AbsListView absListView, int scrollState) {
final Picasso picasso = Picasso.with(context);
if (scrollState == SCROLL_STATE_IDLE || scrollState == SCROLL_STATE_TOUCH_SCROLL) {

This comment has been minimized.

Copy link
@JakeWharton

JakeWharton Sep 22, 2014

Member

No settling state :( Damn you, AbsListView.

* @see Picasso#pauseRequestGroup(String)
* @see Picasso#resumeRequestGroup(String)
*/
public RequestCreator group(String group) {

This comment has been minimized.

Copy link
@JakeWharton

JakeWharton Sep 22, 2014

Member

In my brain I implemented this with just Object so that you could use the activity, fragment, listview (or adapter) instance directly. This would save you from having to contrive names and lets you re-use the objects into which you are already tossing these images. Downsides are that you could leak things if you're careless.

What do you think?

This comment has been minimized.

Copy link
@dallasgutauckis

This comment has been minimized.

Copy link
@lucasr

lucasr Sep 22, 2014

Author Contributor

Interesting idea. I don't feel strongly about this. The flexibility of using Object can be handy. I'll go with it.

// The hunter.detach() call removes the action from the
// list we're iterating. Adjust loop variables accordingly.
i--;
count--;

This comment has been minimized.

Copy link
@JakeWharton

JakeWharton Sep 22, 2014

Member

This can be avoided by looping backwards.

for (int i = joined.size() - 1; i >= 0; i--) {
  // ...
}

This comment has been minimized.

Copy link
@lucasr

lucasr Sep 22, 2014

Author Contributor

Duh, indeed. Fixed.

if (hunter.cancel()) {
it.remove();
if (loggingEnabled) {
log(OWNER_DISPATCHER, VERB_CANCELED, getLogIdsForHunter(hunter), "all actions paused");

This comment has been minimized.

Copy link
@JakeWharton
void resumeAction(Action action) {
Bitmap bitmap = null;
if (!action.skipCache) {
bitmap = quickMemoryCacheCheck(action.getKey());

This comment has been minimized.

Copy link
@JakeWharton

JakeWharton Sep 22, 2014

Member

This is a fun case to have happen from a consumer perspective.

This comment has been minimized.

Copy link
@lucasr

lucasr Sep 22, 2014

Author Contributor

This is just me being strict here. But, yeah, not likely to happen very often.

* Default request group used when no group identifier is explicitly
* set on new requests with {@link RequestCreator#group(String)}.
*/
public static final String DEFAULT_GROUP = "default";

This comment has been minimized.

Copy link
@JakeWharton

JakeWharton Sep 22, 2014

Member

When would you use this? It's a big knob that I don't know when to turn. I feel like the supplied groups are the right size and scope.

This comment has been minimized.

Copy link
@lucasr

lucasr Sep 22, 2014

Author Contributor

Yeah, this is one of my least favourite bits in this PR. Basically, I want to avoid forcing Picasso users to set a group on all requests. DEFAULT_GROUP would be used when using pauseRequestGroup()/resumeRequestGroup() when you don't explicitly set a request group. It makes using the new APIs on existing code a bit easier i.e. no need to update all calling points with group(). See how I used the new APIs in the sample app, for example.

Maybe a cleaner alternative is to have cancelDefaultRequestGroup(), pauseDefaultRequestGroup(), and resumeDefaultRequestGroup() and make DEFAULT_GROUP a private constant.

Thoughts?

This comment has been minimized.

Copy link
@JakeWharton

JakeWharton Sep 22, 2014

Member

I think the idea of a default group is strange because its size changes based on how much or little you use the explicit groups. Is what we want the ability to control all groups (i.e., all requests) at once?

This comment has been minimized.

Copy link
@lucasr

lucasr Sep 22, 2014

Author Contributor

Yeah, maybe that's the use case for non-tagged requests (pause/resume all requests). I'm fine with leaving that out unless you'd like to have something like pauseAllRequests()/resumeAllRequests(). That would add some complexity to the Dispatcher though.

@lucasr
Copy link
Contributor Author

lucasr commented Sep 22, 2014

Updated the branch:

  • Changed group identifier to be an Object instead of String. Also added a thorough cautionary note on the group(Object) documentation.
  • Cleaner loop when pausing a group.
@@ -156,6 +157,35 @@ public RequestCreator error(Drawable errorDrawable) {
}

/**
* Assign this request to a group. Request groups are an easy way to logically
* associate related requests that must managed (paused, resumed, or canceled)

This comment has been minimized.

Copy link
@cypressious

cypressious Sep 22, 2014

Small nitpick: Should be "that must be managed"

This comment has been minimized.

Copy link
@lucasr

lucasr Sep 22, 2014

Author Contributor

Fixed, thanks.

@dnkoutso
Copy link
Collaborator

dnkoutso commented Sep 22, 2014

This is awesome and a great example why I love OSS.

I am curious about the performance gains from this change. I've always thought the sample app had good performance and near 60fps but everynow and then it would skip a frame. I think this will eliminate this.

@JakeWharton
Copy link
Member

JakeWharton commented Sep 22, 2014

What do you think about naming this "tag" for parity with OkHttp and Volley (and eventually maybe Retrofit)?

@lucasr
Copy link
Contributor Author

lucasr commented Sep 22, 2014

@JakeWharton I'm fine with "tag". I started with it but thought the notion of "request groups" sounded cooler :-) Method names looked a bit verbose though e.g. cancelRequestsWithTag, resumeRequestsWithTag, pauseRequestsWithTag. Any suggestions for the method names?

@lucasr
Copy link
Contributor Author

lucasr commented Sep 22, 2014

Maybe just cancelRequests, pauseRequests, and resumeRequests?

@lucasr
Copy link
Contributor Author

lucasr commented Sep 22, 2014

Updated branch:

  • Renamed "group" terminology to "tag" throughout:
    • cancelRequestGroup(Object)cancelRequests(Object)
    • pauseRequestGroup(Object)pauseRequests(Object)
    • resumeRequestGroup(Object)resumeRequests(Object)
@JakeWharton
Copy link
Member

JakeWharton commented Sep 22, 2014

I was going to say we could just do cancelTag, resumeTag, etc. Requests are the only public-facing unit of work we have and we don't emphasize it anywhere in the public API's method names. Plus it's strange to call .tag(Object) on the builder and another method without "tag" in its name.

@lucasr
Copy link
Contributor Author

lucasr commented Sep 22, 2014

@JakeWharton Good point. Done.

@lucasr
Copy link
Contributor Author

lucasr commented Sep 22, 2014

Updated branch:

  • cancelRequests(Object)cancelTag(Object)
  • pauseRequests(Object)pauseTag(Object)
  • resumeRequests(Object)resumeTag(Object)
  • Removed Picasso.DEFAULT_TAG. When tag is null, the Action instance uses itself as its tag—idea borrowed from OkHttp.
  • Moar tests on new API
  • Updated sample app to use an Activity as request tag. Ensures there are no pending requests in the activity by calling cancelTag(this) in onDestroy().
  • Documentation fixes and other small tweaks.

FWIW, I'm happy with the state of the branch now. The only open question I can think of is whether or not we want to have APIs to cancel/pause/resume all requests. We can do this in a follow-up ticket after merging this PR though. Thoughts?

@lucasr
Copy link
Contributor Author

lucasr commented Sep 22, 2014

Also, maybe add this PR to the 2.4 milestone and close #561?

@dnkoutso dnkoutso added this to the Picasso 2.4 milestone Sep 23, 2014
@JakeWharton
Copy link
Member

JakeWharton commented Sep 23, 2014

We don't really use milestones much. Just when were planning issues for the
future, and only about half.
On Sep 22, 2014 7:09 PM, "Lucas Rocha" notifications@github.com wrote:

Also, maybe add this PR to the 2.4 milestone and close #561
#561?


Reply to this email directly or view it on GitHub
#665 (comment).

* associate related requests that can be managed (paused, resumed, or canceled)
* as a tag.
* <p>
* Most of the time, you'll be using {@link String} tags but you can also use objects

This comment has been minimized.

Copy link
@JakeWharton

JakeWharton Sep 23, 2014

Member

I will never use strings so let's not say most.

This comment has been minimized.

Copy link
@lucasr

lucasr Sep 23, 2014

Author Contributor

Done.

@@ -156,6 +157,35 @@ public RequestCreator error(Drawable errorDrawable) {
}

/**
* Assign a tag to this request. Tags are an easy way to logically
* associate related requests that can be managed (paused, resumed, or canceled)
* as a tag.

This comment has been minimized.

Copy link
@JakeWharton

JakeWharton Sep 23, 2014

Member

Sentence starts and ends with "tag". Maybe:

Tags are an easy way to logically associate related requests that can be managed together (e.g., paused, resumed, or canceled).

This comment has been minimized.

Copy link
@lucasr

lucasr Sep 23, 2014

Author Contributor

Done.

* to the tag for as long as this tag is paused and/or have active requests.
* Look out for potential leaks.
*
* @param tag A tag object that identifies the tag of requests.

This comment has been minimized.

Copy link
@JakeWharton

JakeWharton Sep 23, 2014

Member

Delete this little guy. He's redundant.

This comment has been minimized.

Copy link
@lucasr

lucasr Sep 23, 2014

Author Contributor

Removed.

* Most of the time, you'll be using {@link String} tags but you can also use objects
* that naturally define the scope of your requests in your app such as a
* {@link android.content.Context}, an {@link android.app.Activity},
* or a {@link android.app.Fragment}. Keep in mind that Picasso will keep a reference

This comment has been minimized.

Copy link
@JakeWharton

JakeWharton Sep 23, 2014

Member

Break out this sentence to a new paragraph and preface with <strong>WARNING:</strong>. I'd also consider dropping "Keep in mind that" from the beginning then.

This comment has been minimized.

Copy link
@lucasr

lucasr Sep 23, 2014

Author Contributor

Done.

@@ -52,6 +53,7 @@ public RequestWeakReference(Action action, T referent, ReferenceQueue<? super T>
this.errorResId = errorResId;
this.errorDrawable = errorDrawable;
this.key = key;
this.tag = (tag != null ? tag : this);

This comment has been minimized.

Copy link
@JakeWharton

JakeWharton Sep 23, 2014

Member

Is using this actually useful? The instance is never exposed nor used as a tag.

This comment has been minimized.

Copy link
@lucasr

lucasr Sep 23, 2014

Author Contributor

Two reasons for using 'this' here:

  • Ensure non-null invariant in the tag member i.e. no null checks needed and no potential NPEs in tag matching code.
  • Forces this tag to be private when undefined and ensures it won't be mixed with user-entered tags by mistake.
@JakeWharton
Copy link
Member

JakeWharton commented Sep 23, 2014

This looks fantastic. Beers will reign down upon you the next time you come to SF. Thanks for putting up with our sometimes ridiculous nit picking.

@lucasr
Copy link
Contributor Author

lucasr commented Sep 23, 2014

Branch updated with all suggested changes.

if (pausedTags.contains(action.getTag())) {
pausedActions.put(action.getTarget(), action);
if (action.getPicasso().loggingEnabled) {
log(OWNER_DISPATCHER, VERB_IGNORED, action.request.logId(),

This comment has been minimized.

Copy link
@JakeWharton

JakeWharton Sep 23, 2014

Member

I think we want to reuse the "Paused" verb here. "Ignored" makes it seem like we aren't deferring the request.

This comment has been minimized.

Copy link
@lucasr

lucasr Sep 23, 2014

Author Contributor

Done.

@JakeWharton
Copy link
Member

JakeWharton commented Sep 23, 2014

Sorry one more tiny, tiny thing. LGTM otherwise!

@lucasr
Copy link
Contributor Author

lucasr commented Sep 23, 2014

@JakeWharton Branch updated.

@JakeWharton
Copy link
Member

JakeWharton commented Sep 23, 2014

You need to rebase, but only because I merged your other awesome PR.

@lucasr
Copy link
Contributor Author

lucasr commented Sep 23, 2014

Rebased.

JakeWharton added a commit that referenced this pull request Sep 23, 2014
Implement request group and pause/resume support
@JakeWharton JakeWharton merged commit 547ebdb into square:master Sep 23, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@JakeWharton
Copy link
Member

JakeWharton commented Sep 23, 2014

Thanks!

This was referenced Sep 23, 2014
@lucasr
Copy link
Contributor Author

lucasr commented Sep 23, 2014

\o/

@cypressious
Copy link

cypressious commented Sep 23, 2014

When can we expect a release?

@JakeWharton
Copy link
Member

JakeWharton commented Sep 23, 2014

We don't provide ETAs. We'll release when we feel that it's been thoroughly tested and stable.

@felipecsl
Copy link

felipecsl commented Sep 24, 2014

This is awesome. Big kudos!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.