Replaces subtitle in Search Rooms with room context rather than last event #5860

ericdecanini · 2022-04-28T09:53:06Z

Drafting for early discussion purposes

Assignees assigned for taking a look at my query below, didn't want to overfill the reviewers section

Type of change

Feature
Bugfix
Technical
Other :

Content

Instead of showing the last event (last message sent, etc.), room list items in the search screen will show room context (parent space, user id, canonical alias, etc)

Motivation and context

Closes #3943

This brings Android into parity with desktop clients.

Getting the space that the room belonged to was not as straightforward as initially expected. Spaces that rooms belonged to often weren't being saved properly into Realm which meant that rooms that belonged to spaces had an empty list of parents. This is why this PR contains updates to RoomSummaryUpdater and related classes

Other changes in this PR include: Centering of the room title when the subtitle is empty

I also took this opportunity to cleanup some commented code

Screenshots / GIFs

Before	After

As you can see above, for space rooms, we show the space or subspace they belong to. For non-space rooms, we show their alias if its available. For dms, we show the user id of the other recipient. Otherwise, we show nothing

Tests

From the home screen, navigate to the search screen (by clicking on the filter icon in the action bar)
Search for rooms in spaces, standalone rooms, dm rooms, dms, and rooms with aliases. See that the subtitle is appropriate for each of them and matches what you find on the desktop clients

Tested devices

Physical
Emulator
OS version(s): Android 10

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

ericdecanini · 2022-04-28T09:58:25Z

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

+                                    childSum.parents.add(SpaceParentSummaryEntity(
+                                            canonical = true,
+                                            parentRoomId = parent.roomId,
+                                            parentSummaryEntity = parent,
+                                            viaServers = RealmList()
+                                    ))


@BillCarsonFr @bmarty @ganfra can you guys give me more context surrounding this please.

I added this block to add the parents of a room (spaces, etc) as they currently weren't being added, but the canonical = true and viaServers = RealmList() are currently placeholders as I don't know what they are or what they should be.

You should remove this part. Parent is only for m.space.parent state event.
We are not adding a parent link because you are a child.

Does a child have no knowledge of its parent then, other than through flattenParentIds? This is confusing because each roomSummary has the field spaceParents which is currently often empty.

Ultimately as well, we would like to be able to know a room's parent in its summary object so we can achieve the same as what web does here (the space of the room is displayed in search)

Yes parent is really when there is a m.space.parent state event in the room (and a valid one, added by an admin in both rooms).
It's not really used right now, it's more to help for discovery, when you join a room from a link the UI could prompt a tip to propose you to join the space.

I think you can use flattenParentsIds for the use case you mentioned.

ericdecanini · 2022-04-28T09:58:50Z

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

+                                            if (childSum.flattenParentIds?.isEmpty() == false) {
+                                                childSum.flattenParentIds += "|"
+                                            }
+                                            childSum.flattenParentIds += it


Fixes a bug where the first element of flattenParentIds would always be ""

Why is it a bug? What was not working?

E.g. if a room had one parent, its roomSummary.flattenParentIds would be {"", "parentRoomId"}

Idk if it has any user-facing impact but I believe this isn't expected behaviour in the code

I am surprised we didn't get report on this, maybe the RoomSummaryMapper is doing some cleaning? None the less I would submit this as a separate PR.

github-actions · 2022-04-28T10:36:42Z

Unit Test Results

124 files +  2 124 suites +2 2m 9s ⏱️ -14s
220 tests +15 220 ✔️ +15 0 💤 ±0 0 ❌ ±0
726 runs +36 726 ✔️ +36 0 💤 ±0 0 ❌ ±0

Results for commit a5dc8ec. ± Comparison against base commit 4309fdb.

This pull request removes 5 and adds 20 tests. Note that renamed tests count towards both.

im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given changing profile picture is not supported, when updating display name, then updates upstream user display name and completes personalization
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given changing profile picture is supported, when updating display name, then updates upstream user display name and moves to choose profile picture
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given personalisation enabled and registration has started and has dummy step to do, when handling action, then ignores other steps and executes dummy
im.vector.app.features.onboarding.ftueauth.FtueMissingRegistrationStagesComparatorTest ‑ when ordering stages, then prioritizes email
org.matrix.android.sdk.internal.session.pushers.DefaultAddPusherTaskTest ‑ given a persisted push entity and SetPush API fails when adding Pusher then mutates persisted result with Failed registration state and rethrows error

im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given changing profile avatar is not supported, when updating display name, then updates upstream user display name and completes personalization
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given changing profile avatar is supported, when updating display name, then updates upstream user display name and moves to choose profile avatar
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given personalisation enabled and registration has started and has dummy step to do, when handling action, then ignores other steps and does dummy
im.vector.app.features.onboarding.RegistrationActionHandlerTest ‑ given adding an email ThreePid fails with 401, when handling register action, then infer EmailSuccess
im.vector.app.features.onboarding.RegistrationActionHandlerTest ‑ given email verification errors with 401 and succeeds, when checking email validation, then continues to poll until success
im.vector.app.features.onboarding.RegistrationActionHandlerTest ‑ given email verification errors with 401 then fatal error, when checking email validation, then continues to poll until non 401 error
im.vector.app.features.onboarding.ftueauth.MatrixOrgRegistrationStagesComparatorTest ‑ when ordering stages, then prioritizes email
org.matrix.android.sdk.internal.session.pushers.DefaultAddPusherTaskTest ‑ given a persisted push entity and SetPush API fails when adding Pusher then mutates persisted result with Failed registration state and rethrows
org.matrix.android.sdk.internal.session.room.aggregation.poll.PollAggregationProcessorTest ‑ given a poll end event for my own poll without enough redaction power level, when processing, then is processed and returns true
org.matrix.android.sdk.internal.session.room.aggregation.poll.PollAggregationProcessorTest ‑ given a poll end event without enough redaction power level, when is processed, then is ignored and return false
…

♻️ This comment has been updated with latest results.

BillCarsonFr · 2022-04-29T12:10:51Z

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

@@ -26,7 +27,7 @@ class RoomSummaryListController(

    override fun buildModels(data: List<RoomSummary>?) {
        data?.forEach {
-            add(roomSummaryItemFactory.create(it, emptyMap(), emptySet(), listener))
+            add(roomSummaryItemFactory.create(it, emptyMap(), emptySet(), RoomListDisplayMode.ROOMS /* TODO: change */, listener))


BillCarsonFr

Thx, please see my comments.

BillCarsonFr · 2022-05-03T09:01:09Z

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

+                    )
+                }
+            }
+            roomSummary.copy(spaceParents = parents)


it doesn't feel right to copy flattenParents into spaceParents (supposed to be the real m.space.parent)

