Cleanup room summary query params #6143

bmarty · 2022-05-24T16:08:59Z

Type of change

Feature
Bugfix
Technical
Other :

Content

No functional change on this PR.

Mainly work on RoomSummaryQueryParams, to try to match what we will have on the Rust SDK.

Changes:

Remove RoomSummaryQueryParams.roomId. If you need to observe a single room, use the new API RoomService.getRoomSummaryLive(roomId: String)
ActiveSpaceFilter has been renamed to SpaceFilter
RoomCategoryFilter.ALL has been removed, just pass null to not filter on Room category.
Cleanup code, especially remove duplicated line of code.
Add some Kdoc

Remaining work:

RoomSummaryQueryParams.canonicalAlias` could be removed and replaced by something else. This is not available in the Rust API, or at least on the sliding sync API. See https://github.com/matrix-org/matrix-spec-proposals/blob/02e3706f135472f0b23429ef0258ccf45e781c1f/proposals/3575-sync.md?plain=1#L254

Motivation and context

See content ^

Screenshots / GIFs

Tests

Tests are in progress

Test Space filtering: OK
Join a room after a preview, to check the new RoomService.getRoomSummaryLive(roomId: String): OK
Run the sanityTest: OK

Tested devices

Physical
Emulator
OS version(s):

github-actions · 2022-05-24T17:19:09Z

Unit Test Results

124 files 124 suites 2m 32s ⏱️
220 tests 220 ✔️ 0 💤 0 ❌
726 runs 726 ✔️ 0 💤 0 ❌

Results for commit ec498cf.

♻️ This comment has been updated with latest results.

bmarty · 2022-05-25T08:41:12Z

vector/src/main/java/im/vector/app/features/home/HomeDetailViewModel.kt

                                        }
                                ).size

                                roomsInvite = session.roomService().getRoomSummaries(
                                        roomSummaryQueryParams {
                                            memberships = listOf(Membership.INVITE)
                                            roomCategoryFilter = RoomCategoryFilter.ONLY_ROOMS
-                                            activeSpaceFilter = ActiveSpaceFilter.ActiveSpace(groupingMethod.spaceSummary?.roomId)
+                                            spaceFilter = groupingMethod.spaceSummary?.roomId?.let { SpaceFilter.ActiveSpace(it) } ?: SpaceFilter.OrphanRooms


This code is equivalent (I think) to the previous code, hence, I am not sure why we would like to have OrphanRooms here. I am tempted to remove the fallback and pass null instead. WDYT @BillCarsonFr ?
Applicable to all the equivalent change in this file (and in other files too).

could reduce some of the noise by extracting a fun GroupingMethod.activeSpaceOrNull() / orOphans extension

Good idea, will do.

Done c9a292c

bmarty · 2022-05-25T08:42:34Z

vector/src/main/java/im/vector/app/features/home/room/list/RoomListSectionBuilderSpace.kt

                    )
                } else {
                    copy(
-                            activeSpaceFilter = ActiveSpaceFilter.ActiveSpace(currentSpace)
+                            spaceFilter = SpaceFilter.ActiveSpace(currentSpace)


I was tempted to rewrite to something like
spaceFilter = currentSpace?.let{ SpaceFilter.ActiveSpace(it) }
But I think this is better to keep it like that for code clarity.

Imo it's still very clear and it would be nice to be consistent with the change above in line 439. There would be a slight difference in functionality though. What happens if we get SpaceFilter.ActiveSpace(null)?

Using null in SpaceFilter.ActiveSpace(null) is not possible anymore.

bmarty · 2022-05-25T08:43:00Z

vector/src/main/java/im/vector/app/features/roomdirectory/roompreview/RoomPreviewViewModel.kt

-                        it.membership == Membership.JOIN
-                    }
-                    list.firstOrNull { it.roomId == initialState.roomId }?.roomType?.let {
+                .liveRoomSummary(initialState.roomId)


Calling a new API

bmarty · 2022-05-25T08:48:25Z

.../src/main/java/org/matrix/android/sdk/internal/session/room/summary/RoomSummaryDataSource.kt

-                    // nop
-                }
-            }
-        }


Removed because the same code is at line 302 below. Line 287 in the new version of the file.

bmarty · 2022-05-25T08:49:25Z

.../src/main/java/org/matrix/android/sdk/internal/session/room/summary/RoomSummaryDataSource.kt

-                            it.process(RoomSummaryEntityFields.DISPLAY_NAME, queryParams.displayName)
-                        }
-                    }
+                    .process(RoomSummaryEntityFields.ROOM_ID, QueryStringValue.IsNotEmpty)


I keep this check, even if I think we cannot have a room in DB with an empty Id.
This PR is not supposed to bring functional changes.

bmarty · 2022-05-25T08:50:26Z

vector/src/main/java/im/vector/app/features/spaces/manage/SpaceAddRoomsViewModel.kt

@@ -168,9 +168,6 @@ class SpaceAddRoomsViewModel @AssistedInject constructor(
    override fun handle(action: SpaceAddRoomActions) {
        when (action) {
            is SpaceAddRoomActions.UpdateFilter    -> {
-                roomUpdatableLivePageResult.queryParams = roomUpdatableLivePageResult.queryParams.copy(
-                        displayName = QueryStringValue.Contains(action.filter, QueryStringValue.Case.INSENSITIVE)
-                )


Removed because the lines just below are the same.

sonarcloud · 2022-05-25T09:10:16Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

ouchadam · 2022-05-25T09:43:32Z

...-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/RoomSummaryQueryParams.kt

        var canonicalAlias: QueryStringValue = QueryStringValue.NoCondition
        var memberships: List<Membership> = Membership.all()
-        var roomCategoryFilter: RoomCategoryFilter? = RoomCategoryFilter.ALL


I preferred the explicitness of the ALL but null is more consistent with the project conventions

Yes, I also prefer using non-nullability. For this specific API, it's more consistent to use null.

ouchadam · 2022-05-25T09:44:11Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/model/RoomSummary.kt

@@ -28,65 +28,196 @@ import org.matrix.android.sdk.api.session.room.timeline.TimelineEvent
 * It can be retrieved by [org.matrix.android.sdk.api.session.room.Room] and [org.matrix.android.sdk.api.session.room.RoomService]
 */
 data class RoomSummary(


ouchadam · 2022-05-25T10:27:19Z

.../src/main/java/org/matrix/android/sdk/internal/session/room/summary/RoomSummaryDataSource.kt

@@ -333,6 +314,14 @@ internal class RoomSummaryDataSource @Inject constructor(
        return query
    }

+    private fun QueryStringValue.toDisplayNameField(): String {


👍 for extracting

ouchadam · 2022-05-25T10:48:42Z

...sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/DefaultRoomService.kt

+            })
+        } else {
+            listOfNotNull(
+                    getRoomSummary(roomId)?.takeIf { it.membership == Membership.JOIN }


is there a difference between listOf(getRoomSummary()) and getRoomSummaries { this.roomId = QueryStringValue.Equals(roomId) } ?

I'm wondering if all the roomSummaries fetching should be taking into account roomIdOrAlias

RoomSummaryQueryParams.roomId has been removed, so cannot be used anymore.
roomIdOrAlias can be used, but I think for this particular case roomId is enough.

ericdecanini

Looks great overall! Just a couple comments

ericdecanini · 2022-05-25T15:47:09Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/RoomService.kt

+     * A live [RoomSummary] associated with the room with id [roomId].
+     * You can observe this summary to get dynamic data from this room, even if the room is not joined yet
+     */
+    fun getRoomSummaryLive(roomId: String): LiveData<Optional<RoomSummary>>


It feels a bit weird that this uses Optional and the non-live version uses a nullable. Is it possible to use the same for both?

LiveData cannot emit null value IIRC.

ericdecanini · 2022-05-25T15:52:41Z

vector/src/main/java/im/vector/app/features/home/room/list/RoomListSectionBuilderSpace.kt

                    )
                } else {
                    copy(
-                            activeSpaceFilter = ActiveSpaceFilter.ActiveSpace(currentSpace)
+                            spaceFilter = SpaceFilter.ActiveSpace(currentSpace)


Imo it's still very clear and it would be nice to be consistent with the change above in line 439. There would be a slight difference in functionality though. What happens if we get SpaceFilter.ActiveSpace(null)?

ericdecanini

LGTM

…ueryParams.Builder()` Also format and add some Kdoc

Create a new API in RoomService to observe a room summary from a roomId.

It was working fine since in the DB we always have a name using `RoomDisplayNameFallbackProvider`, which in our current implementation always return a non empty String.

Prefer nullability for API coherency of `RoomSummaryQueryParams`

…ter)

Prefer nullability for API coherency of `RoomSummaryQueryParams`

Not 100% of the side effect. There is probably some (fixed?) bugs here.

github-actions · 2022-05-30T10:50:26Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
= passed=20 failures=0 errors=0 skipped=3
[org.matrix.android.sdk.account]
= passed=3 failures=0 errors=0 skipped=2
[org.matrix.android.sdk.internal]
= passed=20 failures=1 errors=0 skipped=1
[org.matrix.android.sdk.ordering]
= passed=16 failures=0 errors=0 skipped=0
[org.matrix.android.sdk.PermalinkParserTest]
= passed=2 failures=0 errors=0 skipped=0

bmarty commented May 25, 2022

View reviewed changes

bmarty requested review from a team, Claire1817 and ericdecanini and removed request for a team May 25, 2022 08:58

bmarty marked this pull request as ready for review May 25, 2022 08:58

ouchadam reviewed May 25, 2022

View reviewed changes

ericdecanini reviewed May 25, 2022

View reviewed changes

