-
-
Notifications
You must be signed in to change notification settings - Fork 937
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): 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional that you are ignoring a false
return here?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc: @miquelbeltran
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.
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed for consistency with the other platforms
if (!launchResult) { | ||
throw Exception('Failed to launch mailto: URI'); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it makes sense.
@miquelbeltran thanks for helping out. This feels fine to me |
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.
Looks good, just not sure if the thrown messages should include $uri instead of mailto: URI.
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.
LGTM
Description
Reverting back to old construct as @stuartmorgan suggested #1295 (comment)
Also remove
canLaunch
check.Checklist
CHANGELOG.md
nor thepubspec.yaml
files.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).