Try avoiding trailing punctuation inside linkified URLs #4214

jmartinesp · 2025-01-29T10:02:53Z

Content

When an URL is linkified automatically by LinkifyCompat, try removing any trailing punctuation and other mismatched symbols from the link span.

This solution is not great and might fail in some cases, but it should cover the most common ones, since it seems like a 'smart' linkification library is hard to find as a replacement.

Motivation and context

It turns out the Linkify component from Google isn't as smart as we thought: as soon as a / char is found inside the URL, it'll greedily fetch as much text as possible. This is probably because a different regex is used internally than for a simple domain.

Should fix #4208.

Screenshots / GIFs

Before	After

Tests

You can play with the ClickableLinkTextWithParenthesisPreview composable preview, or send messages with URLs and problematic formats to be linkified.

Tested devices

Physical
Emulator
OS version(s): 14

Checklist

Changes have been tested on an Android device or Android emulator with API 24
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request title will be used in the release note, it clearly define what will change for the user
Pull request includes screenshots or videos if containing UI changes
You've made a self review of your PR

github-actions · 2025-01-29T10:13:26Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/VcdaPo

ganfra · 2025-01-29T10:13:57Z

...em/src/main/kotlin/io/element/android/libraries/designsystem/components/ClickableLinkText.kt

+@Composable
+internal fun ClickableLinkTextWithParenthesisPreview() = ElementThemedPreview {
+    ClickableLinkText(
+        annotatedString = AnnotatedString("Does this work (url: github.com/element-hq/element-x-android/README?)?!.", ParagraphStyle()),


Maybe add exemple where parenthesis is part of the url?

codecov · 2025-01-29T10:58:12Z

Codecov Report

Attention: Patch coverage is 86.36364% with 6 lines in your changes missing coverage. Please review.

Project coverage is 80.13%. Comparing base (a3732fe) to head (d1fb2fd).
Report is 9 commits behind head on develop.

Files with missing lines	Patch %	Lines
...droid/libraries/androidutils/text/LinkifyHelper.kt	85.00%	1 Missing and 5 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4214      +/-   ##
===========================================
- Coverage    80.13%   80.13%   -0.01%     
===========================================
  Files         2052     2053       +1     
  Lines        54576    54601      +25     
  Branches      6672     6683      +11     
===========================================
+ Hits         43733    43752      +19     
- Misses        8560     8561       +1     
- Partials      2283     2288       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bmarty

Thanks, I have tested and the target link need to be updated as well.

bmarty · 2025-01-29T15:39:54Z

...em/src/main/kotlin/io/element/android/libraries/designsystem/components/ClickableLinkText.kt

+                    end = adjustEndIndex(spannable, start, end)
+                }.onFailure {
+                     Timber.e(it, "Failed to adjust end index for link span")
+                }


This is adjusting the end of the span, so this is fixing the rendering issue, but it does this affect the URL itself, i.e. the target of the link.

I think the line annotation = span.url, below needs to be updated.

It's maybe because it did not work with the example I tried:

Does the Event renderer use ClickableLinkText? It seems it's not the case 😅

Ahhh damn, it used to use it, that's why I tried fixing it here. I'll move the code where it belongs 🥲 .

jmartinesp · 2025-01-31T08:35:36Z

This should be ready to review again: it's now affecting the intended texts (🤦 ) and has some extra unit tests for the new component.

bmarty

Thanks and sorry for the late review. Good to be merged, I believe that I wrote only non-blocking remarks.

bmarty · 2025-02-19T09:04:45Z

...android/features/messages/impl/timeline/factories/event/TimelineItemContentMessageFactory.kt

-        return spannable
-    }
+private fun CharSequence.withFixedURLSpans(): CharSequence {
+    return LinkifyHelper.linkify(this, Linkify.WEB_URLS or Linkify.PHONE_NUMBERS or Linkify.EMAIL_ADDRESSES)


Maybe remove the second param, since it's the default value.
Also maybe move this extension to the file LinkifyHelper and rename it to safeLinkify(), cleverLinkify(), betterLinkify() or something better (can't find something good).

bmarty · 2025-02-19T09:09:15Z

.../io/element/android/features/messages/impl/timeline/components/event/TimelineItemTextView.kt

+@Composable
+internal fun TimelineItemTextViewWithLinkifiedUrlPreview() = ElementPreview {
+    val content = aTimelineItemTextContent(
+        pillifiedBody = LinkifyHelper.linkify("Does this work (url: github.com/element-hq/element-x-android/README?)?.")


Maybe change the text to

Suggested change

pillifiedBody = LinkifyHelper.linkify("Does this work (url: github.com/element-hq/element-x-android/README?)?.")

pillifiedBody = LinkifyHelper.linkify("The link should end after the first '?' (url: github.com/element-hq/element-x-android/README?)?.")

bmarty · 2025-02-19T09:10:44Z

.../io/element/android/features/messages/impl/timeline/components/event/TimelineItemTextView.kt

+@Composable
+internal fun TimelineItemTextViewWithLinkifiedUrlAndNestedParenthesisPreview() = ElementPreview {
+    val content = aTimelineItemTextContent(
+        pillifiedBody = LinkifyHelper.linkify("Does this work ((url: github.com/element-hq/element-x-android/READ(ME)))!")


Maybe change the text to

Suggested change

pillifiedBody = LinkifyHelper.linkify("Does this work ((url: github.com/element-hq/element-x-android/READ(ME)))!")

pillifiedBody = LinkifyHelper.linkify("The link should end after the '(ME)' ((url: github.com/element-hq/element-x-android/READ(ME)))!")

bmarty · 2025-02-19T09:31:32Z

...dutils/src/test/kotlin/io/element/android/libraries/androidutils/text/LinkifierHelperTest.kt

+    fun `linkification finds URL`() {
+        val text = "A url https://matrix.org"
+        val result = LinkifyHelper.linkify(text)
+        val urlSpans = result.toSpannable().getSpans<URLSpan>(start = 0, end = result.length)


All the start = 0, end = result.length in all tests are not necessary, the 2 parameters are the default values.

bmarty · 2025-02-19T09:40:46Z

...androidutils/src/main/kotlin/io/element/android/libraries/androidutils/text/LinkifyHelper.kt

+        // Restore old spans, remove new ones if there is a conflict
+        for ((urlSpan, location) in oldURLSpans) {
+            val (start, end) = location
+            val addedConflictingSpans = spannable.getSpans<URLSpan>(start, end).orEmpty()


.orEmpty() does not seem to be necessary.

For tests, I think the function will return before, at line 29.

bmarty · 2025-02-19T10:28:53Z

...dutils/src/test/kotlin/io/element/android/libraries/androidutils/text/LinkifierHelperTest.kt

+        assertThat(urlSpans.size).isEqualTo(1)
+        assertThat(urlSpans.first().url).isEqualTo("https://github.com/element-hq/element-android/READ(ME)")
+    }
+}


Thanks for all those tests!

Create `LinkfierHelper` and post-process URLSpans added to make sure they honor the actual URLs in text by removing unnecessarily added trailing punctuation.

sonarqubecloud · 2025-02-21T16:45:54Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

jmartinesp added the PR-Bugfix For bug fix label Jan 29, 2025

jmartinesp requested a review from a team as a code owner January 29, 2025 10:02

jmartinesp requested review from bmarty and removed request for a team January 29, 2025 10:02

ganfra reviewed Jan 29, 2025

View reviewed changes

bmarty requested changes Jan 29, 2025

View reviewed changes

jmartinesp force-pushed the fix/jme/avoid-trailing-punctuation-in-linkified-urls branch from 9ef2d3f to 323c811 Compare January 29, 2025 17:03

jmartinesp requested a review from bmarty January 30, 2025 09:28

bmarty approved these changes Feb 19, 2025

View reviewed changes

Try avoiding trailing punctuation inside linkified URLs.

a35a96c

Create `LinkfierHelper` and post-process URLSpans added to make sure they honor the actual URLs in text by removing unnecessarily added trailing punctuation.

jmartinesp force-pushed the fix/jme/avoid-trailing-punctuation-in-linkified-urls branch from 28c8e82 to a35a96c Compare February 21, 2025 15:36

jmartinesp enabled auto-merge (squash) February 21, 2025 15:36

jmartinesp added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Feb 21, 2025

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Feb 21, 2025

ElementBot and others added 2 commits February 21, 2025 16:23

Update screenshots

fad396a

Fix lint issues

d1fb2fd

jmartinesp merged commit 5d8403b into develop Feb 21, 2025
26 checks passed

jmartinesp deleted the fix/jme/avoid-trailing-punctuation-in-linkified-urls branch February 21, 2025 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try avoiding trailing punctuation inside linkified URLs #4214

Try avoiding trailing punctuation inside linkified URLs #4214

jmartinesp commented Jan 29, 2025

github-actions bot commented Jan 29, 2025 •

edited

Loading

ganfra Jan 29, 2025

codecov bot commented Jan 29, 2025 •

edited

Loading

bmarty left a comment

bmarty Jan 29, 2025

bmarty Jan 29, 2025

bmarty Jan 29, 2025

jmartinesp Jan 29, 2025

jmartinesp commented Jan 31, 2025

bmarty left a comment

bmarty Feb 19, 2025

bmarty Feb 19, 2025

bmarty Feb 19, 2025

bmarty Feb 19, 2025

bmarty Feb 19, 2025

bmarty Feb 19, 2025

sonarqubecloud bot commented Feb 21, 2025

	pillifiedBody = LinkifyHelper.linkify("Does this work (url: github.com/element-hq/element-x-android/README?)?.")
	pillifiedBody = LinkifyHelper.linkify("The link should end after the first '?' (url: github.com/element-hq/element-x-android/README?)?.")

Try avoiding trailing punctuation inside linkified URLs #4214

Try avoiding trailing punctuation inside linkified URLs #4214

Conversation

jmartinesp commented Jan 29, 2025

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jan 29, 2025 • edited Loading

Codecov Report

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp commented Jan 31, 2025

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 21, 2025

Quality Gate passed

github-actions bot commented Jan 29, 2025 •

edited

Loading

codecov bot commented Jan 29, 2025 •

edited

Loading