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

Support custom bitmap loading with a new RequestHandler API #512

Merged
merged 1 commit into from Sep 3, 2014

Conversation

@lucasr
Copy link
Contributor

lucasr commented May 22, 2014

I'm trying to streamline all (or most of) image loading code in Firefox for Android's UI around Picasso. We load images from sources that not supported by Picasso e.g. browser tab thumbnails, app icons from Android's PackageManager, images only accessible through a local Java API or through Gecko, etc.

I really want to avoid having to patch Picasso locally in order support our specific use cases. So, here's an idea: allow Picasso users to extend the library to load bitmaps from non-standard sources. The API docs and tests should hopefully demonstrate the general idea well enough. Let me know if something is not clear.

I'm not entirely happy with the class names (RequestHandler, Decoder, etc) and the API can probably be simplified a bit. Looking for some early feedback here.

Thoughts?

@JakeWharton
Copy link
Member

JakeWharton commented May 22, 2014

So you can kind of do this already by wrapping the Downloader and handling alternate URLs. You can see an example of this here which handles mock:// URLs.

This isn't a scalable solution though so we should figure out a better way of supporting this use case. We'll talk about it internally here today and evaluate your approach along with some potential others.

@dnkoutso dnkoutso added this to the Picasso 2.4 milestone May 22, 2014
@lucasr
Copy link
Contributor Author

lucasr commented May 22, 2014

Yeah, considered wrapping Downloader but it felt a bit too hacky given that my custom code is only dealing with local images anyway i.e. wrong place for this kind of abstraction. One thing I like about the 'RequestHandler' approach is the potential for refactoring the built-in BitmapHunters around the same kind of API in the long term. But that's a whole separate discussion.

@lucasr
Copy link
Contributor Author

lucasr commented May 29, 2014

Gentle ping :-) Any feedback?

@JakeWharton
Copy link
Member

JakeWharton commented May 29, 2014

We're releasing 2.3 today or tomorrow. We wanted to get that out the door
before taking a hard look.
On May 29, 2014 7:02 AM, "Lucas Rocha" notifications@github.com wrote:

Gentle ping :-) Any feedback?


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

@JayH5
Copy link
Contributor

JayH5 commented May 31, 2014

For me there are two cases that are not really well served by the existing API:

  1. You want to load some image from some source that Picasso does not support natively via the existing BitmapHunters. Generally you will have some distinct URI scheme for this.
  2. You want to load some image from a source that Picasso does support natively but that image is not there. For example, you might try to load some local image but the image does not exist so you need to download it from somewhere else. In this case it would be good to have some kind of secondary BitmapHunter that picks up requests that fail.

I'm currently making use of a bit of an over-complicated Downloader to cover both cases. I don't really have a nice solution for case 2 in mind but I think any solution would tie in to whatever is used for case 1 so I thought it was worth mentioning. For case 1, this pull request is pretty much what I would like to see.

My only issue is that now the NetworkBitmapHunter becomes a sort of absolute last resort if we can't find any other way to handle the URI. I'd rather have the NetworkBitmapHunter be used only for certain URIs such as http and https ones. Only after testing for all known schemes would these RequestHandlers be tried.

@JakeWharton
Copy link
Member

JakeWharton commented Jun 2, 2014

@JayH5 I'm not sure point 2 is Picasso's responsibility. That sounds application-level logic to me.

@lucasr
Copy link
Contributor Author

lucasr commented Jun 3, 2014

@JayH5 Probably worth pointing out that the API proposed in this pull request involves passing the Downloader instance to the RequestHandler's createDecoder() factory method. This would, in theory, allow you to implement your own network fallback mechanism within Picasso.

Another way to approach point 2 could be to allow queuing URLs around the same target. Very roughly speaking, your code would look like:

Picasso.with(context)
       .load(local_url).or(remote_url)
       .into(imageView);
@lucasr
Copy link
Contributor Author

lucasr commented Aug 21, 2014

Hey folks, just wondering if you had time to discuss this yet?

@JakeWharton
Copy link
Member

JakeWharton commented Aug 21, 2014

I've been thinking about this functionality from a registry approach similar to what you'd have in a library like Gson. The Picasso.Builder has a map of URI scheme to a UriHandler (naming not final) and a default UriHandler as a fallback case (this would be like the Downloader is today). Upon a URI needing to be loaded we'd iterate the list of handlers in order asking if they can handle it. First to say yes wins otherwise it falls to the default handler.

Through this you could register a bunch of custom URI schemes.

@Provides @Singleton providePicasso(@Application Context context, OkHttpClient client) {
  return new Picasso.Builder(context)
      .addUriHandler(new MockUriHandler(context))
      .addUriHandler(new ContactsUriHandler(context))
      .addUriHandler(new GraphUriHandler(context))
      .setDefaultUriHandler(new OkHttpUriHandler(client))
      .build();
}
final class MockSchemeHandler implements UriHandler {
  @Override public boolean canHandleUri(Uri uri) {
    return "mock".equals(uri.getScheme());
  }
  @Override public Result load(Uri uri) {
    return // ...
  }
}

What do you think?

@lucasr
Copy link
Contributor Author

lucasr commented Aug 26, 2014

@JakeWharton Looks good to me. I guess Result would look like:

final class Result {
    private final Uri sourceUri;
    private final Bitmap bitmap;
    private final LoadedFrom loadedFrom;

    public Result(Uri sourceUri, Bitmap bitmap, LoadedFrom loadedFrom) {...}

    public Uri getSourceUri() { ... }
    public Bitmap getBitmap() { ... }
    public LoadedFrom getLoadedFrom() {...}
}

Stuffing this information in a Result instance does seem cleaner than my original one-Decoder-per-request approach.

As I mentioned before, I would be great if the BitmapHunter sub-system was built on top of the UriHandlers as well—to avoid having two separate internal APIs to handle different types of Uri. This refactoring could done in a follow-up ticket though.

You mentioned Picasso.Builder holding "map of URI scheme to a UriHandler" but I think we better keep a simple list and let each UriHandler.canHandleUri() do the matching instead. This would allow more fine-grained Uri matching instead of only matching a specific Uri scheme. On the other hand, doing scheme-only matching would simplify the UriHandler API a bit i.e. no need for a canHandleUri(). Thoughts?

I can work on a new patch with the proposed API. Let me know if you'd prefer to implement this yourself though.

@JakeWharton
Copy link
Member

JakeWharton commented Aug 26, 2014

Yeah sorry I meant a list of them and calling canHandleUri() in order. I'd also like to remove the notion of a "default" handler from my above proposal. The http one would just accept 'http' and 'https' uri schemes.

As to the hunters, I agree. They initially were a function of not having a pluggable system like this. It would be great to kill them and consolidate on the handler's being the source of truth.

I know personally I don't have time to work on this for quite some time. Any contribution toward this feature (even a partial one) would be very welcome.

@lucasr
Copy link
Contributor Author

lucasr commented Aug 27, 2014

Ok, I'll probably have some extra cycles to work on this in the next week or so. I'll keep you posted.

@lucasr
Copy link
Contributor Author

lucasr commented Aug 29, 2014

Ok, here's a new patch implementing what I'm calling RequestHandler API. Quick overview of what it does:

  • Adds a new RequestHandler public class that can be subclassed by Picasso users to extend the library with custom image loading logic. RequestHandler subclasses must implement two methods: canHandleRequest(Request) and load(Request) which returns a RequestHandler.Result instance containing both the resulting Bitmap and where it was loaded from (DISK, MEMORY, NETWORK).
  • New RequestHandlers can be registered via a new Picasso.Builder API addRequestHandler(RequestHandler).
  • All built-in BitmapHunters have been re-implemented as private RequestHandlers. BitmapHunter is now a concrete class backed by a RequestHandler. See changes to BitmapHunter.forRequest().
  • Built-in RequestHandler are unconditionally registered to keep the same behaviour. Custom RequestHandlers are always added after the built-in ones i.e. built-in RequestHandlers always take precedence.
  • Retry/replay support can be defined per RequestHandler (see NetworkRequestHandler for an example). I kept this API private for now. But maybe we could consider exposing it in the future?
  • All tests have been updated accordingly. I had to add some extra bits around Picasso mocking. See changes to TestUtils.

I initially built the API around Uris but some of the built-in RequestHandlers need more than a Uri to figure out what to do with the request. In any case, using a Request as input feels a bit more future-proof as it can be extended with more contextual information later.

I originally started this as a series of smaller patches but there are too many dependencies between the BitmapHunters. A single larger patch is probably more convenient to review in this case.

@JakeWharton
Copy link
Member

JakeWharton commented Aug 29, 2014

From a quick scan it looks great. Will do a more in-depth review tonight. Thanks for taking the time to do this!

}
options.inSampleSize = sampleSize;
options.inJustDecodeBounds = false;
throw new IllegalStateException("Unrecognized type of URI: " + request.uri);

This comment has been minimized.

@JakeWharton

JakeWharton Aug 29, 2014 Member

This message is a bit off since the full Request is being used to pick a handler now.

This comment has been minimized.

@lucasr

lucasr Aug 29, 2014 Author Contributor

Missed this comment. Yeah, replaced URI with request.

/**
* {@link RequestHandler} allows you to extend Picasso to load images
* in ways that are not supported by default in the library.
* <p/>

This comment has been minimized.

@JakeWharton

JakeWharton Aug 29, 2014 Member

JDK 8's doclint tool will flag this as invalid. Just use <p> between paragraphs.

This comment has been minimized.

@lucasr

lucasr Aug 29, 2014 Author Contributor

Fixed.

* {@link RequestHandler}.
*
* @See RequestHandler
* @See RequestHandler.load(Uri uri)

This comment has been minimized.

@JakeWharton

JakeWharton Aug 29, 2014 Member

nit: needs to be @see (lowercase)

This comment has been minimized.

@lucasr

lucasr Aug 29, 2014 Author Contributor

Fixed.

Request request = action.getRequest();

List<RequestHandler> requestHandlers = picasso.getRequestHandlers();
if (requestHandlers != null && !requestHandlers.isEmpty()) {

This comment has been minimized.

@JakeWharton

JakeWharton Aug 29, 2014 Member

Both of these conditionals should always be false and can be removed.

This comment has been minimized.

@lucasr

lucasr Aug 29, 2014 Author Contributor

Indeed, leftover from previous iterations. Removed.

/**
* Whether or not this {@link RequestHandler} can handle a request with the
* given {@link android.net.Uri}.
* @param data

This comment has been minimized.

@JakeWharton

JakeWharton Aug 29, 2014 Member

remove this

This comment has been minimized.

@lucasr

lucasr Aug 29, 2014 Author Contributor

Done.


/**
* Whether or not this {@link RequestHandler} can handle a request with the
* given {@link android.net.Uri}.

This comment has been minimized.

@JakeWharton

JakeWharton Aug 29, 2014 Member

Request, not Uri

This comment has been minimized.

@lucasr

lucasr Aug 29, 2014 Author Contributor

Fixed.

}

/**
* Returns the resulting {@link android.graphics.Bitmap} generated

This comment has been minimized.

@JakeWharton

JakeWharton Aug 29, 2014 Member

DOesn't need to be fully-qualified

This comment has been minimized.

@lucasr

lucasr Aug 29, 2014 Author Contributor

Fixed.

public abstract boolean canHandleRequest(Request data);

/**
* Loads an image for the given {@link android.net.Uri}.

This comment has been minimized.

This comment has been minimized.

@lucasr

lucasr Aug 29, 2014 Author Contributor

Fixed.

if (requestHandlers != null && !requestHandlers.isEmpty()) {
for (RequestHandler requestHandler : requestHandlers) {
// Resource IDs can be used instead of URIs in some requests. We special-case
// ResourceRequestHandler here to avoid null checks on all RequestHandler implementations.

This comment has been minimized.

@JakeWharton

JakeWharton Aug 29, 2014 Member

This can be removed now right? Since we're using full Requests now, that is.

This comment has been minimized.

@lucasr

lucasr Aug 29, 2014 Author Contributor

Removed. We'll have to make sure ResourceRequestHandler is always the first RequestHandler in the list though. This is guaranteed to happen now because we control the order of the built-in RequestHandlers. I added a comment in Picasso's constructor for future reference.

@@ -19,7 +19,7 @@

@RunWith(RobolectricTestRunner.class)
@Config(manifest = Config.NONE)
public class AssetBitmapHunterTest {
public class AssetRequestHandlerTest {
@Mock Context context;
@Mock Picasso picasso;

This comment has been minimized.

@JakeWharton

JakeWharton Aug 29, 2014 Member

Some of these can be deleted now. Like this one!

This comment has been minimized.

@lucasr

lucasr Aug 29, 2014 Author Contributor

Nice catch, audited the other tests for unused members.

public class RequestHandlerTest {

@Test public void bitmapConfig() throws Exception {
for (Bitmap.Config config : Bitmap.Config.values()) {

This comment has been minimized.

allRequestHandlers.add(new AssetRequestHandler(context));
allRequestHandlers.add(new FileRequestHandler(context));
allRequestHandlers.add(new NetworkRequestHandler(dispatcher.downloader, stats));
if (extraRequestHandlers != null && !extraRequestHandlers.isEmpty()) {

This comment has been minimized.

@JakeWharton

JakeWharton Aug 29, 2014 Member

The list should never be empty so the null check is all that's needed.

This comment has been minimized.

@lucasr

lucasr Aug 29, 2014 Author Contributor

Good catch. Done.

@@ -572,6 +594,21 @@ public Builder requestTransformer(RequestTransformer transformer) {
return this;
}

/** Register an Uri handler. */

