plus_plugins icon indicating copy to clipboard operation
plus_plugins copied to clipboard

fix(share_plus): reset isCalledBack on android correctly after closing share-sheet

Open coder-with-a-bushido opened this issue 1 year ago • 9 comments

Description

If the share is done to the same app(self) in android device, this plugin opens the share-sheet only once and throws the following exception for the subsequent method calls:

Capture exception - PlatformException(Share callback error, prior share-sheet did not call back, did you await it? Maybe use non-result variant, null, null)

It however works once again after the app has been closed and re-opened again. I could reproduce it every single time when I try to share a media file to the same app(self).

In android code, I found that the isCalledBack variable is set to true only on initialisation and not after share-sheet has been called back successfully. The reason being onActivityResult method doesn't work, either because of this flutter issue OR it is not fully supported for ACTION_SEND, ACTION_SENDTO intent.

Related Issues

  • Fix https://github.com/fluttercommunity/plus_plugins/issues/1936
  • Fix https://github.com/fluttercommunity/plus_plugins/issues/1351
  • Fix https://github.com/fluttercommunity/plus_plugins/issues/1612

Checklist

  • [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [x] I titled the PR using Conventional Commits.
  • [x] I did not modify the CHANGELOG.md nor the plugin version in pubspec.yaml files.
  • [x] All existing and new tests are passing.
  • [x] 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).
  • [x] No, this is not a breaking change.

coder-with-a-bushido avatar Dec 07 '23 06:12 coder-with-a-bushido

I'm able to locally run the integration test for android and it's working fine:

mymac@Karthikeyans-MacBook-Pro example % flutter test integration_test/share_plus_test.dart

00:00 +0: ...s/packages/share_plus/share_plus/example/integration_test/share_plus_test.dart     R00:14 +0: ...s/packages/share_plus/share_plus/example/integration_test/share_plus_test.d   14.1s
✓  Built build/app/outputs/flutter-apk/app-debug.apk.
00:16 +0: ...s/packages/share_plus/share_plus/example/integration_test/share_plus_test.dart     I00:17 +0: ...s/packages/share_plus/share_plus/example/integration_test/share_plus_test.d 1,906ms
00:19 +3 ~1: All tests passed!

coder-with-a-bushido avatar Dec 07 '23 09:12 coder-with-a-bushido

Thanks for your contribution. I will try to review/test by the end of this week.

vbuberen avatar Dec 07 '23 10:12 vbuberen

I'm able to locally run the integration test for android and it's working fine

Don't worry about them - we have flaky Android emulators for integration tests. If you see failure due to timeout - it is highly likely it is not your code, but emulator being flaky.

vbuberen avatar Dec 07 '23 10:12 vbuberen

Sorry, still didn't get to testing/reproducing issues. Will have time tomorrow.

In the meantime, leaving some comments from the initial code review.

try this https://github.com/fluttercommunity/plus_plugins/issues/1612#issuecomment-1877362918

@coder-with-a-bushido could you please look at the comments

mzdm avatar Jan 04 '24 16:01 mzdm

Hey sure, I will check them this weekend.

coder-with-a-bushido avatar Jan 04 '24 16:01 coder-with-a-bushido

Hey @vbuberen, I've addressed the comments. Could you please re-review this when you're available?

coder-with-a-bushido avatar Jan 08 '24 11:01 coder-with-a-bushido

Could you please re-review this when you're available?

Thanks, will do soon. Sorry for delaying, got some dense end of last year and start of this one.

vbuberen avatar Jan 09 '24 16:01 vbuberen

Hi all, I was asked to take a look as well ;)

I've been looking at the code and how the ShareSuccessManager works.

The issue I see with setting isCalledBack to true manually is that the method returnResult() will not do anything, because the call to isCalledBack.compareAndSet(false, true) will always fail, as it expects that isCalledBack is false.

https://github.com/fluttercommunity/plus_plugins/blob/7d0392b4cb5223384af720aecebc2ec9fa89c147/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/ShareSuccessManager.kt#L55-L60

I guess this is necessary for the "share with result" case, but I cannot test it because I'm having gradle/java issues :) Do you know if the share with result usecase still works?

miquelbeltran avatar Feb 06 '24 14:02 miquelbeltran

I was able to test, and I could verify that this breaks shareWithResult.

The call to shareResult = await Share.shareWithResult(text... never returns, and it is forever suspended.

Note that the pubspec.yml of the example points to share_plus: 7.2.2 and not the path: .. so you need to apply this patch to test properly (and clean up pub before cache too)

diff --git a/packages/share_plus/share_plus/example/pubspec.yaml b/packages/share_plus/share_plus/example/pubspec.yaml
index 3d60efd8..34421fad 100644
--- a/packages/share_plus/share_plus/example/pubspec.yaml
+++ b/packages/share_plus/share_plus/example/pubspec.yaml
@@ -4,7 +4,8 @@ description: Demonstrates how to use the share_plus plugin.
 dependencies:
   flutter:
     sdk: flutter
-  share_plus: ^7.2.2
+  share_plus:
+    path: ..
   image_picker: ^1.0.0
   file_selector: ^1.0.0

Side note @vbuberen we should change all examples so they point to the code and not the published package :sweat_smile:

miquelbeltran avatar Feb 06 '24 14:02 miquelbeltran

This PR is now superseded by https://github.com/fluttercommunity/plus_plugins/pull/2817

The issue with this PR is that the "reset" call should only happen in error case and not always, as we tried to do there, because that breaks the normal function of the ShareSuccessManager and the share with result methods. Besides, there has been no further movement since the last messages in February (edit: wrong month).

Thanks for the effort nevertheless, we sincerely appreciate that!

miquelbeltran avatar Apr 05 '24 11:04 miquelbeltran