Unify the way we decide whether a room is a DM or a group room #3100

jmartinesp · 2024-06-26T10:34:18Z

Content

Add fun isDm(isDirect: Boolean, activeMembersCount: Int): Boolean.
Add extension functions that call this isDm using existing data for MatrixRoom and MatrixRoomInfo.
Apply these changes through the app.

Motivation and context

Up until now, different parts of the app used different definitions of what a DM is or when should we distinguish between a group room and a direct one. The idea is to always use this function as the source of truth in all parts of the app.

Tests

Most things should work as before, but unencrypted direct rooms with 2 people should also be considered DMs and have the alternative UI/logic, matching Element Web's behaviour (except when there are more than 2 people in the room).

Notifications now also use the more strict definition of what a DM is in order to display custom avatar images or texts.

Tested devices

Physical
Emulator
OS version(s): 14

Checklist

Changes have been tested on an Android device or Android emulator with API 23
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
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

jmartinesp · 2024-06-26T10:41:11Z

libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/DmChecker.kt

This is where the magic happens. I'm bad at naming classes 🥲 .

github-actions · 2024-06-26T10:41:40Z

📱 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/b2oG82

bmarty · 2024-06-26T12:41:43Z

libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/DmChecker.kt

+/**
+ * Verifies if a room is considered a direct message one for Element.
+ */
+object DmChecker {


I would not introduce a class for this, but rather create some extensions, like RoomInfo.isElementDm(), MatrixRoomInfo.isElementDm(), etc.

If all those extensions have the same name, and all in the same file, I think it can work.

I'm afraid if we have 3, 4 of these extensions with exactly the same code inside them we might want to update them and forget about some of them. That was why I created this object.

We could make it so the extensions all called the same private function underneath though, so we could ensure the check is done with the same piece of code for every one of them.

. o O (Why do we have so many types to describe the same thing?)

Still it will be cleaner and less error prone to use an extension at the call site.

There are a couple of places where it wouldn't be possible to just use extensions:

RoomSummaryDetailsFactory.create: it uses a Rust RoomInfo which is not available in the matrix.api module.

NotificationMapper.map, we only have the NotificationRoomInfo, which also comes from Rust and is not available in matrix.api.

Either we keep the DmChecker object for those or we create a public fun isRoomDm(isDirect: Boolean, activeMemeberCount: Int): Boolean that can be called for these cases.

WDYT?

Also add extension functions for `MatrixRoom` and `MatrixRoomInfo`.

…ncluding: - Room list. - Room details screen. - Invites. - Notifications. Replace most `isDirect` usages with `isDm`.

codecov · 2024-07-09T11:15:38Z

Codecov Report

Attention: Patch coverage is 88.88889% with 5 lines in your changes missing coverage. Please review.

Project coverage is 76.12%. Comparing base (b0c9091) to head (6f3deb0).
Report is 25 commits behind head on develop.

Files	Patch %	Lines
...push/impl/notifications/NotificationDataFactory.kt	25.00%	0 Missing and 3 partials ⚠️
...eatures/roomlist/impl/components/RoomSummaryRow.kt	66.66%	0 Missing and 1 partial ⚠️
...pl/notifications/DefaultNotifiableEventResolver.kt	80.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3100   +/-   ##
========================================
  Coverage    76.11%   76.12%           
========================================
  Files         1640     1641    +1     
  Lines        38622    38629    +7     
  Branches      7469     7468    -1     
========================================
+ Hits         29396    29405    +9     
+ Misses        5333     5332    -1     
+ Partials      3893     3892    -1

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

bmarty

Thanks, only a few remarks left, let me know what you think.

bmarty · 2024-07-09T14:20:44Z

...ies/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/RoomIsDmCheck.kt

+ * @param activeMembersCount the number of active members in the room (joined or invited)
+ */
+fun isDm(isDirect: Boolean, activeMembersCount: Int): Boolean {
+    return isDirect && activeMembersCount <= 2


Sadly we cannot rely on the number of activeMembersCount since we can have bot like AuditBot or AdminBot in all the DM.

We will have to find a solution to manage such rooms.

But I agree that this is not new with this PR, so we may handle this later, and so this is not a blocker.

Yeah, we should make explicit changes in the SDK to support bots in DMs.

bmarty · 2024-07-09T14:21:25Z

...ies/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/RoomIsDmCheck.kt

+/**
+ * Returns whether the [MatrixRoomInfo] is from a DM.
+ */
+val MatrixRoomInfo.isDm get() = isDm(isDirect, activeMembersCount.toInt())


Thanks for those handy extensions!

bmarty · 2024-07-10T08:40:44Z

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

@@ -290,5 +290,5 @@ internal fun RoomListRoomSummary.toInviteData() = InviteData(
    roomId = roomId,
    // Note: `name` should not be null at this point, but just in case, fallback to the roomId
    roomName = name ?: roomId.value,
-    isDirect = isDirect,
+    isDirect = isDm,


For clarity, maybe rename InviteData.isDirect to InviteData.isDm?
isDirect should be reserved for the value from the Matrix protocol.

Also, and related, on JoinRoomPresenter, maybe replace

isDirect = isDirect

by

isDm = isDm(isDirect, numberOfMembers?.toInt() ?: 2)

bmarty · 2024-07-10T09:00:14Z

.../matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/room/FakeMatrixRoom.kt

@@ -91,6 +90,8 @@ class FakeMatrixRoom(
    canRedactOwn: Boolean = false,
    canRedactOther: Boolean = false,
 ) : MatrixRoom {
+    override val isOneToOne: Boolean = activeMemberCount <= 2L


Do we really need to override the default value (which is activeMemberCount == 2L) here? It looks like this is against what the spec says.

jmartinesp · 2024-07-10T10:01:55Z

@frebib FWIW this PR removes the 'isEncrypted' check for DMs, so I think it should be quite similar to what your previous PR did (only here we try to centralise the check for the whole app).

frebib · 2024-07-10T11:27:27Z

Nice, thanks. I'll give it a try later

bmarty · 2024-07-10T16:03:51Z

.../kotlin/io/element/android/features/invite/impl/response/AcceptDeclineInvitePresenterTest.kt

@@ -265,7 +265,7 @@ class AcceptDeclineInvitePresenterTest {
        return InviteData(
            roomId = roomId,
            roomName = name,
-            isDirect = isDirect
+            isDm = isDirect


Maybe rename also the parameter.

bmarty

Thanks!

sonarcloud · 2024-07-10T16:13:58Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

jmartinesp commented Jun 26, 2024

View reviewed changes

bmarty reviewed Jun 26, 2024

View reviewed changes

Base automatically changed from misc/jme/new-authentication-flow-rust to develop June 27, 2024 09:44

jmartinesp added 2 commits July 9, 2024 12:06

Add centralised 'room is DM' check

e40884c

Also add extension functions for `MatrixRoom` and `MatrixRoomInfo`.

Use the centralised method and extension functions through the app, i…

c610aa3

…ncluding: - Room list. - Room details screen. - Invites. - Notifications. Replace most `isDirect` usages with `isDm`.

jmartinesp force-pushed the misc/jme/3078-unify-dm-room-check branch from 345ad1c to c610aa3 Compare July 9, 2024 10:24

jmartinesp added the PR-Change For updates to an existing feature label Jul 9, 2024

jmartinesp marked this pull request as ready for review July 9, 2024 10:30

jmartinesp requested a review from a team as a code owner July 9, 2024 10:30

jmartinesp requested review from bmarty and removed request for a team July 9, 2024 10:30

jmartinesp added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jul 9, 2024

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jul 9, 2024

jmartinesp force-pushed the misc/jme/3078-unify-dm-room-check branch from dba05a6 to 0ba80b3 Compare July 9, 2024 14:14

Fix unexpected screenshot changes

5266c32

jmartinesp force-pushed the misc/jme/3078-unify-dm-room-check branch from 0ba80b3 to 5266c32 Compare July 9, 2024 14:36

bmarty reviewed Jul 10, 2024

View reviewed changes

jmartinesp added 2 commits July 10, 2024 11:34

InviteData should use isDm, not isDirect

e103163

JoinRoomPresenter should use isDm, not isDirect

84e1c66

Remove unnecessary override in FakeMatrixRoom

c1e23f9

jmartinesp added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jul 10, 2024

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jul 10, 2024

Update screenshots

6f3deb0

jmartinesp requested a review from bmarty July 10, 2024 13:56

bmarty reviewed Jul 10, 2024

View reviewed changes

bmarty approved these changes Jul 10, 2024

View reviewed changes

Fix parameter in test

c583311

jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Jul 10, 2024

jmartinesp enabled auto-merge (squash) July 10, 2024 16:06

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Jul 10, 2024

jmartinesp merged commit 0be7058 into develop Jul 10, 2024
28 checks passed

jmartinesp deleted the misc/jme/3078-unify-dm-room-check branch July 10, 2024 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify the way we decide whether a room is a DM or a group room #3100

Unify the way we decide whether a room is a DM or a group room #3100

jmartinesp commented Jun 26, 2024 •

edited

Loading

jmartinesp Jun 26, 2024

github-actions bot commented Jun 26, 2024 •

edited

Loading

bmarty Jun 26, 2024

jmartinesp Jun 26, 2024

bmarty Jun 26, 2024 •

edited

Loading

jmartinesp Jul 4, 2024 •

edited

Loading

codecov bot commented Jul 9, 2024 •

edited

Loading

bmarty left a comment

bmarty Jul 9, 2024

jmartinesp Jul 10, 2024

bmarty Jul 9, 2024

bmarty Jul 10, 2024 •

edited

Loading

bmarty Jul 10, 2024

jmartinesp commented Jul 10, 2024

frebib commented Jul 10, 2024

bmarty Jul 10, 2024

bmarty left a comment

sonarcloud bot commented Jul 10, 2024

Unify the way we decide whether a room is a DM or a group room #3100

Unify the way we decide whether a room is a DM or a group room #3100

Conversation

jmartinesp commented Jun 26, 2024 • edited Loading

Content

Motivation and context

Tests

Tested devices

Checklist

Choose a reason for hiding this comment

github-actions bot commented Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

jmartinesp Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 9, 2024 • edited Loading

Codecov Report

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

bmarty Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp commented Jul 10, 2024

frebib commented Jul 10, 2024

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jul 10, 2024

Quality Gate passed

jmartinesp commented Jun 26, 2024 •

edited

Loading

github-actions bot commented Jun 26, 2024 •

edited

Loading

bmarty Jun 26, 2024 •

edited

Loading

jmartinesp Jul 4, 2024 •

edited

Loading

codecov bot commented Jul 9, 2024 •

edited

Loading

bmarty Jul 10, 2024 •

edited

Loading