Populate the dedicated view of direct messages #471

aaravlu · 2025-04-28T03:07:10Z

Fix issue #139

Copilot

Pull Request Overview

This PR introduces a dedicated view for direct messages by adding a new flag to room information, updating header labels, and splitting the joined rooms list to separately handle direct messages.

Added an "is_direct" flag in room updates and JoinedRoomInfo to denote direct message rooms.
Updated the collapsible header label from "Direct Messages" to "People".
Modified the room list widget logic to segregate direct messages from joined rooms.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/sliding_sync.rs	Added direct message status detection for joined room updates.
src/shared/collapsible_header.rs	Changed header label for direct message category to "People".
src/home/rooms_list.rs	Extended structures and widget logic to support separate direct message handling.

Comments suppressed due to low confidence (1)

src/shared/collapsible_header.rs:85

If the updated label 'People' for direct messages is intentional, ensure that related documentation and UI guidelines are updated for consistency.

HeaderCategory::DirectMessages => "People",

src/home/rooms_list.rs

aaravlu · 2025-04-28T10:58:07Z

alanpoon

The search for the room list does not apply for People.

alanpoon · 2025-04-30T09:50:07Z

src/home/rooms_list.rs

-    #[rust] displayed_joined_rooms: Vec<OwnedRoomId>,
-    #[rust(true)] is_joined_rooms_header_expanded: bool,
+    ///
+    /// **Direct messages are excluded.**
+    #[rust] displayed_rooms: Vec<OwnedRoomId>,
+    #[rust(true)] is_rooms_header_expanded: bool,


Please do not change the field names.

To be honest, i think it's more scientific to name it that way, because we certainly don't want a field named displayed_joined_direct_messages

@kevinaboos How do you like this?

Naming should consider semantic precision:

all_joined_rooms: All joined rooms (source data, including DMs and non-DMs).

displayed_direct_messages: Rooms filtered from all_joined_rooms where is_direct = true, used for the "People" group.

displayed_joined_rooms: Rooms filtered from all_joined_rooms where is_direct = false, used for the "Rooms" group (or other names for non-DM groups).

is_direct_messages_header_expanded: The expansion state of the "People" group.

is_joined_rooms_header_expanded: The expansion state of the "Rooms" group.

If there is ambiguity, clarify it in the comments, for example:

/// The list of joined rooms (excluding direct messages) currently displayed in the UI, /// in order from top to bottom. /// This is a strict subset of the rooms present in `all_joined_rooms` where `is_direct` is false, /// and should be determined by applying the `display_filter`. #[rust] displayed_joined_rooms: Vec<OwnedRoomId>, #[rust(true)] is_joined_rooms_header_expanded: bool,

alanpoon · 2025-04-30T11:32:16Z

src/home/rooms_list.rs

+            self.displayed_rooms = self.all_joined_rooms.iter()
+                .filter(|(_id, info)|{!info.is_direct})
+                .map(|(id, _info)| id.clone())
+                .collect();
+
+            self.displayed_direct_messages = self.all_joined_rooms.iter()
+                .filter(|(_id, info)|{info.is_direct})
+                .map(|(id, _info)| id.clone())
+                .collect();


Iterate all_joined_rooms once and push to displayed_rooms and displayed_direct_messages separately.

aaravlu · 2025-05-05T09:41:04Z

The search for the room list does not apply for People.

Fixed.

src/home/room_preview.rs

Copilot

Pull Request Overview

This PR adds support for displaying direct messages as a separate section in the UI, addressing issue #139. Key changes include:

Fetching the "is_direct" status for each room and including it in joined room info.
Splitting the displayed joined rooms into separate lists for regular rooms and direct messages.
Updating header labels and corresponding UI logic to accommodate the new direct messages view.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
src/sliding_sync.rs	Adds a check to determine if a room is direct and propagates that info downstream.
src/shared/collapsible_header.rs	Updates the header label for direct messages from "Direct Messages" to "People".
src/home/rooms_list.rs	Introduces new fields and logic to separate direct messages from non-direct rooms.
src/home/room_preview.rs	Minor refactoring and formatting improvements in the room preview widget.

Comments suppressed due to low confidence (1)

src/home/rooms_list.rs:593

Both 'displayed_direct_messages' and 'displayed_rooms' are assigned the same generated list, which does not differentiate direct messages from regular rooms. It is recommended to filter the generated list based on the room's 'is_direct' flag to properly segregate the two types.

let generated_displayed_rooms = generate_displayed_rooms(&self.all_joined_rooms, &self.display_filter, sort_fn.as_deref());

Copilot

Pull Request Overview

This PR implements dedicated support for direct messages by detecting and separating them from regular joined rooms. Key changes include adding an asynchronous check for direct message rooms in the sliding sync logic, updating header labels in the collapsible header component, and modifying the rooms list logic to maintain separate collections for direct messages and regular rooms.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/sliding_sync.rs	Added async detection of direct rooms and passed the flag onward.
src/shared/collapsible_header.rs	Updated header label from "Direct Messages" to "People".
src/home/rooms_list.rs	Introduced separate vectors and updated filtering logic for direct messages.
src/home/room_preview.rs	Minor formatting adjustments and code cleanup in room preview logic.

