Show a room list decoration for notification setting applied #1330

langleyd · 2023-09-14T13:38:22Z

Content

This PR adds a room notification setting decoration to the room list as shown below.
Rebuild room summaries when push rules change.

~~In future we will~~ We only show this decoration for rooms that have a custom setting/exception, not for rooms that are defaulting to the global setting. This is to cut down on noise. A PR on the rust sdk fixes this.

Motivation and context

#547

Screenshots / GIFs

- Add the UI - Rebuild room summaries when push rules change or when user disables notifications(hide them all)

- Only show green indicator for "All Messages" as mentions doesn't work and we should never see it for muted rooms. - Remove code that tried to reflect the notificationsEnabled setting I mis-undertood the requirements by reading the iOS code.

into dla/feature/room_list_decoration

github-actions · 2023-09-14T19:43:43Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/d66XVK

langleyd · 2023-09-14T20:28:13Z

@jmartinesp or @ganfra , mind taking a looking giving me a steer on the right way of doing things here? The code has the right effect but I'm not sure if it's really done correctly.

We need to rebuild the room summaries when the matrix push settings change so that the latest room level notification setting is pulled and reflected on the list with the decoration.

Specifically here I'm observing the changes using the app scope so that when we navigate away from the room list, and the notification settings change (either by us or on another client), the rebuilding of the room summaries is triggered. I'm not quite sure if this is the right approach.

I'm guessing not, all the room list tests are failing as they timeout:

After waiting for 10s, the test coroutine is not completing, there were active child jobs: ["coroutine#12":StandaloneCoroutine{Active}@78f210bf]

ganfra · 2023-09-15T07:18:17Z

Mmmm, I'd expect the rust sdk to handle this for us, so the RoomInfo already have the information. Could this be added?

jmartinesp · 2023-09-15T08:08:27Z

Mmmm, I'd expect the rust sdk to handle this for us, so the RoomInfo already have the information. Could this be added?

Same, to be honest. If we need it for this release I guess we could use this approach, but it's definitely not ideal. In any case, about:

After waiting for 10s, the test coroutine is not completing, there were active child jobs: ["coroutine#12":StandaloneCoroutine{Active}@78f210bf]

You could work around the issue by passing another coroutine scope for appScope in the tests and cancelling it at the end of these tests, I think.

langleyd · 2023-09-15T08:50:17Z

Mmmm, I'd expect the rust sdk to handle this for us, so the RoomInfo already have the information. Could this be added?

@ganfra The the roomInfo does already have the information of the notification setting here. But the callback is from the rust notification settings that informs us the settings have changed. So at the moment we need to pull all the rooms from rust again. This is because If the user changes a global rule, all the rooms could be affected, so I think this is required.

Or am I misunderstanding your suggestion?

ganfra · 2023-09-15T08:53:56Z

Yeah, I was not precise enough, I'd expect the RoomList api to notify us of which room has been affected by the changes

into dla/feature/room_list_decoration

codecov · 2023-09-15T10:27:47Z

Codecov Report

Patch coverage: 66.66% and project coverage change: +0.01% 🎉

Comparison is base (f7ae97f) 57.75% compared to head (c160a37) 57.76%.
Report is 9 commits behind head on develop.

❗ Current head c160a37 differs from pull request most recent head 0d172df. Consider uploading reports for the commit 0d172df to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1330      +/-   ##
===========================================
+ Coverage    57.75%   57.76%   +0.01%     
===========================================
  Files         1120     1120              
  Lines        29719    29765      +46     
  Branches      6102     6113      +11     
===========================================
+ Hits         17163    17193      +30     
- Misses        9873     9887      +14     
- Partials      2683     2685       +2

Files Changed	Coverage Δ
...d/libraries/matrix/api/roomlist/RoomListService.kt	`75.00% <ø> (ø)`
.../matrix/impl/roomlist/RoomSummaryDetailsFactory.kt	`0.00% <0.00%> (ø)`
...s/matrix/impl/roomlist/RoomSummaryListProcessor.kt	`0.00% <0.00%> (ø)`
...raries/matrix/impl/roomlist/RustRoomListService.kt	`0.00% <0.00%> (ø)`
...raries/matrix/test/roomlist/FakeRoomListService.kt	`91.30% <0.00%> (-4.16%)`	⬇️
...eatures/roomlist/impl/components/RoomSummaryRow.kt	`68.85% <78.26%> (+2.18%)`	⬆️
...res/roomlist/impl/datasource/RoomListDataSource.kt	`85.54% <81.81%> (-0.57%)`	⬇️
...eatures/roomlist/impl/model/RoomListRoomSummary.kt	`100.00% <100.00%> (ø)`
...roomlist/impl/model/RoomListRoomSummaryProvider.kt	`100.00% <100.00%> (ø)`
...ment/android/libraries/designsystem/VectorIcons.kt	`100.00% <100.00%> (ø)`
... and 1 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

langleyd · 2023-09-15T10:38:07Z

We're creating an issue to track the work to room list api and will follow up with that.

Look ok otherwise @ganfra @jmartinesp ?

into dla/feature/room_list_decoration

…/vector-im/element-x-android into dla/feature/room_list_decoration

langleyd · 2023-09-15T14:54:16Z

I reverted the change to only show the green dot for rooms set to "All messages" as per advice from product.

sonarqubecloud · 2023-09-15T15:59:48Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

...l/src/main/kotlin/io/element/android/features/roomlist/impl/datasource/RoomListDataSource.kt

.../impl/src/main/kotlin/io/element/android/features/roomlist/impl/model/RoomListRoomSummary.kt

...ist/impl/src/test/kotlin/io/element/android/features/roomlist/impl/RoomListPresenterTests.kt

…/vector-im/element-x-android into dla/feature/room_list_decoration

julioromano

Pre-emptive LGTM to not let this become a release blocker though we ideally need @ganfra 's input about how to best manage the coroutines scope inside RoomListDataSource to avoid potential resource leaks.

…/vector-im/element-x-android into dla/feature/room_list_decoration

into dla/feature/room_list_decoration

ElementBot · 2023-09-18T16:20:33Z

libraries/designsystem/src/main/res/drawable/ic_mention.xml

+    <clip-path
+        android:pathData="M0,0h16v16h-16z"/>
+    <path
+        android:pathData="M8,14.667C7.077,14.667 6.211,14.492 5.4,14.142C4.589,13.792 3.883,13.317 3.283,12.717C2.683,12.117 2.208,11.411 1.858,10.6C1.508,9.789 1.333,8.922 1.333,8C1.333,7.078 1.508,6.211 1.858,5.4C2.208,4.589 2.683,3.883 3.283,3.283C3.883,2.683 4.589,2.208 5.4,1.858C6.211,1.508 7.077,1.333 8,1.333C8.922,1.333 9.789,1.508 10.6,1.858C11.411,2.208 12.116,2.683 12.716,3.283C13.316,3.883 13.791,4.589 14.141,5.4C14.491,6.211 14.666,7.078 14.666,8V9.017C14.666,9.672 14.441,10.228 13.991,10.683C13.541,11.139 12.989,11.367 12.333,11.367C11.944,11.367 11.578,11.286 11.233,11.125C10.889,10.964 10.6,10.733 10.366,10.433C10.055,10.744 9.694,10.981 9.283,11.142C8.872,11.303 8.444,11.383 8,11.383C7.055,11.383 6.255,11.056 5.6,10.4C4.944,9.744 4.616,8.944 4.616,8C4.616,7.055 4.944,6.255 5.6,5.6C6.255,4.944 7.055,4.617 8,4.617C8.944,4.617 9.744,4.944 10.4,5.6C11.055,6.255 11.383,7.055 11.383,8V9.017C11.383,9.272 11.477,9.494 11.666,9.683C11.855,9.872 12.078,9.967 12.333,9.967C12.589,9.967 12.808,9.872 12.991,9.683C13.175,9.494 13.266,9.272 13.266,9.017V8C13.266,6.533 12.755,5.289 11.733,4.267C10.711,3.244 9.466,2.733 8,2.733C6.533,2.733 5.289,3.244 4.266,4.267C3.244,5.289 2.733,6.533 2.733,8C2.733,9.467 3.244,10.711 4.266,11.733C5.289,12.755 6.533,13.267 8,13.267H10.6C10.789,13.267 10.953,13.336 11.091,13.475C11.23,13.614 11.3,13.778 11.3,13.967C11.3,14.156 11.23,14.319 11.091,14.458C10.953,14.597 10.789,14.667 10.6,14.667H8ZM8,9.983C8.555,9.983 9.025,9.792 9.408,9.408C9.791,9.025 9.983,8.555 9.983,8C9.983,7.444 9.791,6.975 9.408,6.592C9.025,6.208 8.555,6.017 8,6.017C7.444,6.017 6.975,6.208 6.591,6.592C6.208,6.975 6.016,7.444 6.016,8C6.016,8.555 6.208,9.025 6.591,9.408C6.975,9.792 7.444,9.983 8,9.983Z"


