-
Notifications
You must be signed in to change notification settings - Fork 27
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
base: main
Are you sure you want to change the base?
Conversation
The current error in CI with cargo check is because the dependent CommandTextInput component has not been updated to the latest version. |
https://github.com/user-attachments/assets/9f43e74e-7f0d-4ed6-a69c-322396f5a2af
|
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. |
the Makepad PR has been merged in now. |
![]() Add a header and other modifications to the popup user list. But it depends on the new changes of the CommandTextInput component. PR 663. |
Currently, getting room members will cache a list locally. Is there any room for improvement in this solution? @kevinaboos |
There was a problem hiding this 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.
Co-authored-by: Kevin Boos <[email protected]>
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 Additionally, this pull request also depends on the merge of the makepad 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. |
There was a problem hiding this 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.
Co-authored-by: Kevin Boos <[email protected]>
Co-authored-by: Kevin Boos <[email protected]>
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 |
Add basic functionality to the
mention_input_bar
component. For issue #278The
mention_input_bar
component requires these modifications to the CommandTextInput component of Makepad to work :Makepad PR #651TODO for the future: