Show direct rooms under a separate collapsible header#471
Show direct rooms under a separate collapsible header#471kevinaboos merged 32 commits intoproject-robius:mainfrom
Conversation
There was a problem hiding this comment.
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",
|
|
alanpoon
left a comment
alanpoon
left a comment
There was a problem hiding this comment.
The search for the room list does not apply for People.
Fixed. |
There was a problem hiding this comment.
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());
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 |
Correct direct messages display when being sortted Co-authored-by: Copilot <[email protected]>
|
“People” should be placed above “Room” to better maintain consistency with user habits from Element. |
ZhangHanDong
left a comment
ZhangHanDong
left a comment
There was a problem hiding this comment.
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.
Let's fix it in the future pr. |
emmettlu
added
waiting-on-review
kevinaboos
left a comment
kevinaboos
left a comment
There was a problem hiding this comment.
I left several comments.
After a few rounds of discussion, I think the best term to use is "direct room" instead of "direct message(s)". Any time you use direct_message or something similar in the code or comments, please change it to direct_room. The reason is that "direct message" sounds like you're referring to an individual message within a timeline, not to a room itself.
kevinaboos
added
waiting-on-author
|
@kevinaboos Thanks for your review. Also, if it is OK, i want to rebase all the commits' messages to one in this pr and force push, then it could be merge safely. |
emmettlu
added
waiting-on-review
No, please do not ever force push to any branch once you have made that branch public. It will remove all of the github PR comments. |
|
If you want me to squash the PR history down to one commit, I can do that automatically via github's merge process. You don't need to do it manually. But in general, no force pushes unless it is a private branch that nobody else has ever seen/reviewed. |
kevinaboos
left a comment
kevinaboos
left a comment
There was a problem hiding this comment.
There were still several places in which you forgot to handle the new division between direct rooms and non-direct (regular) rooms. I have fixed that (and a lot more) in my latest commit, e.g., several other issues like consistency of naming, formatting, and documentation.
Overall, I'd kindly ask you once again to pay more attention to detail. Your PR in this case was partially correct, which is good, but very incomplete. For example, I had to go through every individual usage of displayed_invited_rooms and displayed_regular_rooms in order to ensure that all code paths also properly considered the new displayed_direct_rooms that you introduced. That attention to detail and consideration of completeness is something that you, not me, should have done, ideally before even requesting a review for this PR.
Kindly take a look at my latest commit d4cdf66 to see what you missed. I hope you can apply careful consideration and procedures to your future PRs.
kevinaboos
added
waiting-on-author
kevinaboos
changed the title
Fix issue #139