Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[url_launcher] Added draggable property to link info. #6282

Closed
wants to merge 1 commit into from
Closed

[url_launcher] Added draggable property to link info. #6282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/url_launcher/url_launcher_platform_interface/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 2.2.0

* Adds a new `draggable` property to the data controlling the `Link` widget.

## 2.1.0

* Adds a new `launchUrl` method corresponding to the new app-facing interface.
Expand Down
5 changes: 5 additions & 0 deletions packages/url_launcher/url_launcher_platform_interface/lib/link.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ abstract class LinkInfo {

/// Whether the link is disabled or not.
bool get isDisabled;

/// Whether the link can be dragged e.g. to create a shortcut on the desktop.
///
/// Default is true.
bool get draggable => true;
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Sep 1, 2022

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.

Copy link
Author

@kristoffer-zliide kristoffer-zliide Sep 1, 2022

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.

Copy link
Member

@ditman ditman Sep 15, 2022

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?

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Sep 15, 2022

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.

Copy link
Member

@ditman ditman Sep 16, 2022 •
edited
Loading

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);

}

typedef _SendMessage = Function(String, ByteData?, void Function(ByteData?));
Expand Down
2 changes: 1 addition & 1 deletion packages/url_launcher/url_launcher_platform_interface/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ repository: https://github.com/flutter/plugins/tree/main/packages/url_launcher/u
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+url_launcher%22
# NOTE: We strongly prefer non-breaking changes, even at the expense of a
# less-clean API. See https://flutter.dev/go/platform-interface-breaking-changes
version: 2.1.0
version: 2.2.0

environment:
sdk: ">=2.12.0 <3.0.0"
Expand Down