If we don't wanna do this or the approach done earlier, do we have any way of knowing a room's space parent? (when there isn't a space parent event occurring)

I mean you could do like that but maybe create a new field in the RoomSummary model like flattenParents or any name that would mean parent or grand parent, etc.. or childOf. It's just to reflect that it's the consequence of a m.space.childrelation and not of a m.space.parent. You could then have a list of RoomSummary and not ParentInfo that needs a viaServer/canonical info.

Other solution could be to get the roomSummary from the flattenParentsIds in the view model.

bmarty · 2022-05-05T21:07:43Z

It could be better to show the last message when nothing else is displayed. Or at least ensure that the room name is centered vertically in this case.

ericdecanini · 2022-05-06T11:35:15Z

It could be better to show the last message when nothing else is displayed. Or at least ensure that the room name is centered vertically in this case.

The current behaviour is in parity with web and desktop's "New Search". Imo it would be better to get into parity with them and if it's not the behaviour we want we can raise a meta issue to fix it across all platforms.

This would be a design decision at the end of the day. @niquewoodhouse what do you think?

niquewoodhouse · 2022-05-06T15:15:28Z

It could be better to show the last message when nothing else is displayed. Or at least ensure that the room name is centered vertically in this case.

The current behaviour is in parity with web and desktop's "New Search". Imo it would be better to get into parity with them and if it's not the behaviour we want we can raise a meta issue to fix it across all platforms.

This would be a design decision at the end of the day. @niquewoodhouse what do you think?

I'd rather have parity with web too. I think its rare last message would be helpful in this circumstance, but we should be able to find out in testing and feedback

bmarty · 2022-05-09T15:11:01Z

Ok, then can you try to center vertically the room name please?

is quite not good from a design perspective, it looks like a bug: "nothing is displayed on the second line"

We already have such centering mechanism when subtitle is missing for the room aliases for instance:

…ce-search-room-subheader

bmarty

Just blocking on the way you center text in the item_room.xml.
Happy to discuss this more.
Did not review anything else on this PR.

bmarty · 2022-05-16T12:55:00Z

vector/src/main/res/layout/item_room.xml

+        android:layout_height="15dp"
+        android:visibility="gone"
+        app:layout_constraintStart_toStartOf="parent"
+        app:layout_constraintTop_toBottomOf="@id/topMarginSpace" />


weird way to align text vertically. If the font size is changed, it will not work properly.
Is is possible to do better using ConstraintLayout attributes?
Maybe adding a new sublayout if there is no easier solution.

I tried this with changing consraints programatically, but that affects other bindings as you scroll. There's no easy way to 'reset' the view to its original xml state.

Can you explain more what you mean by sublayout?

sublayout: add a new ConstraintLayout to pack the Title and the Subtitle.
I did not check if this is possible, there are other views around.
I will try to play a bit on this layout to see if I find something better.

I think the best way to do this imo is to inflate an entirely different Epoxy item based on whether there is a subtitle or not. What are your thoughts on that?

I will improve that in the future, there is another bug related, with the placeholder.

Thanks. I am removing myself from the reviewer now.

bmarty

(Did not make a full review)

fedrunov · 2022-05-26T13:26:58Z

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

@@ -193,7 +194,7 @@ internal class RoomSummaryDataSource @Inject constructor(
        }
        val dataSourceFactory = realmDataSourceFactory.map {
            roomSummaryMapper.map(it)
-        }
+        }.map { it.getWithParents() }


I think it would be cool to have an overload of map in RoomSummaryMapper to have a param to request flattened parent

I think it would be better also. Like that it won't impact perf for those who don't need to load additional things?

Good shout!

BillCarsonFr · 2022-05-27T09:13:57Z

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

