-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[url_launcher] Added draggable property to link info. #6282
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| /// Whether the link can be dragged e.g. to create a shortcut on the desktop. | ||
| /// | ||
| /// Default is true. | ||
| bool get draggable => true; |
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.
The analyzer failure in CI is surfacing a significant problem here: the use of this class is via implements, not extends, so adding a default implementation doesn't really do anything in practice; this is a breaking change for url_launcher.
@ditman Do you have any better ideas than making a new abstract class that extends LinkInfo and adds this field, and then migrating to that? Obviously that approach doesn't scale very well, but hopefully it won't be necessary very often.
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.
If was actually wondering why this is an interface in the first place; since it's just information, there's essentially only one way to implement it, so if we're making breaking changes, I suggest removing this variability from the API entirely.
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.
@stuartmorgan looking at how LinkInfo is being used elsewhere, I think the simplest solution would be to first modify the Link class to:
class Link extends StatelessWidget with LinkInfo {Using LinkInfo as a mixin on Link makes the default implementations of LinkInfo available on Link until overridden; reminds me of default interface methods in Java.
(After this change, Link would still is LinkInfo.)
See the following gist:
WDYT?
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.
Seems reasonable, but the problem is that we would still want to make a breaking change here to do that. Even if we make the other change first, someone could (unlikely as it is) update url_launcher_platform_interface without updating url_launcher. That's why I was suggesting a new subclass instead. We could still switch url_launcher to a mixin of the new subclass to avoid this problem going forward, but if we're okay with an extra class we can avoid the breaking change.
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.
In addition to adding the draggable attribute to the subclass, if we want to get rid of the LinkInfo completely, we'll probably have to do something like this in the packages where we define a non-null LinkDelegate typedef (which uses a LinkInfo):
diff --git a/packages/url_launcher/url_launcher/lib/src/link.dart b/packages/url_launcher/url_launcher/lib/src/link.dart
index 91f7389ff..0d50930fd 100644
--- a/packages/url_launcher/url_launcher/lib/src/link.dart
+++ b/packages/url_launcher/url_launcher/lib/src/link.dart
@@ -43,7 +43,7 @@ Future<ByteData> Function(Object?, String) pushRouteToFrameworkFunction =
/// )},
/// );
/// ```
-class Link extends StatelessWidget implements LinkInfo {
+class Link extends StatelessWidget with LinkMetadata {
/// Creates a widget that renders a real link on the web, and uses WebViews in
/// native platforms to open links.
const Link({
@@ -91,12 +91,17 @@ class DefaultLinkDelegate extends StatelessWidget {
/// Given a [link], creates an instance of [DefaultLinkDelegate].
///
/// This is a static method so it can be used as a tear-off.
- static DefaultLinkDelegate create(LinkInfo link) {
+ // TODO: Update this to receive a LinkMetadata instead of an Object, when the
+ // interface has been updated.
+ static DefaultLinkDelegate create(Object link) {
+ if (link is! LinkMetadata) {
+ throw ArgumentError('Must be of type LinkMetadata', 'link');
+ }
return DefaultLinkDelegate(link);
}
/// Information about the link built by the app.
- final LinkInfo link;
+ final LinkMetadata link;
bool get _useWebView {
if (link.target == LinkTarget.self) {The problem is that I can't replace LinkInfo in the LinkDelegate by a subclass, because the type of the parameter must be contravariant (and covariant doesn't work in typedefs):
plugins/packages/url_launcher/url_launcher_platform_interface/lib/link.dart
Lines 22 to 23 in 7686be7
| /// Signature for a delegate function to build the [Link] widget. | |
| typedef LinkDelegate = Widget Function(LinkInfo linkWidget); |
|
Unfortunately I cannot push to this branch: |
|
@kristoffer-zliide Are you planning on updating this PR based on the discussion above? |
|
@ditman |
|
What's the ETA of this work? |
|
Thank you for your contribution. I'm going to close this PR for now since there are outstanding comments, just to get this off our PR review queue. Please don't hesitate to submit a new PR if you have the time to address the review comments. Thanks! |
Platform interface part of allowing use of the Link widget in lists with mouse drag enabled.
Platform interface part of #5430
Issues fixed: #102735
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style].///).