Skip to content

[WIP] handle events that update Room state#2

Merged
poljar merged 29 commits into
matrix-org:masterfrom
DevinR528:room-name
Mar 31, 2020
Merged

[WIP] handle events that update Room state#2
poljar merged 29 commits into
matrix-org:masterfrom
DevinR528:room-name

Conversation

@DevinR528
Copy link
Copy Markdown
Contributor

Add calculate_room_name(s) methods.

@DevinR528
Copy link
Copy Markdown
Contributor Author

The eventual goal would be feature parity with the js client, correct? If you don't mind I'd like to work on adding the necessary handling and structures, as long as I stay away from the encryption stuff, could I start moving the structs into their own files, something like

- models
  - room
  - member
  - ect...

and as you said in the issue, for each bit of event handling adding a corresponding method on the client.

@poljar
Copy link
Copy Markdown
Contributor

poljar commented Mar 28, 2020

could I start moving the structs into their own files, something like

Yes, moving those into separate files is a good idea.

The eventual goal would be feature parity with the js client, correct?

We don't necessarily want everything from the js-sdk, it has methods for automatic pagination which were a bit tricky to modify to take events from differing sources since the Timeline class makes requests on its own. Obviously borrowing rules of Rust will make it a bit tricky to mimic all of the js-sdk as well.

As for how the SDK should be designed I'm more mimicking matrix-nio which has a layered design with a state machine as the heart of the lib. Ruma plays nicely with our goal of being a layered SDK where the IO transport is only added at the last user facing layer as well.

But all of the methods to allow people to easily get user/room display names should be implemented.

for each bit of event handling adding a corresponding method on the client.

The base client already knows how to receive sync responses and forwards events to rooms. Rooms only need to know how to handle each and every event, at least the ones that modify the room state.

Please note that we'll want to support a couple of client types:

  • Clients that do not care about display names at all, they only want a minimal state for encryption to work, Pantalaimon is such a use-case.
  • Clients that want all the batteries included and want the rust-sdk to handle the state, bots are usually this way or single threaded clients.
  • Clients that want the rust-sdk to handle a minimal state for encryption to work but want to handle the full state themselves, (e.g. so they can access room display names in a lock-less manner, the rust-sdk syncs in one thread and forwards events to be displayed to the UI thread), weechat-matrix will be this way.

@DevinR528
Copy link
Copy Markdown
Contributor Author

Should I start a new PR or just amend the name of this one and the fact that it'll be a WIP?

@poljar
Copy link
Copy Markdown
Contributor

poljar commented Mar 28, 2020

You can continue on this one. There are a couple of merge conflicts now since I merged the latest crypto work but they seem to be quite easy ones, sorry about that.

@DevinR528 DevinR528 changed the title Room name [WIP] handle events that update Room state Mar 28, 2020
@DevinR528
Copy link
Copy Markdown
Contributor Author

Don't worry about me!!

Comment thread tests/async_client_tests.rs Outdated
@DevinR528
Copy link
Copy Markdown
Contributor Author

I wanted to see if this works before I merge in all my event addition work... Oh it cargo test but it doesn't cargo build something about conflicting versions.

@poljar
Copy link
Copy Markdown
Contributor

poljar commented Mar 28, 2020

Conflicting version for what? We can get those quite often for one of the ruma crates, since they depend on each other, though latest master builds fine for me and travis as well.

@DevinR528
Copy link
Copy Markdown
Contributor Author

Hmm that's odd it seems to have completely gone away, it was something about ruma-client-api and ruma-events not getting along. The Endpoint trait had two conflicting versions but everything now works perfect now of course 🤣.

@poljar
Copy link
Copy Markdown
Contributor

poljar commented Mar 28, 2020

Yeah that's a common one, if one of the ruma(-client-api?) crates depends on a different ruma(-api?) version than we do.

@DevinR528
Copy link
Copy Markdown
Contributor Author

Yeah I started using cargo tree and it was pretty obvious all those ruma-* crates are delicately balanced.

Ok here is my work on folder structure and keeping the Room struct updated with events. As long as this all works, my next task is to add power level functionality.

@DevinR528
Copy link
Copy Markdown
Contributor Author

Ok I'm gonna say this is ready for review. A few questions

  • what access level do you want for the structs in the models folder now they are pub
  • I left the callbacks in the sync method so I can remove them in the trait PR
  • There is now timeline_event, state_event, presence_event and account_data which covers most of the IncomingResponse object.

Copy link
Copy Markdown
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.

