Skip to content

Enable resource loading from android.resource URI.#292

Merged
dnkoutso merged 1 commit intosquare:masterfrom
aravance:master
Jan 27, 2014
Merged

Enable resource loading from android.resource URI.#292
dnkoutso merged 1 commit intosquare:masterfrom
aravance:master

Conversation

@aravance
Copy link
Copy Markdown
Contributor

Added full support for the android.resource style URI as outlined at the link below.

http://developer.android.com/reference/android/content/ContentResolver.html#openAssetFileDescriptor(android.net.Uri, java.lang.String)

@slvn
Copy link
Copy Markdown

slvn commented Oct 27, 2013

Thumbs up for this pull request !

@dnkoutso
Copy link
Copy Markdown
Collaborator

We are reviewing the possibility to add this to next version. Please hold.

Code LGTM, but we will need to have some tests for this.

@aravance
Copy link
Copy Markdown
Contributor Author

  1. Is there any sort of target date for the next version (tentative even)? I need to decide if I should do a local shaded release of picasso on our private maven server or if I should implement a temporary workaround while I wait for the next release.

  2. What kind of tests do we need? I can easily create tests to verify we get the right type of BitmapHunter like those that exist already. What else? I'm new to unit testing with Android, but I definitely want to learn it.

@JakeWharton
Copy link
Copy Markdown
Collaborator

We don't really do ETAs. Our open source projects are done in free time so it's all dependent on how much we each have. We try to keep releases regular but I really have no idea when the next one will be.

@dnkoutso
Copy link
Copy Markdown
Collaborator

This can make it for 2.2 but will need tests and ensure the build is green (paste built result here).

Ping @aravance

@aravance
Copy link
Copy Markdown
Contributor Author

I added some unit tests to verify some elements of the android.resource uri scheme. Any comments or recommendations are welcome. I'm fairly new to real Android unit testing. I plan to try to put it into practice where I work soon, so learning is a good thing.

I'm not sure what you mean by "the build is green." It builds successfully: 158 tests, 0 failures, 0 errors, 2 skipped.

I have OkHttpDownloaderTest and UrlConnectionDownloadTest @ignored on my machine because they seem to be timing out on my machine. They have to be ignored on my machine with or without this change set.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private static final String PACKAGE

@dnkoutso
Copy link
Copy Markdown
Collaborator

Thanks! please rebase squash into one commit. It seems like CLA is signed so this can go in!

@dnkoutso
Copy link
Copy Markdown
Collaborator

Reviewing this.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why no more private?

@dnkoutso
Copy link
Copy Markdown
Collaborator

A few minor comments left and I'll merge this.

We're ramping up for 2.2 release.

@dnkoutso
Copy link
Copy Markdown
Collaborator

Can you please explain a bit more the use case of this? Are you trying to load bitmaps for other packages? In this case why not use a content provider with a URI?

@aravance
Copy link
Copy Markdown
Contributor Author

In my particular use case, I am loading images from the same package, but the android.resource uri format is not currently fully supported. I have images from USB as well as from package resource that I want to show together, and using one uniform method of access is preferred, i.e. URI.

I figured while I was implementing the full support for the local package, I would go ahead and implement support comparable to that of ContentResolver. See ContentResolver.openAssetFileDescriptor(Uri, String).

I could foresee a use case where one does not have control over a package but knows what is contained therein and wants to access it via URI.

@dnkoutso
Copy link
Copy Markdown
Collaborator

We are reviewing this for merging for 2.2.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method name should be something like getResourceResult.

@dnkoutso
Copy link
Copy Markdown
Collaborator

We want to merge this but I am a bit concerned around the extra allocation for ResourceResult.

While it appears to be nitpicking we want to minimize object allocation as to maintain good performance. You think you can break it up into two methods that provide the resources and the id to use instead?

@dnkoutso
Copy link
Copy Markdown
Collaborator

I like this. You have two System.out.println statements. If you remove I will merge.

@aravance
Copy link
Copy Markdown
Contributor Author

Those snuck their way back in again? Working on multiple machines... I'll
fix it when I get to by computer.
On Jan 26, 2014 2:05 PM, "Dimitris" notifications@github.com wrote:

I like this. You have two System.out.println statements. If you remove I
will merge.


Reply to this email directly or view it on GitHubhttps://github.com//pull/292#issuecomment-33328488
.

@dnkoutso
Copy link
Copy Markdown
Collaborator

Thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants