Refactor components related to mentioning users. Support @room mentions.#496
Conversation
kevinaboos
left a comment
There was a problem hiding this comment.
Thanks Alex!
It looks like there may have been some merge conflicts that were resolved incorrectly. Please ensure that any new changes i made as part of #480 are kept in the code. I identified a few places where it looks like that happened, but I might've missed some others.
The logic in the MentionableTextInput is just wayyyy too complicated. We can't be doing that many complex checks upon each inputted character. See my comments there for ways that it can be massively simplified.
I think I may have underestimated how many pervasive changes are required to pass room member info down through all of the various functions --- i was thinking that it's just the event handler and draw routines, but I forgot about all of the functions that populate widget data, like Avatar methods and the other functions in the RoomScreen.
I wonder if we should rethink the way that the room member list is stored, or if we should keep it as it currently is in this PR, in which we just pass it to every function that needs it.
For now, can you kindly remove all of the usages of the room_members_opt that aren't absolutely mandatory? All of these extra changes where you use the room members list instead of the user profile cache are not strictly necessary, and it makes it hard for me to review this. For example, don't use it in the avatar.rs or room_read_receipt.rs files yet --- we can add that later. For now, only use the room member list in the code components related to mentioning users. That way, it will be very clear where we actually need the room members list to be passed into a function.
kevinaboos
left a comment
There was a problem hiding this comment.
Thanks, this looks much better now. I still think things can be simplified a lot more, and I left some detailed comments where I think that is the case.
-
First, one request: can you please disable your IDE's code auto-formatter when working on Robrix (for all future PRs)? A large majority of the changes here are just formatting changes, which makes it very hard on me as the reviewer to determine what semantics actually changed.
-
Second: when using scope with code like
scope.props.get::<RoomScreenProps>(), we actually want tounwrap()that in order to cause a panic, as it should be considered a developer bug if a function expects something to be passed down viaScopebut it actually isn't there.- Please check all the places you access scope by downcasting it to a specific type (e.g.,
RoomScreenProps) and ensure they're usingunwrap()(orexpect()if you want).
- Please check all the places you access scope by downcasting it to a specific type (e.g.,
-
Third, a general question: there are tons of new log comments everywhere in this PR, most of which seem like they're just there as leftovers from your development work. Please remove all of the logs that aren't useful enough to be seen by all future developers & users, in order to keep the Robrix log brief enough to be easily readable.
Co-authored-by: Kevin Boos <[email protected]>
Co-authored-by: Kevin Boos <[email protected]>
…splay_name and room_avatar_url to RoomScreenProps
|
This PR Design Discussion Points
I reviewed the code again using this checklist, and there shouldn’t be any issues now. @kevinaboos |
|
I’ll remove the room member subscription first and review it later. @kevinaboos |
ok yea, sounds good, thanks for remembering that. I'll wait for your notice to review it. |
|
@kevinaboos |
kevinaboos
left a comment
There was a problem hiding this comment.
Thanks Alex! Looks good and it works well, with a few small caveats. I'll merge this in now, but kindly fix these small issues in another PR as soon as possible.
When there are no matching users, we should not close the mentionable popup, because that makes it seem like the popup has unexpectedly crashed (from the user's perspective).
Previously, the mentionable popup would still be shown if there were no matching users, which is much better UX because it clearly conveys to the user that there are no matches.
A side effect of this incorrect behavior is that if you make a typo when searching, or search for a name that doesn't exist, the mentionable popup will remain permanently closed until you remove all characters after the @, at which point the popup will be shown again.
We must keep the popup open until the user either selects a mention or deletes the original @ trigger character. This will enable the user to correct their search string if no matching users are found.
This PR is a rewrite of PR #463.
Fixes #456.
Fixes #464.
Fixes #481.
Main improvements:
@room feature.Details of Performance Optimization:
Problem: When a room has more than 2,000 users, rendering the mention popup causes the app to lag and show a loading spinner.
Performance Bottlenecks:
Solution:
Performance Improvements: