-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(share_plus): reset isCalledBack on android correctly after closing share-sheet #2446
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
fix(share_plus): reset isCalledBack on android correctly after closing share-sheet #2446
Conversation
|
I'm able to locally run the integration test for android and it's working fine: |
|
Thanks for your contribution. I will try to review/test by the end of this week. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, still didn't get to testing/reproducing issues. Will have time tomorrow.
In the meantime, leaving some comments from the initial code review.
packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/Share.kt
Outdated
Show resolved
Hide resolved
packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/Share.kt
Outdated
Show resolved
Hide resolved
packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/Share.kt
Outdated
Show resolved
Hide resolved
packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/Share.kt
Outdated
Show resolved
Hide resolved
try this #1612 (comment) @coder-with-a-bushido could you please look at the comments |
|
Hey sure, I will check them this weekend. |
217bd94 to
25c2526
Compare
|
Hey @vbuberen, I've addressed the comments. 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. |
|
Hi all, I was asked to take a look as well ;) I've been looking at the code and how the The issue I see with setting Lines 55 to 60 in 7d0392b
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? |
|
I was able to test, and I could verify that this breaks The call to Note that the Side note @vbuberen we should change all examples so they point to the code and not the published package 😅 |
|
This PR is now superseded by #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! |
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:
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
isCalledBackvariable is set to true only on initialisation and not after share-sheet has been called back successfully. The reason beingonActivityResultmethod doesn't work, either because of this flutter issue OR it is not fully supported forACTION_SEND,ACTION_SENDTOintent.Related Issues
Checklist
CHANGELOG.mdnor the plugin version inpubspec.yamlfiles.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?
!in the title as explained in Conventional Commits).