Support contact Uri #394

[R4md4c]

So I've supported these kind of Uris for issue #394 , also wrote a simple sample for this feature.

content://com.android.contacts/contacts/5/display_photo
content://com.android.contacts/contacts/lookup/3570i61d948d30808e537
content://com.android.contacts/contacts/38
content://com.android.contacts/contacts/lookup/3570i61d948d30808e537

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[R4md4c]

I've signed it!

[googlebot]

CLAs look good, thanks!

[coveralls]

Coverage decreased (-0.2%) to 83.137% when pulling 696f86c on R4md4c:3.0 into cb47a64 on bumptech:3.0.

[TWiStErRob]

I think match() result could be passed in, it's enough to parse the Uri once.

[TWiStErRob]

Nice, thank you!

See my comments, treat them as idea-sparking ones, not as sacred texts = feel free to argue them ;)

[TWiStErRob]

@sjudd is there any point doing this for FileDescriptorLocalUriFetcher too?

[R4md4c]

@TWiStErRob

Thanks a lot for your comments, it was really helpful!
I changed the code so that the .match() call happens only once, instead of several times.

[TWiStErRob]

What do you think about inlining this call to simplify?
matchedUri == UriMatcher.NO_MATCH is the same as the default in the switch.

[R4md4c]

I'm thinking of doing something like

return matchedUri != UriMatcher.NO_MATCH ? loadResourceFromContactUri(uri, contentResolver, matchedUri)  : contentResolver.openInputStream(uri);

What do you think?

[TWiStErRob]

That's not nice to read, consider:

switch (URI_MATCHER.match(uri)) {
    case ID_LOOKUP:
    case ID_CONTACT: {
        Uri contactUri = uri;
        ...
        return openContactPhotoInputStream(contentResolver, contactUri);
    }
    case ID_THUMBNAIL:
    case ID_DISPLAY_PHOTO:
    case UriMatcher.NO_MATCH:
    default:
        return contentResolver.openInputStream(uri);
}

this is what happens anyway, just with bigger cyclomatic complexity and duplicated call to openInputStream.

[R4md4c]

I've updated the PR, into to your more simplified version.

[coveralls]

Coverage decreased (-0.09%) to 83.229% when pulling 89454c6 on R4md4c:3.0 into cb47a64 on bumptech:3.0.

[sjudd]

two space indents please.

[TWiStErRob]

This is v3, didn't you introduce Google style in v4?

[sjudd]

Yup good point, disregard.

[sjudd]

@TWiStErRob No need to do this for file descriptors, at least not right now. They're currently only used for loading videos. That said, I actually think they'd be more efficient than InputStreams for local content retrieved via a a ContentResolver, so we may revisit that in the future.

[sjudd]

Also thanks for putting this up! Other than one or two minor comments looks good.

[coveralls]

Coverage decreased (-0.09%) to 83.229% when pulling 4f633ec on R4md4c:3.0 into cb47a64 on bumptech:3.0.

[R4md4c]

Hi @TWiStErRob,

It is been 4 days since the last patch, but I haven't heard any feedback from you, is there something missing from my side?

[TWiStErRob]

Oh, sorry, I'm ok with it, but @sjudd has the final word (=presses the merge button). He said those two nits, which you fixed, but we don't get notifications for force push commits (nor normal commits for that matter), you have to comment manually, which you've done now :)

[sjudd]

Thanks!