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.

Sign up for GitHub

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

Jump to bottom

Use a NetworkBitmapClient as a wrapper for Picasso #139

Merged
merged 1 commit into from
Mar 31, 2016
Merged

Use a NetworkBitmapClient as a wrapper for Picasso #139

merged 1 commit into from
Mar 31, 2016

Conversation

lenguyenthanh
Copy link
Contributor

We should use a wrapper for Picasso instead of using it directly. With a wrapper we can easily switch to another ImageCaching client such as: Glide.

@lenguyenthanh lenguyenthanh force-pushed the use-wrapper-for-picasso branch 2 times, most recently from c54611a to 662479a Compare March 28, 2016 14:44
@lenguyenthanh lenguyenthanh changed the title User a NetworkBitmapClient as a wrapper for Picasso Use a NetworkBitmapClient as a wrapper for Picasso Mar 28, 2016
@artem-zinnatullin
Copy link
Owner

Yes yes yes + 1000 to that proposal, I'll try to review PR in a few days (very busy, sorry), thanks!

@artem-zinnatullin artem-zinnatullin added this to the v1.0 milestone Mar 28, 2016
@artem-zinnatullin artem-zinnatullin self-assigned this Mar 28, 2016
@lenguyenthanh
Copy link
Contributor Author

No problem mate!

@codecov-io
Copy link

Current coverage is 81.62%

Merging #139 into master will increase coverage by +0.20% as of c975bfc

@@            master    #139   diff @@
======================================
  Files           37      38     +1
  Stmts          549     555     +6
  Branches        47      47       
  Methods          0       0       
======================================
+ Hit            447     453     +6
  Partial         29      29       
  Missed          73      73

Review entire Coverage Diff as of c975bfc

Powered by Codecov. Updated on successful CI builds.

vanniktech
vanniktech reviewed Mar 28, 2016
View reviewed changes
app/src/main/java/com/artemzin/qualitymatters/models/PicassoBitmapClient.java

private final Picasso picasso;

public PicassoBitmapClient(final Picasso picasso) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could not we just use Constructor injection here?

@vanniktech
Copy link
Contributor

Nice 👍 left a few comments here and there

@lenguyenthanh
Copy link
Contributor Author

Thanks @vanniktech, I'm going to fix them.

@lenguyenthanh lenguyenthanh force-pushed the use-wrapper-for-picasso branch from 662479a to d9aca5c Compare March 28, 2016 15:17
artem-zinnatullin
artem-zinnatullin reviewed Mar 29, 2016
View reviewed changes
app/src/main/java/com/artemzin/qualitymatters/models/NetworkBitmapClient.java
import android.widget.ImageView;

public interface NetworkBitmapClient {
void downloadInto(String url, ImageView imageView);
Copy link
Owner

Choose a reason for hiding this comment

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

nullability annotations

Copy link
Owner

Choose a reason for hiding this comment

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

Do we want to put @UiThread annotation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we really need it? Idk

@lenguyenthanh
Copy link
Contributor Author

Just wake up, and receive a lot of comments. Thanks you guys. I'm gonna fix them. Have a nice day ^^

@lenguyenthanh lenguyenthanh force-pushed the use-wrapper-for-picasso branch 2 times, most recently from d274e19 to cbca2d1 Compare March 30, 2016 05:48
@lenguyenthanh
Copy link
Contributor Author

Just want to check status of this PR, no rush. I fixed some comments and replied some comments. How do you think about this @artem-zinnatullin, @vanniktech 😄

vanniktech
vanniktech reviewed Mar 31, 2016
View reviewed changes
app/src/unitTests/java/com/artemzin/qualitymatters/models/PicassoImageLoaderTest.java

@SuppressWarnings("NullableProblems") // Initialized in @Before.
@NonNull
RequestCreator loadRequestCreator;
Copy link
Contributor

Choose a reason for hiding this comment

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

can be private

@vanniktech
Copy link
Contributor

Few nits 👍

artem-zinnatullin
artem-zinnatullin reviewed Mar 31, 2016
View reviewed changes
app/src/main/java/com/artemzin/qualitymatters/ApplicationModule.java
@@ -49,4 +51,9 @@ public Picasso providePicasso(@NonNull Application qualityMattersApp, @NonNull O
.downloader(new OkHttp3Downloader(okHttpClient))
.build();
}

@Provides @NonNull @Singleton
public QualityMattersImageLoader provideNetworkBitmapClient(@NonNull PicassoImageLoader picassoBitmapClient) {
Copy link
Owner

Choose a reason for hiding this comment

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

Should be renamed to provideImageLoader or something :)

@artem-zinnatullin
Copy link
Owner

few things, otherwise 👍

@lenguyenthanh lenguyenthanh force-pushed the use-wrapper-for-picasso branch 4 times, most recently from 331e25e to e6a5e38 Compare March 31, 2016 15:10
@lenguyenthanh
Copy link
Contributor Author

Fixed all the comments and some inaccurate variable names.

@vanniktech
Copy link
Contributor

👍

artem-zinnatullin
artem-zinnatullin reviewed Mar 31, 2016
View reviewed changes
app/src/main/java/com/artemzin/qualitymatters/models/PicassoImageLoader.java
@NonNull
private final Picasso picasso;

@Inject
Copy link
Owner

Choose a reason for hiding this comment

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

not needed

@artem-zinnatullin
Copy link
Owner

Sorry, didn't see @Inject, last thing to fix

@lenguyenthanh
Copy link
Contributor Author

We need @Inject because we don't instantiate by constructor here.

@Provides @NonNull @Singleton
public QualityMattersImageLoader provideImageLoader(@NonNull PicassoImageLoader picassoImageLoader) {
    return picassoImageLoader;
}

Follow this comment: #139 (comment)

@artem-zinnatullin
Copy link
Owner

Oh damn, can you please just new it in the provideImageLoader?

@lenguyenthanh
Copy link
Contributor Author

Fix it now :)

@lenguyenthanh lenguyenthanh force-pushed the use-wrapper-for-picasso branch from e6a5e38 to c975bfc Compare March 31, 2016 16:08
@lenguyenthanh
Copy link
Contributor Author

Fixed it, just wait for CIs ;)

@artem-zinnatullin
Copy link
Owner

Great, thanks!

@artem-zinnatullin artem-zinnatullin merged commit a2d6fd0 into artem-zinnatullin:master Mar 31, 2016
@vanniktech
Copy link
Contributor

@artem-zinnatullin why don't you like constructor injection there?

@artem-zinnatullin
Copy link
Owner

It's same implicit thing as @NoGson for me, scope of the object becomes unclear and you can't actually do ALT + F7 to find where and who creates it.

Not a fan of all Square decisions/solutions but they said that they switched to modules-only too because of implicity and some other problems https://www.youtube.com/watch?v=3B7F7emCc64

@lenguyenthanh lenguyenthanh deleted the use-wrapper-for-picasso branch April 1, 2016 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@artem-zinnatullin artem-zinnatullin

Labels
enhancement
Projects
None yet
Milestone
v1.0
Development

Successfully merging this pull request may close these issues.
5 participants
@lenguyenthanh @artem-zinnatullin @codecov-io @vanniktech @Plastix