the base domain permalinks don't have the mxid in the first param but… #3735
Conversation
… in the second after /user/mxid
|
should solve #3454 |
| @@ -53,7 +53,12 @@ object PermalinkParser { | |||
| .filter { it.isNotEmpty() } | |||
| .take(2) | |||
|
|
|||
| val identifier = params.getOrNull(0) | |||
| // the base domain permalinks don't have the mxid in the first param but in the second after /user/mxid | |||
There was a problem hiding this comment.
Can you provide an example of such URL? Maybe in the doc of this method.
There was a problem hiding this comment.
added an example. Thx for reviewing
|
I added the bugfix file, thanks for reviewing @bmarty |
| */ | ||
| fun parse(uriString: String): PermalinkData { | ||
| val uri = Uri.parse(uriString) | ||
| return parse(uri) | ||
| } | ||
|
|
||
| /** | ||
| * Turns an uri to a [PermalinkData] | ||
| * Turns a uri to a [PermalinkData] | ||
| */ | ||
| fun parse(uri: Uri): PermalinkData { | ||
| if (!uri.toString().startsWith(PermalinkService.MATRIX_TO_URL_BASE)) { |
There was a problem hiding this comment.
You won't get to your change, uri has to start by https://matrix.to/#/
There was a problem hiding this comment.
I changed the MATRIX_TO_URL_BASE variable on my fork to be the element-based domain permalink, that's why for me it worked, I think this is easier to find this issue than to find a hard coded array, I could add this to a comment but it will of course be better to make a variable out of this, I don't think I can get to doing that easily but if anyone else wants to pick this PR and do it before I do, please do... I understand if you don't want to merge like this, I just wanted others to not spend a day looking for this issue 🙈 on our fork it's working now... @bmarty
There was a problem hiding this comment.
Ah, we build matrix.to links, so it's OK I think...
There was a problem hiding this comment.
For you it's fine but other forks use element-based domain permalinks 😬 @bmarty
There was a problem hiding this comment.
Ok, thanks for the explaination. We definitely need to improve this code, and add unit test.
Also I guess you will have the same problem for room or group URL, for instance:
https://develop.element.io/#/room/#element-android:matrix.org
There was a problem hiding this comment.
yep thanks, we don't have room links but thx for the heads up 😉
| @@ -53,7 +55,12 @@ object PermalinkParser { | |||
| .filter { it.isNotEmpty() } | |||
| .take(2) | |||
|
|
|||
| val identifier = params.getOrNull(0) | |||
| // the the element-based domain permalinks (e.g. https://app.element.io/#/user/@chagai95:matrix.org) don't have the mxid in the first param (like matrix.to does - https://matrix.to/#/@chagai95:matrix.org) but rather in the second after /user/ so /user/mxid | |||
There was a problem hiding this comment.
Thanks for the comment. There is a double the but it's not a big deal.
| if (identifier.equals("user")) { | ||
| identifier = params.getOrNull(1) | ||
| } | ||
|
|
||
| val extraParameter = params.getOrNull(1) |
There was a problem hiding this comment.
For a domain room url then it will have to become params.getOrNull(2)
|
Do I need to do anything to pass the code quality checks? @bmarty |
|
So I reset to the last commit we spoke about, is there anything missing still? @bmarty |
… in the second after /user/mxid
Pull Request Checklist
Signed-off-by: Chagai Friedlander [email protected]