element-android icon indicating copy to clipboard operation
element-android copied to clipboard

Create the DM when sending an event

Open Florian14 opened this issue 2 years ago • 3 comments

Type of change

  • [X] 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

  1. 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)
  2. Send an event from the composer view (message, image or anything else)
  3. The DM room should be created, the selected users should be invited and we can continue our discussion in that room.
  4. Press the back button should redirect to the home screen, the local room is not displayed anymore.

Tested devices

  • [ ] Physical
  • [X] 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
  • [X] Pull request is based on the develop branch
  • [X] Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
  • [x] Pull request includes screenshots or videos if containing UI changes
  • [ ] Pull request includes a sign off
  • [x] 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()

Florian14 avatar May 23 '22 13:05 Florian14

Unit Test Results

124 files  ±0  124 suites  ±0   2m 17s :stopwatch: -5s 220 tests ±0  220 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0  726 runs  ±0  726 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0 

Results for commit 5c078bd1. ± Comparison against base commit 521ecfb6.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar May 23 '22 13:05 github-actions[bot]

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!

Florian14 avatar Jul 19 '22 09:07 Florian14

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 avatar Aug 26 '22 07:08 Florian14