fix(share_plus): remove canLaunch check
#1315
Conversation
| } else { | ||
| throw Exception('Unable to share on web'); | ||
| } | ||
| await urlLauncher.launchUrl(uri.toString(), const LaunchOptions()); |
There was a problem hiding this comment.
Is it intentional that you are ignoring a false return here?
There was a problem hiding this comment.
I just reverted it back. Initially it was not returning anything. Also, the API is not returning anything to the user.
There was a problem hiding this comment.
Also, the API is not returning anything to the user.
But per the docs for launchUrl:
/// Returns true if the URL was launched successful, otherwise either returns
/// false or throws a [PlatformException] depending on the failure.
Is having share throw an exception for some failures, but failing completely silently for other failures, really the desired behavior?
There was a problem hiding this comment.
cc: @miquelbeltran
There was a problem hiding this comment.
For what I can see, we can check the result and throw an exception if it was false.
I think using a plain Exception would be fine here. I will push the changes.
|
Why only Linux? Other implementations were using the same construction. |
Ahh... missed it. Thought it was only used in Linux. Will change them as well. |
SirusCodes
changed the title
canLaunch check
| @@ -19,7 +19,6 @@ class SharePlusLinuxPlugin extends SharePlatform { | |||
| } | |||
|
|
|||
| /// Share text. | |||
| /// Throws a [PlatformException] if `mailto:` scheme cannot be handled. | |||
There was a problem hiding this comment.
Removed for consistency with the other platforms
| if (!launchResult) { | ||
| throw Exception('Failed to launch mailto: URI'); |
There was a problem hiding this comment.
As commented, the launchUrl can either return false or fail with an exception, in the case of returning false we throw an Exception.
I did not use PlatformException here because I don't think we should be creating them outside platform code, so I think just a generic Exception is fine here.
There was a problem hiding this comment.
Yeah, it makes sense.
|
@miquelbeltran thanks for helping out. This feels fine to me |
miquelbeltran
requested review from
stuartmorgan,
nohli and
vbuberen
and removed request for
stuartmorgan
Description
Reverting back to old construct as @stuartmorgan suggested #1295 (comment)
Also remove
canLaunchcheck.Checklist
CHANGELOG.mdnor thepubspec.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).