src/home/rooms_list.rs

Copilot

Pull Request Overview

This PR adds support for a dedicated view of direct messages by distinguishing between direct message rooms and regular joined rooms. Key changes include:

Adding an asynchronous check for direct messages in sliding_sync.
Introducing an is_direct boolean method in the FilterableRoom trait and updating its implementations.
Splitting the joined rooms list into separate displayed lists for direct messages and standard rooms in the home view.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/sliding_sync.rs	Adds an asynchronous direct message flag for new rooms
src/shared/collapsible_header.rs	Updates the header label from "Direct Messages" to "People"
src/room/room_display_filter.rs	Adds the is_direct method to the FilterableRoom trait
src/home/rooms_list.rs	Splits and manages displayed rooms between direct messages and others
src/home/room_preview.rs	Applies minor formatting adjustments for clarity

src/home/rooms_list.rs

Correct direct messages display when being sortted Co-authored-by: Copilot <[email protected]>

ZhangHanDong · 2025-05-19T09:58:23Z

“People” should be placed above “Room” to better maintain consistency with user habits from Element.

ZhangHanDong

In addition to the review suggestions left in the code, there is another issue that has not been considered in this PR:

If the is_direct status of a room changes for some reason (for example, if member changes cause the SDK’s is_direct() return value to change, or if the m.direct tag is modified), the current update_room logic does not seem to detect this specific change and send a dedicated update to RoomsList to adjust the grouping of that room.

It is necessary to confirm whether matrix-sdk’s room.is_direct().await will, after a change in the room’s DM property (such as the m.direct tag or member changes affecting its determination), trigger update_room via the VectorDiff::Set mechanism, and whether new_room.is_direct().await can immediately reflect this latest status. If not, then the real-time update of the room type conversion will depend on other events that trigger update_room.

ZhangHanDong · 2025-05-19T10:13:56Z

src/home/rooms_list.rs

-    #[rust] displayed_joined_rooms: Vec<OwnedRoomId>,
-    #[rust(true)] is_joined_rooms_header_expanded: bool,
+    ///
+    /// **Direct messages are excluded.**
+    #[rust] displayed_rooms: Vec<OwnedRoomId>,
+    #[rust(true)] is_rooms_header_expanded: bool,


Naming should consider semantic precision:

all_joined_rooms: All joined rooms (source data, including DMs and non-DMs).

displayed_direct_messages: Rooms filtered from all_joined_rooms where is_direct = true, used for the "People" group.

displayed_joined_rooms: Rooms filtered from all_joined_rooms where is_direct = false, used for the "Rooms" group (or other names for non-DM groups).

is_direct_messages_header_expanded: The expansion state of the "People" group.

is_joined_rooms_header_expanded: The expansion state of the "Rooms" group.

If there is ambiguity, clarify it in the comments, for example:

/// The list of joined rooms (excluding direct messages) currently displayed in the UI, /// in order from top to bottom. /// This is a strict subset of the rooms present in `all_joined_rooms` where `is_direct` is false, /// and should be determined by applying the `display_filter`. #[rust] displayed_joined_rooms: Vec<OwnedRoomId>, #[rust(true)] is_joined_rooms_header_expanded: bool,

ZhangHanDong · 2025-05-19T10:39:15Z

src/home/rooms_list.rs

