Fix/15322 upgrade Stories-Android to android 11 api level 30 #715

ravishanker · 2021-09-14T05:45:24Z

Upgrade to Android 11 (API level 30)

Please see paqN3M-mr-p2 for details on mandatory requirement to upgrade to Android 11 by November 2021. Also this paqN3M-n4-p2, for details on planned approach to upgrade

upgrade targetSdkVersion and compileSdkVersion to 30
fix and tweak a few files
remove redundant permission WRITE_EXTERNAL_STORAGE

NOTE: Stories is first cab off the rank for upgrade as a dependency for WPAndroid upgrade

Upgraded the following to 30 from 28 targetSdkVersion, compileSdkVersion Made tweaks to a few files mostly guard against nulls

Remove redundant permission

peril-automattic · 2021-09-14T05:49:14Z

You can test the changes on this Pull Request by downloading the APK here.

ParaskP7

👋 @ravishanker !

I have reviewed and tested this PR as per the instructions, it seems to work as expected, good job! 🌟

I have left a couple of minor suggestions (💡) for you to consider. I am NOT going to approve this PR for now and until we make sure that WordPress-Android is ready as well, since it seems to me that the integration should happen at the same time. Correct me if I am wrong.

I also suggest we add @oguzkocer to the picture just to he is aware of this upcoming change.

Btw, have you tried to test this more thoroughly by integrating this branch with WordPress-Android? I honestly didn't go that far, but it seems wise if we do so before merging this PR.

From a conversation that I had with @oguzkocer a while ago (see here) one can check out the branch in libs/stories-android folder of WordPress-Android. Then, if the integration works as well, we can have more confidence that there is nothing more that needs to be done for this upgrade. PS: Also, since we are upgrading two versions at once (from SDK 28 to 30), I suggest we should be more careful with our testing here.

build.gradle

ParaskP7 · 2021-09-14T13:49:32Z

app/src/main/java/com/automattic/loop/StoryComposerActivity.kt