⚠️ Very long vector path (1705 characters), which is bad for performance. Considering reducing precision, removing minor details or rasterizing vector.

ElementBot · 2023-09-18T16:20:33Z

libraries/designsystem/src/main/res/drawable/ic_mute.xml

+    android:viewportWidth="15"
+    android:viewportHeight="15">
+  <path
+      android:pathData="M1.905,0.491C1.515,0.1 0.882,0.1 0.491,0.491C0.101,0.881 0.101,1.515 0.491,1.905L3.152,4.566C3.033,4.951 2.969,5.359 2.969,5.781V8.703C2.969,9.477 2.539,9.992 2.032,10.293C1.689,10.499 1.25,10.783 1.25,11.127C1.25,11.522 1.5,11.788 2.015,11.788H7.266H10.374L13.095,14.509C13.486,14.9 14.119,14.9 14.509,14.509C14.9,14.119 14.9,13.486 14.509,13.095L1.905,0.491ZM11.563,8.703C11.563,8.779 11.567,8.852 11.575,8.923L4.886,2.234C5.346,1.921 5.864,1.693 6.411,1.576C6.408,1.546 6.407,1.515 6.407,1.484C6.407,1.01 6.791,0.625 7.266,0.625C7.741,0.625 8.125,1.01 8.125,1.484C8.125,1.515 8.124,1.546 8.121,1.576C10.02,1.984 11.563,3.713 11.563,5.781V8.703ZM6.054,12.656C6.003,12.794 5.977,12.931 5.977,13.086C5.977,13.799 6.553,14.375 7.266,14.375C7.979,14.375 8.555,13.799 8.555,13.086C8.555,12.931 8.521,12.794 8.478,12.656H6.054Z"


⚠️ Very long vector path (823 characters), which is bad for performance. Considering reducing precision, removing minor details or rasterizing vector.

ElementBot · 2023-09-18T16:20:33Z

libraries/designsystem/src/main/res/drawable/ic_mute.xml

+  <path
+      android:pathData="M1.905,0.491C1.515,0.1 0.882,0.1 0.491,0.491C0.101,0.881 0.101,1.515 0.491,1.905L3.152,4.566C3.033,4.951 2.969,5.359 2.969,5.781V8.703C2.969,9.477 2.539,9.992 2.032,10.293C1.689,10.499 1.25,10.783 1.25,11.127C1.25,11.522 1.5,11.788 2.015,11.788H7.266H10.374L13.095,14.509C13.486,14.9 14.119,14.9 14.509,14.509C14.9,14.119 14.9,13.486 14.509,13.095L1.905,0.491ZM11.563,8.703C11.563,8.779 11.567,8.852 11.575,8.923L4.886,2.234C5.346,1.921 5.864,1.693 6.411,1.576C6.408,1.546 6.407,1.515 6.407,1.484C6.407,1.01 6.791,0.625 7.266,0.625C7.741,0.625 8.125,1.01 8.125,1.484C8.125,1.515 8.124,1.546 8.121,1.576C10.02,1.984 11.563,3.713 11.563,5.781V8.703ZM6.054,12.656C6.003,12.794 5.977,12.931 5.977,13.086C5.977,13.799 6.553,14.375 7.266,14.375C7.979,14.375 8.555,13.799 8.555,13.086C8.555,12.931 8.521,12.794 8.478,12.656H6.054Z"
+      android:fillColor="#A6ADB7"
+      android:fillType="evenOdd"/>


⚠️ Attribute fillType is only used in API level 24 and higher (current min is 23)

langleyd · 2023-09-19T08:30:47Z

Link to an issue to track the proper solution -> matrix-org/matrix-rust-sdk#2574

langleyd added 5 commits September 14, 2023 14:24

Show a room list decoration for notification setting applied

ed1949a

- Add the UI - Rebuild room summaries when push rules change or when user disables notifications(hide them all)

Fix test compilation

7a7cc26

Merge branch 'develop' of https://github.com/vector-im/element-x-android

9a234a8

into dla/feature/room_list_decoration

Update RoomListDataSource.kt

57aeac9

langleyd marked this pull request as ready for review September 14, 2023 20:28

langleyd requested a review from a team as a code owner September 14, 2023 20:28

langleyd requested review from bmarty, jmartinesp and ganfra and removed request for a team and bmarty September 14, 2023 20:28

langleyd and others added 4 commits September 15, 2023 10:25

Cancel scope in tests.

0cdc5f5

Merge branch 'develop' of https://github.com/vector-im/element-x-android

90fc536

into dla/feature/room_list_decoration

