Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(share_plus)!: Share API cleanup #2832

Merged
merged 17 commits into from
Apr 15, 2024
Merged

Conversation

miquelbeltran
Copy link
Member

@miquelbeltran miquelbeltran commented Apr 10, 2024

Description

This PR contains a major cleanup of the share_plus plugin.

In detail:

  • Reduces API to three methods: shareUri, share and shareXFiles(removed deprecated methods).
  • All methods return a ShareResult.
  • Cleanup of Android implementation.
    • Modified the ShareSuccessManager so it would not deadlock users anymore, instead, calling to share while there is another call waiting, causes that the previous call will be called back with ShareResult.unavailable, instead of causing an error.
    • Tested on Pixel 5 for all three methods sharing on Telegram, they all work with ShareResult correctly returned.
  • Clean of iOS implementation.
    • Tested on iPhone 12 mini, sharing uri, text, and text+image on Telegram, all work and return ShareResult correctly.
  • Cleanup macOs implementation
    • Note: originally the shareUri implementation was missing, maybe to do someday?
    • Tested sharing text and images to Telegram desktop
  • Linux cleanup and testing (just launches mailto as before)
  • Windows
    • Legacy versions cleanup and testing (just launches mailto as before)
    • Cleanup of the c++ implementation for Windows 10+
  • web cleanup and testing
    • Ensure all web methods return either a ShareResult.undefined or throw an exception on error. Do not hide errors.
  • Update README.md
  • Updated tests.
  • Fixed integration tests

Related Issues

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I titled the PR using Conventional Commits.
  • I did not modify the CHANGELOG.md nor the plugin version in pubspec.yaml files.
  • All existing and new tests are passing.
  • The analyzer (flutter analyze) does not report any problems on my PR.

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 that with a ! in the title as explained in Conventional Commits).
  • No, this is not a breaking change.

@miquelbeltran miquelbeltran changed the title fix(share_plus)!: API cleanup [WIP] refactor(share_plus)!: API cleanup [WIP] Apr 10, 2024
@miquelbeltran miquelbeltran changed the title refactor(share_plus)!: API cleanup [WIP] refactor(share_plus)!: Share API cleanup Apr 10, 2024
@miquelbeltran miquelbeltran marked this pull request as ready for review April 10, 2024 12:22
@miquelbeltran miquelbeltran marked this pull request as draft April 10, 2024 12:51
if (isWithResult && !manager.setCallback(result)) return
if (isWithResult)
manager.setCallback(result)
Copy link
Member Author

Choose a reason for hiding this comment

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

this is part of the change that fixes any possible deadlocks, since setCallback will force any pending call to be returned with an "unavailable" ShareResult

Comment on lines -28 to +35
callback.error(
"Share callback error",
"prior share-sheet did not call back, did you await it? Maybe use non-result variant",
null,
)
false
// Ensure no deadlocks.
// Return result of any waiting call.
// e.g. user called to `share` but did not await for result.
this.callback?.success(RESULT_UNAVAILABLE)

// Prepare all state for new share
SharePlusPendingIntent.result = ""
isCalledBack.set(false)
this.callback = callback
Copy link
Member Author

Choose a reason for hiding this comment

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

instead of failing with an error, the call to share that is waiting for a response will get a ShareResult.unavailable, and the new call would be properly registered

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.

Great one 👏🏻

packages/share_plus/share_plus/README.md Outdated Show resolved Hide resolved
packages/share_plus/share_plus/README.md Outdated Show resolved Hide resolved
miquelbeltran and others added 2 commits April 15, 2024 22:48
Co-authored-by: Volodymyr Buberenko <vbuberen@users.noreply.github.com>
Co-authored-by: Volodymyr Buberenko <vbuberen@users.noreply.github.com>
@miquelbeltran miquelbeltran merged commit fd0511c into main Apr 15, 2024
4 checks passed
@miquelbeltran miquelbeltran deleted the miquelbeltran/2819 branch April 15, 2024 20:49
@fisforfaheem
Copy link

Good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants