Skip to content

feat: Add the @room feature #463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

ZhangHanDong
Copy link
Contributor

Increase the @room in the mention user list. Fixed issue #456

Screenshot 2025-04-10 at 16 14 05

Screenshot 2025-04-10 at 16 13 53

Screenshot 2025-04-10 at 16 14 22

@ZhangHanDong ZhangHanDong added the waiting-on-review This issue is waiting to be reviewed label Apr 10, 2025
@ZhangHanDong ZhangHanDong marked this pull request as draft April 10, 2025 08:25
@ZhangHanDong ZhangHanDong added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Apr 10, 2025
@ZhangHanDong ZhangHanDong added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Apr 10, 2025
@ZhangHanDong ZhangHanDong requested a review from kevinaboos April 10, 2025 13:54
@ZhangHanDong ZhangHanDong marked this pull request as ready for review April 10, 2025 13:55
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

This PR has a lot more code changes than I expected to see, which indicates to me that the design is overly complicated.

In general, I don't think the EditingPane, RoomScreen, RoomInputBar, and the MentionableTextInput all need to be aware of changes to the room user list, room power levels, and other stuff.
That's just way too much duplication of state.... and these states aren't small, they could be massive lists.


There is also a lot of duplication in how we are updating states in widgets. For example, to update power levels, you tend to both emit an action and directly call a method that sets power levels. We only need to do one of those, and the action approach is better because each widget can handle it independently.


Speaking of too many actions, I actually don't see any reason why MentionableTextInputAction is necessary at all.

  1. The docs say "Actions emitted by the MentionableTextInput component", but that seems to be backwards. Most of those actions are emitted by other widgets, and consumed by the MentionableTextInput widget.
  2. None of them seem to be actually required for correct operation?

Finally, there are too many Matrix-level requests being made, which are mostly redundant with each other. It seems like every widget listed above (EditingPane, RoomScreen, RoomInputBar, MentionableTextInput) are all making new requests to get the room members and the room power levels upon every time they are opened. That gets expensive very fast, not to mention is a huge bandwidth (and energy) cost.

Instead, we only need to make those requests once when the room screen is opened, and then each widget can handle any actions that update those lists as they are received from the backend.
EDIT: this is wrong, we do need to make the request to get the latest full user list every time a MentionableTextInput widget is shown, but not all of the above 4 widgets need to make the same request individually. I think your room user list subscriber should make that easy, though.

@kevinaboos kevinaboos added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Apr 10, 2025
@ZhangHanDong ZhangHanDong added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Apr 11, 2025
@ZhangHanDong
Copy link
Contributor Author

@kevinaboos

Based on your suggestions, I've refactored the code for these components:

  1. Eliminated duplicate matrix requests and redundant code.
  2. Centralized state management in mentionable_text_input, while room_input_bar and editing_pane no longer store or process any state, resulting in better consistency.

@ZhangHanDong ZhangHanDong requested a review from kevinaboos April 11, 2025 07:43
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice, looks much better! I like the reduction in complexity of the RoomInputBar and other widgets.

I left a few comments about memory efficiency and looking for @room strings.

(
RoomMessageEventContent::text_html(html_text, html_text),
self.view.room_input_bar(id!(input_bar))
.mentionable_text_input(id!(message_input))
.get_real_mentions_in_html_text(html_text),
html_text.contains("@room")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while this technically works, I think it'll produce a lot of false positives. For example, if someone just typed "`@room`" without selecting the actual room mention from the pop-up users list, they probably don't want to trigger a room mention. (For example, if someone is quoting a message that had @room in it, the text of their new message will indeed contain "@room" but shouldn't be treated as a real mention.)

I think what we actually want to do is to change the get_real_mentions_in_*_text() functions to return a Mentions struct. This would also require adding a field like possible_room_mention: bool to the MentionableTextInput (alongside the existing possible_mentions field).

@kevinaboos kevinaboos added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author This issue is waiting on the original author for a response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants