[share_plus] Fix types, lower Android minSDK by vbuberen · Pull Request #797 · fluttercommunity/plus_plugins

vbuberen · 2022-03-19T15:18:39Z

Description

In this PR I fixed issues with type in result.error(), which some users reported in #758. Saw that the code wasn't formatted according to our editorconfig file, which was added some time ago, so reformatted the code.

I didn't like that share_plus now has minSDK 23 as it cuts quite a lot of users/devices. I thought we could make it much lower, but it seems that 22 is the minimum possible, because Intent.createChooser() with sender parameter appeared only in SDK 22. So I made required changes to set minSDK to 22 and some additional small fixes.

Finally, I saw that in #788 there were changes to min Flutter version, but not all platforms were updated. Fixed it as well, so share_plus requires Flutter 1.20 on all platforms now.

Related Issues

Closes #758
Partially closes #789

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
All existing and new tests are passing.
I updated the version in pubspec.yaml and CHANGELOG.md.
I updated/added relevant documentation (doc comments with ///).
The analyzer (flutter analyze) does not report any problems on my PR.
I read and followed the Flutter Style Guide.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

…erywhere

Mosc · 2022-03-19T15:49:25Z

Not sure how these cases are generally handled, but I would prefer the minSdkVersion to go all the way back to 16 and simply return an appropriate result.error(...) for shareWithResult and shareFilesWithResult on SDK versions lower than 22. That would allow users to just continue to use share and shareFiles as they did before v4.0.0.

Coronon · 2022-03-19T16:00:59Z

Not sure how these cases are generally handled, but I would prefer the minSdkVersion to go all the way back to 16 and simply return an appropriate result.error(...) for shareWithResult and shareFilesWithResult on SDK versions lower than 22. That would allow users to just continue to use share and shareFiles as they did before v4.0.0.

As said - I am pretty new to Android development. If that compiles and works as expected, it is probably the best option we have. The only risk I see there is people unintentionally using shareWithResult with a minSdkVersion lower than 16 and not noticing because their development environment is API level 22+. This could lead to users effected by unforeseen errors - can we somehow statically check this at compile time? And API version 22 is from 2015, so is this really necessary?

Mosc · 2022-03-19T18:52:33Z

Not sure how these cases are generally handled, but I would prefer the minSdkVersion to go all the way back to 16 and simply return an appropriate result.error(...) for shareWithResult and shareFilesWithResult on SDK versions lower than 22. That would allow users to just continue to use share and shareFiles as they did before v4.0.0.

As said - I am pretty new to Android development. If that compiles and works as expected, it is probably the best option we have. The only risk I see there is people unintentionally using shareWithResult with a minSdkVersion lower than 16 and not noticing because their development environment is API level 22+. This could lead to users effected by unforeseen errors - can we somehow statically check this at compile time? And API version 22 is from 2015, so is this really necessary?

I understand your concerns. A runtime error is certainly not ideal, although to be fair, shareWithResult and shareFilesWithResult already fail on runtime on web, Windows and Linux despite the package claiming those platforms are supported! We rely on the user reading the documentation for those specific methods.

How about falling back to the regular non-result based share operations and just returning ShareResultStatus.unavailable for Android SDKs under 22?

vbuberen · 2022-03-19T19:15:26Z

How about falling back to the regular non-result based share operations and just returning ShareResultStatus.unavailable for Android SDKs under 22?

I agree that we should rework the solution with sharing with result. Not sure I can jump right into it (because I am an Android dev for quite some time and might be the right person to take care of it) as I am quite occupied with some activities here in Ukraine due to infamous events, so don't have much time. But let's see how it goes.

ekuleshov · 2022-03-19T19:44:26Z

+                }
+            }
+            "shareWithResult" -> {
+                expectMapArguments(call)


could these *withResults(..) methods extended with check like this?

if(Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP_MR1) result.notImplemented() else ...

Then it won't be necessary to bump the minSdkVersion version for Android platform and it could be kept at the level supported in 3.x version of plugin.

I think so. If anyone can submit a separated PR with a that change, that would be awesome!

Coronon · 2022-03-20T02:25:40Z

How about falling back to the regular non-result based share operations and just returning ShareResultStatus.unavailable for Android SDKs under 22?

That sounds like a good idea, as we already have that status and would not need to break any existing APIs as of version 4.0.1

I might even try to implement that tomorrow

PS: I am starting to grasp why Microsoft still supports DOS elements: People seem to hate to upgrade xD We should all adopt the “live at head” mentality abseil has… ^^

ekuleshov · 2022-03-20T13:42:27Z

How about falling back to the regular non-result based share operations and just returning ShareResultStatus.unavailable for Android SDKs under 22?

Thank you for looking into this.

Now, if some platform would return ShareResultStatus.unavailable, perhaps it would be more consistent to do the same on web, linux and windows too instead of tossing not supported runtime exception on those?

Coronon · 2022-03-20T17:57:55Z

I will fork this fork to base my PR on the better formatted code

vbuberen · 2022-03-20T18:20:02Z

I will fork this fork to base my PR on the better formatted code

I wouldn't bother yourself with that to depend on this PR, since I don't know for sure as to when it gets a review.

If it is just about formatting - auto formatting should work fine for you no matter which IDE you use as the project has an editorconfig file in its root, so code format should be forced according to riles in that file.

Coronon · 2022-03-20T18:22:00Z

If it is just about formatting - auto formatting should work fine for you no matter which IDE you use as the project has an editorconfig file in its root, so code format should be forced according to riles in that file.

I only use melos format, which seems to not yield the same result as your formatting, but ok, will depend on the main branch then

Coronon · 2022-03-20T20:49:00Z

Just pushed my changes, please try this example project with your low API version devices/emulators: https://github.com/Coronon/share_plus_result_example

I also added graceful fallback for other platforms to the normal share experience with ShareResult.unavailable being returned.
The documentation is also clearer now I hope.

miquelbeltran

Thank you @vbuberen !

miquelbeltran · 2022-03-21T09:28:30Z

and published

vbuberen added 3 commits March 19, 2022 14:46

[share_plus] fix type mismatch in Android, set min Flutter to 1.20 ev…

3466bea

…erywhere

[share_plus] lower Android minSDK to 22, format Kotlin code

2e33bd8

[share_plus] Android code cleanup

029b93d

vbuberen added the share_plus Feature, Enhancement, Bug Fixes for Share Plus Plugin label Mar 19, 2022

vbuberen requested a review from miquelbeltran March 19, 2022 15:18

Mosc reviewed Mar 19, 2022

View reviewed changes

Comment thread ...plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt

Coronon reviewed Mar 19, 2022

View reviewed changes

Comment thread ...us/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/ShareSuccessManager.kt Outdated

[share_plus] update Android code for readability

5c65831

ekuleshov reviewed Mar 19, 2022

View reviewed changes

vbuberen mentioned this pull request Mar 19, 2022

kotlin_version = "1.6.10" build error #794

Closed

Coronon mentioned this pull request Mar 20, 2022

share_plus Android minSdk bump drops support for a lot of devices #789

Closed

miquelbeltran approved these changes Mar 21, 2022

View reviewed changes

vbuberen merged commit a4a5218 into fluttercommunity:main Mar 21, 2022

vbuberen deleted the fix/share_plus_types branch March 21, 2022 07:53

Coronon mentioned this pull request Mar 21, 2022

[share_plus] minSdkVersion 16 & graceful fallback #800

Merged

10 tasks

github-actions Bot locked as resolved and limited conversation to collaborators Feb 2, 2025

Uh oh!

Conversation

vbuberen commented Mar 19, 2022

Description

Related Issues

Checklist

Breaking Change

Uh oh!

Uh oh!

Uh oh!

Mosc commented Mar 19, 2022

Uh oh!

Coronon commented Mar 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mosc commented Mar 19, 2022

Uh oh!

vbuberen commented Mar 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekuleshov Mar 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miquelbeltran Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

Coronon commented Mar 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekuleshov commented Mar 20, 2022

Uh oh!

Coronon commented Mar 20, 2022

Uh oh!

vbuberen commented Mar 20, 2022

Uh oh!

Coronon commented Mar 20, 2022

Uh oh!

Coronon commented Mar 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miquelbeltran left a comment

Choose a reason for hiding this comment

Uh oh!

miquelbeltran commented Mar 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Coronon commented Mar 19, 2022 •

edited

Loading

vbuberen commented Mar 19, 2022 •

edited

Loading

ekuleshov Mar 19, 2022 •

edited

Loading

Coronon commented Mar 20, 2022 •

edited

Loading

Coronon commented Mar 20, 2022 •

edited

Loading