ouchadam approved these changes May 30, 2022

View reviewed changes

ericdecanini approved these changes May 30, 2022

View reviewed changes

bmarty added 10 commits May 30, 2022 11:34

Always use roomSummaryQueryParams, and not directly a `RoomSummaryQ…

12b3afb

…ueryParams.Builder()` Also format and add some Kdoc

Always use roomSummaryQueryParams, and not directly a `RoomSummaryQ…

49a29fb

…ueryParams.Builder()` Also format and add some Kdoc

Remove roomId from RoomSummaryQueryParams.Builder().

bfdc885

Create a new API in RoomService to observe a room summary from a roomId.

Remove duplicated code lines

6a728e3

Small rework for nicer code

2b0077b

displayName default value is now QueryStringValue.NoCondition.

8d7a54a

It was working fine since in the DB we always have a name using `RoomDisplayNameFallbackProvider`, which in our current implementation always return a non empty String.

Remove RoomCategoryFilter.ALL

8843fe0

Prefer nullability for API coherency of `RoomSummaryQueryParams`

Remove duplicated lines of code (the same code is done a few lines la…

3442829

…ter)

Add some Kdoc

289f27b

Remove ActiveSpaceFilter.None

0080902

Prefer nullability for API coherency of `RoomSummaryQueryParams`

bmarty added 8 commits May 30, 2022 11:34

Rename ActiveSpaceFilter to SpaceFilter

c7997ed

Create SpaceFilter.OrphanRooms to improve the API.

0b6f35b

Not 100% of the side effect. There is probably some (fixed?) bugs here.

Add some Kdoc

5fbcec0

Add some Kdoc

cffdb7f

Fix test compilation

ad60467

Add changelog

d7a1ae3

Create extension String?.toActiveSpaceOrOrphanRooms() to reduce noise.

c071dc5

Cleaner code

ec498cf

bmarty force-pushed the feature/bma/cleanup_RoomSummaryQueryParams branch from f5194d3 to ec498cf Compare May 30, 2022 09:38

bmarty enabled auto-merge May 30, 2022 09:44

bmarty merged commit 17ccccc into develop May 30, 2022

bmarty deleted the feature/bma/cleanup_RoomSummaryQueryParams branch May 30, 2022 10:22

BillCarsonFr mentioned this pull request Jun 10, 2022

"Show all rooms in Home" doesn't work in 1.4.19 at all #6272

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup room summary query params #6143

Cleanup room summary query params #6143

bmarty commented May 24, 2022 •

edited

Loading

github-actions bot commented May 24, 2022 •

edited

Loading

bmarty May 25, 2022

ouchadam May 25, 2022

bmarty May 25, 2022

bmarty May 30, 2022 •

edited

Loading

bmarty May 25, 2022

ericdecanini May 25, 2022

bmarty May 25, 2022

bmarty May 30, 2022

bmarty May 25, 2022

bmarty May 25, 2022

bmarty May 25, 2022

bmarty May 25, 2022

sonarcloud bot commented May 25, 2022

ouchadam May 25, 2022

bmarty May 30, 2022

ouchadam May 25, 2022

ouchadam May 25, 2022

ouchadam May 25, 2022 •

edited

Loading

bmarty May 30, 2022

ericdecanini left a comment

ericdecanini May 25, 2022

bmarty May 25, 2022

ericdecanini May 25, 2022

ericdecanini left a comment

github-actions bot commented May 30, 2022

Cleanup room summary query params #6143

Cleanup room summary query params #6143

Conversation

bmarty commented May 24, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

github-actions bot commented May 24, 2022 • edited Loading

Unit Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty May 30, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented May 25, 2022

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

ouchadam May 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericdecanini 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

ericdecanini left a comment

Choose a reason for hiding this comment

github-actions bot commented May 30, 2022

Matrix SDK

Integration Tests Results:

bmarty commented May 24, 2022 •

edited

Loading

github-actions bot commented May 24, 2022 •

edited

Loading

bmarty May 30, 2022 •

edited

Loading

ouchadam May 25, 2022 •

edited

Loading