#4533: improve headers UI in room/messages lists #5181

mnaturel · 2022-02-08T13:52:02Z

Type of change

Feature
Bugfix
Technical
Other : Enhancement

Content

Improvement for UI of headers sections in the rooms and messages lists. A counter of the list items has been added and the notification badge has been moved to the right.

Motivation and context

Closes #4533

Screenshots / GIFs

New header style :

Tests

Go to the home screen
Select the rooms tab
Check headers are correctly displayed with the correct number of items
Check headers are collapsable/expandable
Check notification counter is correctly displayed
Select the messages tab
Check headers are correctly displayed with the correct number of items
Check headers are collapsable/expandable
Check notification counter is correctly displayed
Open the drawer menu
Switch space
Check the lists are correctly updated
Go to search screen
Check number of items is correct when changing text search

Tested devices

Physical
Emulator
OS version(s): Android 11

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()

mnaturel · 2022-02-08T13:58:17Z

I have achieved most of the required changes. However, without making to many changes, I have some troubles to understand how to listen for query parameters changes to update the item list count to cover missing some use cases:

Search by text
Space switching

github-actions · 2022-02-08T14:27:58Z

Unit Test Results

  98 files ±0   98 suites ±0 1m 3s ⏱️ -11s
174 tests ±0 174 ✔️ ±0 0 💤 ±0 0 ❌ ±0
568 runs ±0 568 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 8bb0a5c. ± Comparison against base commit acfeb7f.

♻️ This comment has been updated with latest results.

github-actions · 2022-02-08T15:16:45Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
passed=
[org.matrix.android.sdk.account]
passed=
[org.matrix.android.sdk.internal]
passed=
[org.matrix.android.sdk.ordering]
passed=
[org.matrix.android.sdk.PermalinkParserTest]
passed=

ouchadam · 2022-02-08T15:49:32Z

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

+                    .withRealm { realm -> roomSummariesQuery(realm, queryParams).findAllAsync() }
+                    .toFlow()
+                    // need to create the flow on a context dispatcher with a thread with attached Looper
+                    .flowOn(coroutineDispatchers.main)


this is a little bit strange, we need to switch to the main thread in order to query the count?

we have some other flow based pipelines we don't need to do the switch https://github.com/vector-im/element-android/blob/develop/matrix-sdk-android-flow/src/main/java/org/matrix/android/sdk/flow/FlowSession.kt 🤔

Yes, I see why it could be strange but this is the only solution I found to make it work. I am using the toFlow() extension of RealmResult and I didn't find any existing usage in the project. I have seen there is an issue about the documentation of this extension meaning the thread we collect the flow should be the same as the one used for creating the result...

Do you have any idea of how to avoid Dispatchers.Main ?

having read a little more into this, it's not as troublesome as I originally thought, realm's async is keeping us off of the main thread but it needs to be triggered from a thread with a looper

initially I was thought this might have pushed us into doing the database query on the main thread

ouchadam · 2022-02-10T09:32:50Z

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

        holder.titleView.text = title
+        with(holder.counterView) {
+            text = ""


does this reset the counter amount?

Here it does not display the counter at all. For what I understood of the code, this item may be used in a screen to add a room but I didn't succeed to find where in the app. I didn't reach creation of this item in debug mode. So, I just supposed we don't need a counter in this use case. Do you know how to open the screen which uses this item ?

from what I can tell RoomCategoryItem/RoomCategoryItem_ is only used by the AddRoomListController which is used by the SpaceAddRoomFragment

which I believe is this screen...

to get here I did, menu drawer -> add space -> private, just me, this screen should also be accessible by editing an existing space to add/remove rooms

if we don't want to have a counter here, it could be nice to extract a constant for the empty copy with a name like

const val NO_COUNTER = ""

what do you think?

I will try to add the counter in this screen as well to have consistent UI.

bmarty

Some few remarks, but globally looking good to me!

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

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

bmarty · 2022-02-17T14:26:21Z

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

-            val expandedArrowDrawableRes = if (roomsSectionData.isExpanded) R.drawable.ic_expand_more else R.drawable.ic_expand_less
-            val expandedArrowDrawable = ContextCompat.getDrawable(binding.root.context, expandedArrowDrawableRes)?.also {
-                DrawableCompat.setTint(it, tintColor)
+            with(binding) {


I am not a big fan of with (and apply for the same reason). It ends up with several this in the block and this is a source of bugs, and not good for the readability, especially on GitHub). We can discuss this later.

On my side, I use a lot operators like with, apply, run or let when there are at least 2 operators applied to a variable. But I reverted back the usage here. I understand your point of view since it changes the scope, you must pay attention what you do inside.

mnaturel · 2022-03-14T11:40:49Z

@ouchadam I have fixed all comments and rebase the branch on develop. Can you take a look again if everything is okay to be merged?

ouchadam

thanks for updating, LGTM 💯 feel free to merge once the CI is happy!

opusforlife2 · 2022-03-14T13:50:46Z

Also, sections should only appear here if they have items in them.

I can't tell from the PR description. Was this addressed?

mnaturel · 2022-03-14T15:28:55Z

Also, sections should only appear here if they have items in them.

I can't tell from the PR description. Was this addressed?

This part was in fact already implemented before this PR. There is a field named hidden in the SectionHeaderAdapter class. This field is set to true if there are no items corresponding to the header.

opusforlife2 · 2022-03-14T18:00:48Z

Oh wait. That was the wrong question, sorry. What I want to know is, if Direct Messages or Rooms are the only sections visible, does the section header disappear too? Since there is only one section, there is no need for the header, after all.

mnaturel · 2022-03-15T08:45:48Z

Oh wait. That was the wrong question, sorry. What I want to know is, if Direct Messages or Rooms are the only sections visible, does the section header disappear too? Since there is only one section, there is no need for the header, after all.

Okay I see. When there is only one visible section, we decided to show the header. It is necessary to keep it since it includes the notifications counter and the current number of items as well.

opusforlife2 · 2022-03-15T17:34:05Z

I see. This was the main reason I opened #4533:

It is annoyingly frequent that I accidentally tap the header and hide the chat list when I'm intending to open the top chat.

So, my issue isn't actually solved by this PR. 😅

mnaturel · 2022-03-28T09:48:36Z

I see. This was the main reason I opened #4533:

It is annoyingly frequent that I accidentally tap the header and hide the chat list when I'm intending to open the top chat.

So, my issue isn't actually solved by this PR. 😅

Hello, sorry for the late reply. I see your point. The height of the headers has been increased. Maybe now it is better to avoid this scenario? If it does not fix this problem, I do not have a solution right now for that. It will need to rethink the UI and for all platforms to be consistent. CC: @daniellekirkwood

opusforlife2 · 2022-04-01T16:45:56Z

Actually, the description of the merged PR #5580 seems to imply that the change I'm seeking is already implemented. There are no screenshots, though, so I'm not sure. Could you please check?

mnaturel · 2022-04-04T07:56:16Z

The actual changes is in fact in #5616. When there is a single section, it is not collapsable. Is was released in the v1.4.7. It looks like the capture below:

opusforlife2 · 2022-04-05T17:52:59Z

Alright! That does indeed solve my problem. Thanks for confirming!

Maybe #4533 should be linked to that PR instead, for posterity?

ouchadam reviewed Feb 8, 2022

View reviewed changes

mnaturel force-pushed the feature/mna/4533-dm-rooms-headers branch from 0419054 to 31b30a6 Compare February 9, 2022 12:29

mnaturel marked this pull request as ready for review February 9, 2022 13:14

mnaturel added the Z-WTF WTF moment: High Impact, Low Effort label Feb 9, 2022

mnaturel mentioned this pull request Feb 9, 2022

Update Direct Messages and Rooms headers from UI #4533

Closed

amshakal approved these changes Feb 9, 2022

View reviewed changes

ouchadam reviewed Feb 10, 2022

View reviewed changes

mnaturel force-pushed the feature/mna/4533-dm-rooms-headers branch 2 times, most recently from b0eee1d to 4ecda36 Compare February 16, 2022 10:01

bmarty reviewed Feb 17, 2022

View reviewed changes

mnaturel force-pushed the feature/mna/4533-dm-rooms-headers branch 2 times, most recently from 66e07b4 to a5b7ad3 Compare February 23, 2022 09:10

mnaturel changed the title ~~Issue 4533: improve headers UI in room/messages lists~~ #4533: improve headers UI in room/messages lists Mar 1, 2022

Maxime Naturel added 14 commits March 14, 2022 11:15

Adding changelog entry

134d7b2

Changing style of header

70481e3

Adding number of items (UI part)

b1d1090

Fix remove of imports

eb38c9d

(DRAFT) Room counter flow

c7dae34

Using flow to show items counter

0aaa650

Moving notification badge on the right side

53c24d2

Setting item count text only when > 0

ad9d36e

Adding TODO

d534516

Removing unused imports

b72c357

Fix switching space use case

c79aa26

Fix filtering use case

3d27d9d

Updating remaining category item

30c6518

Fix coding style

20749e0

Maxime Naturel added 5 commits March 14, 2022 11:15

Adding item counter in add existing room to space screen

f327eaa

Fix some doc comment

291d7d7

Renaming observe item count method

a826a50

Removing some with() code pattern

0942c6d

Fixing wrong remove of import

8bb0a5c

mnaturel force-pushed the feature/mna/4533-dm-rooms-headers branch from a5b7ad3 to 8bb0a5c Compare March 14, 2022 11:30

ouchadam approved these changes Mar 14, 2022

View reviewed changes

mnaturel merged commit 4939a98 into develop Mar 14, 2022

mnaturel deleted the feature/mna/4533-dm-rooms-headers branch March 14, 2022 12:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4533: improve headers UI in room/messages lists #5181

#4533: improve headers UI in room/messages lists #5181

mnaturel commented Feb 8, 2022 •

edited

Loading

mnaturel commented Feb 8, 2022

github-actions bot commented Feb 8, 2022 •

edited

Loading

github-actions bot commented Feb 8, 2022 •

edited

Loading

ouchadam Feb 8, 2022

mnaturel Feb 9, 2022 •

edited

Loading

ouchadam Feb 9, 2022

ouchadam Feb 10, 2022

mnaturel Feb 10, 2022

ouchadam Feb 10, 2022 •

edited

Loading

ouchadam Feb 10, 2022 •

edited

Loading

mnaturel Feb 10, 2022

bmarty left a comment

bmarty Feb 17, 2022

mnaturel Feb 18, 2022

mnaturel commented Mar 14, 2022

ouchadam left a comment •

edited

Loading

opusforlife2 commented Mar 14, 2022

mnaturel commented Mar 14, 2022

opusforlife2 commented Mar 14, 2022

mnaturel commented Mar 15, 2022

opusforlife2 commented Mar 15, 2022

mnaturel commented Mar 28, 2022

opusforlife2 commented Apr 1, 2022

mnaturel commented Apr 4, 2022

opusforlife2 commented Apr 5, 2022

#4533: improve headers UI in room/messages lists #5181

#4533: improve headers UI in room/messages lists #5181

Conversation

mnaturel commented Feb 8, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

mnaturel commented Feb 8, 2022

github-actions bot commented Feb 8, 2022 • edited Loading

Unit Test Results

github-actions bot commented Feb 8, 2022 • edited Loading

Matrix SDK

Integration Tests Results:

ouchadam Feb 8, 2022

Choose a reason for hiding this comment

mnaturel Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

ouchadam Feb 9, 2022

Choose a reason for hiding this comment

ouchadam Feb 10, 2022

Choose a reason for hiding this comment

mnaturel Feb 10, 2022

Choose a reason for hiding this comment

ouchadam Feb 10, 2022 • edited Loading

Choose a reason for hiding this comment

ouchadam Feb 10, 2022 • edited Loading

Choose a reason for hiding this comment

mnaturel Feb 10, 2022

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

bmarty Feb 17, 2022

Choose a reason for hiding this comment

mnaturel Feb 18, 2022

Choose a reason for hiding this comment

mnaturel commented Mar 14, 2022

ouchadam left a comment • edited Loading

Choose a reason for hiding this comment

opusforlife2 commented Mar 14, 2022

mnaturel commented Mar 14, 2022

opusforlife2 commented Mar 14, 2022

mnaturel commented Mar 15, 2022

opusforlife2 commented Mar 15, 2022

mnaturel commented Mar 28, 2022

opusforlife2 commented Apr 1, 2022

mnaturel commented Apr 4, 2022

opusforlife2 commented Apr 5, 2022

mnaturel commented Feb 8, 2022 •

edited

Loading

github-actions bot commented Feb 8, 2022 •

edited

Loading

github-actions bot commented Feb 8, 2022 •

edited

Loading

mnaturel Feb 9, 2022 •

edited

Loading

ouchadam Feb 10, 2022 •

edited

Loading

ouchadam Feb 10, 2022 •

edited

Loading

ouchadam left a comment •

edited

Loading