fix(base): Rewrite `RoomNotableTags` #3111

Hywan · 2024-02-08T15:51:29Z

This PR must be reviewed commit-by-commit.

This PR entirely revisits the RoomNotableTags API (introduced in #3071 and #3075). The idea is to compute the notable tags as a single 8-bits unsigned integer (u8) and to store it inside RoomInfo. The benefits are numerous:

The type and the API is noticeably smaller that the existing RoomNotableTags,
It's part of RoomInfo so:
- We don't need an async API to fetch the tags from the store and to compute the notable tags,
- It can be put in the store,
- The observable/subscribe mechanism is supported by RoomInfo behind observable instead of introducing a new subscribe mechanism.

The PR ends up providing Room::is_favourite and Room::is_low_priority as simple non-async getters, which is needed by #3021.

Address [Task] Mark a room or dm as favourite #3005
Address [RoomList Filters] Create the "Favourites" filter #3021

I believe this small refactoring makes the code simpler to read, esp. when more event type will be added.

This patch is the first step to remove the [`RoomNotableTags`] API. The idea is to compute the notable tags as a single 8-bit unsigned integer (`u8`) and to store it inside `RoomInfo`. The benefits are numerous: 1. The type is smaller that `RoomNotableTags`, 2. It's part of `RoomInfo` so: - We don't need an async API to fetch the tags from the store and to compute the notable tags, - It can be put in the store, - The observable/subscribe mechanism is supported by `RoomInfo` behind observable instead of introducing a new subscribe mechanism.

The previous patch has introduced the new `NotableTags` type to replace the `RoomNotableTags`. This patch removes the latter. This patch keeps the `Room::set_is_favorite` and `::set_is_low_priority` methods. However, this patch adds the `Room::is_favourite` and `::is_low_priority` methods, with the consequence of entirely hiding the notable tags type from the public API.

…vourite`. The Matrix specification uses the `m.favourite` orthography. Let's use the same in our code. So `set_is_favorite` becomes `set_is_favourite`. This patch updates this in various places for the sake of consistency.

Now that `NotableTags` has replaced `RoomNotableTags` entirely, this patch can rename `NotableTags` to `RoomNotableTags` as it's a better name for it. Note that this type is private to `matrix_sdk_base`, so it's fine to rename it.

crates/matrix-sdk/tests/integration/room/tags.rs

Hywan · 2024-02-08T17:43:03Z

The test is failing because the new way the notable tags are handled in Client::handle_room_account_data updates RoomInfo, but it's overwritten just after.

The following extracts are from the main branch, not my patch, but the idea is exactly the same.

So here, we handle the room account data:

matrix-rust-sdk/crates/matrix-sdk-base/src/client.rs

Lines 844 to 845 in 9bf48ef

    
           self.handle_room_account_data(&room_id, &new_info.account_data.events, &mut changes) 
        
               .await;

A new RoomInfo has been created. But a couple lines below:

matrix-rust-sdk/crates/matrix-sdk-base/src/client.rs

Line 880 in 9bf48ef

changes.add_room(room_info);

the “previous” room_info is saved again, which overwrites our changes.

A solution would be to run handle_room_account_data after the encryption, but I'm not sure it's a good idea. @poljar, @bnjbvr?

This API is really fragile. I'm OK to revisit it in this PR if that's required, or in another PR.

This patch tests the `is_favourite` and `is_low_priority` methods.

The workflow inside `Client::receive_sync_response` is a little bit fragile. When `Client::handle_room_account_data` is called, it modifies `RoomInfo`. The problem is that a current `RoomInfo` is being computed before `handle_room_account_data` is called, and saved a couple lines after, resulting in overwriting the modification made by `handle_room_account_data`. This patch ensures that the new `RoomInfo` is saved before `handle_room_account_data` is called.

This patch rewrites all the integration tests for the notable tags. The tests were good, but we could merge some together. The names were too long, they have been shortened.

codecov · 2024-02-09T08:46:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3e04590) 83.72% compared to head (1052c8d) 83.76%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3111      +/-   ##
==========================================
+ Coverage   83.72%   83.76%   +0.03%     
==========================================
  Files         229      229              
  Lines       23654    23646       -8     
==========================================
+ Hits        19805    19807       +2     
+ Misses       3849     3839      -10

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

bnjbvr

lgtm, thanks!

crates/matrix-sdk/tests/integration/room/tags.rs

bnjbvr · 2024-02-09T10:27:42Z

crates/matrix-sdk-base/src/client.rs

+            // Save the new `RoomInfo`.
+            changes.add_room(room_info);
+
            self.handle_room_account_data(&room_id, &new_info.account_data.events, &mut changes)
                .await;

+            // `Self::handle_room_account_data` might have updated the `RoomInfo`. Let's
+            // fetch it again.
+            //
+            // SAFETY: `unwrap` is safe because the `RoomInfo` has been inserted 2 lines
+            // above.
+            let mut room_info = changes.room_infos.get(&room_id).unwrap().clone();


So the way it's done in the sliding-sync variant of this function, is that we edit the room_info, and don't mess at all with the changes until the end of the function where we put the room info into the changes. We should do that here too, for consistency at least. I'll write a followup.

crates/matrix-sdk-base/src/rooms/mod.rs

bindings/matrix-sdk-ffi/src/room.rs

bnjbvr · 2024-02-09T10:35:00Z

crates/matrix-sdk-base/src/rooms/mod.rs

+    pub fn handle_notable_tags(&mut self, tags: &Tags) {
+        let mut notable_tags = RoomNotableTags::empty();
+
+        if tags.contains_key(&TagName::Favorite) {


haha, now Ruma needs an update for TagName::Favourite too ^^

Signed-off-by: Benjamin Bouvier <public@benj.me>

Signed-off-by: Ivan Enderlin <ivan@mnt.io>

ganfra · 2024-02-13T10:27:17Z

@bnjbvr If I understand correctly, the migration is not handled properly, the existing tags (saved in the account data store) won't be inserted in room info, right?

bnjbvr · 2024-02-13T10:31:00Z

@ganfra Good question; if the tags aren't sent as part of an initial sync response all the time, then yes they might be missing. Otherwise if they're in the initial sync response, we should handle them properly. Are you observing some issues about that?

ganfra · 2024-02-13T10:37:05Z

I've not yet used the new PR, will report any issue with that.

Hywan added 6 commits February 8, 2024 14:33

chore(base): Introduce a small helper to clarify the code.

fab1c1c

I believe this small refactoring makes the code simpler to read, esp. when more event type will be added.

chore(base): Update bitflags.

385e693

chore(base): Rename NotableTags to RoomNotableTags.

8b298df

Now that `NotableTags` has replaced `RoomNotableTags` entirely, this patch can rename `NotableTags` to `RoomNotableTags` as it's a better name for it. Note that this type is private to `matrix_sdk_base`, so it's fine to rename it.

Hywan requested a review from a team as a code owner February 8, 2024 15:51

Hywan requested review from poljar and ganfra and removed request for a team February 8, 2024 15:51

ganfra reviewed Feb 8, 2024

View reviewed changes

crates/matrix-sdk/tests/integration/room/tags.rs Outdated Show resolved Hide resolved

ganfra approved these changes Feb 8, 2024

View reviewed changes

manuroe mentioned this pull request Feb 9, 2024

[Story] Manually mark a room as unread element-hq/element-meta#2201

Closed

9 tasks

Hywan added 5 commits February 9, 2024 09:29

test(base): Test Room::is_favourite and ::is_low_priority.

75454de

This patch tests the `is_favourite` and `is_low_priority` methods.

chore(base): Clean up tests.

e6dadf2

doc(base): Add missing documentation.

61c7a96

chore(base): Remove warnings when e2e-encryption is disabled.

65774aa

Hywan force-pushed the fix-base-notable-tags branch from 5352036 to 65774aa Compare February 9, 2024 08:32

test(sdk): Revisit the tests for notable tags.

bcb125a

This patch rewrites all the integration tests for the notable tags. The tests were good, but we could merge some together. The names were too long, they have been shortened.

bnjbvr approved these changes Feb 9, 2024

View reviewed changes

bnjbvr removed the request for review from poljar February 9, 2024 10:36

nit: change favorite to favourite in a few places

bebb733

Signed-off-by: Benjamin Bouvier <public@benj.me>

bnjbvr enabled auto-merge (rebase) February 9, 2024 10:38

Merge branch 'main' into fix-base-notable-tags

1052c8d

Signed-off-by: Ivan Enderlin <ivan@mnt.io>

auto-merge was automatically disabled February 9, 2024 16:22
Rebase failed

Hywan merged commit 008330a into matrix-org:main Feb 9, 2024
34 checks passed

Hywan mentioned this pull request Feb 19, 2024

feat(ui,ffi): Add the favourite room list filter #3137

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(base): Rewrite `RoomNotableTags` #3111

fix(base): Rewrite `RoomNotableTags` #3111

Hywan commented Feb 8, 2024

Hywan commented Feb 8, 2024 •

edited

Loading

codecov bot commented Feb 9, 2024 •

edited

Loading

bnjbvr left a comment

bnjbvr Feb 9, 2024

bnjbvr Feb 9, 2024

Hywan Feb 9, 2024

ganfra commented Feb 13, 2024

bnjbvr commented Feb 13, 2024

ganfra commented Feb 13, 2024

fix(base): Rewrite RoomNotableTags #3111

fix(base): Rewrite RoomNotableTags #3111

Conversation

Hywan commented Feb 8, 2024

Hywan commented Feb 8, 2024 • edited Loading

codecov bot commented Feb 9, 2024 • edited Loading

Codecov Report

bnjbvr left a comment

Choose a reason for hiding this comment

bnjbvr Feb 9, 2024

Choose a reason for hiding this comment

bnjbvr Feb 9, 2024

Choose a reason for hiding this comment

Hywan Feb 9, 2024

Choose a reason for hiding this comment

ganfra commented Feb 13, 2024

bnjbvr commented Feb 13, 2024

ganfra commented Feb 13, 2024

fix(base): Rewrite `RoomNotableTags` #3111

fix(base): Rewrite `RoomNotableTags` #3111

Hywan commented Feb 8, 2024 •

edited

Loading

codecov bot commented Feb 9, 2024 •

edited

Loading