Update `minSdkVersion` (from `21` to `24`) #2564

ParaskP7 · 2022-11-08T15:19:03Z

This PR upgrades FluxC to minSdkVersion to 24. This has been unblocked because:

WPAndroid was already on minSdkVersion = 24, for some time now (see here).
WCAndroid has been recently upgraded to minSdkVersion = 24 as well (see here).

Warning (⚠️): This update is currently blocked by the common upgrade of targetSdkVersion to 31 (see here).

As part of this minSdkVersion = 24 upgrade the below changes were also applied in order to fix any additional warnings that this change brought-up:

Warnings Resolution List:

PS: @hichamboushaba @ovitrif I added you as the main reviewers, that is, in addition to the @wordpress-mobile/apps-infrastructure team itself, but randomly, since I just wanted someone from both, the WordPress and Woo Commerce teams, to sign-off on that change for WPAndroid and WCAndroid alike.

Testing instructions

There is nothing much to test here.
Verifying that all the CI checks are successful should be enough.
However, if you want to be thorough about reviewing this change, you could:
- Quickly smoke test the FluxC Example app and see if it works as expected.
- Try and install the FluxC Example app on Android 5 (API 21/22) and Android 6 (API 23) devices and verify that you can't (see API Levels).

Merge instructions

Wait for Update targetSdkVersion (from 22 to 31) #2563 to be approved and merged to trunk.
Make sure this PR's base is automatically updated to trunk.
Update PR with latest trunk.
Remove [PR] Not Ready For Merge] label.
Merge PR to trunk.

Although the 'assemble' and 'lint' task run successfully, a quick analysis showed that updating to 'minSdkVersion = 24' added/removed the below extra warnings: Lint Warnings: - +2 x ObsoleteSdkInt (plugins:woocommerce) All these newly created warnings will be dealt with in subsequent commits. Also, any existing such warnings will be resolved too.

Warning Message: "Unnecessary; SDK_INT is always >= 24" This 'VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN' check was, and is, no longer necessary as the 'minSdkVersion' is now '24'.

Warning Message: "Unnecessary; SDK_INT is always >= 24" This 'VERSION.SDK_INT >= Build.VERSION_CODES.N' check is no longer necessary as the 'minSdkVersion' is now '24'. As such, the if condition has been simplified.

Warning Message: "Unnecessary; SDK_INT is always >= 24" Both, this '@RequiresApi(VERSION_CODES.N)' annotation and 'VERSION.SDK_INT >= Build.VERSION_CODES.N' check, are no longer necessary as the 'minSdkVersion' is now '24'.

Warning Message: "Unnecessary; SDK_INT is always >= 24" This 'assumeTrue(Build.VERSION.SDK_INT >= 23)' test assertion is no longer necessary as the 'minSdkVersion' is now '24'.

hichamboushaba

Looks good, thanks @ParaskP7 for working on this.
I left one comment, but it's not blocking, so I'm pre-approving.

hichamboushaba · 2022-11-09T13:15:20Z

...lin/org/wordpress/android/fluxc/network/rest/wpcom/wc/leaderboards/LeaderboardProductItem.kt

+    private fun fromHtmlWithSafeApiCall(source: String?) = source?.let {
+        Html.fromHtml(source, Html.FROM_HTML_MODE_LEGACY)
+    } ?: Html.fromHtml(source)


I think we can simplify this function by removing the call to the parameterless fromHtml, as it's equivalent to the other one using FROM_HTML_MODE_LEGACY: https://developer.android.com/reference/android/text/Html#fromHtml(java.lang.String). This was necessary before because of the takeIf, but not anymore I think.

Great, thanks for the comment @hichamboushaba ! I agree, the cleaner the better, I'll apply this change and let you know as soon as I push its commit. 🙇

👋 @hichamboushaba !

This is now done, see ba0b906 and 8637d1c.

You will note that I went a bit further and tried to also resolve the Deprecated warning, but I only could go as far as doing so for 1/3 of those. You will find further detail in the commit description itself. Let me know what you think and thank you! 🙇

Thanks @ParaskP7 for making the changes 🙏 , I left some comments below.
All of this is minor, so no need for spending too much time on it 🙂.

ParaskP7 · 2022-11-09T13:35:36Z

Thank you so much for reviewing and testing this PR @hichamboushaba , you rock! 🙏

This 'fromHtmlWithSafeApiCall(...)' function served a purpose only when it was combined with `takeIf`. However, now that this was removed due to the 'minSdkVersion = 24' upgrade, this whole function is no longer necessary and can be replaced with a direct 'Html.fromHtml(...)' call.

Replacing 'Html.fromHtml(html)' with the 'HtmlCompat.fromHtml(html, HtmlCompat.FROM_HTML_MODE_LEGACY)' compat related version is the recommended action to resolve this kind of Lint warnings. However, note that this was only possible for 'priceAmountHtmlTag'. Doing the same for the other two, 'priceAmountHtmlTag' and 'itemHtmlTag', isn't possible as those fields can end up being nullable, that is, due to the fact that the 'LeaderboardItemRow.display' constructor argument is nullable as well. As such, and because 'HtmlCompat.fromHtml(html, HtmlCompat.FROM_HTML_MODE_LEGACY)' accepts a non-null String 'source', it is not possible to resolve these two Lint warnings.

hichamboushaba · 2022-11-10T11:00:45Z

...lin/org/wordpress/android/fluxc/network/rest/wpcom/wc/leaderboards/LeaderboardProductItem.kt

@@ -84,7 +85,7 @@ class LeaderboardProductItem(
                ?.split(";")
                ?.filter { it.contains("&#") }
                ?.reduce { total, new -> "$total$new" }
-                ?.run { Html.fromHtml(this) }
+                ?.run { HtmlCompat.fromHtml(this, HtmlCompat.FROM_HTML_MODE_LEGACY) }


np, you don't have to change it.
HtmlCompat is useful only for apps with minSdk less than 24, for us, we can directly call the method that was introduced in API 24 https://developer.android.com/reference/android/text/Html#fromHtml(java.lang.String,%20int)

Ah, thank you @hichamboushaba , you are totally right, I somehow got afraid of the fact that the String source is neither marked @NonNull nor @Nullable on the Html.java side and thus didn't want to make this change, but that doesn't matter, you have to trust that it works for @Nullable just like that as it anyways defaults to using the same mechanism underneath.

Thus, done, this update seems safe and apologies for the trouble with this review (😅): 5acc19a

hichamboushaba · 2022-11-10T11:03:06Z

...lin/org/wordpress/android/fluxc/network/rest/wpcom/wc/leaderboards/LeaderboardProductItem.kt

@@ -150,7 +150,7 @@ class LeaderboardProductItem(
     * using the [SpannableStringBuilder] implementation in order to parse it
     */
    private val link by lazy {
-        fromHtmlWithSafeApiCall(itemHtmlTag)
+        Html.fromHtml(itemHtmlTag)


For using the new API instead of the deprecated one, WDYT about something like this?

itemHtmlTag?.let { Html.fromHtml(it, Html.FROM_HTML_MODE_LEGACY) } ?.run { this as? SpannableStringBuilder } ...

👋 @hichamboushaba !

I think maybe this change is enough for now, wdyt? 🤔

PS: I really don't want to change any of the functionality, even by that slight bit, not that it will change much, but you know, you can never be too careful, and then you need to really test that, even if you change that little bit.

Good point, and I think we are safe, when the argument is null, we were already calling Html#fromHtml(String) which proxies the call to the same function we are using now.

Exactly, once more, thank YOU Hicham ! 🙇

As correctly noted 'HtmlCompat' is only useful for apps with `minSdkVersion` less than '24', thus it is now unneeded and can be completely replaced with the direct usage of 'Html'. See discussion here: https://github.com/wordpress-mobile/ WordPress-FluxC-Android/pull/2564#discussion_r1018958379

ovitrif

Changes LGTM 🎉

Though I wonder with regards to the existing discussion here, in WPAndroid we always use HtmlCompat, yet minSdkVersion is 24, isn't it then in fact not necessary? 😅

ParaskP7 · 2022-11-14T10:53:36Z

Thank you for the review and testing @ovitrif ! 🙇 ❤️ 🚀

Though I wonder with regards to the existing discussion here, in WPAndroid we always use HtmlCompat, yet minSdkVersion is 24, isn't it then in fact not necessary? 😅

Yeap, yeap, this is indeed the case, I just copy-pasted my solution from WCAndroid into WPAndroid, and since I used HtmlCompat there, I used it blindly as is on WPAndroid as well. But this is indeed not necessary and we could optimize on it. 💯

PS: Feel free to do a quick PR on it if you like, else I'll get to that at some point myself. 👍

…ndroid into build/update-min-sdk-version-to-24 � Conflicts: � settings.gradle

This is done once more, due to the fact that a merge conflict resulting in that update being lost.

Since the 'targetSdkVersion' has been now update to point to '31', disabling this 'ExpiredTargetSdkVersion' Lint error is no longer necessary.

ParaskP7 added 5 commits November 8, 2022 16:37

Analysis: Resolve obsolete sdk int lint warning for well sql config

986fc83

Warning Message: "Unnecessary; SDK_INT is always >= 24" This 'VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN' check was, and is, no longer necessary as the 'minSdkVersion' is now '24'.

Analysis: Resolve obsolete sdk int lint warning for mocked statck test

ef933c2

Warning Message: "Unnecessary; SDK_INT is always >= 24" This 'assumeTrue(Build.VERSION.SDK_INT >= 23)' test assertion is no longer necessary as the 'minSdkVersion' is now '24'.

ParaskP7 added [PR] Not Ready for Merge Tech Debt Tooling labels Nov 8, 2022

ParaskP7 requested review from hichamboushaba, ovitrif and a team November 8, 2022 15:19

ParaskP7 self-assigned this Nov 8, 2022

ParaskP7 mentioned this pull request Nov 8, 2022

Update targetSdkVersion (from 22 to 31) #2563

Merged

hichamboushaba approved these changes Nov 9, 2022

View reviewed changes

ParaskP7 added 2 commits November 9, 2022 18:13

hichamboushaba reviewed Nov 10, 2022

View reviewed changes

ovitrif approved these changes Nov 14, 2022

View reviewed changes

Base automatically changed from build/update-target-sdk-version-to-31 to trunk November 16, 2022 13:36

ParaskP7 added 3 commits November 16, 2022 15:38

Merge branch 'trunk' of github.com:wordpress-mobile/WordPress-FluxC-A…

83a62c7

…ndroid into build/update-min-sdk-version-to-24 � Conflicts: � settings.gradle

Build: Update min sdk version to 24

b2a6272

This is done once more, due to the fact that a merge conflict resulting in that update being lost.

Analysis: Remove unnecessary expired target sdk version lint exclusion

438d4de

Since the 'targetSdkVersion' has been now update to point to '31', disabling this 'ExpiredTargetSdkVersion' Lint error is no longer necessary.

ParaskP7 merged commit 5a4f813 into trunk Nov 16, 2022

ParaskP7 deleted the build/update-min-sdk-version-to-24 branch November 16, 2022 14:17

ParaskP7 removed the [PR] Not Ready for Merge label Nov 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `minSdkVersion` (from `21` to `24`) #2564

Update `minSdkVersion` (from `21` to `24`) #2564

ParaskP7 commented Nov 8, 2022 •

edited

Loading

hichamboushaba left a comment

hichamboushaba Nov 9, 2022

ParaskP7 Nov 9, 2022

ParaskP7 Nov 9, 2022

hichamboushaba Nov 10, 2022

ParaskP7 commented Nov 9, 2022

hichamboushaba Nov 10, 2022

ParaskP7 Nov 10, 2022

hichamboushaba Nov 10, 2022

ParaskP7 Nov 10, 2022

hichamboushaba Nov 10, 2022

ParaskP7 Nov 10, 2022

ovitrif left a comment

ParaskP7 commented Nov 14, 2022

Update minSdkVersion (from 21 to 24) #2564

Update minSdkVersion (from 21 to 24) #2564

Conversation

ParaskP7 commented Nov 8, 2022 • edited Loading

Testing instructions

Merge instructions

hichamboushaba 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

Choose a reason for hiding this comment

ParaskP7 commented Nov 9, 2022

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

ovitrif left a comment

Choose a reason for hiding this comment

ParaskP7 commented Nov 14, 2022

Update `minSdkVersion` (from `21` to `24`) #2564

Update `minSdkVersion` (from `21` to `24`) #2564

ParaskP7 commented Nov 8, 2022 •

edited

Loading