Create the DM when sending an event #6127

Florian14 · 2022-05-23T13:24:18Z

Type of change

WIP Feature
Bugfix
Technical
Other :

Content

Depends on #6051

Create the DM room when an event is sent from the local room timeline (text, image, audio, poll, location sharing...)

Motivation and context

Continue #5525

Screenshots / GIFs

	Sender	Receiver
By text	https://user-images.githubusercontent.com/11990514/178769378-f9348056-c725-418a-a830-f0afbad358bd.mp4	https://user-images.githubusercontent.com/11990514/178769798-f7f90ce8-d410-47c2-9a07-672bd4278e06.mp4
By image	https://user-images.githubusercontent.com/11990514/178770472-850fa810-aaa2-41b9-9a81-989a96da9d95.mp4	https://user-images.githubusercontent.com/11990514/178770509-30da044d-4b56-4f25-aa8e-fb78d29d5aa8.mp4
By location	https://user-images.githubusercontent.com/11990514/178770654-7bef2d98-33a0-4eef-bae9-5682bede97f1.mp4	https://user-images.githubusercontent.com/11990514/178770719-80cb39ec-231b-4d51-bedb-00282632f600.mp4

Tests

Open a local room timeline (by clicking on the fab button from the DM room list, selecting one or several users, and pressing the "GO" button)
Send an event from the composer view (message, image or anything else)
The DM room should be created, the selected users should be invited and we can continue our discussion in that room.
Press the back button should redirect to the home screen, the local room is not displayed anymore.

Tested devices

Physical
Emulator
OS version(s): API 32

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

github-actions · 2022-05-23T13:56:52Z

Unit Test Results

124 files ±0 124 suites ±0 2m 17s ⏱️ -5s
220 tests ±0 220 ✔️ ±0 0 💤 ±0 0 ❌ ±0
726 runs ±0 726 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 5c078bd. ± Comparison against base commit 521ecfb.

♻️ This comment has been updated with latest results.

mnaturel · 2022-07-19T07:56:17Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/SendEventTask.kt

@@ -78,6 +86,12 @@ internal class DefaultSendEventTask @Inject constructor(
        }
    }

+    private suspend fun createRoomAndSendEvent(params: SendEventTask.Params): String {


Maybe we could add a unit test for the case of local room id?

mnaturel · 2022-07-19T07:59:01Z

...oid/src/main/java/org/matrix/android/sdk/internal/session/room/create/CreateLocalRoomTask.kt

@@ -77,6 +81,7 @@ internal class DefaultCreateLocalRoomTask @Inject constructor(
        @SessionDatabase private val realmConfiguration: RealmConfiguration,
        private val createRoomBodyBuilder: CreateRoomBodyBuilder,
        private val userService: UserService,
+        private val cryptoService: DefaultCryptoService,


I think we should inject the interface CryptoService instead, no?

Oh it is because the onStateEvent is not part of the service interface... Strange maybe we should move the method to the interface. @bmarty What do you think?

Yes, I would not change it, I need it to register the local room in the crypto store in the same way as it is done for an existing room. I agree that it should be refactored, but I suppose that the intention was to not expose it in the interface.

We should not inject service inside the SDK, and we should not inject implementations as well, but we have some exceptions...
The Service interface should contain only methods useful for the application, not the internal stuff.
So OK to keep it like this.

mnaturel · 2022-07-19T08:12:46Z

...oid/src/main/java/org/matrix/android/sdk/internal/session/room/create/CreateLocalRoomTask.kt

@@ -228,11 +237,40 @@ internal class DefaultCreateLocalRoomTask @Inject constructor(
            )
        }

+        val localRoomThreePidEvents = invited3Pids.map { body ->


Maybe this could be extracted in 2 private methods? And I am struggling to understand the need for both LocalRoomThirdPartyInviteContent and RoomThirdPartyInviteContent. Since we are creating a local room, I thought we would only have LocalRoomThirdPartyInviteContent. Maybe this could be explained through methods names or code comments.

I created a LocalRoomThirdPartyInviteContent because I had to persist the full ThreePid to propagate it to create the "real" room. When we invite someone by email to a room, the email address is obfuscated in the RoomThirdPartyInviteContent and there is no way to retrieve it.
I created a new object for this purpose because I would not modify the existing object behaviour.
I also need the "RoomThirdPartyInviteContent" for the display of the timeline of the local room which should be as close as possible to a real room.
That's why I have both objects, I'll see to clarify this part.

Update: I don't need it anymore the local event for the purpose of the room creation but it is still necessary for the correct display of the local timeline. I can remove it if we agree that the obfuscated three pid is enough.

mnaturel · 2022-07-19T08:17:38Z

...dk-android/src/main/java/org/matrix/android/sdk/internal/session/room/state/SendStateTask.kt

            }
        }
    }
+
+    private suspend fun createRoomAndSendEvent(params: SendStateTask.Params): String {


We could also add a unit test for this specific case.

...ava/im/vector/app/features/home/room/detail/timeline/helper/TimelineEventVisibilityHelper.kt

...main/java/org/matrix/android/sdk/internal/session/room/create/CreateRoomFromLocalRoomTask.kt

mnaturel

Cool feature! I added some suggestions mostly to ease reading of code. Let me know what do you think about it. I didn't test it but seeing the videos of the PR it looks good.

Florian14 · 2022-07-19T09:24:02Z

Cool feature! I added some suggestions mostly to ease reading of code. Let me know what do you think about it. I didn't test it but seeing the videos of the PR it looks good.

@mnaturel Thanks for your review! I totally agree about adding tests, I didn't mention it in the PR description but I'll add some unit tests, I would do it in a dedicated PR but I'll see if I can add some in this PR. BTW it's good to have your feedback on which part of code you suggest adding tests!

bmarty

Thanks for the PR! A few remarks on the implementation, will do some tests this afternoon.

bmarty · 2022-08-25T10:23:29Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/events/model/EventType.kt

@@ -70,6 +70,9 @@ object EventType {
    const val STATE_ROOM_ENCRYPTION = "m.room.encryption"
    const val STATE_ROOM_SERVER_ACL = "m.room.server_acl"

+    // This type is for local purposes, it should never be processed by the server
+    const val LOCAL_STATE_ROOM_THIRD_PARTY_INVITE = "local.room.third_party_invite"


Could be internal, as well as LocalRoomThirdPartyInviteContent (and maybe move this class to the internal package?)

bmarty · 2022-08-25T10:26:24Z

...droid/src/main/java/org/matrix/android/sdk/api/session/room/model/create/CreateRoomParams.kt

@@ -61,12 +64,12 @@ open class CreateRoomParams {
     * A list of user IDs to invite to the room.
     * This will tell the server to invite everyone in the list to the newly created room.
     */
-    val invitedUserIds = mutableListOf<String>()
+    var invitedUserIds: MutableList<String> = mutableListOf()


Was it necessary to change from val to var?

Yes, otherwise Moshi will not be able to restore this field.
Btw I do not need to explicitly declare the type 90d688c

Are you sure? All our Json data classes are using val. When Moshi parse some Json, it stores the objects in temporary val, and then invoke the data class constructor. So it should work with val.

This is not a data class, when I originally tried by keeping the val, some fields were not persisted, I thought that it was for another reason but changing to var fixed the issue. I suppose that as these are not constructor fields, Moshi cannot do the same trick 😞

This is not a data class

My bad, OK then!

bmarty · 2022-08-25T10:29:24Z

...n/java/org/matrix/android/sdk/internal/session/room/create/CreateLocalRoomStateEventsTask.kt

+ */
+internal interface CreateLocalRoomStateEventsTask : Task<Params, List<Event>> {
+    data class Params(
+            val roomCreatorUserId: String,


The value will always be the current userId, no? In this case, it can be injected in the implementation.

bmarty · 2022-08-25T10:36:33Z

...n/java/org/matrix/android/sdk/internal/session/room/create/CreateLocalRoomStateEventsTask.kt

+            listOf(localThirdPartyInviteEvent, thirdPartyInviteEvent)
+        }.flatten()
+        addAll(threePidEvents)
+    }


Maybe rewrite to:

private fun MutableList<Event>.createRoomThreePidEvents() { createRoomBody.invite3pids.orEmpty().forEach { body -> val localThirdPartyInviteEvent = createLocalStateEvent( type = EventType.LOCAL_STATE_ROOM_THIRD_PARTY_INVITE, content = LocalRoomThirdPartyInviteContent( isDirect = createRoomBody.isDirect.orFalse(), membership = Membership.INVITE, displayName = body.address, thirdPartyInvite = body.toThreePid() ).toContent(), ) val thirdPartyInviteEvent = createLocalStateEvent( type = EventType.STATE_ROOM_THIRD_PARTY_INVITE, content = RoomThirdPartyInviteContent( displayName = body.address, keyValidityUrl = null, publicKey = null, publicKeys = null ).toContent(), ) add(localThirdPartyInviteEvent) add(thirdPartyInviteEvent) } }

for clarity

💯 I don't know why I made it complicated! cac4df7

bmarty · 2022-08-25T10:38:04Z

...n/java/org/matrix/android/sdk/internal/session/room/create/CreateLocalRoomStateEventsTask.kt

+     * Generate the create state event related to the power levels using the given overridden values or the default values according to the specification.
+     * Ref: https://spec.matrix.org/latest/client-server-api/#mroompower_levels
+     */
+    private fun MutableList<Event>.createRoomPowerLevelsEvent() = apply {


Using = apply is not necessary, since the return value is not used.
Better to use {. (at all place in this class)

I thought by mistake it was necessary to chain the calls but not for my usage. 110caba

…sting

sonarqubecloud · 2022-08-25T15:29:44Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

65.3% Coverage
0.0% Duplication

bmarty

Thanks for the update.
I have tested the feature, nomical case, i.e. inviting a matrixId and sending a text message, and this is working pretty well.
2 small glitches observed: first one on the timeline, when the room is created. 2nd one when going back to the room list, I can see twice the rooms for a few ms.
Nothing blocking though!

Approved!

Florian14 · 2022-08-26T07:32:55Z

Thanks for the update. I have tested the feature, nomical case, i.e. inviting a matrixId and sending a text message, and this is working pretty well. 2 small glitches observed: first one on the timeline, when the room is created. 2nd one when going back to the room list, I can see twice the rooms for a few ms. Nothing blocking though!

Approved!

Thanks for your feedback!

The first glitch should be fixed by adding a loading wheel during the creation but I did not find a way to do it atm.
The second one is more annoying because the local rooms are deleted when displaying the room list, I can try to do the deletion as soon as the new room has been created, but not sure it will fix the glitch

Florian14 force-pushed the feature/fre/start_dm_on_first_msg_impl branch from feeb70b to 5c078bd Compare May 25, 2022 15:31

giomfo mentioned this pull request Jun 9, 2022

Implement the DM creation on new message tchapgouv/tchap-android#516

Closed

Florian14 force-pushed the feature/fre/start_dm_on_first_msg_impl branch 2 times, most recently from 84a3240 to 596bd0b Compare June 16, 2022 16:04

Florian14 force-pushed the feature/fre/start_dm_on_first_msg branch from b805645 to d42a3da Compare June 17, 2022 22:56

Florian14 force-pushed the feature/fre/start_dm_on_first_msg_impl branch from 596bd0b to 53c73dc Compare June 17, 2022 22:57

Florian14 force-pushed the feature/fre/start_dm_on_first_msg_impl branch from 53c73dc to 2a597a8 Compare July 1, 2022 22:29

Florian14 force-pushed the feature/fre/start_dm_on_first_msg_impl branch 2 times, most recently from e2ff5c4 to 79c6305 Compare July 12, 2022 07:20

Base automatically changed from feature/fre/start_dm_on_first_msg to develop July 12, 2022 13:12

Florian14 force-pushed the feature/fre/start_dm_on_first_msg_impl branch 2 times, most recently from cc0280b to 4b3d27a Compare July 18, 2022 14:05

Florian14 marked this pull request as ready for review July 18, 2022 14:35

Florian14 requested review from a team and mnaturel and removed request for a team July 18, 2022 14:54