Remove spec v1.3 check for threads #5997
Conversation
There was a problem hiding this comment.
Just a small remark, also a file for towncrier will have to be added.
I let @ariskotsomitopoulos take over this PR.
| @@ -74,8 +74,8 @@ internal fun Versions.isLoginAndRegistrationSupportedBySdk(): Boolean { | |||
| * Indicate if the homeserver support MSC3440 for threads | |||
| */ | |||
| internal fun Versions.doesServerSupportThreads(): Boolean { | |||
| return getMaxVersion() >= HomeServerVersion.v1_3_0 || | |||
| unstableFeatures?.get(FEATURE_THREADS_MSC3440_STABLE) ?: false | |||
| // TODO: Check for v1.3 or whichever spec version formally specifies MSC3440. | |||
There was a problem hiding this comment.
Does this TODO means that it has to be done before merging this PR?
There was a problem hiding this comment.
No, this is a future note so we can find the code site.
|
Also I think the version parser should support version without patch number like "v1.3". The code to update is here: https://github.com/vector-im/element-android/blob/main/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/auth/version/HomeServerVersion.kt#L41 Some unit test could be added too. |
There was a problem hiding this comment.
Thanks for the PR.
We should keep in mind that already existing clients will have a bad experience. For instance:
- Bob is a user with the latest Android app
- Server release v1_3_0 without threads support
- Bob will try to use dynamic threads instead of the local implementation 👎 <-- (edited this line)
- When bob (and if) updates the app, will see the expected user experience
I think the only solution to that problem would be if the server waits for threads to go to v1_3_0 and name the new version in something like v1_2_15
|
@bmarty yes - the versioning will need fixing, but probably left to a different PR to handle. Will get that changelog added. @ariskotsomitopoulos thanks for the review! We're removing the v1.3 check because from the spec side we can't guarantee that the server will have support in v1.3, but all known implementations of threads should be available using the stable flag check and thus be fine. This is more a formality for spec compliance as threads will be released in a future spec version. fwiw, the modern versions (not- |
|
I'm not sure why the tests are failing on this, or what the failure is. Can someone take a look? |
matrix-sdk-android/src/test/java/org/matrix/android/sdk/api/auth/data/VersionsKtTest.kt
Show resolved
Hide resolved
Matrix SDKIntegration Tests Results:
|
Citation: https://matrix.to/#/!ewdjhNcPcEmYNKzlWp:t2l.io/$CkPuvKdFZyFL547JCy5J3MfvLaWUo_a1XEdmiop1PKc?via=matrix.org&via=element.io&via=envs.net
Type of change