SES-1227 : Split out parentheses from URL - Android #1925
Conversation
|
I think there are way too many custom logic here and it will become hard to understand later.
|
I pushed a commit that used auto-link instead |
| val raw = text.substring(start, end) | ||
|
|
||
| val url = when (s.type) { | ||
| LinkType.WWW -> "https://$raw" |
There was a problem hiding this comment.
Can we safely assume the link to be https://? I'm surprised that autolink doesn't give you the protocol
There was a problem hiding this comment.
Yeah autolink seems to be a text detector only. Defaulting the scheme to https:// seems common, iOS seems to do this with their links. Should we default to http or not add scheme at all?
There was a problem hiding this comment.
Are you saying that it will also detect links like www.google.com? If that's the case then yes we should probably do https by default. I was only curious what happens to when there's a full link http://www.google.com, will you still only get the www.google.com?
There was a problem hiding this comment.
Ah, yes it will detect both with and without the scheme and we will get the full link http://www.google.com if the text contains the scheme.
There was a problem hiding this comment.
Pull request overview
Updates URL span detection in conversation message rendering by replacing Android Linkify with autolink-java, improving URL boundary handling (notably around )).
Changes:
- Add
org.nibor.autolink:autolinkdependency via version catalog and app dependencies. - Replace
Linkify.addLinks()usage with a newSpannable.addUrlSpansWithAutolink()helper. - Introduce URL span extraction + application logic in
conversation/v2/Util.kt.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| gradle/libs.versions.toml | Adds version + library entry for autolink. |
| app/build.gradle.kts | Includes autolink dependency in the app module. |
| app/src/main/java/org/thoughtcrime/securesms/conversation/v2/messages/VisibleMessageContentView.kt | Switches URL detection from Linkify to autolink-backed helper. |
| app/src/main/java/org/thoughtcrime/securesms/conversation/v2/Util.kt | Adds Spannable extension to extract/apply URL spans using autolink-java. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // replace URLSpans with ModalURLSpans | ||
| body.getSpans<URLSpan>(0, body.length).toList().forEach { urlSpan -> | ||
| val updatedUrl = urlSpan.url.let { it.toHttpUrlOrNull().toString() } | ||
| val updatedUrl = urlSpan.url.toHttpUrlOrNull().toString() |
There was a problem hiding this comment.
toHttpUrlOrNull() can return null; calling .toString() on it will produce the literal string "null", which would then be used as the URL for ModalURLSpan. Consider using a safe-call and either skipping replacement when parsing fails or falling back to the original urlSpan.url.
| val updatedUrl = urlSpan.url.toHttpUrlOrNull().toString() | |
| val updatedUrl = urlSpan.url.toHttpUrlOrNull()?.toString() ?: urlSpan.url |
| val spans = autoLinkExtractor.extractLinks(text) | ||
|
|
||
| // iterate detected link and keep only those that represent real links | ||
| for (s in spans) { | ||
| if (s !is LinkSpan) continue | ||
|
|
||
| // This is the exact range autolink detected | ||
| val start = s.beginIndex | ||
| val end = s.endIndex | ||
| val raw = text.substring(start, end) | ||
|
|
||
| val url = when (s.type) { |
There was a problem hiding this comment.
The s !is LinkSpan check appears redundant given extractLinks(...) yields LinkSpan items (and the current variable name spans is ambiguous next to Android spans). This can be simplified by iterating as for (link in ...) and removing the type-check, which makes the logic easier to follow.
| val spans = autoLinkExtractor.extractLinks(text) | |
| // iterate detected link and keep only those that represent real links | |
| for (s in spans) { | |
| if (s !is LinkSpan) continue | |
| // This is the exact range autolink detected | |
| val start = s.beginIndex | |
| val end = s.endIndex | |
| val raw = text.substring(start, end) | |
| val url = when (s.type) { | |
| val links = autoLinkExtractor.extractLinks(text) | |
| // iterate detected links and keep only those that represent real links | |
| for (link in links) { | |
| // This is the exact range autolink detected | |
| val start = link.beginIndex | |
| val end = link.endIndex | |
| val raw = text.substring(start, end) | |
| val url = when (link.type) { |
| val text = toString() | ||
| val spans = autoLinkExtractor.extractLinks(text) | ||
|
|
||
| // iterate detected link and keep only those that represent real links |
There was a problem hiding this comment.
Minor grammar issue in the comment: consider changing to "iterate detected links" (plural) for clarity.
| // iterate detected link and keep only those that represent real links | |
| // iterate detected links and keep only those that represent real links |
3 participants
This pr updates the logic for trimming spannable URL. This happens because Linkify does not recognize ")" as a potential break point for the URL.