Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix mentions using matrix.to rather than client defined permalink base url #5544

Merged
merged 11 commits into from
Mar 15, 2022
Merged

Fix mentions using matrix.to rather than client defined permalink base url #5544

merged 11 commits into from
Mar 15, 2022

Conversation

aringenbach
Copy link
Contributor

@aringenbach aringenbach commented Mar 15, 2022 •
edited
Loading

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Mentions permalinks are now based on the client defined permalink base url if available rather than using matrix.to (default).

Motivation and context

Fixes #5521

Tests

  • Step 1: Define clientPermalinkBaseUrl in MatrixConfiguration (don't forget to add it to permalink_supported_hosts as well)
  • Step 2: Test communication with another client with the same configuration and the usability of created permalinks (ideally with iOS and/or Web)

Tested devices

  • Physical
  • Emulator
  • OS version(s): 12

Checklist

@aringenbach aringenbach requested review from a team and ariskotsomitopoulos and removed request for a team March 15, 2022 13:55
ariskotsomitopoulos
ariskotsomitopoulos approved these changes Mar 15, 2022
View reviewed changes
Copy link
Contributor

@ariskotsomitopoulos ariskotsomitopoulos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems fine to me, I added some comments

...android/src/main/java/org/matrix/android/sdk/internal/session/permalinks/PermalinkFactory.kt Outdated
@@ -105,6 +105,28 @@ internal class PermalinkFactory @Inject constructor(
?.substringBeforeLast("?")
}

fun createHtmlMentionSpanTemplate(forceMatrixTo: Boolean): String {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would combine createMdMentionSpanTemplate and createHtmlMentionSpanTemplate and pass MENTION_SPAN_TO_HTML_TEMPLATE_BEGIN or MENTION_SPAN_TO_MD_TEMPLATE_BEGIN as parameter to avoid code duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. I actually made an enum at the interface level so it's not bind to what's in the factory :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@github-actions
Copy link

github-actions bot commented Mar 15, 2022 •
edited
Loading

Unit Test Results

  98 files  ±0    98 suites  ±0   1m 14s ⏱️ -5s
180 tests +1  180 ✔️ +1  0 💤 ±0  0 ❌ ±0 
590 runs  +2  590 ✔️ +2  0 💤 ±0  0 ❌ ±0 

Results for commit 15e3f25. ± Comparison against base commit 1a76914.

♻️ This comment has been updated with latest results.

@aringenbach aringenbach merged commit a7639f4 into develop Mar 15, 2022
@aringenbach aringenbach deleted the aringenbach/5521_permalink_base_url_mention branch March 15, 2022 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ariskotsomitopoulos ariskotsomitopoulos ariskotsomitopoulos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HTML/MD mentions spans are based on matrix.to rather than the client defined permalink base url
2 participants
@aringenbach @ariskotsomitopoulos