feat(share_plus)!: SharePlus refactor #3404
Conversation
|
One last big piece missing is the Windows refactor, which I plan to do tomorrow or so. Then I'll write up a migration guide on the README.md. The API and functionality is still backwards compatible, but just getting deprecated warnings. The old The
|
miquelbeltran
changed the title
|
Hey @StanleyCocos I am very sorry I wanted to have this done during the winter break, and then I got sidetracked. Can I ask you a favor? Could you take a look and give me a review? |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the share_plus package by unifying sharing methods into a single non-static approach using a singleton instance and the ShareParams class.
- Consolidated sharing methods into a single instance method call.
- Updated examples and documentation in the README.md to reflect the new API usage.
- Added a migration guide for converting from the deprecated static API.
Files not reviewed (19)
- packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt: Language not supported
- packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/Share.kt: Language not supported
- packages/share_plus/share_plus/example/ios/Runner/AppDelegate.swift: Language not supported
- packages/share_plus/share_plus/example/lib/main.dart: Language not supported
- packages/share_plus/share_plus/example/windows/flutter/CMakeLists.txt: Language not supported
- packages/share_plus/share_plus/example/windows/runner/Runner.rc: Language not supported
- packages/share_plus/share_plus/ios/share_plus/Sources/share_plus/FPPSharePlusPlugin.m: Language not supported
- packages/share_plus/share_plus/lib/share_plus.dart: Language not supported
- packages/share_plus/share_plus/lib/src/share_plus_linux.dart: Language not supported
- packages/share_plus/share_plus/lib/src/share_plus_web.dart: Language not supported
- packages/share_plus/share_plus/lib/src/share_plus_windows.dart: Language not supported
- packages/share_plus/share_plus/macos/share_plus/Sources/share_plus/SharePlusMacosPlugin.swift: Language not supported
- packages/share_plus/share_plus/test/share_plus_linux_test.dart: Language not supported
- packages/share_plus/share_plus/test/share_plus_test.dart: Language not supported
- packages/share_plus/share_plus/test/share_plus_windows_test.dart: Language not supported
- packages/share_plus/share_plus/windows/share_plus_plugin.cpp: Language not supported
- packages/share_plus/share_plus/windows/share_plus_windows_plugin.h: Language not supported
- packages/share_plus/share_plus_platform_interface/lib/method_channel/method_channel_share.dart: Language not supported
- packages/share_plus/share_plus_platform_interface/lib/platform_interface/share_plus_platform.dart: Language not supported
Co-authored-by: Copilot <[email protected]>
miquelbeltran
changed the title
|
Fixing the analysis errors, and main is merged in as well. I am going to mark this as a breaking change, although technically it is not, using this package will cause deprecated warnings to appear and the change is big enough that it deserves it. I am setting myself the goal to merge this by April 18th (two weeks from now). Please, @StanleyCocos and anyone else interested, I'd appreciate a lot to have a review of the changes. |
|
Of course, I’ll try to take a look and offer some suggestions from my side. Things have been quite busy for me recently, but as you mentioned, the merge is in two weeks — I’ll do my best to wrap this up before then. Thanks again! |
...ges/share_plus/share_plus_platform_interface/lib/platform_interface/share_plus_platform.dart
Outdated
Show resolved
Hide resolved
packages/share_plus/share_plus_platform_interface/lib/method_channel/method_channel_share.dart
Outdated
Show resolved
Hide resolved
packages/share_plus/share_plus_platform_interface/test/share_plus_platform_interface_test.dart
Show resolved
Hide resolved
|
Can we add some test cases specifically for sharing to ensure the platform-side code works correctly? |
I'd definitely want to add more native tests, not sure in this PR or later on. Thanks a lot for the review @StanleyCocos ! I'm still going to wait until 18th to merge, in case any new suggestions come. |
|
@miquelbeltran |
|
@StanleyCocos I can't find your new comments |
| /// Throws [ArgumentError] if [ShareParams] are invalid. | ||
| /// | ||
| /// Throws other types of exceptions if the share method fails. | ||
| Future<ShareResult> share(ShareParams params) async { |
There was a problem hiding this comment.
I think we can make the share method static. Right now, we can only use SharePlus.instance, but I feel that using SharePlus.share would be more concise.
There was a problem hiding this comment.
Nope :) that was the whole point of the refactor, to make SharePlus instantiable and not use static methods. We had reports that the old Share class was hard to mock/fake because of that so we are moving to this pattern where you can either create an instance of the class or use SharePlus.instance, which I have seen in other packages e.g. Firebase.
| return; | ||
| } | ||
| // Check if text provided is valid | ||
| if (shareText && shareText.length == 0) { |
There was a problem hiding this comment.
The title parameter is not supposed to be mandatory. If it’s set to an empty string, the feature might not work properly. For optional features like this, I don’t think it should be validated this way.
There was a problem hiding this comment.
await SharePlus.instance.share(
ShareParams(
text: 'test',
subject: '',
title: '',
),
);When I share like this, the following exception occurs.
If the title is an empty string (''), can we just ignore it?
Unhandled Exception: PlatformException(error, Non-empty title expected, null, null)
#0 StandardMethodCodec.decodeEnvelope (package:flutter/src/services/message_codecs.dart:648:7)
#1 MethodChannel._invokeMethod (package:flutter/src/services/platform_channel.dart:334:18)
#2 MethodChannelShare.share (package:share_plus_platform_interface/method_channel/method_channel_share.dart:25:20)
#3 DemoAppState._onShareWithResult (package:share_plus_example/main.dart:218:4)
There was a problem hiding this comment.
Probably this comment was for line 288, as this if-condition is checking text, not title.
Title is not mandatory indeed. This could probably be done indeed, although I am not sure how the UI would behave with an empty string. I am fine to do this change afterward checking how the UI works in that case, I am not sure also if there is a usecase where the title should be an empty string as opposed to ignore it.
| /// | ||
| /// * Supported platforms: iOS, Android | ||
| /// Fallsback to sharing the URI as text on other platforms. | ||
| final Uri? uri; |
There was a problem hiding this comment.
From what I see, when a uri is provided, the text becomes ineffective because uri is checked first. So I’m wondering if we could define a priority—for example, if a uri is passed, then text is ignored—instead of making the two parameters mutually exclusive.
There was a problem hiding this comment.
I am replying about this question here: https://github.com/fluttercommunity/plus_plugins/pull/3404/files/bad0154682593755febf2ded51963c162edc6346#r2048771496
| } | ||
|
|
||
| if (params.uri != null && params.text != null) { | ||
| throw ArgumentError('uri and text cannot be provided at the same time'); |
There was a problem hiding this comment.
Can we define a priority? For example, if there’s a Uri, we can ignore the text. At the very least, let the user share successfully. Right now, directly aborting like this feels a bit too arbitrary.
SharePlus.instance.share(
ShareParams(
text: 'test',
subject: '',
title: '',
uri: Uri.tryParse('https://google.com')
),
);Unhandled Exception: Invalid argument(s): uri and text cannot be provided at the same time
E/flutter (17265): #0 SharePlus.share (package:share_plus/share_plus.dart:78:7)
E/flutter (17265): #1 DemoAppState._onShareWithResult (package:share_plus_example/main.dart:218:29)
E/flutter (17265): #2 DemoAppState.build.. (package:share_plus_example/main.dart:167:33)
E/flutter (17265): #3 _InkResponseState.handleTap (package:flutter/src/material/ink_well.dart:1170:21)
E/flutter (17265): #4 GestureRecognizer.invokeCallback (package:flutter/src/gestures/recognizer.dart:351:24)
E/flutter (17265): #5 TapGestureRecognizer.handleTapUp (package:flutter/src/gestures/tap.dart:656:11)
E/flutter (17265): #6 BaseTapGestureRecognizer._checkUp (package:flutter/src/gestures/tap.dart:313:5)
E/flutter (17265): #7 BaseTapGestureRecognizer.handlePrimaryPointer (package:flutter/src/gestures/tap.dart:246:7)
E/flutter (17265): #8 PrimaryPointerGestureRecognizer.handleEvent (package:flutter/src/gestures/recognizer.dart:703:9)
E/flutter (17265): #9 PointerRouter._dispatch (package:flutter/src/gestures/pointer_router.dart:98:12)
E/flutter (17265): #10 PointerRouter._dispatchEventToRoutes. (package:flutter/src/gestures/pointer_router.dart:143:9)
E/flutter (17265): #11 _LinkedHashMapMixin.forEach (dart:collection-patch/compact_hash.dart:633:13)
E/flutter (17265): #12 PointerRouter._dispatchEventToRoutes (package:flutter/src/gestures/pointer_router.dart:141:18)
E/flutter (17265): #13 PointerRouter.route (package:flutter/src/gestures/pointer_router.dart:127:7)
E/flutter (17265): #14 GestureBinding.handleEvent (package:flutter/src/gestures/binding.dart:501:19)
E/flutter (17265): #15 GestureBinding.dispatchEvent (package:flutter/src/gestures/binding.dart:481:22)
E/flutter (17265): #16 RendererBinding.dispatchEvent (package:flutter/src/rendering/binding.dart:450:11)
E/flutter (17265): #17 GestureBinding._handlePointerEventImmediately (package:flutter/src/gestures/binding.dart:426:7)
E/flutter (17265): #18 GestureBinding.handlePointerEvent (package:flutter/src/gestures/binding.dart:389:5)
E/flutter (17265): #19 GestureBinding._flushPointerEventQueue (package:flutter/src/gestures/binding.dart:336:7)
E/flutter (17265): #20 GestureBinding._handlePointerDataPacket (package:flutter/src/gestures/binding.dart:305:9)
E/flutter (17265): #21 _invoke1 (dart:ui/hooks.dart:328:13)
E/flutter (17265): #22 PlatformDispatcher._dispatchPointerDataPacket (dart:ui/platform_dispatcher.dart:442:7)
E/flutter (17265): #23 _dispatchPointerDataPacket (dart:ui/hooks.dart:262:31)
E/flutter (17265):
There was a problem hiding this comment.
I prefer to take an offensive programming approach. My experience with this project tells me that users don't really read the documentation, and instead they go straight to the issue tracker to complain when something isn't as they expect, so letting users provide both text and uri will lead to users thinking both can be shared at the same time, instead I prefer that the package enforces providing data correctly.
There was a problem hiding this comment.
From what I understand, in many scenarios, the content to be shared is not necessarily predetermined — it is decided at runtime or fetched from a server API. In such cases, both text and uri might be present. If I had to choose between (1) failing to share and (2) only sharing part of the content, I would definitely choose the latter. I believe our goal is to enable sharing. We should do everything we can to ensure the share succeeds, rather than blocking it just because the parameters don’t exactly meet our expectations.
There was a problem hiding this comment.
I think we can leverage asser() to ensure the production environment while also restricting the development environment. I’m not sure if this would be better.
There was a problem hiding this comment.
I see your point, and if it was an existing API, I would definitely go with asserts instead. I'd say we merge it like this and see how it goes, and depending on the feedback we can move it back.
packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/Share.kt
Outdated
Show resolved
Hide resolved
|
@miquelbeltran Sorry, I just realized I didn’t submit that comment. |
|
Thanks again @StanleyCocos for taking the time helping with this, your feedback was very appreciated! As promised, today is merging day, I will prepare a release afterward (edit: on Monday, don't release on Friday) |
Description
This PR contains several improvements on the share_plus package:
SharePlusclass viainstancesingleton, rather than using static methods.SharePlatformfor testing.subjectandtitle.ShareParamsclass.mailToFallbackEnabledto disable web mailTo failback (enabled by default)downloadFallbackEnabledis no longer a static setting but part of theShareParamsuriis now supported on all platforms by sharing the URI as plain textNot a breaking change, since the old API is still compatible, only it has been deprecated.
But we should consider bumping the version of
share_plusto a major release nevertheless, since the deprecation will break CI/CD lint analysis.TODO:
Related Issues
share_plusrefactor #3403EXTRA_TITLEin share Intent on Android #3307Checklist
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).