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

Replace Volley with Glide in the PostsListAdapter #7989

Merged
merged 6 commits into from
Jul 16, 2018
Merged

Replace Volley with Glide in the PostsListAdapter #7989

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0a68bc3
Replace Volley with Glide in the PostsListAdapter
malinajirka Jul 3, 2018
c627dc5
Revert switch statement refactoring
malinajirka Jul 10, 2018
d054ebf
Merge branch 'develop' into feature/glide-pages-posts-list
malinajirka Jul 11, 2018
b405674
Clean up companion object methods in ImageManager
malinajirka Jul 11, 2018
4403677
Replace ImageManager's companion methods with singleton
malinajirka Jul 11, 2018
2129142
Add JvmStatic annotation to singleton instance in ImageManager
malinajirka Jul 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion WordPress/src/main/java/org/wordpress/android/ui/posts/PostsListActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public void onCreate(Bundle savedInstanceState) {

setContentView(R.layout.post_list_activity);

Toolbar toolbar = (Toolbar) findViewById(R.id.toolbar);
Toolbar toolbar = findViewById(R.id.toolbar);
setSupportActionBar(toolbar);

if (savedInstanceState == null) {
Expand Down
79 changes: 48 additions & 31 deletions WordPress/src/main/java/org/wordpress/android/ui/posts/adapters/PostsListAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,13 @@
import android.view.animation.DecelerateInterpolator;
import android.widget.AdapterView;
import android.widget.ImageView;
import android.widget.ImageView.ScaleType;
import android.widget.ProgressBar;
import android.widget.TextView;

import org.apache.commons.lang3.StringUtils;
import org.apache.commons.text.StringEscapeUtils;
import org.wordpress.android.BuildConfig;
import org.wordpress.android.R;
import org.wordpress.android.WordPress;
import org.wordpress.android.fluxc.Dispatcher;
Expand All @@ -53,8 +55,9 @@
import org.wordpress.android.util.DisplayUtils;
import org.wordpress.android.util.ImageUtils;
import org.wordpress.android.util.SiteUtils;
import org.wordpress.android.util.image.ImageManager;
import org.wordpress.android.util.image.ImageType;
import org.wordpress.android.widgets.PostListButton;
import org.wordpress.android.widgets.WPNetworkImageView;

import java.text.SimpleDateFormat;
import java.util.ArrayList;
Expand Down Expand Up @@ -110,6 +113,7 @@ public enum LoadMode {
@Inject protected PostStore mPostStore;
@Inject protected MediaStore mMediaStore;
@Inject protected UploadStore mUploadStore;
@Inject protected ImageManager mImageManager;

public PostsListAdapter(Context context, @NonNull SiteModel site, boolean isPage) {
((WordPress) context.getApplicationContext()).component().inject(this);
Expand Down Expand Up @@ -160,7 +164,7 @@ private boolean isValidPostPosition(int position) {
}

@Override
public void onAttachedToRecyclerView(RecyclerView recyclerView) {
public void onAttachedToRecyclerView(@NonNull RecyclerView recyclerView) {
super.onAttachedToRecyclerView(recyclerView);
mRecyclerView = recyclerView;
}
Expand All @@ -183,7 +187,7 @@ public int getItemCount() {
}

@Override
public RecyclerView.ViewHolder onCreateViewHolder(ViewGroup parent, int viewType) {
public RecyclerView.ViewHolder onCreateViewHolder(@NonNull ViewGroup parent, int viewType) {
if (viewType == VIEW_TYPE_ENDLIST_INDICATOR) {
View view = mLayoutInflater.inflate(R.layout.endlist_indicator, parent, false);
view.getLayoutParams().height = mEndlistIndicatorHeight;
Expand All @@ -210,7 +214,7 @@ private boolean canPublishPost(PostModel post) {
}

@Override
public void onBindViewHolder(RecyclerView.ViewHolder holder, int position) {
public void onBindViewHolder(@NonNull RecyclerView.ViewHolder holder, int position) {
// nothing to do if this is the static endlist indicator
if (getItemViewType(position) == VIEW_TYPE_ENDLIST_INDICATOR) {
return;
Expand Down Expand Up @@ -333,24 +337,25 @@ public void onClick(View v) {
});
}

private void showFeaturedImage(int postId, WPNetworkImageView imgFeatured) {
private void showFeaturedImage(int postId, ImageView imgFeatured) {
String imageUrl = mFeaturedImageUrls.get(postId);
if (imageUrl == null) {
imgFeatured.setVisibility(View.GONE);
mImageManager.cancelRequestAndClearImageView(imgFeatured);
} else if (imageUrl.startsWith("http")) {
String photonUrl = ReaderUtils.getResizedImageUrl(
imageUrl, mPhotonWidth, mPhotonHeight, !SiteUtils.isPhotonCapable(mSite));
imgFeatured.setVisibility(View.VISIBLE);
imgFeatured.setImageUrl(photonUrl, WPNetworkImageView.ImageType.PHOTO);
mImageManager.load(imgFeatured, ImageType.PHOTO, photonUrl, ScaleType.CENTER_CROP);
} else {
Bitmap bmp = ImageUtils.getWPImageSpanThumbnailFromFilePath(
imgFeatured.getContext(), imageUrl, mPhotonWidth);
if (bmp != null) {
imgFeatured.setImageUrl(null, WPNetworkImageView.ImageType.NONE);
imgFeatured.setVisibility(View.VISIBLE);
imgFeatured.setImageBitmap(bmp);
mImageManager.load(imgFeatured, bmp);
} else {
imgFeatured.setVisibility(View.GONE);
mImageManager.cancelRequestAndClearImageView(imgFeatured);
}
}
}
Expand Down Expand Up @@ -431,6 +436,7 @@ private void updateStatusTextAndImage(TextView txtStatus, ImageView imgStatus, P
if ((PostStatus.fromPost(post) == PostStatus.PUBLISHED) && !post.isLocalDraft() && !post.isLocallyChanged()) {
txtStatus.setVisibility(View.GONE);
imgStatus.setVisibility(View.GONE);
mImageManager.cancelRequestAndClearImageView(imgStatus);
} else {
int statusTextResId = 0;
int statusIconResId = 0;
Expand Down Expand Up @@ -490,6 +496,16 @@ private void updateStatusTextAndImage(TextView txtStatus, ImageView imgStatus, P
statusIconResId = R.drawable.ic_gridicons_page;
statusColorResId = R.color.alert_red;
break;
case UNKNOWN:
// no-op
break;
case PUBLISHED:
// no-op
break;
default:
if (BuildConfig.DEBUG) {
throw new IllegalStateException("Missing switch case.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as this one

}
}
}

Expand All @@ -506,10 +522,11 @@ private void updateStatusTextAndImage(TextView txtStatus, ImageView imgStatus, P
if (drawable != null) {
drawable = DrawableCompat.wrap(drawable);
DrawableCompat.setTint(drawable, resources.getColor(statusColorResId));
imgStatus.setImageDrawable(drawable);
imgStatus.setVisibility(View.VISIBLE);
mImageManager.load(imgStatus, drawable);
} else {
imgStatus.setVisibility(View.GONE);
mImageManager.cancelRequestAndClearImageView(imgStatus);
}
}
}
Expand Down Expand Up @@ -731,7 +748,7 @@ private class PostViewHolder extends RecyclerView.ViewHolder {
private final PostListButton mBtnTrash;
private final PostListButton mBtnBack;

private final WPNetworkImageView mImgFeatured;
private final ImageView mImgFeatured;
private final ViewGroup mLayoutButtons;

private final View mDisabledOverlay;
Expand All @@ -741,27 +758,27 @@ private class PostViewHolder extends RecyclerView.ViewHolder {
PostViewHolder(View view) {
super(view);

mTxtTitle = (TextView) view.findViewById(R.id.text_title);
mTxtExcerpt = (TextView) view.findViewById(R.id.text_excerpt);
mTxtDate = (TextView) view.findViewById(R.id.text_date);
mTxtStatus = (TextView) view.findViewById(R.id.text_status);
mImgStatus = (ImageView) view.findViewById(R.id.image_status);
mTxtTitle = view.findViewById(R.id.text_title);
mTxtExcerpt = view.findViewById(R.id.text_excerpt);
mTxtDate = view.findViewById(R.id.text_date);
mTxtStatus = view.findViewById(R.id.text_status);
mImgStatus = view.findViewById(R.id.image_status);

mBtnEdit = (PostListButton) view.findViewById(R.id.btn_edit);
mBtnView = (PostListButton) view.findViewById(R.id.btn_view);
mBtnPublish = (PostListButton) view.findViewById(R.id.btn_publish);
mBtnMore = (PostListButton) view.findViewById(R.id.btn_more);
mBtnEdit = view.findViewById(R.id.btn_edit);
mBtnView = view.findViewById(R.id.btn_view);
mBtnPublish = view.findViewById(R.id.btn_publish);
mBtnMore = view.findViewById(R.id.btn_more);

mBtnStats = (PostListButton) view.findViewById(R.id.btn_stats);
mBtnTrash = (PostListButton) view.findViewById(R.id.btn_trash);
mBtnBack = (PostListButton) view.findViewById(R.id.btn_back);
mBtnStats = view.findViewById(R.id.btn_stats);
mBtnTrash = view.findViewById(R.id.btn_trash);
mBtnBack = view.findViewById(R.id.btn_back);

mImgFeatured = (WPNetworkImageView) view.findViewById(R.id.image_featured);
mLayoutButtons = (ViewGroup) view.findViewById(R.id.layout_buttons);
mImgFeatured = view.findViewById(R.id.image_featured);
mLayoutButtons = view.findViewById(R.id.layout_buttons);

mDisabledOverlay = view.findViewById(R.id.disabled_overlay);

mProgressBar = (ProgressBar) view.findViewById(R.id.post_upload_progress);
mProgressBar = view.findViewById(R.id.post_upload_progress);
}
}

Expand All @@ -778,15 +795,15 @@ private class PageViewHolder extends RecyclerView.ViewHolder {

PageViewHolder(View view) {
super(view);
mTxtTitle = (TextView) view.findViewById(R.id.text_title);
mTxtStatus = (TextView) view.findViewById(R.id.text_status);
mImgStatus = (ImageView) view.findViewById(R.id.image_status);
mTxtTitle = view.findViewById(R.id.text_title);
mTxtStatus = view.findViewById(R.id.text_status);
mImgStatus = view.findViewById(R.id.image_status);
mBtnMore = view.findViewById(R.id.btn_more);
mDividerTop = view.findViewById(R.id.divider_top);
mDateHeader = (ViewGroup) view.findViewById(R.id.header_date);
mTxtDate = (TextView) mDateHeader.findViewById(R.id.text_date);
mDateHeader = view.findViewById(R.id.header_date);
mTxtDate = mDateHeader.findViewById(R.id.text_date);
mDisabledOverlay = view.findViewById(R.id.disabled_overlay);
mProgressBar = (ProgressBar) view.findViewById(R.id.post_upload_progress);
mProgressBar = view.findViewById(R.id.post_upload_progress);
}
}

Expand Down
2 changes: 1 addition & 1 deletion WordPress/src/main/java/org/wordpress/android/ui/reader/adapters/ReaderPostAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -906,7 +906,7 @@ private void updateBookmarkView(final ReaderPostViewHolder holder, final ReaderP
} else {
bookmarkButton.setVisibility(View.GONE);
}
bookmarkButton.setImageResource(post.isBookmarked
mImageManager.load(bookmarkButton, post.isBookmarked
? R.drawable.ic_bookmark_blue_medium_18dp
: R.drawable.ic_bookmark_grey_min_18dp);
if (post.isBookmarked) {
Expand Down
17 changes: 17 additions & 0 deletions WordPress/src/main/java/org/wordpress/android/util/image/ImageManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@ class ImageManager @Inject constructor(val placeholderManager: ImagePlaceholderM
.into(imageView)
}

@JvmOverloads
fun load(imageView: ImageView, resourceId: Int, scaleType: ImageView.ScaleType = CENTER) {
GlideApp.with(imageView.context)
.load(resourceId)
.applyScaleType(scaleType)
.into(imageView)
}

fun loadIntoCircle(imageView: ImageView, imageType: ImageType, imgUrl: String) {
GlideApp.with(imageView.context)
.load(imgUrl)
Expand Down Expand Up @@ -121,6 +129,15 @@ class ImageManager @Inject constructor(val placeholderManager: ImagePlaceholderM
ImageManager(ImagePlaceholderManager()).load(imageView, drawable, scaleType)
}

@JvmStatic
@Deprecated("Use injected ImageManager",
ReplaceWith("imageManager.load(imageView, resourceId, scaleType)",
"org.wordpress.android.util.image.ImageManager"))
@JvmOverloads
fun loadImage(imageView: ImageView, resourceId: Int, scaleType: ImageView.ScaleType = CENTER) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method doesn't seem to be used, am I missing something? Adding a deprecated, unused method feels really weird to me. I am not familiar with the history of the project, could you let me know why we needed to add these deprecated methods in the first place?

Actually, I just checked and none of them seems to be used. We have this comment Object for backward compatibility with code which doesn't support DI, but why wouldn't the DI be supported?

I am really confused. I have a few more questions, but I'll wait until I understand a bit more about what's going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are basically two related reasons for introducing deprecated methods

  1. It's easier to do incremental refactoring than trying to refactor everything at once.
  • suppose you have an activity/fragment, which doesn't use dagger yet. It's easier to migrate from Volley to Glide in first iteration and don't care about dagger and then add support for dagger in the second iteration. This is just a dump example, as adding dagger to an activity/fragment is one line of code, but I hope you get the point.
  1. Some classes don't use dagger and adding dagger isn't a good idea. Great example is ReaderThumbnailStrip. It's a View class which contains business logic. It needs to be refactored, but doing the refactoring is out of the scope of this PR (even of this project).

Since we want everyone to be able to use the ImageManager without the need for huge refactoring, I believe it should provide static methods. On the other hand we want to make sure no-one uses the static methods when they can use Dagger, hence the methods are deprecated.

Let me know if you have any further questions or if you believe I should take another approach:).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the problem is that these deprecated methods are not currently in use (at least in this branch). Until they are actually needed, I'd strongly advice against adding them. I can't find our documentation for it, but I know that we don't want any unused code in our repositories. If they are actually in use and I am just missing it, please let me know.

Specifically for this case, adding that code right now compared to adding that code when it's needed, it makes it not a part of the correct PR review. I can't comment on that code at all right now, because I don't know how it's going to be used. However, if that deprecated method was added in a PR that uses it and there is a better way to do it without the need of the deprecated method, I can say that let's not add it and do this alternative way instead. Does that make sense?

Side note: For the deprecated methods, creating a new instance ImagePlaceholderManager() for every time it's called is kind of a big waste. If the caller is a listview and this is called many times, we'd consume a lot of memory for no reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have always been a fan of no dead/unused code rule and I think every company should have such rule. On the other hand every rule has valid exceptions. I believe this is a case where it makes sense to make an exception.

  1. The static methods don't do anything else than call the original instance methods. It's kind of a simple adapter in a way. All the instance methods are used so you can post logic related comments there.

  2. Implementing these static methods during development of the class has some advantages.

  • The ImageManager class is more of 'library' class which provides an interface. Other developers won't need to touch the ImageManager class and they can just use it.
  • If we don't have these static methods, other developers (or me in the future) might not realize they should annotate them with @Deprecated when they create them.
  1. I basically agree with your point that it's hard to review an unused method and you can't propose an alternative way, because you don't now how it's going to be used. However, this PR introduces just another static methods, so we have all the instance methods available in the static context. I'd completely agree with you, if I introduced first static method and it wasn't used.
    Here is an example of a usage.

Probably the biggest advantage of implementing the methods before they are actually used comes when more people are working on the refactoring. It prevents everyone from creating their own version of such method. Since I'm the only one working on this, I'm okay with removing all the unused methods. However, I'd like to hear another opinion on this topic, so when we work on refactoring in a group I know whether it's a valid approach or not.

@hypest Your comments are always wise. Could you please give us your opinion on this? Thanks!

Copy link
Contributor Author

@malinajirka malinajirka Jul 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Side note: For the deprecated methods, creating a new instance ImagePlaceholderManager() for every time it's called is kind of a big waste. If the caller is a listview and this is called many times, we'd consume a lot of memory for no reason.

Tbh I can't think of any other way -> we could let the client create an instance of the ImageManager and send it to the method, but it wouldn't make sense to have static methods in the first place (the client could directly call the instance methods). Disadvantage of calling instance methods on a manually created instance is, that those methods are not Deprecated. I guess I could create a lazy loaded singleton, but it feels like overkill. The ImageManager is basically just an Object with few extra methods. It's creation and memory footprint should be neglectable. Moreover current garbage collection techniques are optimized for collecting many short-lived objects so the memory footprint might be actually lower than creating one longer living object. There are several great articles about this topic (for instance here and here). Nevertheless, I'll be happy to rewrite it. Do you have any specific suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please give us your opinion on this? Thanks!

Thanks for pinging @malinajirka !

Here is an example of a usage.

So, it seems there is a usage for one of the static methods. I guess it would help to merge develop in this PR so that becomes easier to follow.

The ImageManager class is more of 'library' class which provides an interface.

The companion object is effectively a utility class and since it's not part of an actual library, the incentive to cater for the needs of a random user of the lib is low I think.

In this case, and since this was brought up during the review, I'd be leaning towards cleaning up the companion object, only leaving in the static method that is actually used.

I'd expect any future static method additions to the companion object to "copy" the one method already defined and mark it as deprecated on inception. That can be part of the PR review of that future addition.

I'd also advocate that even if there are good (potential) uses the static methods, we'd still like to "push" for using the DI'ed singleton instead. With that in mind, offering a complete set of static methods like currently in the PR would enable people to use them, beating the purpose of "pushing" to DI :)

All in all, I think the utility of the static methods is minimal at the moment and we can easily handle adding more in the future if needed so, I'd recommend not offering unused methods in this PR.

Hope I helped!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you both;), I'm going to clean it up.

Copy link
Contributor

Choose a reason for hiding this comment

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

@malinajirka Is there a reason you don't just add a method like sharedInstance() that simply returns a singleton for the deprecated case? You can deprecate that method and you won't need to implement any other static methods since the regular methods could be used on that singleton. That also fixes both the unused methods and unnecessary memory consumption issues.

ImageManager(ImagePlaceholderManager()).load(imageView, resourceId, scaleType)
}

@JvmStatic
@Deprecated("Use injected ImageManager",
ReplaceWith("imageManager.loadIntoCircle(imageView, imgType, imgUrl)",
Expand Down
3 changes: 1 addition & 2 deletions WordPress/src/main/res/layout/post_cardview.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,10 @@
android:background="?android:selectableItemBackground"
android:orientation="vertical">

<org.wordpress.android.widgets.WPNetworkImageView
<ImageView
android:id="@+id/image_featured"
android:layout_width="match_parent"
android:layout_height="@dimen/postlist_featured_image_height"
android:scaleType="centerCrop"
android:visibility="gone"
android:contentDescription="@string/post_cardview_featured_desc"
tools:visibility="visible" />
Expand Down