Telerik-Verified-Plugins / ImagePicker

Cordova Plugin For Multiple Image Selection
MIT License
181 stars 328 forks source link

Fix for missing activity error on platform re-add #232

Open brassier opened 3 years ago

brassier commented 3 years ago

When this plugin is already added, and you do a platform remove and then a platform add, the node is not added to the android manifest (like it should here). Removing the plugin and re-adding it seems to fix it, but this should not be needed. With no activity in the manifest, the end result is a successful build, but using the plugin results in this error:

Unable to find explicit activity class {....synconset.MultiImageChooserActivity}; have you declared this activity in your AndroidManifest.xml?

This PR is assuming that the problem is <edit-config> being buggy in cordova. The PR moves to a hook approach of manually adding this requestLegacyExternalStorage element. The buggy assumption is documented in the tickets below.

More details can be found in tickets below:

sithwarrior commented 3 years ago

Great work @brassier I've stumbled on the same problem, and created an issue on Cordova-Common https://github.com/apache/cordova-common/issues/153 about exactly this.

But when I think about it, maybe we could fix it, be removing it from plugin.xml, and put a note in the readme, and perhaps a pinned issue, that if you target Android 10, please include the below code in your config.xml (instead of having it in the plugin.xml)

<edit-config file="app/src/main/AndroidManifest.xml" mode="merge" target="/manifest/application" xmlns:android="http://schemas.android.com/apk/res/android">
 <application android:allowBackup="false"/>
</edit-config>

My issue with the hook approach, is that plugin hooks are not supported on all CI/CD environments, as its rather unsafe to run random js code.

brassier commented 3 years ago

Interesting point @sithwarrior. I wasn't aware of the hook concern in CI/CD. I'm certainly open to your alternative. The down side is it does require a manual step for anyone targeting Android 10, which in theory should be everyone now since the Play Store policies require app updates to target 10 as of Nov 2.

Again, I'm open to the alternative manual step, but one more consideration is that there are some fairly popular plugins using hooks in the wild today. So this wouldn't be too much of an outlier.

I don't know much about scoped storage and this android:requestLegacyExternalStorage flag, but the long-term alternative would likely be to make this plugin not require the legacy storage setting. That's likely a bigger effort though. I'm not sure how close this one is? If it is close, maybe we don't even need the legacy flag anymore and we can just remove it?

sithwarrior commented 3 years ago

Regarding the hooks, thats exactly why we couldnt get sqlite storage working on CI/CD.

Hooks could work, but as you mention, any fix is temporary, as the requestlegacy flag, is ignorred already in Android 11. (I believe api target is 9 for app udates and 10 for new apps?)

Im all for this fix, if we can get a working version of the plugin again.

brassier commented 3 years ago

API target is 10 for updates as of November 2 (per here and here). So we probably are OK with this legacy flag...until Android 11 becomes more mainstream.

When you upload an APK, it needs to meet Google Play’s target API level requirements. New apps must target Android 10 (API level 29)* or higher and app updates must target Android 9 (API level 28) or higher.

**Note: From 2 November 2020, app updates must target Android 10 (API level 29) or higher.***

sbellver commented 3 years ago

Any news about this PR? With API 29 this (great!) plugin dont works :(


I can confirm applying this patch I can select multiple images again and fixes Filetransfer plugin too

martinbertinat commented 3 years ago

Hi @brassier and @sbellver!

Do you have news about this? I'm having this problem with android 10 (target sdk). Thank you!

brassier commented 3 years ago

I don't have news on this PR, specifically. I personally used the latest version of the plugin and then implemented some hooks to fix the broken pieces. This PR would prevent the need to do that.