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 EditPostSettings fragment #7985

Merged
merged 12 commits into from
Jul 10, 2018
Merged

Replace Volley with Glide in the EditPostSettings fragment #7985

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2304670
Replace Volley with Glide in EditPostSettingsFragment
malinajirka Jun 22, 2018
d24c947
Add fallthrough comment to switch
malinajirka Jun 29, 2018
bdb5d76
Merge branch 'develop' into feature/glide-post-settings2
malinajirka Jul 3, 2018
57d31c0
Merge branch 'develop' into feature/glide-post-settings2
malinajirka Jul 3, 2018
acc9d22
Add placeholder and error images to post feature image preview
malinajirka Jul 3, 2018
a2ef20b
WPMediaUtils cleanup
malinajirka Jul 3, 2018
8eaf4f5
EditPostSettings switch cleanup
malinajirka Jul 3, 2018
94b4bcf
Simplify if statement in WPMediaUtils
malinajirka Jul 10, 2018
101e325
Refactor switch in EditPostSettingFragment
malinajirka Jul 10, 2018
2bacc14
Fix string duplication
malinajirka Jul 10, 2018
aa63ef5
Fix string duplication
malinajirka Jul 10, 2018
9a6e643
Revert switch statement refactoring
malinajirka Jul 10, 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
83 changes: 46 additions & 37 deletions WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostSettingsFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import android.support.annotation.Nullable;
import android.support.v4.app.Fragment;
import android.support.v4.app.FragmentManager;
import android.support.v7.app.AppCompatActivity;
import android.support.v7.view.menu.MenuPopupHelper;
import android.support.v7.widget.CardView;
import android.support.v7.widget.PopupMenu;
Expand All @@ -21,6 +20,8 @@
import android.view.View;
import android.view.ViewGroup;
import android.widget.Button;
import android.widget.ImageView;
import android.widget.ImageView.ScaleType;
import android.widget.LinearLayout;
import android.widget.TextView;

Expand All @@ -36,9 +37,11 @@
import org.apache.commons.text.StringEscapeUtils;
import org.greenrobot.eventbus.Subscribe;
import org.greenrobot.eventbus.ThreadMode;
import org.wordpress.android.BuildConfig;
import org.wordpress.android.R;
import org.wordpress.android.WordPress;
import org.wordpress.android.fluxc.Dispatcher;
import org.wordpress.android.fluxc.action.TaxonomyAction;
import org.wordpress.android.fluxc.generated.SiteActionBuilder;
import org.wordpress.android.fluxc.generated.TaxonomyActionBuilder;
import org.wordpress.android.fluxc.model.MediaModel;
Expand Down Expand Up @@ -69,8 +72,8 @@
import org.wordpress.android.util.PhotonUtils;
import org.wordpress.android.util.SiteUtils;
import org.wordpress.android.util.ToastUtils;
import org.wordpress.android.util.WPMediaUtils;
import org.wordpress.android.widgets.WPNetworkImageView;
import org.wordpress.android.util.image.ImageManager;
import org.wordpress.android.util.image.ImageType;

import java.lang.reflect.Field;
import java.lang.reflect.Method;
Expand Down Expand Up @@ -112,7 +115,7 @@ public class EditPostSettingsFragment extends Fragment {
private TextView mPostFormatTextView;
private TextView mPasswordTextView;
private TextView mPublishDateTextView;
private WPNetworkImageView mFeaturedImageView;
private ImageView mFeaturedImageView;
private Button mFeaturedImageButton;

private PostLocation mPostLocation;
Expand All @@ -124,6 +127,7 @@ public class EditPostSettingsFragment extends Fragment {
@Inject MediaStore mMediaStore;
@Inject TaxonomyStore mTaxonomyStore;
@Inject Dispatcher mDispatcher;
@Inject ImageManager mImageManager;


interface EditPostActivityHook {
Expand Down Expand Up @@ -210,26 +214,27 @@ public void onDestroy() {
}

@Override
public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
public View onCreateView(@NonNull LayoutInflater inflater, @Nullable ViewGroup container,
@Nullable Bundle savedInstanceState) {
ViewGroup rootView = (ViewGroup) inflater.inflate(R.layout.edit_post_settings_fragment, container, false);

if (rootView == null) {
return null;
}

mExcerptTextView = (TextView) rootView.findViewById(R.id.post_excerpt);
mSlugTextView = (TextView) rootView.findViewById(R.id.post_slug);
mLocationTextView = (TextView) rootView.findViewById(R.id.post_location);
mCategoriesTextView = (TextView) rootView.findViewById(R.id.post_categories);
mTagsTextView = (TextView) rootView.findViewById(R.id.post_tags);
mStatusTextView = (TextView) rootView.findViewById(R.id.post_status);
mPostFormatTextView = (TextView) rootView.findViewById(R.id.post_format);
mPasswordTextView = (TextView) rootView.findViewById(R.id.post_password);
mPublishDateTextView = (TextView) rootView.findViewById(R.id.publish_date);
mExcerptTextView = rootView.findViewById(R.id.post_excerpt);
mSlugTextView = rootView.findViewById(R.id.post_slug);
mLocationTextView = rootView.findViewById(R.id.post_location);
mCategoriesTextView = rootView.findViewById(R.id.post_categories);
mTagsTextView = rootView.findViewById(R.id.post_tags);
mStatusTextView = rootView.findViewById(R.id.post_status);
mPostFormatTextView = rootView.findViewById(R.id.post_format);
mPasswordTextView = rootView.findViewById(R.id.post_password);
mPublishDateTextView = rootView.findViewById(R.id.publish_date);

mFeaturedImageView = (WPNetworkImageView) rootView.findViewById(R.id.post_featured_image);
mFeaturedImageButton = (Button) rootView.findViewById(R.id.post_add_featured_image_button);
CardView featuredImageCardView = (CardView) rootView.findViewById(R.id.post_featured_image_card_view);
mFeaturedImageView = rootView.findViewById(R.id.post_featured_image);
mFeaturedImageButton = rootView.findViewById(R.id.post_add_featured_image_button);
CardView featuredImageCardView = rootView.findViewById(R.id.post_featured_image_card_view);

if (AppPrefs.isVisualEditorEnabled() || AppPrefs.isAztecEditorEnabled()) {
registerForContextMenu(mFeaturedImageView);
Expand All @@ -249,71 +254,71 @@ public void onClick(View view) {
featuredImageCardView.setVisibility(View.GONE);
}

mExcerptContainer = (LinearLayout) rootView.findViewById(R.id.post_excerpt_container);
mExcerptContainer = rootView.findViewById(R.id.post_excerpt_container);
mExcerptContainer.setOnClickListener(new View.OnClickListener() {
@Override
public void onClick(View view) {
showPostExcerptDialog();
}
});

final LinearLayout slugContainer = (LinearLayout) rootView.findViewById(R.id.post_slug_container);
final LinearLayout slugContainer = rootView.findViewById(R.id.post_slug_container);
slugContainer.setOnClickListener(new View.OnClickListener() {
@Override
public void onClick(View view) {
showSlugDialog();
}
});

final LinearLayout locationContainer = (LinearLayout) rootView.findViewById(R.id.post_location_container);
final LinearLayout locationContainer = rootView.findViewById(R.id.post_location_container);
locationContainer.setOnClickListener(new View.OnClickListener() {
@Override
public void onClick(View view) {
showLocationPickerOrPopupMenu(view);
}
});

mCategoriesContainer = (LinearLayout) rootView.findViewById(R.id.post_categories_container);
mCategoriesContainer = rootView.findViewById(R.id.post_categories_container);
mCategoriesContainer.setOnClickListener(new View.OnClickListener() {
@Override
public void onClick(View view) {
showCategoriesActivity();
}
});

mTagsContainer = (LinearLayout) rootView.findViewById(R.id.post_tags_container);
mTagsContainer = rootView.findViewById(R.id.post_tags_container);
mTagsContainer.setOnClickListener(new View.OnClickListener() {
@Override
public void onClick(View view) {
showTagsActivity();
}
});

final LinearLayout statusContainer = (LinearLayout) rootView.findViewById(R.id.post_status_container);
final LinearLayout statusContainer = rootView.findViewById(R.id.post_status_container);
statusContainer.setOnClickListener(new View.OnClickListener() {
@Override
public void onClick(View view) {
showStatusDialog();
}
});

mFormatContainer = (LinearLayout) rootView.findViewById(R.id.post_format_container);
mFormatContainer = rootView.findViewById(R.id.post_format_container);
mFormatContainer.setOnClickListener(new View.OnClickListener() {
@Override
public void onClick(View view) {
showPostFormatDialog();
}
});

final LinearLayout passwordContainer = (LinearLayout) rootView.findViewById(R.id.post_password_container);
final LinearLayout passwordContainer = rootView.findViewById(R.id.post_password_container);
passwordContainer.setOnClickListener(new View.OnClickListener() {
@Override
public void onClick(View view) {
showPostPasswordDialog();
}
});

final LinearLayout publishDateContainer = (LinearLayout) rootView.findViewById(R.id.publish_date_container);
final LinearLayout publishDateContainer = rootView.findViewById(R.id.publish_date_container);
publishDateContainer.setOnClickListener(new View.OnClickListener() {
@Override
public void onClick(View view) {
Expand Down Expand Up @@ -511,7 +516,7 @@ private void showStatusDialog() {
}

int index = getCurrentPostStatusIndex();
FragmentManager fm = ((AppCompatActivity) getActivity()).getSupportFragmentManager();
FragmentManager fm = getActivity().getSupportFragmentManager();
PostSettingsListDialogFragment fragment =
PostSettingsListDialogFragment.newInstance(DialogType.POST_STATUS, index);
fragment.show(fm, PostSettingsListDialogFragment.TAG);
Expand All @@ -533,7 +538,7 @@ private void showPostFormatDialog() {
}
}

FragmentManager fm = ((AppCompatActivity) getActivity()).getSupportFragmentManager();
FragmentManager fm = getActivity().getSupportFragmentManager();
PostSettingsListDialogFragment fragment =
PostSettingsListDialogFragment.newInstance(DialogType.POST_FORMAT, checkedIndex);
fragment.show(fm, PostSettingsListDialogFragment.TAG);
Expand Down Expand Up @@ -564,7 +569,7 @@ private void showPostDateSelectionDialog() {
Calendar calendar = getCurrentPublishDateAsCalendar();
PostDatePickerDialogFragment fragment =
PostDatePickerDialogFragment.newInstance(PickerDialogType.DATE_PICKER, getPost(), calendar);
FragmentManager fm = ((AppCompatActivity) getActivity()).getSupportFragmentManager();
FragmentManager fm = getActivity().getSupportFragmentManager();
fragment.show(fm, PostDatePickerDialogFragment.TAG_DATE);
}

Expand All @@ -576,7 +581,7 @@ private void showPostTimeSelectionDialog() {
Calendar calendar = getCurrentPublishDateAsCalendar();
PostDatePickerDialogFragment fragment =
PostDatePickerDialogFragment.newInstance(PickerDialogType.TIME_PICKER, getPost(), calendar);
FragmentManager fm = ((AppCompatActivity) getActivity()).getSupportFragmentManager();
FragmentManager fm = getActivity().getSupportFragmentManager();
fragment.show(fm, PostDatePickerDialogFragment.TAG_TIME);
}

Expand Down Expand Up @@ -762,10 +767,16 @@ private int getCurrentPostStatusIndex() {
return 2;
case PRIVATE:
return 3;
default:
// PUBLISHED, SCHEDULED, UNKNOWN
case TRASHED:
case UNKNOWN:
case PUBLISHED:
case SCHEDULED:
return 0;
}
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.

I think we should just use a lint rule for this. I assume we can't do it right away as there are probably some switch statements that don't have all the branches, but we could fix them and enable the lint rule afterwards.

Even if this is the approach we decide to take, let's not make it a part of an unrelated PR and instead try to add it for each switch statement as a single PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We actually have this rule already enabled. This switch statement is part of the lint-baseline file, so it doesn't break the build.
I'll remove the if, as you suggested;).

}
return 0;
}

// Post Format Helpers
Expand Down Expand Up @@ -856,7 +867,7 @@ private void updateFeaturedImageView() {
mediaUri = PhotonUtils.getPhotonImageUrl(mediaUri, size, 0);
}

WPMediaUtils.loadNetworkImage(mediaUri, mFeaturedImageView);
mImageManager.load(mFeaturedImageView, ImageType.PHOTO, mediaUri, ScaleType.FIT_CENTER);
}

private void launchFeaturedMediaPicker() {
Expand Down Expand Up @@ -899,10 +910,8 @@ public void onTaxonomyChanged(OnTaxonomyChanged event) {
AppLog.e(T.POSTS, "An error occurred while updating taxonomy with type: " + event.error.type);
return;
}
switch (event.causeOfChange) {
case FETCH_CATEGORIES:
updateCategoriesTextView();
break;
if (event.causeOfChange == TaxonomyAction.FETCH_CATEGORIES) {
updateCategoriesTextView();
}
}

Expand Down
43 changes: 13 additions & 30 deletions WordPress/src/main/java/org/wordpress/android/util/WPMediaUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
import android.view.ContextThemeWrapper;
import android.view.ViewConfiguration;

import com.android.volley.toolbox.NetworkImageView;

import org.wordpress.android.R;
import org.wordpress.android.analytics.AnalyticsTracker;
import org.wordpress.android.fluxc.model.MediaModel;
Expand All @@ -28,7 +26,6 @@
import org.wordpress.android.ui.RequestCodes;
import org.wordpress.android.ui.prefs.AppPrefs;
import org.wordpress.android.util.AppLog.T;
import org.wordpress.android.widgets.WPNetworkImageView;
import org.wordpress.passcodelock.AppLockManager;

import java.io.File;
Expand Down Expand Up @@ -205,8 +202,19 @@ String getErrorMessage(final Context context, final MediaModel media, final Medi
return context.getString(R.string.error_media_parse_error);
case GENERIC_ERROR:
return context.getString(R.string.error_generic_error);
case EXCEEDS_SITE_SPACE_QUOTA_LIMIT:
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure how I feel about adding these missing cases with just null values. I understand that technically there is no change. However, missing switch cases and specifically having them return null values have different meanings to me.

I am also not sure whether it makes sense to make it a part of this PR. I really like that you are fixing a bunch of things and I am all for it. I just think it's better to keep the PRs focused if possible (which I fail myself at times). A different approach might have been to have a really small PR that adds Glide to EditPostSettingsFragment and another one fixing these issues. In that case, I'd more confidently say that we should add proper error messages for the cases you just added.

Just my 2 cents.

Copy link
Contributor Author

@malinajirka malinajirka Jul 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The goal here is to have all switch statements with explicitly specified cases. It all derives from Fail ASAP principle -> when a method expects just certain enum values, it should throw an exception for all the other values (or at least log an error message). We can't just start throwing an exception or logging error messages in a legacy code, since even missing cases might be expected, so explicitly specifying all the cases is all we can do.
The biggest advantage is when we create a new enum value, lint won't let us build the project unless we add handling for the new enum value to all switch statements, which is very convenient.(we need to remove this switch statement from the lint-baseline obviously, but I do that from time to time not in each PR separately)

I am also not sure whether it makes sense to make it a part of this PR.

This is a topic worth discussing with everyone on the team.

Thank for bringing this up @oguzkocer. I want to write a post about both these topics in a near future. Basically one of the reasons why I don't put small refactoring changes in separate PR is that Android Studio warns me about these issues when committing a change, so it's super convenient to fix it right away.

Copy link
Contributor

Choose a reason for hiding this comment

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

The goal here is to have all switch statements with explicitly specified cases.

Which I agree with you, but I don't see how it has anything to do with a PR titled as Replace Volley with Glide.

The biggest advantage is when we create a new enum value, lint won't let us build the project unless we add handling for the new enum value to all switch statements, which is very convenient.

Again, I think I wasn't very clear in my comment. I have no arguments against having these missing cases. What I am worried about though is returning null. As I also tried to explain in my previous comment, having a null value for these cases means that we specifically want to return null whereas not having the cases at all means we just made a mistake by not updating this switch case.

I just feel that a better way to fix these issues is to open a separate PR where we return specific error messages for the missing cases. Hope that clears it up!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which I agree with you, but I don't see how it has anything to do with a PR titled as Replace Volley with Glide.

I'll create a post about this topic. I feel like minor refactoring (without changing behavior) should be part of each commit/PR, even though it's not strictly related to the PR itself.

I just feel that a better way to fix these issues is to open a separate PR where we return specific error messages for the missing cases. Hope that clears it up!

I'm still not sure it's the way we should go. We'd change the behavior by returning specific error message and I don't think it's save to assume that all missing cases are errors -> they might be omitted on purpose, because the author wanted to return null. Explicitly returning null is definitely not ideal, but it's a change which doesn't change the behavior and moves us towards state, where it'll be safe to assume that lint takes care of warning us about all missing cases.
It would definitely be better to take one switch statement at a time and do research whether we can safely add an error message or throw an exception. But this approach would take a lot of time and would be error prone with very little gain. That's the main reason why I decided to start fixing such switch statements by returning a default value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like minor refactoring (without changing behavior) should be part of each commit/PR, even though it's not strictly related to the PR itself.

I definitely agree with you. My concern was the change in the meaning and that it was not very relevant to the feature.

We'd change the behavior by returning specific error message and I don't think it's save to assume that all missing cases are errors -> they might be omitted on purpose, because the author wanted to return null

My concern was exactly this, by adding those cases we'd just be changing the meaning of that code from missing cases to these cases are specifically returning null.

It would definitely be better to take one switch statement at a time and do research whether we can safely add an error message or throw an exception.

If we are not willing to spend that time, we could at least write a comment in the code explaining why they are currently returning null.

return null;
case NOT_AUTHENTICATED:
return null;
case INVALID_ID:
return null;
case NULL_MEDIA_ARG:
return null;
case MALFORMED_MEDIA_ARG:
return null;
case DB_QUERY_FAILURE:
return null;
}

return null;
}

Expand Down Expand Up @@ -378,32 +386,6 @@ public static boolean canDeleteMedia(MediaModel mediaModel) {
return state == null || (!state.equalsIgnoreCase("uploading") && !state.equalsIgnoreCase("deleted"));
}

/**
* Loads the given network image URL into the {@link NetworkImageView}.
*/
public static void loadNetworkImage(String imageUrl, WPNetworkImageView imageView) {
if (imageUrl != null) {
Uri uri = Uri.parse(imageUrl);
String filepath = uri.getLastPathSegment();

// re-use the default background drawable as error image for now.
// See: https://github.com/wordpress-mobile/WordPress-Android/pull/6295#issuecomment-315129759
imageView.setErrorImageResId(R.drawable.media_item_background);

// default image while downloading
imageView.setDefaultImageResId(R.drawable.media_item_background);

if (MediaUtils.isValidImage(filepath)) {
imageView.setTag(imageUrl);
imageView.setImageUrl(imageUrl, WPNetworkImageView.ImageType.PHOTO);
} else {
imageView.setImageResource(R.drawable.media_item_background);
}
} else {
imageView.setImageResource(0);
}
}

/**
* Returns a poster (thumbnail) URL given a VideoPress video URL
*
Expand Down Expand Up @@ -440,6 +422,7 @@ public void onScanCompleted(String path, Uri uri) {
/*
* returns true if the current user has permission to upload new media to the passed site
*/
@SuppressWarnings("SimplifiableIfStatement")
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look like a very complicated if statement, why don't we just simplify it?

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 felt like the comment self-hosted sites don't have capabilities so always return true won't be so clear, but you are right I guess.

public static boolean currentUserCanUploadMedia(@NonNull SiteModel site) {
if (site.isUsingWpComRestApi()) {
return site.getHasCapabilityUploadFiles();
Expand Down
5 changes: 3 additions & 2 deletions WordPress/src/main/res/layout/edit_post_settings_fragment.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,13 +140,14 @@
android:layout_height="wrap_content"
android:text="@string/post_settings_set_featured_image"/>

<org.wordpress.android.widgets.WPNetworkImageView
<ImageView
android:id="@+id/post_featured_image"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:maxHeight="@dimen/post_settings_featured_image_height_max"
android:minHeight="@dimen/post_settings_featured_image_height_min"
android:scaleType="fitStart"
android:contentDescription="@string/post_settings_featured_desc"
android:adjustViewBounds="true"
android:visibility="gone"/>
</LinearLayout>

Expand Down
1 change: 1 addition & 0 deletions WordPress/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2132,6 +2132,7 @@
<string name="plugin_detail_banner_desc">plugin banner</string>
<string name="plugin_detail_logo_desc">plugin logo</string>
<string name="post_cardview_featured_desc">featured image</string>
<string name="post_settings_featured_desc">featured image</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless we expect the translations to be different, we should have a string for featured image and either use it directly or reference it from these two string resources: post_cardview_featured_desc & post_settings_featured_desc. From their names, I don't think the translators would interpret them any differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point:)

<string name="reader_cardview_post_play_video_desc">play featured video</string>
<string name="photo_picker_thumbnail_desc">Play video</string>
<string name="reader_list_item_suggestion_remove_desc">delete</string>
Expand Down