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.

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

Use stable endpoint for alias management #6288

Merged
merged 4 commits into from Jun 30, 2022

Conversation

deepbluev7
Copy link
Contributor

@deepbluev7 deepbluev7 commented Jun 12, 2022

This increases compatibility with homeservers and allows them to remove
Element Android specific workaround.

fixes #4830
see ruma/ruma#936
see matrix-org/synapse#8334
see matrix-org/synapse#9224

Signed-off-by: Nicolas Werner nicolas.werner@hotmail.de

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

It makes Element Android (as the last client still relying on the MSC endpoint) use the stable endpoint to list aliases. Fixes #4830

THIS HAS NOT BEEN TESTED SINCE I DON'T HAVE ANY ELEMENT COMPATIBLE DEVICE AND I AM TOO LAZY TO SET UP A DEVELOPMENT ENVIRONMENT FOR THIS.

Motivation and context

Servers shouldn't have to implement MSCs, that have long been merged, to allow Element Android to run on them. This allows the removal of workarounds (or the unstable endpoints) in several server implementations and makes development of new servers easier.

Screenshots / GIFs

Tests

  • Step 1
  • Step 2
  • Step ...

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

This increases compatibility with homeservers and allows them to remove
Element Android specific workaround.

fixes element-hq#4830
see ruma/ruma#936
see matrix-org/synapse#8334
see matrix-org/synapse#9224

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
@bmarty bmarty added the Z-Community-PR Issue is solved by a community member's PR label Jun 12, 2022
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
We will check that it's working well.
This line has to be modified as well: https://github.com/vector-im/element-android/blob/main/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/network/ApiPath.kt#L165. May I ask you to do it please?

Thanks!

@bmarty bmarty requested review from a team, ouchadam and fedrunov and removed request for a team June 12, 2022 14:18
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
@deepbluev7
Copy link
Contributor Author

Done, thank you for spotting that :3

@deepbluev7
Copy link
Contributor Author

There is also some reference to the MSC here: https://github.com/vector-im/element-android/blob/main/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/alias/GetRoomLocalAliasesTask.kt#L37

I'm not sure if that should be removed or it is a good idea to keep it, since the app might not check for r0.6.1 either?

@ouchadam ouchadam self-assigned this Jun 13, 2022
@bmarty
Copy link
Member

bmarty commented Jun 13, 2022

There is also some reference to the MSC here: https://github.com/vector-im/element-android/blob/main/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/alias/GetRoomLocalAliasesTask.kt#L37

I'm not sure if that should be removed or it is a good idea to keep it, since the app might not check for r0.6.1 either?

Good point. I would vote to simply remove this comment.

@ouchadam
Copy link
Contributor

should the feature be guarded by a homeserver version check?

@deepbluev7
Copy link
Contributor Author

@ouchadam The old one wasn't guarded by a homeserver version check either and this has been in synapse versions for about a year, I think. A homeserver version check in general would be good, but I think on that old servers a lot of other stuff is broken by now as well and I don't really know, where I would start adding such a check .-.

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
@ouchadam
Copy link
Contributor

agreed, it's not a blocker for this PR! thinking out loud, I wonder if we should have minimum supported homeserver version (if we don't already)

@deepbluev7
Copy link
Contributor Author

Does this need anything from me?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use stable version of the room alias endpoints instead of MSC2432
4 participants