Skip to content
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

[Feature] Add basic functionality to the mention_input_bar component #372

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

Conversation

ZhangHanDong
Copy link
Contributor

@ZhangHanDong ZhangHanDong commented Feb 1, 2025

Add basic functionality to the mention_input_bar component. For issue #278

The mention_input_bar component requires these modifications to the CommandTextInput component of Makepad to work :Makepad PR #651

2
3

TODO for the future:

  1. Add a header to the user list to display the current number of users in the room.
  2. Implement scrolling functionality for the user list.
  3. Enable sorting for the user list to show currently online users.
  4. Optimize performance and add a loading animation for the user list.

@ZhangHanDong
Copy link
Contributor Author

The current error in CI with cargo check is because the dependent CommandTextInput component has not been updated to the latest version.
This PR needs to be merged. Makepad PR #651

@ZhangHanDong ZhangHanDong added the waiting-on-review This issue is waiting to be reviewed label Feb 6, 2025
@alanpoon
Copy link
Contributor

Screenshot 2025-02-10 at 3 19 16 PM

  1. Mentionable List should not appear when only the "@" is being typed, it should appear only after "@ and some match characters"
  2. There should be a header "Users" at the top of the mention-able List
  3. Mentionable List seems to contain current user and does not contain one remaining room user.

mention2
The typing textbox needs to also be able to show avatar, after selecting the mentioned user.
5.
mention3
The typing textbox is not able to send the correct format for the mentioning the user. After mentioning the user, the message should show a hyperlink for the mentioned user.

https://github.com/user-attachments/assets/9f43e74e-7f0d-4ed6-a69c-322396f5a2af
The highlighted background by the mouse should be same color as default highlighted mentionable user/

  1. In the mentionable List, the user_id should have a "@" at the front.

@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 Feb 11, 2025
@ZhangHanDong
Copy link
Contributor Author

ZhangHanDong commented Feb 11, 2025

Screenshot 2025-02-10 at 3 19 16 PM

  1. Mentionable List should not appear when only the "@" is being typed, it should appear only after "@ and some match characters"
  2. There should be a header "Users" at the top of the mention-able List
  3. Mentionable List seems to contain current user and does not contain one remaining room user.

mention2 The typing textbox needs to also be able to show avatar, after selecting the mentioned user. 5. mention3 The typing textbox is not able to send the correct format for the mentioning the user. After mentioning the user, the message should show a hyperlink for the mentioned user.

https://github.com/user-attachments/assets/9f43e74e-7f0d-4ed6-a69c-322396f5a2af The highlighted background by the mouse should be same color as default highlighted mentionable user/

  1. In the mentionable List, the user_id should have a "@" at the front.

I don’t quite understand the reason for the first point.

I will continue to address the suggestions 2/6/7.

The others can be considered as features to be added in the future. This is just the initial version.

@kevinaboos
Copy link
Member

The current error in CI with cargo check is because the dependent CommandTextInput component has not been updated to the latest version. This PR needs to be merged. Makepad PR #651

the Makepad PR has been merged in now.

@ZhangHanDong
Copy link
Contributor Author

Screenshot 2025-02-24 at 18 30 57

Add a header and other modifications to the popup user list. But it depends on the new changes of the CommandTextInput component. PR 663.

@ZhangHanDong
Copy link
Contributor Author

Currently, getting room members will cache a list locally. Is there any room for improvement in this solution? @kevinaboos

@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 Feb 24, 2025
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, great start on this complex feature!

First, there are quite a few places where you're using chars/characters, e.g., in find_mention_trigger_position(). We can't use characters, we always must use unicode graphemes instead.

Second, I don't think we need a cache for the list of members in a room -- that's not something that is used frequently, so we shouldn't really cache it. The room user list will be cached internally by the client's EventCache, so we don't really want to cache it separately.
We can just store the latest list of Vec<RoomMember>s in the MentionInputBar's room_members field (and optionally, also in the TimelineUiState struct if needed), we don't need a full cache.

@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 Feb 25, 2025
@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 Mar 3, 2025
@ZhangHanDong
Copy link
Contributor Author

ZhangHanDong commented Mar 9, 2025

@kevinaboos

I have extracted the MentionableTextInput component and added the @ user functionality to the EditPane component.

To share the Members in the MentionableTextInput component across multiple components, I added the RoomMemberManager module, which provides a sub/pub mode, allowing multiple components dependent on the MentionableTextInput component to share member data without copying. The components currently sharing the MentionableTextInput component are RoomInputBar and EditPane.

The basic functionality is complete and the code can be reviewed first. Currently, the room_screen code is too large, and it needs to be merged as soon as possible to avoid too many conflicts, which would be difficult to modify.

Additionally, this pull request also depends on the merge of the makepad CommandTextInput pull request #663.

Finally, to unify everyone’s Rust fmt format, I added a rustfmt.toml, so that running cargo fmt locally will no longer result in a large number of unrelated files being modified.

@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 Mar 9, 2025
@ZhangHanDong ZhangHanDong requested a review from kevinaboos March 9, 2025 18:07
@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 Mar 9, 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 Mar 10, 2025
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, overall I do think the PR looks good. Is it intended to be merged into main in the current state? There are tons of log statements and some things look a bit unfinished -- were you leaving those in until after my final approval, or should i remove myself or let you remove them now before final approval?

My main comment is that I don't think we need such a complicated solution for subscribing to a changed list of room members, since we already have so many mechanisms for inter-thread/task communication (actions, existing queues, channels), .... but we can always simplify that later, so let's not worry about it now. Plus, the code quality of your implementation is very good, so I'm inclined to leave it in for now since we may have future needs for inter-widget synchronization that actually justify that complex pub-sub implementation.

I appreciate the rustfmt configuration, but ideally we would have left those changes to be a separate PR. There are just so many unrelated formatting changes in this PR now that it makes it very hard for me to review the actual functionality changes in isolation from trivial formatting changes.

Can you undo a lot of the unrelated and unnecessary formatting changes?

Anyway, since this PR does introduce formatting config, my hand was forced so I did end up going through the entire docs of rustfmt. I have some thoughts and left comments in certain places in the code; also, here are some config options I'd like to add:

inline_attribute_width: 50 (or higher, we'll see)
imports_granularity: Crate
group_imports: StdExternalCrate

fn_single_line: true
format_generated_files: false

blank_lines_upper_bound: 3
hex_literal_case: Upper
reorder_modules: false
spaces_around_ranges: true
use_try_shorthand: true

Note: please don't apply rustfmt to all files in this PR -- let's do that later.

@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 Mar 10, 2025
@ZhangHanDong
Copy link
Contributor Author

ZhangHanDong commented Mar 13, 2025

I have made the corrections according to your review comments. @kevinaboos

Regarding the introduction of the sub/pub subscription pattern, I also wanted to take this opportunity to try out whether this solution is feasible, in preparation for future needs.

The rustfmt.toml content has also been added with the configurations you wanted to include. I did not run cargo fmt for this commit. Let’s keep the rustfmt.toml for future use.

@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 Mar 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-review This issue is waiting to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants