Replace Volley with Glide in the EditPostSettings fragment #7985

malinajirka · 2018-07-03T08:06:05Z

Replaces Volley with Glide in the EditPostSettings.

Adds support for adding animated feature images.

MaxHeight attribute of the ImageView is taken into account - it was ignored before.

To test:

Click on the create post button
Type any title
Click on three dots in the top right corner of the screen
Click on Post Settings
Click on the Set feature image button
Select any GIF image
Notice the animation is working and click on the Publish button
Go to Reader - Followed Sites
Make sure the post has animated GIF image

oguzkocer

@malinajirka I left some comments, but they are a bit nitpicky and more of a discussion type of comments. Let me know what you think!

I also tested the PR and it's working great!

oguzkocer · 2018-07-03T17:21:30Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostSettingsFragment.java

                return 0;
        }
+        if (BuildConfig.DEBUG) {
+            throw new IllegalStateException("Missing switch case.");


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.

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;).

oguzkocer · 2018-07-03T17:26:52Z

WordPress/src/main/java/org/wordpress/android/util/WPMediaUtils.java

@@ -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:


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.

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.

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!

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.

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.

oguzkocer · 2018-07-03T17:34:31Z

WordPress/src/main/res/values/strings.xml

@@ -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>


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.

Good point:)

oguzkocer · 2018-07-03T17:36:27Z

WordPress/src/main/java/org/wordpress/android/util/WPMediaUtils.java

@@ -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")


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

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.

malinajirka · 2018-07-10T09:44:37Z

Thank you @oguzkocer !

I've fixed all the issue you've pointed out and I left a comment with my reasoning. However, I'll create a post so we can have a discussion as a team.

oguzkocer

Looks good

malinajirka added 7 commits June 22, 2018 13:20

Replace Volley with Glide in EditPostSettingsFragment

2304670

Add fallthrough comment to switch

d24c947

Merge branch 'develop' into feature/glide-post-settings2

bdb5d76

Merge branch 'develop' into feature/glide-post-settings2

57d31c0

Add placeholder and error images to post feature image preview

acc9d22

WPMediaUtils cleanup

a2ef20b

EditPostSettings switch cleanup

8eaf4f5

malinajirka requested a review from oguzkocer July 3, 2018 08:06

malinajirka added this to the 10.5 milestone Jul 3, 2018

malinajirka added the Posting/Editing label Jul 3, 2018

oguzkocer reviewed Jul 3, 2018

View reviewed changes

oguzkocer self-assigned this Jul 3, 2018

oguzkocer mentioned this pull request Jul 3, 2018

Replace Volley with Glide in the PostsListAdapter #7989

Merged

malinajirka added 5 commits July 10, 2018 11:33

Simplify if statement in WPMediaUtils

94b4bcf

Refactor switch in EditPostSettingFragment

101e325

Fix string duplication

2bacc14

Fix string duplication

aa63ef5

Revert switch statement refactoring

9a6e643

oguzkocer approved these changes Jul 10, 2018

View reviewed changes

oguzkocer merged commit 3e8a512 into develop Jul 10, 2018

oguzkocer deleted the feature/glide-post-settings2 branch July 10, 2018 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Volley with Glide in the EditPostSettings fragment #7985

Replace Volley with Glide in the EditPostSettings fragment #7985

malinajirka commented Jul 3, 2018

oguzkocer left a comment

oguzkocer Jul 3, 2018

malinajirka Jul 10, 2018

oguzkocer Jul 3, 2018

malinajirka Jul 10, 2018 •

edited

Loading

oguzkocer Jul 10, 2018

malinajirka Jul 10, 2018

oguzkocer Jul 10, 2018

oguzkocer Jul 3, 2018

malinajirka Jul 10, 2018

oguzkocer Jul 3, 2018

malinajirka Jul 10, 2018

malinajirka commented Jul 10, 2018

oguzkocer left a comment

Replace Volley with Glide in the EditPostSettings fragment #7985

Replace Volley with Glide in the EditPostSettings fragment #7985

Conversation

malinajirka commented Jul 3, 2018

oguzkocer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malinajirka Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malinajirka commented Jul 10, 2018

oguzkocer left a comment

Choose a reason for hiding this comment

malinajirka Jul 10, 2018 •

edited

Loading