improve search filter capabilities#71
Conversation
Signed-off-by: Jonas Fentker <jonas@fentker.eu>
Signed-off-by: Jonas Fentker <jonas@fentker.eu>
poljar
left a comment
poljar
left a comment
There was a problem hiding this comment.
Looks generally sane, but we should add tests to check if it works, also the search config doesn't seem to allow setting multiple rooms nor senders.
| /// * `room_id` - The unique id of the room. | ||
| pub fn for_room(&mut self, room_id: &str) -> &mut Self { | ||
| self.room_id = Some(room_id.to_owned()); | ||
| self.room_ids = Some(vec![room_id.to_owned()]); |
There was a problem hiding this comment.
We need to allow multiple room ids to be set, no? That is we should allow the for_room method to be called multiple times, every time adding another room to the vec.
There was a problem hiding this comment.
Both seshat-node and radical-native just deserialize the json for the searchConfig, so I didn't add any methods.
But I've now added them for the test 😃
| } else { | ||
| term.to_owned() | ||
| for room in rooms { | ||
| room_parts.push(format!("room_id:\"{}\"", room)) |
There was a problem hiding this comment.
This seems sensible, but did you test that this works as expected? I'm not sure how the query parser reacts to this. As I mentioned, having tests for this would be very welcome.
There was a problem hiding this comment.
I black-box-tested it with radical-native (and now added a test)
| if let Some(senders) = &config.sender_ids { | ||
| let mut sender_parts = Vec::new(); | ||
| keys.push(self.sender_field); | ||
| for sender in senders { |
There was a problem hiding this comment.
Same for the senders as for the multiple room ids.
| parts.push(format!("+({})", room_parts.join(" "))) | ||
| }; | ||
|
|
||
| if let Some(senders) = &config.sender_ids { |
There was a problem hiding this comment.
There doesn't seem to be a method to set the sender in the search config, or am I missing something?
Signed-off-by: Jonas Fentker <jonas@fentker.eu>
|
I tried to get this working with your matrix-react-sdk PR but the tab to select "All rooms" vs "This room" seems to be gone because of it, is there some other way to select which room to search? The screenshot seems to suggest that it's still using the tab. The tests seem sane but I wanted to check this out a bit on live Element before merging. |
|
Updated the OP at matrix-org/matrix-react-sdk#4156 to make it a little more obvious that room filter has been moved to the new autocomplete filter feature |
poljar
left a comment
poljar
left a comment
There was a problem hiding this comment.
Ok, tested this and was confused why it didn't work, we (or rather I) forgot that serde isn't used to create a SearchConfig in the node bindings.
So the node bindings need to be updated as well, something like
if let Ok(k) = argument.get(&mut *cx, "sender_ids") {
if let Ok(k) = k.downcast::<JsArray>() {
let mut ids: Vec<Handle<JsValue>> = k.to_vec(&mut *cx)?;
let ids: Vec<String> = ids
.drain(..)
.filter_map(|id| id.downcast::<JsString>().ok())
.map(|s| s.value())
.collect();
config.for_senders(&ids);
}
}needs to be added somewhere over here.
The new options should also be documented over here.
I found the way we set which room to search in inside of Element a bit confusing, I still haven't figured out how to search in a single DM considering that they won't complete after a # but that's something to figure out on the Element side of things.
2 participants
Companion to matrix-org/matrix-react-sdk#4156