Mostly looks fine, a couple changes needed. I didn't go into the detail if display name calculation is correct if there are issues with it we can fix them later on and unit tests and real world usage should reveal those soon enough.

Comment thread src/async_client.rs Outdated
Comment thread src/async_client.rs Outdated
Comment thread src/base_client.rs Outdated
Comment thread src/base_client.rs Outdated
Comment thread src/event_emitter/mod.rs Outdated
Comment thread src/models/room_member.rs Outdated
Comment thread src/base_client.rs Outdated
Comment thread src/models/room.rs Outdated
Comment thread src/models/user.rs
/// If the user should be considered active.
pub currently_active: Option<bool>,
/// The events that created the state of the current user.
// TODO do we want to hold the whole state or just update our structures.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know what the right answer is. Redacting membership events is a valid thing to do in Matrix, e.g. if someone changes the display name to something offensive. If we hold the full event chain here we can easily go back to the previous state, though we are not guaranteed to have the full chain anyways. So perhaps holding on to the previous display name and avatar URL and event ids so we know when redactions mess things up is enough. Lets leave it as is for now and optimize later.

assert!(client.sync_token().await.is_some());
}

#[tokio::test]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add some more tests? This already adds quite a bit functionality and we're not covering much.

E.g. room display name calculation isn't tested at all. We track coverage with tarpaulin, it just isn't yet set up for CI.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm having troubles installing tarpaulin but I think I may have just figured it out.

Do you want me to add internal tests for the pub(crate) methods, for Room, RoomMember, the base client, ect.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you have the time, sure. The more tests the better.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I got tarpaulin to install yay! haha stupid libssh

@DevinR528
Copy link
Copy Markdown
Contributor Author

I still have to add tests.

@DevinR528
Copy link
Copy Markdown
Contributor Author

the coverage report

[INFO tarpaulin] Coverage Results:
|| Tested/Total Lines:
|| src/async_client.rs: 115/229
|| src/base_client.rs: 88/156
|| src/crypto/error.rs: 0/2
|| src/crypto/machine.rs: 0/300
|| src/crypto/memory_stores.rs: 0/27
|| src/crypto/olm.rs: 0/83
|| src/crypto/store/sqlite.rs: 0/202
|| src/error.rs: 0/4
|| src/models/room.rs: 67/111
|| src/models/room_member.rs: 45/58
|| src/models/user.rs: 30/45
|| tests/async_client_tests.rs: 49/49
|| 
31.12% coverage, 394/1266 lines covered

It appears that tarpaulin relies on libssl-dev and matrix-rust-sdk relies on libssl-dev so every time I get one to work the other fails and I have to reinstall the respective version?

Copy link
Copy Markdown
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.

I think we can merge after the splitting on : thing is clarified or fixed.

Comment thread src/base_client.rs Outdated
Copy link
Copy Markdown
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.

Looks good, will merge soon.

@poljar
Copy link
Copy Markdown
Contributor

poljar commented Mar 31, 2020

Can I just get a sign-off from you. Just a line stating that you're the author like it's done here.

@DevinR528
Copy link
Copy Markdown
Contributor Author

Signed-off-by: Devin Ragotzy devin.ragotzy@gmail.com

@poljar poljar merged commit 453c9f3 into matrix-org:master Mar 31, 2020
@poljar poljar mentioned this pull request Apr 3, 2020
jplatte pushed a commit that referenced this pull request Dec 23, 2022
jmartinesp added a commit that referenced this pull request Dec 18, 2024
# This is the 1st commit message:

feat(room): add `Room::room_member_updates_sender`

This sender will notify receivers when new room members are received: this can happen either when reloading the full room member list from the HS or when new member events arrive during a sync.

The sender will emit a `RoomMembersUpdate`, which can be either a full reload or a partial one, including the user ids of the members that changed.

# The commit message #2 will be skipped:

# Fix test in wasm
stefanceriu added a commit that referenced this pull request Aug 29, 2025
stefanceriu added a commit that referenced this pull request Aug 29, 2025
stefanceriu added a commit that referenced this pull request Aug 29, 2025
stefanceriu added a commit that referenced this pull request Aug 29, 2025
Hywan added a commit to Hywan/matrix-rust-sdk that referenced this pull request Nov 11, 2025
Hywan added a commit to Hywan/matrix-rust-sdk that referenced this pull request Nov 11, 2025
Hywan added a commit to Hywan/matrix-rust-sdk that referenced this pull request Nov 18, 2025
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