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

feat(ui): Add the none filter #2594

Merged
merged 4 commits into from
Sep 21, 2023

Conversation

Hywan
Copy link
Member

@Hywan Hywan commented Sep 21, 2023

A room list can be filtered by using filters. So far, we have:

  • all that accepts Filled and Invalidated room list entries, and rejects Empty entries,
  • fuzzy_match_room_name that runs a fuzzy matcher on the room name,
  • normalized_match_room_name that runs a normalized matcher on the room name.

When the pattern is empty, both the last filters accept the room list entry. That's a long-term well-known philosophical debate. I'm glad that the behavior is the same for both of the filters.

But that's a technical problem for us. There is no way to empty the room list based on a filter. It becomes a technical challenge for the client app implementors:

  • When a user searches for a pattern, before any input is received, the room list must be hidden,
  • When a user starts providing a pattern, the room list must be shown again,
  • When the filter is changed, the client app receives a VectorDiff::Reset { values: [] }, but it might takes time for the app UI to remove all items from the room list, thus resulting a UI blinking glitch between the time the room list is shown, and the time the room list is actually emptied.

The solution is simple though. This patch introduces a new none filter. It rejects all room list entries, thus effectively emptying the room list. The client app won't hide and show the room list again, this logic is no longer necessary. Instead, the new logic is the following:

  • Use the all filter for the “classic view”,
  • User wants to search something: switch to the none filter,
  • User has provided a non-empty pattern: switch to the normalized_match_room_name filter for example,
  • User removes its pattern: switch back to the none filter,
  • User goes back to the “classic view”: switch to the all filter.

It's a matter of switching the filter, which is more robust and deterministic than hiding or showing the room list based on some events etc.


@Hywan Hywan requested a review from a team as a code owner September 21, 2023 07:31
@Hywan Hywan requested review from poljar and stefanceriu and removed request for a team September 21, 2023 07:31
@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.03% ⚠️

Comparison is base (1a15802) 77.84% compared to head (ce2bac6) 77.81%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2594      +/-   ##
==========================================
- Coverage   77.84%   77.81%   -0.03%     
==========================================
  Files         183      184       +1     
  Lines       19608    19610       +2     
==========================================
- Hits        15263    15260       -3     
- Misses       4345     4350       +5     
Files Changed Coverage Δ
...room_list_service/filters/fuzzy_match_room_name.rs 100.00% <ø> (ø)
...matrix-sdk-ui/src/room_list_service/filters/mod.rs 100.00% <ø> (ø)
...list_service/filters/normalized_match_room_name.rs 66.66% <ø> (ø)
...atrix-sdk-ui/src/room_list_service/filters/none.rs 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all well, but I'm not sure I like that we have that many filters.

It sounds like it should be a configuration on the fuzzy matcher filters, i.e. have a setting for them:

  1. Filter mode, starts with a full room list and removes entries that don't match the pattern.
  2. Search mode, starts with an empty room list and adds entries that match the pattern.

If the matching is normalized or not sounds also like it should have been a configuration option. Otherwise we risk the continued proliferation of filters that do almost the same thing.

I think having this as configuration options for a single filter would also make the documentation of this feature easier to follow.

That being said, in case this is immediately needed by EX, perhaps we should just open an issue and refactor and rework the API here later on.

@Hywan
Copy link
Member Author

Hywan commented Sep 21, 2023

I would agree if we would have more filters. I don't believe we would need more filters now, except maybe to filter by DM or by notifications, stuff like that, in a mid-term future. That's not what I consider a lot of filters though (personal opinion).

Adding a Search mode as you suggest is likely to make the code more complex. Right now, the filter is filtering a Stream<Item = RoomListEntry>. If we introduce a Search mode, we would need to complexify this method:

pub fn entries_with_dynamic_adapters(
&self,
page_size: usize,
) -> (impl Stream<Item = Vec<VectorDiff<RoomListEntry>>>, RoomListDynamicEntriesController)
{
let list = self.sliding_sync_list.clone();
let filter_fn_cell = AsyncCell::shared();
let limit = SharedObservable::<usize>::new(page_size);
let limit_stream = limit.subscribe();
let dynamic_entries_controller = RoomListDynamicEntriesController::new(
filter_fn_cell.clone(),
page_size,
limit,
list.maximum_number_of_rooms_stream(),
);
let stream = stream! {
loop {
let filter_fn = filter_fn_cell.take().await;
let (values, stream) = list
.room_list_filtered_stream(filter_fn)
.dynamic_limit_with_initial_value(page_size, limit_stream.clone());
// Clearing the stream before chaining with the real stream.
yield stream::once(ready(vec![VectorDiff::Reset { values }]))
.chain(stream);
}
}
.switch();
(stream, dynamic_entries_controller)
}
}

It's already a switched stream over a filtered + limited stream. Hopefully everything is well-tested, and I trust there is no bug in this code, but if we add a search mode, it makes the whole things more complex, while the solution is a simple none filter.

Alternatively, we can ensure that an empty pattern on fuzzy_… and normalized_… filters actually rejects the room list entry. That would be what one may seem reasonable and expected. As I said, it's almost a philosophical debate, I would be happy to discuss about it.

@Hywan
Copy link
Member Author

Hywan commented Sep 21, 2023

I'm going to merge this PR since it's required as a hotfix for iOS. But happy to discuss about it!

@Hywan Hywan merged commit 0989dd7 into matrix-org:main Sep 21, 2023
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants