Skip to content

Conversation

Coronon
Copy link
Contributor

@Coronon Coronon commented Mar 21, 2022

Description

  • Revert minSdkVersion back to 16 and dynamically determine support for shareWithResult at runtime
  • Gracefully fall back to "normal" share without result if the environment does not support shareWithResult
  • Improve documentation for shareWithResult

Related Issues

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.

@Coronon
Copy link
Contributor Author

Coronon commented Mar 21, 2022

The conflicts are due to #797. I will resolve them tonight (mainly formatting).

Update: rebased

@miquelbeltran
Copy link
Member

Thanks! The original share method, could it be replaced by the one with the result? There's no difference except for the return type, which was void in the original share method, and now it would be ShareResult.

@miquelbeltran miquelbeltran self-assigned this Mar 22, 2022
@Coronon
Copy link
Contributor Author

Coronon commented Mar 22, 2022

Did not think about that - yeah, that would definitely simplify the API. The only problem I see is people not awaiting the "old" share methods and then complaining about the "not awaited..." error because lets face it: most people just update and don't read the changelog ^^

Copy link
Collaborator

@vbuberen vbuberen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments on namings.

What we are currently missing is updates to docs, which appear on Plus plugins website.
Would be good if you also modify https://github.com/fluttercommunity/plus_plugins/blob/main/docs/share_plus/usage.mdx to mention new methods and description about fallback on unsupported platforms.

@Coronon
Copy link
Contributor Author

Coronon commented Mar 23, 2022

Also added shareWithResult and shareFilesWithResult to the usage.mdx docs.

Will commit & push when isWithResultAvailable naming is resolved.

EDIT: pushed

@Mosc
Copy link

Mosc commented Mar 23, 2022

Thanks! The original share method, could it be replaced by the one with the result? There's no difference except for the return type, which was void in the original share method, and now it would be ShareResult.

@Coronon already touched on it a bit, but there's a difference in behavior between share and this updated shareWithResult (at least on Android SDK >= 22, iOS and macOS). awaiting the former completes immediately while the latter completes only after the share dialog is dismissed. Replacing share's implementation could cause issues in existing code. I'm not necessarily against such a change, but as I tried to argue on #797, any breaking change should be made with caution and should be accompanied by an appropriate increase in version number. The current PR is itself already a nice improvement, so I suggest we try to merge this iteration first.

Copy link
Collaborator

@vbuberen vbuberen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for this contribution!
Would like to wait for a review from Miguel as he was the person, who started reviewing this PR.

@miquelbeltran
Copy link
Member

LGTM too, I will go ahead and merge & release

@uc-asa
Copy link

uc-asa commented Sep 1, 2022

I'm still getting this issue and android always return null in raw

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

share_plus Android minSdk bump drops support for a lot of devices
5 participants