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
LinkedText (Linkify) #125927
LinkedText (Linkify) #125927
Conversation
Nice. I wonder if |
@guidezpl Thank you, I was hoping a better name would come to me! I'll change it to TextLink. |
… everything on this at the core
@guidezpl I went back and forth but I've currently got it named |
final RangesFinder rangesFinder; | ||
|
||
// TODO(justinmc): Consider revising this regexp. | ||
static final RegExp _urlRegExp = RegExp(r'((http|https|ftp):\/\/)?([a-zA-Z\-]*\.)?[a-zA-Z0-9\-]*\.[a-zA-Z]*'); |
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.
Here's a better RegEx to use as this allows users to have port numbers, paths, query params, and fragments in the URL.
Here are some URLs that match my updated RegEx:
http://example.com/
https://www.example.org/
ftp://subdomain.example.net
example.com
subdomain.example.io
www.example123.co.uk
http://example.com:8080/
https://www.example.com/path/to/resource
http://www.example.com/index.php?query=test#fragment
https://subdomain.example.io:8443/resource/file.html?search=query#result
And the RegEx itself:
((https?|ftp):\/\/)?(([a-zA-Z\-]*\.)?[a-zA-Z0-9\-]+)\.[a-zA-Z]+(?::\d{1,5})?(?:\/[^\s]*)?(?:\?[^\s#]*)?(?:#[^\s]*)?
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.
@Reprevise "example." return true with your Regex
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.
example.
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.
@justinmc Your regex also fails in this case
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.
regex has been updated
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.
Fine by me, though it does look weird seeing the ftp://
not highlighted but that will be the case for other protocols too so 🤷🏻
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.
Here's my attempt at fixing the "ftp://" thing:
(((https?:\/\/)(([a-zA-Z0-9\-]*\.)*[a-zA-Z0-9\-]+(\.[a-zA-Z]+)+))|((?<!\/|\.|[a-zA-Z0-9\-])((([a-zA-Z0-9\-]*\.)*[a-zA-Z0-9\-]+(\.[a-zA-Z]+)+))))(?::\d{1,5})?(?:\/[^\s]*)?(?:\?[^\s#]*)?(?:#[^\s]*)?
Now any URL with a scheme that isn't http or https won't be highlighted at all:
I had to duplicate part of the regex, so if anyone knows a better way to express that let me know.
My list of test URLs
'www.example123.co.uk\nasdf://subdomain.example.net\nsubdomain.example.net\nftp.subdomain.example.net\nhttp://subdomain.example.net\nhttps://subdomain.example.net\nhttp://example.com/\nhttps://www.example.org/\nftp://subdomain.example.net\nexample.com\nsubdomain.example.io\nwww.example123.co.uk\nhttp://example.com:8080/\nhttps://www.example.com/path/to/resource\nhttp://www.example.com/index.php?query=test#fragment\nhttps://subdomain.example.io:8443/resource/file.html?search=query#result\n"example.com"\n\'example.com\'\n(example.com)\nsubsub.www.example.com\nhttps://subsub.www.example.com'
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.
Hey @justinmc, here's a simpler regex that maintains the functionality:
(?<![\/\.a-zA-Z0-9-])((https?:\/\/)?(([a-zA-Z0-9-]*\.)*[a-zA-Z0-9-]+(\.[a-zA-Z]+)+))(?::\d{1,5})?(?:\/[^\s]*)?(?:\?[^\s#]*)?(?:#[^\s]*)?
It also matches your list exactly.
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.
Thank you! Just added it and it works perfectly. Sorry I've been working on supporting TextSpans so I missed this for a while.
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 tweaked the regex again. Now it explicitly ignores email addresses.
My list of test URLs
'www.example123.co.uk\nasdf://subdomain.example.net\nsubdomain.example.net\nftp.subdomain.example.net\nhttp://subdomain.example.net\nhttps://subdomain.example.net\nhttp://example.com/\nhttps://www.example.org/\nftp://subdomain.example.net\nexample.com\nsubdomain.example.io\nwww.example123.co.uk\nhttp://example.com:8080/\nhttps://www.example.com/path/to/resource\nhttp://www.example.com/index.php?query=test#fragment\nhttps://subdomain.example.io:8443/resource/file.html?search=query#result\n"example.com"\n\'example.com\'\n(example.com)\nsubsub.www.example.com\nhttps://subsub.www.example.com\nexample@example.com\nexample.com@example.com'
Ah I see now how this works on the entire text, that makes sense. |
/// the given [RegExp]. | ||
InlineLinkedText.textLinkers({ | ||
super.style, | ||
required String text, |
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.
should we also support textspan as input?
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 think you're right and it should. People are going to have existing span trees in which they want to highlight links. I'm going to think about that.
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.
Instead of just textit's now possible to pass
spans` instead 👍 .
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.
Just some concerns around text linker API, the other part LGTM
/// [InlineLinkBuilder], so it's the responsibility of the caller to do so. | ||
/// See [TextSpan.recognizer] for more. | ||
TextLinker({ | ||
required this.textRangesFinder, |
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'm imagining looking up matches in a large Map or some data structure
Even so the regex should be enough?
Is supporting linking without a regex part of the use cases we want to support, it seems to me at that point what we are doing is helping people parse textspan into a smaller pieces given a text range. but the return data (displayString and linkString) does not seem useful....
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
I don't think anything actually failed in the Google tests, but I've rerun them just in case. |
flutter/flutter@61b890b...58ba6c2 2023-09-14 engine-flutter-autoroll@skia.org Roll Packages from 06cd9e9 to 275b76c (1 revision) (flutter/flutter#134734) 2023-09-14 dacoharkes@google.com Update plugin_ffi generated file to match FFIgen 9.0.0 (flutter/flutter#134614) 2023-09-14 jmccandless@google.com LinkedText (Linkify) (flutter/flutter#125927) 2023-09-14 polinach@google.com _DayPicker should build days using separate stetefull widget _Day. (flutter/flutter#134607) 2023-09-13 31859944+LongCatIsLooong@users.noreply.github.com Remove `Path.combine` call from `CupertionoTextSelectionToolbar` (flutter/flutter#134369) 2023-09-13 katelovett@google.com Update KeepAlive.debugTypicalAncestorWidgetClass (flutter/flutter#133498) 2023-09-13 84124091+opxdelwin@users.noreply.github.com Fix null check crash by ReorderableList (flutter/flutter#132153) 2023-09-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from 154d6fd601a3 to cd90cc8469fb (3 revisions) (flutter/flutter#134691) 2023-09-13 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.21.5 to 2.21.6 (flutter/flutter#134692) 2023-09-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from b71b366e3de3 to 154d6fd601a3 (6 revisions) (flutter/flutter#134683) 2023-09-13 christopherfujino@gmail.com [flutter_tools] Run ShutdownHooks when handling signals (flutter/flutter#134590) 2023-09-13 jhy03261997@gmail.com Dispose routes in navigator when throwing exception (flutter/flutter#134596) 2023-09-13 jonahwilliams@google.com [framework] reduce ink sparkle uniform count. (flutter/flutter#133897) 2023-09-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5e671d5c90f9 to b71b366e3de3 (4 revisions) (flutter/flutter#134676) 2023-09-13 15619084+vashworth@users.noreply.github.com Set the CONFIGURATION_BUILD_DIR in generated xcconfig when debugging core device (flutter/flutter#134493) 2023-09-13 zanderso@users.noreply.github.com Bump gradle heap size limit in *everywhere* (flutter/flutter#134665) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
I'm concerned that this PR makes it easier to make links but does not make them work on web, where links are most common and where not matching the platform is the most disruptive and where we have a solution for making links that have high fidelity with the core platform. Would it make sense to make this a package instead? It does seem like functionality that does not need to be in the core framework, and our general policy is that we should put things in packages if possible. In particular, would it make sense to put this in url_launcher? |
This works on web just like any other platform, unless I'm missing something. Is the concern with the need to use url_launcher? Using LinkedText like this: import 'package:url_launcher/url_launcher.dart';
...
LinkedText(
text: 'example.com',
onTapUri: (Uri uri) async {
if (!await launchUrl(uri)) {
throw 'Could not launch $uri';
}
},
), What is the existing high fidelity solution? I assumed that people were using things like TextSpan.recognizer + url_launcher (see StackOverflow). Using TextSpan.recognizer is tricky due to needing to manually manage a TapGestureRecognizer. My concern with putting this inside of url_launcher is that devs might want to do something other than launch a URL when tapped. Maybe do something in-app when a user taps on a username or something like that? I'm just guessing though. I would definitely be down to move this to its own package if you think that's best. |
This reverts commit 4db47db.
Reverts #125927 context: b/300804374 Looks like a g3 fix might involve changing the names of widget on the customer app, and I am not sure if that would be the right approach forward. Putting up a revert to be safe for now.
if i right click on the link, do i get the browser's menu? |
Yes, but no link options like "Open link in new tab". Do you know if we have a way to do that on the web now? Maybe this is a good opportunity to automatically wire that up for people if it's possible. |
I believe the Having it in url_opener wouldn't require that the developer use it to open a URL, it could still allow a callback. The idea would just be to default to the "right thing" for links. |
(It looks like there's an ongoing discussion here and on discord. Please let me know when the PR is ready to reland and I'll get the g3fix CL ready). |
Ah I see (Link docs). LinkedText does seem to work with Link on web and the other platforms (example below), but I would have to rethink this as part of url_launcher to make that super slick by default. LinkedText + Link exampleLinkedText.textLinkers(
text: 'Hello https://www.example.com world.', // spans seem to work too.
textLinkers: <TextLinker>[
TextLinker(
regExp: LinkedText.defaultUriRegExp,
linkBuilder: (String displayString, String linkString) {
return WidgetSpan(
child: Link(
uri: Uri.parse(linkString),
builder: (BuildContext context, FollowLink? followLink) {
return Text.rich(
TextSpan(
style: const TextStyle(color: Colors.blue),
text: displayString,
// Omitting lifecycle management for this recognizer for brevity.
recognizer: TapGestureRecognizer()..onTap = followLink,
),
);
},
),
);
},
),
],
), If that's the best option here then I'll probably have to revisit this in January when I return from leave. |
New LinkedText widget and TextLinker class for easily adding hyperlinks to text.
Reverts flutter#125927 context: b/300804374 Looks like a g3 fix might involve changing the names of widget on the customer app, and I am not sure if that would be the right approach forward. Putting up a revert to be safe for now.
On Android, you can use Linkify to turn URLs in some text into tappable links. Swift can also do this with its link property on NSAttributedString.
This PR attempts to bring this functionality to Flutter.
API
In order to avoid an explicitly dependency on url_launcher, the onTap handler is left up to the user. Otherwise, the API aims to handle the case of URLs by default as easily as possible, while also being as flexible as possible in non-default cases.
Urls (default)
Twitter handles
Both Urls and Twitter handles at the same time
TextSpan trees
Reference
Fixes #40505