Use userDefinedNotificationMode.

4a3ffd3

Update screenshots

db02b0c

langleyd added 3 commits September 15, 2023 15:47

Reverting the change to only show the green dot for all messages.

f287a65

Merge branch 'develop' of https://github.com/vector-im/element-x-android

e25280b

into dla/feature/room_list_decoration

Merge branch 'dla/feature/room_list_decoration' of https://github.com…

21ca59e

…/vector-im/element-x-android into dla/feature/room_list_decoration

Update screenshots

6d7e51c

Merge branch 'develop' into dla/feature/room_list_decoration

bc29a31

julioromano reviewed Sep 18, 2023

View reviewed changes

langleyd added 2 commits September 18, 2023 15:05

Fix typo, remove unnecessary filter of all rooms in the datasource.

e48a9ee

Merge branch 'dla/feature/room_list_decoration' of https://github.com…

c160a37

…/vector-im/element-x-android into dla/feature/room_list_decoration

julioromano removed the request for review from jmartinesp September 18, 2023 14:16

julioromano approved these changes Sep 18, 2023

View reviewed changes

julioromano requested a review from jmartinesp September 18, 2023 14:19

manuroe added X-Release-Blocker Z-Schedule labels Sep 18, 2023

langleyd added 2 commits September 18, 2023 16:03

Add test.

6497d21

Merge branch 'develop' into dla/feature/room_list_decoration

8098193

langleyd enabled auto-merge September 18, 2023 15:19

langleyd added 5 commits September 18, 2023 16:24

Fix unused import.

67b9e94

Merge branch 'dla/feature/room_list_decoration' of https://github.com…

70d9fee

…/vector-im/element-x-android into dla/feature/room_list_decoration

Merge branch 'develop' into dla/feature/room_list_decoration

f465231

Merge branch 'develop' of https://github.com/vector-im/element-x-android

bd97cca

into dla/feature/room_list_decoration

Merge branch 'develop' into dla/feature/room_list_decoration

0d172df

ElementBot reviewed Sep 18, 2023

View reviewed changes

langleyd merged commit 50fbc5e into develop Sep 18, 2023

langleyd deleted the dla/feature/room_list_decoration branch September 18, 2023 16:27

bmarty mentioned this pull request Sep 19, 2023

ANR on RoomList #1370

Closed

julioromano mentioned this pull request Sep 19, 2023

RoomList : fix ANR (#1370) #1371

Merged

langleyd mentioned this pull request Oct 19, 2023

Add notification setting icons in the rooms list #547

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show a room list decoration for notification setting applied #1330

Show a room list decoration for notification setting applied #1330

langleyd commented Sep 14, 2023 •

edited

Loading

github-actions bot commented Sep 14, 2023 •

edited

Loading

langleyd commented Sep 14, 2023

ganfra commented Sep 15, 2023

jmartinesp commented Sep 15, 2023

langleyd commented Sep 15, 2023

ganfra commented Sep 15, 2023

codecov bot commented Sep 15, 2023 •

edited

Loading

langleyd commented Sep 15, 2023

langleyd commented Sep 15, 2023

sonarqubecloud bot commented Sep 15, 2023

julioromano left a comment

ElementBot Sep 18, 2023

ElementBot Sep 18, 2023

ElementBot Sep 18, 2023

langleyd commented Sep 19, 2023

Show a room list decoration for notification setting applied #1330

Show a room list decoration for notification setting applied #1330

Conversation

langleyd commented Sep 14, 2023 • edited Loading

Content

Motivation and context

Screenshots / GIFs

github-actions bot commented Sep 14, 2023 • edited Loading

langleyd commented Sep 14, 2023

ganfra commented Sep 15, 2023

jmartinesp commented Sep 15, 2023

langleyd commented Sep 15, 2023

ganfra commented Sep 15, 2023

codecov bot commented Sep 15, 2023 • edited Loading

Codecov Report

langleyd commented Sep 15, 2023

langleyd commented Sep 15, 2023

sonarqubecloud bot commented Sep 15, 2023

julioromano left a comment

Choose a reason for hiding this comment

ElementBot Sep 18, 2023

Choose a reason for hiding this comment

ElementBot Sep 18, 2023

Choose a reason for hiding this comment

ElementBot Sep 18, 2023

Choose a reason for hiding this comment

langleyd commented Sep 19, 2023

langleyd commented Sep 14, 2023 •

edited

Loading

github-actions bot commented Sep 14, 2023 •

edited

Loading

codecov bot commented Sep 15, 2023 •

edited

Loading