-
Notifications
You must be signed in to change notification settings - Fork 29
Search messages in a room #395
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
Search messages in a room #395
Conversation
} | ||
|
||
// Create a global sender/receiver pair for search results | ||
pub static SEARCH_RESULTS_SENDER: OnceLock<crossbeam_channel::Sender<SearchUpdate>> = OnceLock::new(); |
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.
This shouldn't be public ideally, but adding it as a part of the RoomScreen
struct makes it so I would need to add an associated function for making a new RoomScreen
object, which I'm not sure is the right thing to do. I'd appreciate guidance on this!
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.
In general, the right pattern for this kind of thing would be to create a channel when you first show the UI widget/view that will display the search results. Then, the UI widget holds onto to the receiver
side of the channel, but sends the sender
side to the background worker thread as part of the MatrixRequest::SearchRoomMessages
.
That way, the background worker task can use the sender
to send search updates to the UI widget, which will continuously poll the receiver
for new updates upon any signal.
We use this pattern quite often (see how TimelineUpdates are sent from a background task to the RoomScreen UI widget). With this pattern, you don't need a static global variable like the SEARCH_RESULTS_SENDER
here.
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.
Right! That makes sense! Thank you!
Hi, not sure why this is tagged 'waiting-on-author'. Can someone clarify? |
9ef2d9e
to
a58f63a
Compare
Please resolve the conflict. It is 944 commit behind main |
You have marked this as a "Draft PR" , “waiting on author” means waiting for you to prepare this PR. |
I’ve resolved conflicts and merged the latest changes from search-messages-in-room into my local branch, as suggested by @alanpoon (thank you!). The PR currently says "waiting-on-author", but I actually need more input before proceeding. When @kevinaboos suggested using LoadingPane as a reference for structuring the search logic, it hadn't yet been merged into the upstream main branch, so my current logic doesn’t reflect any insights from it. Now that LoadingPane is available, I’m unsure whether I should rewrite the search logic from scratch using it as a guide or adjust what I’ve already written. Some checks are failing, but they seem related to code warnings rather than functional issues, let me know if there’s anything specific you'd like me to address. I want to move forward but need your guidance on how best to approach the search logic now. Could someone review the current flow I outlined in the first comment and advise whether a full rewrite is necessary or if there’s a way to refine what's already there? Thanks! |
hi @smarizvi110, thanks for the contribution, and my apologies for the delay! This PR got lost in the fray. Yes, your logic and diagram look generally correct! I haven't dug into the code deeply yet, but this looks appropriate for unencrypted rooms.
What I was suggesting here is only relevant for encrypted rooms, where the The main idea is to implement a pagination-based search by re-using the same logic that we already use for To implement this, you could change pub enum BackwardsPaginateWhileConditionRequest {
UntilEventFound {
room_id: OwnedRoomId,
target_event_id: OwnedEventId,
starting_index: usize,
current_tl_len: usize,
},
WhileSearchingFor {
room_id: OwnedRoomId,
conditions_to_match: OwnedEventId,
search_update_sender: crossbeam_channel::Sender<SearchUpdate>,
}
} and then handle these two conditions in the This is not a simple feature, so you should probably skip this feature and save it for a future PR. |
When you feel that your basic search functionality (for unencrypted rooms) is complete and ready for review, please request a review from me (in the upper-right corner of this PR screen). |
I suggest you to do self-testing of matrix sdk of search api in a separate test repo and ask question in Robius General Element.io chat group. Create a pull request when the integration test / proof of concept is done. |
Thank you all for the input! Yes, this is a very mind-boggling issue. I will try to incorporate all the feedback, especially alanpoon's suggestion of testing. I suppose I will keep this on the back burner for now like so, and look for a simpler feature to implement. If anyone is already aware of any issue that would be suitable, I'd be grateful for the guidance on that front too! |
@smarizvi110 Hi, could you help to review and test my PR #483 in replacement for this PR? |
Hello @alanpoon. Sure, I'd be happy to! |
Draft PR for adding functionality to search messages, fixes #122
So far, the code is for writing the logic for this. Once that is finalized, work can be done on the UI for it.
After a lot of confusion, this is what I've come up with:
submit_async_request(MatrixRequest::SearchRoomMessages)
.search_events::v3::Request
with search criteria.client.send(search_request, None).await
.event_id
s fromAnyTimelineEvent::{MessageLike, State}
.fetch_details_for_event(event_id)
for each result.SEARCH_RESULTS_SENDER
sendsSearchUpdate::NewResults
.process_search_updates()
updates the UI and redraws.next_batch
exists, callspaginate_search_results()
.MatrixRequest::SearchRoomMessages
withnext_batch
.Could someone verify this flow to be correct? @kevinaboos, I tried to gain some insight through
LoadingPane
as you had mentioned in the comments of #122, but I was not able to find something of that sort to get an idea of going about this. The logic and code in this PR is based on the logic and code for the pagination done for the Timeline in a Room, or at least on what I could understand from it all