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

Glide - comments #8038

Merged
merged 18 commits into from
Jul 23, 2018
Merged

Glide - comments #8038

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
4d959d5
Replace Glide with Volley - Comments and ReaderComments
malinajirka Jul 12, 2018
c08efde
Replace Volley with Glide in comments
malinajirka Jul 16, 2018
2077af6
Remove hungarian notation from GlideImageGetter
malinajirka Jul 16, 2018
eb309fc
Add DI into GlideImageGetter and minor refactoring
malinajirka Jul 16, 2018
099f74b
Fix checkstyle
malinajirka Jul 16, 2018
f814cc1
Rename and Add comments to WPCustomImageGetter and WPRemoteResourceTa…
malinajirka Jul 17, 2018
6c80b7b
Merge branch 'develop' into feature/glide-comments
malinajirka Jul 17, 2018
fc53936
Add comment to WPRemoteResourceViewTarget
malinajirka Jul 17, 2018
d70e029
Remove inner modifier from RemoteDrawableWrapper
malinajirka Jul 17, 2018
98678ee
Fix comment
malinajirka Jul 19, 2018
9d289a2
Simplify calculation
malinajirka Jul 19, 2018
d082585
Refactor minor issues
malinajirka Jul 19, 2018
6df0d32
Extract Drawable.Callback from WPCustomImageGetter to separate class
malinajirka Jul 19, 2018
26ddac0
Extract getScaledBounds from RemoteResourceViewTarget to utils
malinajirka Jul 19, 2018
e94aefd
Fix checkstyle
malinajirka Jul 19, 2018
fea9639
Fix checkstyle
malinajirka Jul 19, 2018
ba594f2
Add comment to RemoteDrawable wrapper
malinajirka Jul 23, 2018
be33714
Merge 'develop' into 'feature/glide-comments'
malinajirka Jul 23, 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
3 changes: 3 additions & 0 deletions WordPress/src/main/java/org/wordpress/android/modules/AppComponent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@
import org.wordpress.android.ui.uploads.UploadService;
import org.wordpress.android.util.HtmlToSpannedConverter;
import org.wordpress.android.util.WPWebViewClient;
import org.wordpress.android.util.image.getters.WPCustomImageGetter;

import javax.inject.Singleton;

Expand Down Expand Up @@ -388,6 +389,8 @@ public interface AppComponent extends AndroidInjector<WordPress> {

void inject(WordPressGlideModule object);

void inject(WPCustomImageGetter wpCustomImageGetter);

// Allows us to inject the application without having to instantiate any modules, and provides the Application
// in the app graph
@Component.Builder
Expand Down
23 changes: 13 additions & 10 deletions WordPress/src/main/java/org/wordpress/android/ui/comments/CommentAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@
import org.wordpress.android.util.GravatarUtils;
import org.wordpress.android.util.StringUtils;
import org.wordpress.android.util.WPHtml;
import org.wordpress.android.widgets.WPNetworkImageView;
import org.wordpress.android.util.image.ImageManager;
import org.wordpress.android.util.image.ImageType;

import java.util.Collections;
import java.util.Comparator;
Expand Down Expand Up @@ -81,25 +82,26 @@ interface OnCommentPressedListener {
private SiteModel mSite;

@Inject CommentStore mCommentStore;
@Inject ImageManager mImageManager;

class CommentHolder extends RecyclerView.ViewHolder implements View.OnClickListener, View.OnLongClickListener {
private final TextView mTxtTitle;
private final TextView mTxtComment;
private final TextView mTxtStatus;
private final TextView mTxtDate;
private final WPNetworkImageView mImgAvatar;
private final ImageView mImgAvatar;
private final ImageView mImgCheckmark;
private final ViewGroup mContainerView;

CommentHolder(View view) {
super(view);
mTxtTitle = (TextView) view.findViewById(R.id.title);
mTxtComment = (TextView) view.findViewById(R.id.comment);
mTxtStatus = (TextView) view.findViewById(R.id.status);
mTxtDate = (TextView) view.findViewById(R.id.text_date);
mImgCheckmark = (ImageView) view.findViewById(R.id.image_checkmark);
mImgAvatar = (WPNetworkImageView) view.findViewById(R.id.avatar);
mContainerView = (ViewGroup) view.findViewById(R.id.layout_container);
mTxtTitle = view.findViewById(R.id.title);
mTxtComment = view.findViewById(R.id.comment);
mTxtStatus = view.findViewById(R.id.status);
mTxtDate = view.findViewById(R.id.text_date);
mImgCheckmark = view.findViewById(R.id.image_checkmark);
mImgAvatar = view.findViewById(R.id.avatar);
mContainerView = view.findViewById(R.id.layout_container);

itemView.setOnClickListener(this);
itemView.setOnLongClickListener(this);
Expand Down Expand Up @@ -241,10 +243,11 @@ public void onBindViewHolder(RecyclerView.ViewHolder viewHolder, int position) {
int checkmarkVisibility;
if (mEnableSelection && isItemSelected(position)) {
checkmarkVisibility = View.VISIBLE;
mImageManager.cancelRequestAndClearImageView(holder.mImgAvatar);
holder.mContainerView.setBackgroundColor(mSelectedColor);
} else {
checkmarkVisibility = View.GONE;
holder.mImgAvatar.setImageUrl(getAvatarForDisplay(comment, mAvatarSz), WPNetworkImageView.ImageType.AVATAR);
mImageManager.loadIntoCircle(holder.mImgAvatar, ImageType.AVATAR, getAvatarForDisplay(comment, mAvatarSz));
holder.mContainerView.setBackgroundColor(mUnselectedColor);
}

Expand Down
18 changes: 9 additions & 9 deletions WordPress/src/main/java/org/wordpress/android/ui/comments/CommentDetailFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,9 @@
import org.wordpress.android.util.SiteUtils;
import org.wordpress.android.util.ToastUtils;
import org.wordpress.android.util.WPLinkMovementMethod;
import org.wordpress.android.util.image.ImageManager;
import org.wordpress.android.util.image.ImageType;
import org.wordpress.android.widgets.SuggestionAutoCompleteText;
import org.wordpress.android.widgets.WPNetworkImageView;

import java.util.EnumSet;
import java.util.List;
Expand Down Expand Up @@ -140,6 +141,7 @@ public class CommentDetailFragment extends Fragment implements NotificationFragm
@Inject CommentStore mCommentStore;
@Inject SiteStore mSiteStore;
@Inject FluxCImageLoader mImageLoader;
@Inject ImageManager mImageManager;

private boolean mIsSubmittingReply = false;
private NotificationsDetailListFragment mNotificationsDetailListFragment;
Expand Down Expand Up @@ -641,7 +643,7 @@ private void showComment() {
mCommentContentLayout.addView(mLayoutButtons);
}

final WPNetworkImageView imgAvatar = getView().findViewById(R.id.image_avatar);
final ImageView imgAvatar = getView().findViewById(R.id.image_avatar);
final TextView txtName = getView().findViewById(R.id.text_name);
final TextView txtDate = getView().findViewById(R.id.text_date);

Expand All @@ -650,18 +652,16 @@ private void showComment() {
WordPress.getContext()));

int maxImageSz = getResources().getDimensionPixelSize(R.dimen.reader_comment_max_image_size);
CommentUtils.displayHtmlComment(mTxtContent, mComment.getContent(), maxImageSz, mImageLoader);
CommentUtils.displayHtmlComment(mTxtContent, mComment.getContent(), maxImageSz);

int avatarSz = getResources().getDimensionPixelSize(R.dimen.avatar_sz_large);
String avatarUrl = "";
if (mComment.getAuthorProfileImageUrl() != null) {
imgAvatar.setImageUrl(GravatarUtils.fixGravatarUrl(mComment.getAuthorProfileImageUrl(), avatarSz),
WPNetworkImageView.ImageType.AVATAR);
avatarUrl = GravatarUtils.fixGravatarUrl(mComment.getAuthorProfileImageUrl(), avatarSz);
} else if (mComment.getAuthorEmail() != null) {
String avatarUrl = GravatarUtils.gravatarFromEmail(mComment.getAuthorEmail(), avatarSz);
imgAvatar.setImageUrl(avatarUrl, WPNetworkImageView.ImageType.AVATAR);
} else {
imgAvatar.setImageUrl(null, WPNetworkImageView.ImageType.AVATAR);
avatarUrl = GravatarUtils.gravatarFromEmail(mComment.getAuthorEmail(), avatarSz);
}
mImageManager.loadIntoCircle(imgAvatar, ImageType.AVATAR, avatarUrl);

updateStatusViews();

Expand Down
16 changes: 3 additions & 13 deletions WordPress/src/main/java/org/wordpress/android/ui/comments/CommentUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,22 @@

import android.graphics.Canvas;
import android.graphics.Paint;
import android.graphics.drawable.Drawable;
import android.support.v4.content.ContextCompat;
import android.text.Layout;
import android.text.SpannableString;
import android.text.Spanned;
import android.text.style.LeadingMarginSpan;
import android.text.util.Linkify;
import android.widget.TextView;

import com.android.volley.toolbox.ImageLoader;

import org.wordpress.android.R;
import org.wordpress.android.util.EmoticonsUtils;
import org.wordpress.android.util.HtmlUtils;
import org.wordpress.android.util.helpers.WPImageGetter;
import org.wordpress.android.util.image.getters.WPCustomImageGetter;

public class CommentUtils {
/*
* displays comment text as html, including retrieving images
*/
public static void displayHtmlComment(TextView textView, String content, int maxImageSize,
ImageLoader imageLoader) {
public static void displayHtmlComment(TextView textView, String content, int maxImageSize) {
if (textView == null) {
return;
}
Expand All @@ -50,11 +44,7 @@ public static void displayHtmlComment(TextView textView, String content, int max
// now convert to HTML with an image getter that enforces a max image size
final Spanned html;
if (maxImageSize > 0 && content.contains("<img")) {
Drawable loading = ContextCompat.getDrawable(textView.getContext(),
R.drawable.legacy_dashicon_format_image_big_grey);
Drawable failed = ContextCompat.getDrawable(textView.getContext(),
R.drawable.ic_notice_grey_500_48dp);
html = HtmlUtils.fromHtml(content, new WPImageGetter(textView, maxImageSize, imageLoader, loading, failed));
html = HtmlUtils.fromHtml(content, new WPCustomImageGetter(textView, maxImageSize));
} else {
html = HtmlUtils.fromHtml(content);
}
Expand Down
35 changes: 16 additions & 19 deletions WordPress/src/main/java/org/wordpress/android/ui/reader/adapters/ReaderCommentAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import android.view.LayoutInflater;
import android.view.View;
import android.view.ViewGroup;
import android.widget.ImageView;
import android.widget.ProgressBar;
import android.widget.RelativeLayout;
import android.widget.TextView;
Expand All @@ -19,7 +20,6 @@
import org.wordpress.android.datasets.ReaderPostTable;
import org.wordpress.android.fluxc.store.AccountStore;
import org.wordpress.android.fluxc.store.SiteStore;
import org.wordpress.android.fluxc.tools.FluxCImageLoader;
import org.wordpress.android.models.ReaderComment;
import org.wordpress.android.models.ReaderCommentList;
import org.wordpress.android.models.ReaderPost;
Expand All @@ -42,7 +42,8 @@
import org.wordpress.android.util.GravatarUtils;
import org.wordpress.android.util.NetworkUtils;
import org.wordpress.android.util.ToastUtils;
import org.wordpress.android.widgets.WPNetworkImageView;
import org.wordpress.android.util.image.ImageManager;
import org.wordpress.android.util.image.ImageType;

import java.util.Date;

Expand Down Expand Up @@ -76,7 +77,7 @@ public class ReaderCommentAdapter extends RecyclerView.Adapter<RecyclerView.View

@Inject AccountStore mAccountStore;
@Inject SiteStore mSiteStore;
@Inject FluxCImageLoader mImageLoader;
@Inject ImageManager mImageManager;

public interface RequestReplyListener {
void onRequestReply(long commentId);
Expand All @@ -93,7 +94,7 @@ class CommentHolder extends RecyclerView.ViewHolder {
private final TextView mTxtText;
private final TextView mTxtDate;

private final WPNetworkImageView mImgAvatar;
private final ImageView mImgAvatar;
private final View mSpacerIndent;
private final View mAuthorContainer;
private final ProgressBar mProgress;
Expand All @@ -104,20 +105,20 @@ class CommentHolder extends RecyclerView.ViewHolder {
CommentHolder(View view) {
super(view);

mContainer = (ViewGroup) view.findViewById(R.id.layout_container);
mContainer = view.findViewById(R.id.layout_container);

mTxtAuthor = (TextView) view.findViewById(R.id.text_comment_author);
mTxtText = (TextView) view.findViewById(R.id.text_comment_text);
mTxtDate = (TextView) view.findViewById(R.id.text_comment_date);
mTxtAuthor = view.findViewById(R.id.text_comment_author);
mTxtText = view.findViewById(R.id.text_comment_text);
mTxtDate = view.findViewById(R.id.text_comment_date);

mImgAvatar = (WPNetworkImageView) view.findViewById(R.id.image_comment_avatar);
mImgAvatar = view.findViewById(R.id.image_comment_avatar);
mSpacerIndent = view.findViewById(R.id.spacer_comment_indent);
mProgress = (ProgressBar) view.findViewById(R.id.progress_comment);
mProgress = view.findViewById(R.id.progress_comment);

mAuthorContainer = view.findViewById(R.id.layout_author);

mReplyView = (ViewGroup) view.findViewById(R.id.reply_container);
mCountLikes = (ReaderIconCountView) view.findViewById(R.id.count_likes);
mReplyView = view.findViewById(R.id.reply_container);
mCountLikes = view.findViewById(R.id.count_likes);

mTxtText.setLinksClickable(true);
mTxtText.setMovementMethod(ReaderLinkMovementMethod.getInstance(mIsPrivatePost));
Expand Down Expand Up @@ -239,12 +240,8 @@ public void onClick(View view) {
}
commentHolder.mTxtDate.setText(DateTimeUtils.javaDateToTimeSpan(dtPublished, WordPress.getContext()));

if (comment.hasAuthorAvatar()) {
String avatarUrl = GravatarUtils.fixGravatarUrl(comment.getAuthorAvatar(), mAvatarSz);
commentHolder.mImgAvatar.setImageUrl(avatarUrl, WPNetworkImageView.ImageType.AVATAR);
} else {
commentHolder.mImgAvatar.showDefaultGravatarImageAndNullifyUrl();
}
String avatarUrl = GravatarUtils.fixGravatarUrl(comment.getAuthorAvatar(), mAvatarSz);
mImageManager.loadIntoCircle(commentHolder.mImgAvatar, ImageType.AVATAR, avatarUrl);

// tapping avatar or author name opens blog preview
if (comment.hasAuthorBlogId()) {
Expand Down Expand Up @@ -282,7 +279,7 @@ public void onClick(View view) {
}

int maxImageWidth = mContentWidth - indentWidth;
CommentUtils.displayHtmlComment(commentHolder.mTxtText, comment.getText(), maxImageWidth, mImageLoader);
CommentUtils.displayHtmlComment(commentHolder.mTxtText, comment.getText(), maxImageWidth);

// different background for highlighted comment, with optional progress bar
if (mHighlightCommentId != 0 && mHighlightCommentId == comment.commentId) {
Expand Down
11 changes: 11 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 @@ -5,6 +5,9 @@ import android.graphics.drawable.Drawable
import android.widget.ImageView
import android.widget.ImageView.ScaleType
import android.widget.ImageView.ScaleType.CENTER
import android.widget.TextView
import com.bumptech.glide.request.target.ViewTarget
import org.wordpress.android.WordPress
import org.wordpress.android.modules.GlideApp
import org.wordpress.android.modules.GlideRequest
import org.wordpress.android.util.AppLog
Expand Down Expand Up @@ -55,6 +58,14 @@ class ImageManager @Inject constructor(val placeholderManager: ImagePlaceholderM
.into(imageView)
}

fun load(viewTarget: ViewTarget<TextView, Drawable>, imageType: ImageType, imgUrl: String) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, is it at all possible to avoid this many different load methods in ImageManager? Can we use a builder pattern or named parameters with default values to avoid it? This, of course, doesn't need to be part of this PR, but just a general concern.

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've consider it before starting this project, however I felt it'd be nice to have limited options how to load images, so we can easily be consistent across the app. I also didn't want to have any Glide classes in the interface, but it could be solved by wrapping GlideRequest, I guess. However, I don't have a strong opinion on this. Wdyt?

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 some kind of refactor would be great for this. I have to look into it more to figure out what the best way might be, but I think we simply have too many load functions right now.

As I said, this is not related to this PR, but something to consider before we finalize the Glide project.

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'd appreciate any help with it. We might allocate 30 minutes and resolve it during a call. Wdys?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure thing. Do you want to open an issue for this and assign it to both of us, so we can check it out once all Glide PRs are merged in?

GlideApp.with(WordPress.getContext())
.load(imgUrl)
.addFallback(imageType)
.addPlaceholder(imageType)
.into(viewTarget)
}

fun loadIntoCircle(imageView: ImageView, imageType: ImageType, imgUrl: String) {
GlideApp.with(imageView.context)
.load(imgUrl)
Expand Down
2 changes: 2 additions & 0 deletions WordPress/src/main/java/org/wordpress/android/util/image/ImagePlaceholderManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ class ImagePlaceholderManager @Inject constructor() {
ImageType.AVATAR -> R.drawable.ic_placeholder_gravatar_grey_lighten_20_100dp
ImageType.BLAVATAR -> R.drawable.ic_placeholder_blavatar_grey_lighten_20_40dp
ImageType.THEME -> R.color.grey_lighten_30
ImageType.UNKNOWN_DIMENSIONS -> R.drawable.ic_notice_grey_500_48dp
}
}

Expand All @@ -23,6 +24,7 @@ class ImagePlaceholderManager @Inject constructor() {
ImageType.AVATAR -> R.drawable.shape_oval_grey_light
ImageType.BLAVATAR -> R.color.grey_light
ImageType.THEME -> R.drawable.theme_loading
ImageType.UNKNOWN_DIMENSIONS -> R.drawable.legacy_dashicon_format_image_big_grey
}
}
}
3 changes: 2 additions & 1 deletion WordPress/src/main/java/org/wordpress/android/util/image/ImageType.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,6 @@ enum class ImageType {
VIDEO,
AVATAR,
BLAVATAR,
THEME
THEME,
UNKNOWN_DIMENSIONS
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 we should just say UNKNOWN here. The enum is name as ImageType and unknown dimensions as a type doesn't make sense to me. We can do UNKNOWN_TYPE but I think that's clear from the enum name. What do you think?

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 renamed it from UNKNOWN -> UNKNOWN_DIMENSION and I wasn't sure whether it was a good call or not. You made this decision easier -> I've renamed it back to UNKNOWN.

}
Loading