#4319: Fix DM navigation in member profile screen #5292

mnaturel · 2022-02-22T09:32:11Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Moving the navigation handling to open direct message from TimelineFragment to RoomMemberProfileFragment.

Motivation and context

Closes #4319

Screenshots / GIFs

Tests

Open drawer menu
Click on a space menu
Click on "List members" button
Click on a member
Click on "Direct message"
Check the screen with corresponding direct message room is displayed
Go to a room where there are different members
Click on a member in the timeline
Click on "Direct message"
Check the screen with corresponding direct message room is displayed

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

github-actions · 2022-02-22T10:05:35Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
passed="25" failures="17" errors="0" skipped="2"
[org.matrix.android.sdk.account]
passed="5" failures="0" errors="0" skipped="2"
[org.matrix.android.sdk.internal]
passed="158" failures="12" errors="0" skipped="38"
[org.matrix.android.sdk.ordering]
passed="16" failures="0" errors="0" skipped="0"
[org.matrix.android.sdk.PermalinkParserTest]
passed="2" failures="0" errors="0" skipped="0"

github-actions · 2022-02-22T10:11:17Z

Unit Test Results

  88 files ±0   88 suites ±0 1m 4s ⏱️ +3s
159 tests ±0 159 ✔️ ±0 0 💤 ±0 0 ❌ ±0
512 runs ±0 512 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 96f041a. ± Comparison against base commit 80bc3af.

♻️ This comment has been updated with latest results.

bmarty

The fix is fine, but this is a change in the navigation. Is it confirmed that we want to do this change?

Maybe a video before / after showing the diff can help @daniellekirkwood to understand the diff.

bmarty · 2022-02-22T13:10:27Z

vector/src/main/java/im/vector/app/features/roommemberprofile/RoomMemberProfileFragment.kt

@@ -297,8 +302,7 @@ class RoomMemberProfileFragment @Inject constructor(
    }

    override fun onOpenDmClicked() {
-        roomDetailPendingActionStore.data = RoomDetailPendingAction.OpenOrCreateDm(fragmentArgs.userId)
-        vectorBaseActivity.finish()
+        viewModel.handle(RoomMemberProfileAction.OpenOrCreateDm(fragmentArgs.userId))


This has the effect to add a room to the Activity stack. This is fine, but this is a side effect of this change.
So when the user will close the RoomDetailActivity with the DM, they will see the RoomMemberProfile Fragment again.

Yes It was intended. In my opinion it is consistent to go back to member profile screen in term of navigation. But I will try to make a video to explain the changes as you suggested.

bmarty · 2022-02-22T13:11:59Z

vector/src/main/java/im/vector/app/features/roommemberprofile/RoomMemberProfileViewModel.kt

+                _viewEvents.post(RoomMemberProfileViewEvents.OpenRoom(roomId = roomId))
+            }
+        }
+    }


This code is duplicate of the code in TimelineViewModel.
We should find a way to avoid code duplication in ViewModels (maybe out of scope of this PR)

What is the code which is duplicated? Normally, I moved out this method handleOpenOrCreateDm from TimelineViewModel into the RoomMemberProfileViewModel.

Ah, my bad, sorry

daniellekirkwood · 2022-02-22T15:41:12Z

CC: @kittykat

mnaturel · 2022-02-22T15:52:52Z

CC: @kittykat

I have just seen your comment. I have just added GIFs in the associated issue.

kittykat · 2022-02-22T16:19:03Z

@mnaturel Can we make the back button go to the timeline instead of the user profile? (Please wait for confirmation from @niquewoodhouse before changing behaviour.)

My thoughts are that normally a user would want to message only one user from a memberlist rather than continuing to take other actions on that user's profile or on the member list.

mnaturel · 2022-02-23T09:04:26Z

@mnaturel Can we make the back button go to the timeline instead of the user profile? (Please wait for confirmation from @niquewoodhouse before changing behaviour.)

My thoughts are that normally a user would want to message only one user from a memberlist rather than continuing to take other actions on that user's profile or on the member list.

Hello @kittykat , to answer your question, I can explain why I changed the back navigation. There are 2 navigation paths leading to a room member profile:

From room timeline, click on a profile picture
From home, click on drawer menu, click on space settings, click on list members entry

For the case 1, I see why we would like to go back to timeline and it may be easy to handle technically. But for the case 2, which is the original bug we fix here, I don't understand why we would go back to a room timeline and if so, which room we would choose since the user is coming back from home screen.
This is the reason, I choose to be consistent in navigation paths and I kept the back stack history to make it simple in both cases.

So I may find a solution to handle case 1, but for case 2, I am not sure we should do something else.

bmarty · 2022-02-23T10:51:33Z

Is it possible to fix #4319, so case 1, without changing the other existing navigation of case 2?

kittykat · 2022-02-24T15:31:00Z

[I talked to mnaturel and current status of PR is Nique and I are considering best course of action. I would be leaning towards Space header menu -> List members -> User -> Direct message -back button-> Space for the second case.]

mnaturel · 2022-02-24T16:19:41Z

[I talked to mnaturel and current status of PR is Nique and I are considering best course of action. I would be leaning towards Space header menu -> List members -> User -> Direct message -back button-> Space for the second case.]

@kittykat Okay. Just to be sure I understood well, to sum up:

when coming from a room timeline we want to go back to that timeline instead of the member profile.
when coming from "List members" entry of a space menu settings, we want to go back to the home screen (see picture below) instead of the member profile.

Is that correct?

kittykat · 2022-02-25T14:46:47Z

@mnaturel Please stay with your original proposal for now (the back button goes back screen by screen along the whole path) and we can review the interaction when we're not in a rush as we would ideally do some user testing around it because it's a big change.

bmarty

Thanks for the discussion.
We may have complain about the change of navigation when opening the DM from the room member list, but we will see!
It's worth noting that we can also open a DM from the timeline by clicking on a user avatar and choosing "Direct message". So I guess the long path is less used.

bmarty reviewed Feb 22, 2022

View reviewed changes

mnaturel mentioned this pull request Feb 22, 2022

Direct message not initiated after telling the app to do so via the space member list #4319

Closed

kittykat requested review from niquewoodhouse and kittykat February 22, 2022 16:19

mnaturel force-pushed the feature/mna/4319-dm-space-members-list branch from 78b7543 to 958d7f7 Compare February 23, 2022 09:11

mnaturel force-pushed the feature/mna/4319-dm-space-members-list branch from 958d7f7 to e8ac9c4 Compare February 24, 2022 15:20

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

Maxime Naturel added 4 commits February 28, 2022 14:58

Adding changelog entry

1ef26e6

DRAFT TODO

836bf4e

Moving DM creation/opening into the member profile screen

46854b4

Remove handling of direct message from timeline fragment

96f041a

mnaturel force-pushed the feature/mna/4319-dm-space-members-list branch from e8ac9c4 to 96f041a Compare February 28, 2022 13:59

bmarty approved these changes Feb 28, 2022

View reviewed changes

bmarty merged commit 781a477 into develop Feb 28, 2022

bmarty deleted the feature/mna/4319-dm-space-members-list branch February 28, 2022 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4319: Fix DM navigation in member profile screen #5292

#4319: Fix DM navigation in member profile screen #5292

mnaturel commented Feb 22, 2022

github-actions bot commented Feb 22, 2022 •

edited

Loading

github-actions bot commented Feb 22, 2022 •

edited

Loading

bmarty left a comment

bmarty Feb 22, 2022

mnaturel Feb 22, 2022

bmarty Feb 22, 2022

mnaturel Feb 22, 2022

bmarty Feb 22, 2022

daniellekirkwood commented Feb 22, 2022

mnaturel commented Feb 22, 2022

kittykat commented Feb 22, 2022

mnaturel commented Feb 23, 2022

bmarty commented Feb 23, 2022

kittykat commented Feb 24, 2022

mnaturel commented Feb 24, 2022

kittykat commented Feb 25, 2022

bmarty left a comment

#4319: Fix DM navigation in member profile screen #5292

#4319: Fix DM navigation in member profile screen #5292

Conversation

mnaturel commented Feb 22, 2022

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

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

Matrix SDK

Integration Tests Results:

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

Unit Test Results

bmarty left a comment

Choose a reason for hiding this comment

bmarty Feb 22, 2022

Choose a reason for hiding this comment

mnaturel Feb 22, 2022

Choose a reason for hiding this comment

bmarty Feb 22, 2022

Choose a reason for hiding this comment

mnaturel Feb 22, 2022

Choose a reason for hiding this comment

bmarty Feb 22, 2022

Choose a reason for hiding this comment

daniellekirkwood commented Feb 22, 2022

mnaturel commented Feb 22, 2022

kittykat commented Feb 22, 2022

mnaturel commented Feb 23, 2022

bmarty commented Feb 23, 2022

kittykat commented Feb 24, 2022

mnaturel commented Feb 24, 2022

kittykat commented Feb 25, 2022

bmarty left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 22, 2022 •

edited

Loading

github-actions bot commented Feb 22, 2022 •

edited

Loading