This comment has been minimized.

This comment has been minimized.

@lucasr

lucasr Aug 29, 2014 Author Contributor

Done.

throw new IllegalArgumentException("RequestHandler must not be null.");
}
if (requestHandlers == null) {
requestHandlers = new ArrayList();

This comment has been minimized.

@JakeWharton

JakeWharton Aug 29, 2014 Member

Missing generics

This comment has been minimized.

@lucasr

lucasr Aug 29, 2014 Author Contributor

Fixed.


/**
* Lazily create {@link android.graphics.BitmapFactory.Options} based in given
* {@link com.squareup.picasso.Request}, only instantiating them if needed.

This comment has been minimized.

@JakeWharton

JakeWharton Aug 29, 2014 Member

Types don't need to be fully qualified since they're imported

This comment has been minimized.

@lucasr

lucasr Aug 29, 2014 Author Contributor

Fixed.

* @See RequestHandler
* @See RequestHandler.load(Uri uri)
*/
public static class Result {

This comment has been minimized.

This comment has been minimized.

@lucasr

lucasr Aug 29, 2014 Author Contributor

Indeed. Done.

@JakeWharton
Copy link
Member

JakeWharton commented Aug 29, 2014

Still looks great after a detailed scan. I had only minor things to say. Overall I'm excited to get this in and released because we're currently using the Downloader subclassing right now to accomplish this internally.

@lucasr
Copy link
Contributor Author

lucasr commented Aug 29, 2014

Here's an update patch with all comments addressed.

@JakeWharton
Copy link
Member

JakeWharton commented Sep 3, 2014

LGTM. Once @dnkoutso reviews we'll merge.

@@ -276,70 +276,17 @@ static void updateThreadName(Request data) {
Thread.currentThread().setName(builder.toString());
}

static BitmapHunter forRequest(Context context, Picasso picasso, Dispatcher dispatcher,

This comment has been minimized.

@dnkoutso

dnkoutso Sep 3, 2014 Collaborator

I am so glad this is almost dead.

This comment has been minimized.

@lucasr

lucasr Sep 3, 2014 Author Contributor

\o/

options.inJustDecodeBounds = justBounds;
if (hasConfig) {
options.inPreferredConfig = data.config;
for (RequestHandler requestHandler : picasso.getRequestHandlers()) {

This comment has been minimized.

@dnkoutso

dnkoutso Sep 3, 2014 Collaborator

This is a nit, mostly around Picasso we avoid foreach for allocating iterators.

This comment has been minimized.

@lucasr

lucasr Sep 3, 2014 Author Contributor

Fair enough, replaced with a for loop.

@@ -572,6 +597,21 @@ public Builder requestTransformer(RequestTransformer transformer) {
return this;
}

/** Register a request handler. */

This comment has been minimized.

@dnkoutso

dnkoutso Sep 3, 2014 Collaborator

Nit use {@link RequestHandler}.

This comment has been minimized.

@lucasr

lucasr Sep 3, 2014 Author Contributor

Done.

this.context = context;
this.dispatcher = dispatcher;
this.cache = cache;
this.listener = listener;
this.requestTransformer = requestTransformer;

final List<RequestHandler> allRequestHandlers = new ArrayList<RequestHandler>(7);

This comment has been minimized.

@dnkoutso

dnkoutso Sep 3, 2014 Collaborator

Nit: If we use the capacity constructor we can take into consideration the extraRequestHandlers size also.

This comment has been minimized.

@lucasr

lucasr Sep 3, 2014 Author Contributor

Done.

@dnkoutso
Copy link
Collaborator

dnkoutso commented Sep 3, 2014

LGTM. A few tiny nits.

@lucasr
Copy link
Contributor Author

lucasr commented Sep 3, 2014

Thanks for the review @dnkoutso. Branch updated with suggested changes.

dnkoutso added a commit that referenced this pull request Sep 3, 2014
Support custom bitmap loading with a new RequestHandler API
@dnkoutso dnkoutso merged commit d3a6b45 into square:master Sep 3, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@dnkoutso
Copy link
Collaborator

dnkoutso commented Sep 3, 2014

@lucasr thanks for all the work on this one!

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.

None yet

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