+    private fun RoomSummary.getWithParents(): RoomSummary {
+        val parents = flattenParentIds.mapNotNull { parentId ->
+            getRoomSummary(parentId)?.let { parentSummary ->
+                SpaceParentInfo(


Why not just return the RoomSummaries here instead of fake parentInfo with fake/wrong canonical & viaservers?

Idk why I didn't do this to start with lmao

github-actions · 2022-05-30T09:43:03Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
[org.matrix.android.sdk.account]
= passed=3 failures=0 errors=0 skipped=2
[org.matrix.android.sdk.internal]
= passed=21 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

ericdecanini added 7 commits April 26, 2022 15:48

Replaces izPublic with isPublic

0250f61

Replaces else cases in when branches to RoomListDisplayMode.FILTERED

a3367d4

Adds space name to rooms in filtered search

9e53e6c

Adds user id and canonical alias to search result subtitles

70cded2

Disables typing indicator in filtered search

87ad35d

Adds canonical named argument to RoomSummaryUpdater

3347560

Adds more named arguments to RoomSummaryUpdater

b280358

ericdecanini commented Apr 28, 2022

View reviewed changes

ericdecanini assigned bmarty, ganfra and BillCarsonFr Apr 28, 2022

ericdecanini added 2 commits April 28, 2022 12:02

Fixes lint error

4784717

Fixes lint error

962e9ab

ericdecanini added 4 commits April 28, 2022 12:37

Fixes lint error

7e415e8

Refactors RoomSummaryItem

7cc79fe

Adds displayMode to RoomSummaryListController

a355b62

Changes IncomingShareController display mode to FILTERED

f70a24d

BillCarsonFr reviewed Apr 29, 2022

View reviewed changes

BillCarsonFr requested changes Apr 29, 2022

View reviewed changes

ericdecanini added 3 commits May 2, 2022 14:11

Replaces method for getting the space parents of rooms

47493fc

Changes ordering of room subtitles used

c9b32fe

Adds changelog file

b46794d

ericdecanini requested review from BillCarsonFr and fedrunov May 2, 2022 12:44

ericdecanini marked this pull request as ready for review May 3, 2022 08:06

BillCarsonFr reviewed May 3, 2022

View reviewed changes

ericdecanini requested a review from BillCarsonFr May 4, 2022 08:30

ericdecanini added 5 commits May 12, 2022 14:36

Merge remote-tracking branch 'origin/develop' into feature/eric/repla…

52c404a

…ce-search-room-subheader

Adds vertical centering of title when no subtitle is present

21fe5a2

Adds flattenParents field to RoomSummary and corresponding mapping

50839c2

Adds centering of items with no subtitles

7c1d1c3

Fixes lint error

83bd9bc

bmarty requested changes May 16, 2022

View reviewed changes

Uses second layout to center room summary item title

03acf45

ericdecanini requested a review from bmarty May 17, 2022 16:04

Fixes lint errors

d12ab17

bmarty requested review from bmarty and removed request for bmarty May 23, 2022 10:44

bmarty approved these changes May 23, 2022

View reviewed changes

bmarty removed their assignment May 23, 2022

fedrunov reviewed May 26, 2022

View reviewed changes

BillCarsonFr reviewed May 27, 2022

View reviewed changes

Only gets flattenParents if specifically requested

a5dc8ec

ericdecanini requested review from BillCarsonFr and fedrunov May 27, 2022 13:08

BillCarsonFr approved these changes May 27, 2022

View reviewed changes

fedrunov approved these changes May 30, 2022

View reviewed changes

fedrunov self-requested a review May 30, 2022 08:13

fedrunov approved these changes May 30, 2022

View reviewed changes

ericdecanini merged commit eeb7d60 into develop May 30, 2022

ericdecanini deleted the feature/eric/replace-search-room-subheader branch May 30, 2022 09:13

fedrunov mentioned this pull request Jun 6, 2022

Fix slow paged room list regression #6233

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaces subtitle in Search Rooms with room context rather than last event #5860

Replaces subtitle in Search Rooms with room context rather than last event #5860

ericdecanini commented Apr 28, 2022 •

edited

Loading

ericdecanini Apr 28, 2022

BillCarsonFr Apr 29, 2022

ericdecanini Apr 29, 2022

BillCarsonFr Apr 29, 2022

ericdecanini Apr 28, 2022

BillCarsonFr Apr 29, 2022

ericdecanini Apr 29, 2022

BillCarsonFr Apr 29, 2022

github-actions bot commented Apr 28, 2022 •

edited

Loading

BillCarsonFr Apr 29, 2022

BillCarsonFr left a comment

BillCarsonFr May 3, 2022 •

edited

Loading

ericdecanini May 3, 2022

BillCarsonFr May 6, 2022 •

edited

Loading

bmarty commented May 5, 2022

ericdecanini commented May 6, 2022

niquewoodhouse commented May 6, 2022

bmarty commented May 9, 2022

bmarty left a comment

bmarty May 16, 2022

ericdecanini May 17, 2022

bmarty May 17, 2022

ericdecanini May 17, 2022

bmarty May 17, 2022

ericdecanini May 17, 2022

bmarty May 23, 2022

bmarty left a comment

fedrunov May 26, 2022

BillCarsonFr May 27, 2022

ericdecanini May 27, 2022

BillCarsonFr May 27, 2022

ericdecanini May 27, 2022

BillCarsonFr May 27, 2022

github-actions bot commented May 30, 2022

Replaces subtitle in Search Rooms with room context rather than last event #5860

Replaces subtitle in Search Rooms with room context rather than last event #5860

Conversation

ericdecanini commented Apr 28, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

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

github-actions bot commented Apr 28, 2022 • edited Loading

Unit Test Results

Choose a reason for hiding this comment

BillCarsonFr left a comment

Choose a reason for hiding this comment

BillCarsonFr May 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillCarsonFr May 6, 2022 • edited Loading

Choose a reason for hiding this comment

bmarty commented May 5, 2022

ericdecanini commented May 6, 2022

niquewoodhouse commented May 6, 2022

bmarty commented May 9, 2022

bmarty 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

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

bmarty 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 30, 2022

Matrix SDK

Integration Tests Results:

ericdecanini commented Apr 28, 2022 •

edited

Loading

github-actions bot commented Apr 28, 2022 •

edited

Loading

BillCarsonFr May 3, 2022 •

edited

Loading

BillCarsonFr May 6, 2022 •

edited

Loading