Repository navigation
feat(ffi): expose Client.sync() - #6359
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6359 +/- ##
==========================================
- Coverage 89.87% 89.86% -0.01%
==========================================
Files 376 376
Lines 103093 103093
Branches 103093 103093
==========================================
- Hits 92657 92649 -8
- Misses 6863 6872 +9
+ Partials 3573 3572 -1 ☔ View full report in Codecov by Sentry. |
Merging this PR will improve performance by 49.34%
Performance Changes
Comparing |
bnjbvr
left a comment
There was a problem hiding this comment.
Code looks fine to me. I'm a bit curious about how it would be used, as returning the room IDs in the sync responses seem very minimal.
As an opportunity to not flood the client.rs file which is already a bit big, can you move this code to a new sync_v2.rs sub-module?
Also, I think it'd be nice to make it clear that this is the previous sync mechanism, so can you include a v2 suffix to all the methods and types, please? (e.g. SyncSettingsV2, sync_v2(), sync_once_v2(), etc.)
Thanks!
| requiring Sliding Sync support on the homeserver. `Client.sync()` | ||
| accepts a `SyncListener` callback that receives a `SyncResponse` | ||
| after each successful sync. | ||
| ([#6348](https://github.com/matrix-org/matrix-rust-sdk/issues/6348)) |
| pub struct SyncSettings { | ||
| /// Timeout in milliseconds for the server long-poll. | ||
| /// If not set, defaults to 30 seconds. | ||
| #[uniffi(default = None)] |
There was a problem hiding this comment.
Curious, but are these annotations any useful? I'd imagine that uniffi uses the Default::default implementation for each type by default, which will set to None/false for these two types?
There was a problem hiding this comment.
I wanted to provided sensible defaults on the Swift side. This is meant to avoid having to provide a default value when constructing the Swift structure. Otherwise the implementor has to provide the arguments, such as nil in this particular line.
// Without the annotations:
SyncSettings(timeoutMs: nil, fullState: false) // must provide all args
// With #[uniffi(default = ...)]:
SyncSettings() // can provide no args
SyncSettings(timeoutMs: 0) // ... some args only
SyncSettings(fullState: false)
SyncSettings(timeoutMs: 0, fullState: false) // or all argsThere was a problem hiding this comment.
Sweet, thanks for explaining!
| /// Room IDs of joined rooms that had updates. | ||
| pub joined: Vec<String>, |
There was a problem hiding this comment.
Curious about your use case; is it sufficient to only get room IDs here? How do you get new events/timelines for these rooms, out of curiosity?
There was a problem hiding this comment.
Here's the flow I tested and that suits our needs:
- Call
syncand register aSyncListener - When
SyncListener.on_updateis called:- For all
invitedrooms: join viaclient.joinRoomById, then subscribe to the room timeline - For all
joinedrooms: subscribe to the room timeline
- For all
From there, the TimelineListener callback gives me the events, state events, and such.
There was a problem hiding this comment.
@bnjbvr
Note that I am relying on what does not seems like a production-ready API to get the data I need from the event (EventTimelineItem)
event.lazyProvider.debugInfo().originalJsonUnfortunately I could not find another API to get the raw event data. Any suggestions? I was going to create another issue to tackle this specific request.
There was a problem hiding this comment.
Interesting. Out of curiosity, why do you need the raw event data? That makes sense to me for custom events, even though we could probably provide some new variant of TimelineItemContent for those. But for other events, you should have everything you need as part of the EventTimelineItem::content, likely?
There was a problem hiding this comment.
Yes, we need it exactly to handle custom events.
I created a PR and provided explanation there: #6387
|
@bnjbvr I actually started with "v2" in the names, but then reverted back to
Sounds like you're fine with it though, so I will proceed with your suggestion 👍 |
Signed-off-by: Philippe Bertin <pbertin@teladochealth.com>
Link PRs instead of Issues
Rename to sync_v2 and create sub-module
|
The PR is ready for another review. I addressed the requested changes in 6356ef2. Thanks |
bnjbvr
left a comment
There was a problem hiding this comment.
Thank you! Feel free to ask extra questions in our dev room :-)
Expose sync v2 API through FFI via
Client.sync()andClient.sync_once()#6348Mobile clients can now sync using the traditional
/syncv2 endpoint through the FFI bindings, without requiring Sliding Sync (MSC4186) support on the homeserver.This PR introduces a new API (non-breaking change).
Implementation notes
I chose to expose only the list of room IDs (invited, joined, left, knocked) in the
SyncResponse, rather than forwarding the full per-room event payloads. This keeps the FFI surface simple. Clients that need room details can query them through the existingRoomAPIs after being notified of changes via the listener.Two entry points are provided:
Client.sync(settings, listener)starts a continuous sync loop, returning aTaskHandlefor cancellation. TheSyncListenercallback fires after each successful sync response.Client.sync_once(settings)performs a single sync call, useful for initial sync or one-off use cases.SyncSettingsexposestimeout_msandfull_stateBuild
I verified building the
XCFrameworkI also verified integrating this build into our test iOS application and used the new API, with success.
Use of AI
I used AI as a reference while making this change. I reviewed and validated the implementation myself, and take full accountability for it.
Closes #6348
CHANGELOG.mdfiles.