@@ -425,9 +445,9 @@ impl RoomsList {
                }
                RoomsListUpdate::RemoveRoom { room_id, new_state: _ } => {


There is no attempt to remove from displayed_direct_messages! This is a bug. If a DM (direct message) room is removed, it will not be cleared from the displayed_direct_messages list.

Yes, the case direct messages were not handled, fixed.

ZhangHanDong · 2025-05-19T10:43:26Z

src/home/rooms_list.rs

+            &self.display_filter,
+            sort_fn.as_deref(),
+        )
+        .into_iter().for_each(|room_id|{


generate_rooms iterates over all_joined_rooms once. The resulting list is then traversed again to split it.

This is still two passes (one full filtering, one splitting of the filtered result).

If generate_rooms could directly return two lists (DM and non-DM), or return an intermediate structure containing is_direct information, the second traversal could be avoided.

However, the current implementation is probably fast enough for most cases, since the second traversal is over a smaller, already-filtered set.

Consideration: If performance does become an issue, generate_rooms (or its usage) could be modified to produce separated lists more directly. But for now, the readability is acceptable.

ZhangHanDong · 2025-05-19T10:48:36Z

src/home/rooms_list.rs

-        let total_count = status_label_id + 1; // +1 for the status label
+            + if self.is_rooms_header_expanded { self.displayed_rooms.len() } else { 0 };
+
+        let index_of_direct_messages_header = should_show_direct_messages_header.then_some(index_after_joined_rooms);


The conditions and addition operations in this part of the logic have increased. It is important to carefully verify correctness to ensure that, under all combinations (such as empty lists, headers expanded/collapsed), the indices do not get mixed up and total_count remains correct.

Readability: This section of logic has become somewhat difficult to grasp at a glance due to the multiple conditional branches and index accumulations.

Suggestions:
• Add unit tests to cover various edge cases in these index calculations.
• Consider encapsulating the index calculation logic into one or more private helper functions, for example:
fn calculate_section_indices(&self, show_header: bool, is_expanded: bool, num_items: usize, previous_section_end_index: usize) -> (Option <usize>, usize, usize)

This would make the draw_walk function itself cleaner.

ZhangHanDong · 2025-05-19T10:50:00Z

src/home/rooms_list.rs

@@ -716,8 +784,42 @@ impl Widget for RoomsList {
                }
                else if let Some(joined_room_id) = get_joined_room_id(portal_list_index) {
                    if let Some(joined_room) = self.all_joined_rooms.get_mut(joined_room_id) {
+                        if !joined_room.is_direct {


Your use of if !joined_room.is_direct implies that displayed_rooms may contain direct messages (DMs), which is inconsistent with the comment and intended use of this variable.

Therefore, you should reconsider the field naming.

Your use of if !joined_room.is_direct implies that displayed_rooms may contain direct messages (DMs)

It won't contain, which must be a redundant error, so i removed if !joined_room.is_direct check.

aaravlu · 2025-05-20T02:49:52Z

If the is_direct status of a room changes for some reason (for example, if member changes cause the SDK’s is_direct() return value to change, or if the m.direct tag is modified), the current update_room logic does not seem to detect this specific change and send a dedicated update to RoomsList to adjust the grouping of that room.

Let's fix it in the future pr.

aaravlu · 2025-05-20T04:23:15Z

“People” should be placed above “Room” to better maintain consistency with user habits from Element.

Fixed.

Populate the dedicated view of direct messages

b3ed70d

aaravlu mentioned this pull request Apr 28, 2025

Populate the dedicated view of direct messages ("People") in the rooms list #323

Closed

aaravlu requested a review from Copilot April 28, 2025 03:08

Copilot AI reviewed Apr 28, 2025

View reviewed changes

src/home/rooms_list.rs Outdated Show resolved Hide resolved

aaravlu added 2 commits April 28, 2025 13:08

Fix current actived room for direct messages

5fc2ae7

Remove redadunt log files

e011700

aaravlu marked this pull request as ready for review April 28, 2025 08:12

alanpoon requested changes Apr 30, 2025

View reviewed changes

aaravlu added 2 commits May 5, 2025 16:43

Merge branch 'main' into fix139-2

f7bdace

Resolve conflicts

6cf216a

tyreseluo reviewed May 8, 2025

View reviewed changes

src/home/room_preview.rs Outdated Show resolved Hide resolved

aaravlu requested a review from Copilot May 13, 2025 02:27

Copilot AI reviewed May 13, 2025

View reviewed changes

aaravlu requested a review from Copilot May 14, 2025 06:05

Copilot AI reviewed May 14, 2025

View reviewed changes

src/home/rooms_list.rs Outdated Show resolved Hide resolved

Add 'is_direct' function for trait 'FilterableRoom'

3e4ef67

aaravlu requested a review from Copilot May 14, 2025 06:16

Copilot AI reviewed May 14, 2025

View reviewed changes

src/home/rooms_list.rs Outdated Show resolved Hide resolved

aaravlu and others added 6 commits May 14, 2025 14:20

Update src/home/rooms_list.rs

6dab7a4

Correct direct messages display when being sortted Co-authored-by: Copilot <[email protected]>

Format code

1011f8e

Merge branch 'main' into fix139-2

2ebca24

Seperate rooms and direct messages when sorting

b13dbf6

Restore redundant changes

cd01514

Merge branch 'main' into fix139-2

71d6a87

ZhangHanDong reviewed May 19, 2025

View reviewed changes

'People' header aboved 'Rooms' header

0eb7b79

aaravlu added 5 commits May 20, 2025 12:25

Improve 'RoomsListUpdate' for direct messages

f384fe9

Remove redundant direct messages check

48a64de

Merge branch 'main' into fix139-2

efcb508

Clarify 'RoomsList's 'draw_walk'

ced416f

Fix typo

aa85b7d

ZhangHanDong self-requested a review May 26, 2025 15:12

		@@ -425,9 +445,9 @@ impl RoomsList {
		}
		RoomsListUpdate::RemoveRoom { room_id, new_state: _ } => {

Populate the dedicated view of direct messages #471

Are you sure you want to change the base?

Populate the dedicated view of direct messages #471

Uh oh!

Conversation

aaravlu commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

aaravlu commented Apr 28, 2025

Uh oh!

alanpoon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaravlu May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaravlu commented May 5, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

ZhangHanDong commented May 19, 2025

Uh oh!

ZhangHanDong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaravlu commented May 20, 2025

Uh oh!

aaravlu commented May 20, 2025

Uh oh!

Uh oh!

aaravlu commented Apr 28, 2025 •

edited

Loading

aaravlu May 5, 2025 •

edited

Loading