-                // if there are any external Uris in this list, copy them locally before trying to use them
-                processExternalUris(uriList)
+                val mediaUris = data.getStringArrayExtra(PhotoPickerActivity.EXTRA_MEDIA_URIS)
+                if (mediaUris != null) {


Suggestion (💡): Using ?.let { ... } instead of an if not null check it might save us a line and make this a bit more idiomatic:

PS: Same applies to setMediaUris(...) on the ComposeLoopFrameActivity.kt class.

👋 @ravishanker !

What I meant was the data.getStringArrayExtra(PhotoPickerActivity.EXTRA_MEDIA_URIS)?.let { ... } equivalent, which will save a line, but please don't bother since this is totally minor, I just posted it as a nice-to-have.

😅

ParaskP7 · 2021-09-14T13:59:50Z

app/src/main/AndroidManifest.xml

@@ -5,7 +5,6 @@
    <uses-permission android:name="android.permission.INTERNET" />
    <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />
    <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" />
-    <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" />


Question (❓): Are we sure about that? Why was that there is the first place?

PS: Obviously, this is just an example app used to demonstrate that the stories functionality works as expected, prior to integrating that with WordPress-Android, but still, this is good to know, maybe this will be needed to be done on the main repo as well. 🤷

Not sure why it was there. It could be that because there's a READ so, WRITE was added by default. It is definitely not needed here. I've it on my list to check this again on main WPAndroid and test it and remove it.

Thank you @ravishanker ! 🙏

👋 - this is there so others wanting to integrate the library in their projects can use the demo app as an example to be followed in their own apps, regarding what things are needed. In this sense, the <uses-permission> tag is not strictly needed for a demo app to be built or run, but it serves a purpose that goes beyond that. That specific tag is used by Google Play to filter apps through, so it will be needed if users of the library want to publish their apps there. For reference, here's the documentation.

Also:

I've it on my list to check this again on main WPAndroid and test it and remove it

It won't affect WPAndroid, given this is only within the demo app's manifest.

Given the above reasons, please, can we get it back as it was @ravishanker ? 🙏

As a side note, I believe this change is not related to the SDK change as advertised in the PR description. I would highly encourage unrelated changes to be either explained or be kept to a different PR when possible.

oguzkocer · 2021-09-14T14:13:20Z

@ravishanker @ParaskP7 I am currently on support rotation, so I am not supposed to review any PRs 😅 Please ping me on Slack if you are blocked and I'll do my best to unblock you.

Btw, have you tried to test this more thoroughly by integrating this branch with WordPress-Android?

@ParaskP7 is correct. Any stories-android library change should be tested in WPAndroid before merging the PR.

We also always ping @aforcier or @cameronvoell for a review when we make a change to stories-android library and get their approval before merge. (unless it's only a tooling change)

Please see this P2 post for details on mandatory requirement to upgrade to Android 11 by November 2021.

@ravishanker We prefer not to link internal documents directly in the public repositories. There is a way to do this using a shortcode, you can see an example of how @AliSoftware does it in this PR. I personally don't link any internal documents, not even as shortcodes, and prefer to provide/duplicate the necessary information in the public space for the benefit of non-a12s, however using shortcodes is widely accepted, so it's up to you.

ParaskP7 · 2021-09-14T15:17:40Z

Thank you for your input @oguzkocer , much appreciated, happy support rotation! 🙇

AliSoftware · 2021-09-14T15:22:32Z

@ravishanker We prefer not to link internal documents directly in the public repositories. There is a way to do this using a shortcode

I've edited the PR description to use the shortcode instead.

@ravishanker if you want to avoid letting links to internal docs slip in public repos in the future, I highly recommend installing our internal "GitHub Highlighter/Shorthander" tampermonkey script. You can find it by searching github highlighter in MGS.

ravishanker · 2021-09-15T02:09:30Z

👋 @ravishanker !

I have reviewed and tested this PR as per the instructions, it seems to work as expected, good job! 🌟

I have left a couple of minor suggestions (💡) for you to consider. I am NOT going to approve this PR for now and until we make sure that WordPress-Android is ready as well, since it seems to me that the integration should happen at the same time. Correct me if I am wrong.

I also suggest we add @oguzkocer to the picture just to he is aware of this upcoming change.

Btw, have you tried to test this more thoroughly by integrating this branch with WordPress-Android? I honestly didn't go that far, but it seems wise if we do so before merging this PR.

From a conversation that I had with @oguzkocer a while ago (see here) one can check out the branch in libs/stories-android folder of WordPress-Android. Then, if the integration works as well, we can have more confidence that there is nothing more that needs to be done for this upgrade. PS: Also, since we are upgrading two versions at once (from SDK 28 to 30), I suggest we should be more careful with our testing here.

Thanks for the review @ParaskP7. I started from WPAndroid, then moved on to stories-android. I realized that, I need to fix all the dependencies starting with stories first. So, yeah, I've tested with WPAndroid.

ParaskP7 · 2021-09-15T08:59:27Z

👋 @ravishanker !

Thanks for the review @ParaskP7. I started from WPAndroid, then moved on to stories-android. I realized that, I need to fix all the dependencies starting with stories first. So, yeah, I've tested with WPAndroid.

Great, thanks for confirming that you tested this branch with WPAndroid, nice job! 🌟

PS: Then, I can approve this PR as nothing seems to be blocking. I however think that it might be better to merge than when WPAndroid is ready with the upgrade as well, wdyt?

mzorz · 2021-09-15T09:05:26Z

👋 hello @ravishanker @ParaskP7 - let me give this one a quick round of testing before we merge 👍

ParaskP7 · 2021-09-15T09:07:52Z

👋 @mzorz !

👋 hello @ravishanker @ParaskP7 - let me give this one a quick round of testing before we merge 👍

💯 and many thanks! 🙇

mzorz

Thank you for taking on this task, @ravishanker !

I've smoke-tested both the demo app and WordPress-Android app with the stories-android library pointing to this branch, and using this test branch on WPAndroid (as some changes needed to be made in there as well).

camera: taking pictures and recording videos worked as expected
saving: both new pics and videos were composed successfully
sourcing media: using the media picker also worked well (local media, WP media in the WP app, 3rd party app media, remote media).
integration with Gutenberg (creating a post from zero, then editing, then editing an existing Post created on another device) seemed to work fine.

All of this using a Google Pixel 4a.

I also verified most of the changes in this PR are due to new build errors that appear from changing the targetSdk value to 30, so that's fine.

IMPORTANT: It is worth noting that when compiling the WordPress-Android app, the stories library will take the targetSdk and compileSdk from the parent project so, what this really means is that when building WPAndroid if we don't perform the changes as expressed in this diff then we'd only have tested the code changes in this PR but not the actual SDK change which this PR's main aim is supposed to be cc @ravishanker . I'd only ask that we make a more thorough list of what's been tested and how next time 👍 .

So, this needs to be re-tested once the parent app (WPAndroid) is ready for the change to API level 30, before we update the commit hash of submodule /libs/stories-android in the parent repository.

That said, let's please revert the <uses-permission> tag removal, and we can merge afterwards

Thank you!

mzorz · 2021-09-15T11:24:52Z

app/src/main/AndroidManifest.xml

@@ -5,7 +5,6 @@
    <uses-permission android:name="android.permission.INTERNET" />
    <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />
    <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" />
-    <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" />


👋 - this is there so others wanting to integrate the library in their projects can use the demo app as an example to be followed in their own apps, regarding what things are needed. In this sense, the <uses-permission> tag is not strictly needed for a demo app to be built or run, but it serves a purpose that goes beyond that. That specific tag is used by Google Play to filter apps through, so it will be needed if users of the library want to publish their apps there. For reference, here's the documentation.

Also:

I've it on my list to check this again on main WPAndroid and test it and remove it

It won't affect WPAndroid, given this is only within the demo app's manifest.

Given the above reasons, please, can we get it back as it was @ravishanker ? 🙏

As a side note, I believe this change is not related to the SDK change as advertised in the PR description. I would highly encourage unrelated changes to be either explained or be kept to a different PR when possible.

mzorz · 2021-09-15T11:26:36Z

stories/src/main/java/com/wordpress/stories/compose/ComposeLoopFrameActivity.kt

@@ -925,6 +917,15 @@ abstract class ComposeLoopFrameActivity : AppCompatActivity(), OnStoryFrameSelec
        }
    }

+    private fun setMediaUris(intent: Intent) {
+        val mediaUris = intent.getStringArrayExtra(requestCodes.EXTRA_MEDIA_URIS)


Thank you for finding this opportunity to refactor 👍

Nitpick: val mediaUris here can be removed and the expression simplified to intent.getStringArrayExtra(requestCodes.EXTRA_MEDIA_URIS)?.let {...}, given it's only used once

done, thank you

Revert WRITE_EXTERNAL_STORAGE permission

ravishanker · 2021-09-16T03:43:48Z

Thank you for taking on this task, @ravishanker !

I've smoke-tested both the demo app and WordPress-Android app with the stories-android library pointing to this branch, and using this test branch on WPAndroid (as some changes needed to be made in there as well).
* camera: taking pictures and recording videos worked as expected

* saving: both new pics and videos were composed successfully

* sourcing media: using the media picker also worked well (local media, WP media in the WP app, 3rd party app media, remote media).

* integration with Gutenberg (creating a post from zero, then editing, then editing an existing Post created on another device) seemed to work fine.
All of this using a Google Pixel 4a.

I also verified most of the changes in this PR are due to new build errors that appear from changing the targetSdk value to 30, so that's fine.

IMPORTANT: It is worth noting that when compiling the WordPress-Android app, the stories library will take the targetSdk and compileSdk from the parent project so, what this really means is that when building WPAndroid if we don't perform the changes as expressed in this diff then we'd only have tested the code changes in this PR but not the actual SDK change which this PR's main aim is supposed to be cc @ravishanker . I'd only ask that we make a more thorough list of what's been tested and how next time 👍 .

So, this needs to be re-tested once the parent app (WPAndroid) is ready for the change to API level 30, before we update the commit hash of submodule /libs/stories-android in the parent repository.

That said, let's please revert the <uses-permission> tag removal, and we can merge afterwards

Thank you!

@mzorz Thank you for reviewing and testing comprehensively. 🙌 Thanks for the assurance

I've reverted the permission. I removed it for testing and also AS prompting me constantly that it is redundant.

I've been testing with parent WPAndroid app. I should have pushed that too, to spare you the trouble of having to make them for testing. I'll push the WPAndroid as well now.

mzorz · 2021-09-16T09:14:33Z

I've reverted the permission. I removed it for testing and also AS prompting me constantly that it is redundant.

I've been testing with parent WPAndroid app. I should have pushed that too, to spare you the trouble of having to make them for testing. I'll push the WPAndroid as well now

Thank you @ravishanker 👍 - left a comment there.

In reading more about how this all fits in your migration plan paqN3M-n4-p2, I found that we might need some more thorough testing on devices running on Android 11 when targeting SDK 30. For this, TIL there's a tool we can use to force the flags needed. (see App compatibility Changes screen in Settings -> System > Advanced > Developer options > App Compatibility Changes.).

With this in mind, I'll run some more tests on this PR before we proceed to merge. If you can do the same, please by all means do 👍 🙇 cc @ParaskP7

mzorz · 2021-09-16T09:51:58Z

I've uninstalled the demo (loop) app, re-installed it, changed the flag FORCE_ENABLED_SCOPED_STORAGE to ON as shown in the screenshot, and was able to use the media picker and navigate the contents both shown right away and also using the device picker (by tapping on the bottom button, then Choose photo from device or Choose video from device, both worked well).

However, I encountered a crash on WPAndroid, may be unrelated though, but created an issue here wordpress-mobile/WordPress-Android#15333

ravishanker · 2021-10-06T00:58:51Z

Thank you for taking on this task, @ravishanker !

I've smoke-tested both the demo app and WordPress-Android app with the stories-android library pointing to this branch, and using this test branch on WPAndroid (as some changes needed to be made in there as well).
* camera: taking pictures and recording videos worked as expected

* saving: both new pics and videos were composed successfully

* sourcing media: using the media picker also worked well (local media, WP media in the WP app, 3rd party app media, remote media).

* integration with Gutenberg (creating a post from zero, then editing, then editing an existing Post created on another device) seemed to work fine.
All of this using a Google Pixel 4a.

I also verified most of the changes in this PR are due to new build errors that appear from changing the targetSdk value to 30, so that's fine.

IMPORTANT: It is worth noting that when compiling the WordPress-Android app, the stories library will take the targetSdk and compileSdk from the parent project so, what this really means is that when building WPAndroid if we don't perform the changes as expressed in this diff then we'd only have tested the code changes in this PR but not the actual SDK change which this PR's main aim is supposed to be cc @ravishanker . I'd only ask that we make a more thorough list of what's been tested and how next time 👍 .

So, this needs to be re-tested once the parent app (WPAndroid) is ready for the change to API level 30, before we update the commit hash of submodule /libs/stories-android in the parent repository.

That said, let's please revert the <uses-permission> tag removal, and we can merge afterwards

Thank you!

@mzorz I'm attempting to complete upgrade task. I've already addressed all the changes suggested. Are you able to remove this, to help me proceed with merger. Thank you 🙏

mzorz · 2021-10-06T08:34:11Z

Could you update this branch with develop? Also I can see the related WPAndroid PR has conflicts, should these be fixed before we do a final round of testing and merge please? 🙏 @ravishanker

Also, a PR has just been merged for composite builds. Do we know how this update to SDK 30 may play together? (I haven't thought of it through deeply, so summoning here @oguzkocer just to double check whether we have a clear path to merge develop into this branch, test, then merge the PR or we need something else at this point).

oguzkocer · 2021-10-06T17:08:12Z

Thanks for the ping @mzorz. As long as this PR and WPAndroid counterpart are updated with latest develop (and merge conflicts are resolved), we should be all good.

ravishanker · 2021-10-07T00:58:01Z

Thanks for the ping @mzorz. As long as this PR and WPAndroid counterpart are updated with latest develop (and merge conflicts are resolved), we should be all good.

Thanks @oguzkocer, Updated this PR with latest develop, it's green now.

ravishanker · 2021-10-07T01:01:26Z

Could you update this branch with develop? Also I can see the related WPAndroid PR has conflicts, should these be fixed before we do a final round of testing and merge please? 🙏 @ravishanker

Also, a PR has just been merged for composite builds. Do we know how this update to SDK 30 may play together? (I haven't thought of it through deeply, so summoning here @oguzkocer just to double check whether we have a clear path to merge develop into this branch, test, then merge the PR or we need something else at this point).

Thanks @mzorz, Updated this PR with latest develop and also WPAndroid PR and resolved conflicts. Once we merge this, need to update its develop-# on WPAndroid PR before merging it.

mzorz

Thank you for these changes @ravishanker ! 🙌
LGTM - I'll merge and create a new release tag on the repo that can be referenced by WPAndroid

ravishanker added 2 commits September 14, 2021 14:49

Upgrade to 30

4c57ca5

Upgraded the following to 30 from 28 targetSdkVersion, compileSdkVersion Made tweaks to a few files mostly guard against nulls

Update AndroidManifest.xml

baab02c

Remove redundant permission

ravishanker requested review from ParaskP7 and malinajirka September 14, 2021 05:45

ravishanker added the Hack Week Hack Week candidate label Sep 14, 2021

ParaskP7 self-assigned this Sep 14, 2021

ParaskP7 reviewed Sep 14, 2021

View reviewed changes

ParaskP7 requested a review from oguzkocer September 14, 2021 14:01

ParaskP7 assigned ravishanker Sep 14, 2021

Update for review

44c8c83

mzorz self-requested a review September 15, 2021 07:58

ParaskP7 self-requested a review September 15, 2021 08:59

ParaskP7 approved these changes Sep 15, 2021

View reviewed changes

mzorz requested changes Sep 15, 2021

View reviewed changes

Update for review

dc88c49

Revert WRITE_EXTERNAL_STORAGE permission

ravishanker mentioned this pull request Sep 16, 2021

Fix/15322 upgrade WordPress-Android to android 11 api level 30 wordpress-mobile/WordPress-Android#15331

Merged

3 tasks

mzorz mentioned this pull request Sep 16, 2021

[SDK 30 upgrade] Media Picker: crash when picking media from device wordpress-mobile/WordPress-Android#15333

Closed

malinajirka removed their request for review September 16, 2021 12:45

ravishanker changed the title ~~Fix/15322 upgrade to android 11 api level 30~~ Fix/15322 upgrade Stories-Android to android 11 api level 30 Sep 17, 2021

ravishanker mentioned this pull request Sep 17, 2021

Upgrade stories-android to Android 11 wordpress-mobile/WordPress-Android#15339

Closed

Merge branch 'develop' into fix/15322-upgrade-to-android-11-api-level-30

d23cda2

mzorz approved these changes Oct 7, 2021

View reviewed changes

mzorz merged commit 6919e00 into develop Oct 7, 2021

mzorz deleted the fix/15322-upgrade-to-android-11-api-level-30 branch October 7, 2021 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/15322 upgrade Stories-Android to android 11 api level 30 #715

Fix/15322 upgrade Stories-Android to android 11 api level 30 #715

ravishanker commented Sep 14, 2021 •

edited by ParaskP7

Loading

peril-automattic bot commented Sep 14, 2021 •

edited

Loading

ParaskP7 left a comment

ParaskP7 Sep 14, 2021

ravishanker Sep 15, 2021

ParaskP7 Sep 15, 2021

ParaskP7 Sep 14, 2021

ravishanker Sep 15, 2021

ParaskP7 Sep 15, 2021

mzorz Sep 15, 2021

oguzkocer commented Sep 14, 2021

ParaskP7 commented Sep 14, 2021

AliSoftware commented Sep 14, 2021 •

edited

Loading

ravishanker commented Sep 15, 2021

ParaskP7 commented Sep 15, 2021

mzorz commented Sep 15, 2021

ParaskP7 commented Sep 15, 2021

mzorz left a comment

mzorz Sep 15, 2021

mzorz Sep 15, 2021

ravishanker Sep 16, 2021

ravishanker commented Sep 16, 2021

mzorz commented Sep 16, 2021

mzorz commented Sep 16, 2021

ravishanker commented Oct 6, 2021

mzorz commented Oct 6, 2021

oguzkocer commented Oct 6, 2021

ravishanker commented Oct 7, 2021

ravishanker commented Oct 7, 2021

mzorz left a comment

Fix/15322 upgrade Stories-Android to android 11 api level 30 #715

Fix/15322 upgrade Stories-Android to android 11 api level 30 #715

Conversation

ravishanker commented Sep 14, 2021 • edited by ParaskP7 Loading

peril-automattic bot commented Sep 14, 2021 • edited Loading

ParaskP7 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oguzkocer commented Sep 14, 2021

ParaskP7 commented Sep 14, 2021

AliSoftware commented Sep 14, 2021 • edited Loading

ravishanker commented Sep 15, 2021

ParaskP7 commented Sep 15, 2021

mzorz commented Sep 15, 2021

ParaskP7 commented Sep 15, 2021

mzorz 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

ravishanker commented Sep 16, 2021

mzorz commented Sep 16, 2021

mzorz commented Sep 16, 2021

ravishanker commented Oct 6, 2021

mzorz commented Oct 6, 2021

oguzkocer commented Oct 6, 2021

ravishanker commented Oct 7, 2021

ravishanker commented Oct 7, 2021

mzorz left a comment

Choose a reason for hiding this comment

ravishanker commented Sep 14, 2021 •

edited by ParaskP7

Loading

peril-automattic bot commented Sep 14, 2021 •

edited

Loading

AliSoftware commented Sep 14, 2021 •

edited

Loading