Download module #114
Download module #114
Conversation
| imageViewRef = new WeakReference<>(imageView); | ||
| } | ||
|
|
||
| synchronized void setUrl(String url) { |
There was a problem hiding this comment.
I rather avoid a synchronized method calling a synchronized method. Even though it works, it's a bit confusing and not as readable.
Add a private not-synchronizedd doStart() methods that directly starts the download - then both public methods (start and setUrl()) can call it if the conditions are right. Makes sense?
| ImageRequest imageRequest = imageRequestBuilder.build(); | ||
| GenericDraweeHierarchy genericDraweeHierarchy = genericDraweeHierarchyBuilder.build(); | ||
|
|
||
| DraweeView draweeView = (DraweeView) imageView; |
There was a problem hiding this comment.
Instead of failing on a cast here, please throw an IllegalArgumentException (at the beginning of the method) if the image view is not a DraweeView
| progressBar.setVisibility(View.GONE); | ||
| } | ||
| }); | ||
| currentUrl = url.generate(); |
There was a problem hiding this comment.
Please add a TODO here to put back the progress bar visibility code once we implement callbacks in the strategy.
download/build.gradle
Outdated
| compileOnly 'com.squareup.picasso:picasso:2.71828' | ||
| compileOnly 'com.facebook.fresco:fresco:2.2.0' | ||
| compileOnly 'com.github.bumptech.glide:glide:4.11.0' | ||
| annotationProcessor 'com.github.bumptech.glide:compiler:4.11.0' |
There was a problem hiding this comment.
Is this needed here? The glide integration is a separate module and I didn't spot a usage of extensions in this PR.
| public static void setup() { | ||
| Context context = InstrumentationRegistry.getInstrumentation().getTargetContext(); | ||
| MediaManager.init(context); | ||
| MediaManager.get().getCloudinary().config.cloudName = TEST_CLOUD_NAME; |
There was a problem hiding this comment.
The cloud info should be injected from the environment instead of hard coded constants.
You can see the full workflow in the core tests. The test manifest has a template place holder and the build process replaces it with with the env variable.
https://github.com/cloudinary/cloudinary_android/blob/master/core/src/androidTest/AndroidManifest.xml and
https://github.com/cloudinary/cloudinary_android/blob/master/core/build.gradle (the manifestPlaceholders line).
Let me know if you need assistance with that.
| } | ||
| } else if (source instanceof Integer) { | ||
| downloadRequestBuilderStrategy.load((Integer) source); | ||
| } else { |
There was a problem hiding this comment.
Please separate into two exceptions - illegal argument exception in case the source is not a string/int either, and an illegal state exception if it's explicitly null.
It's true that currently, your check is 100% accurate (since we only allow to send in a string or an int), but this may change in a future commit and we can easily miss this check and cause unexpected error messages.
No description provided.