feat: add a new NotificationClient API 📬
#2235
Conversation
| /// | ||
| /// In particular, it takes care of running a full decryption sync, in case the | ||
| /// event in the notification was impossible to decrypt beforehands. | ||
| pub struct NotificationClient { |
There was a problem hiding this comment.
Since we are in matrix_sdk_ui, I think it makes sense to rename the type to simply Notification. We don't have SlidingSyncClient nor RoomListClient after all :-).
There was a problem hiding this comment.
I don't like it, we'd be using the same type name for different things in different places.
There was a problem hiding this comment.
Do you have preferences for the names? The obvious ideas don't sound really good to my ears either: NotificationHandler, NotificationResolver, NotificationService (despite me not loving the Service suffix, there's an argument to be made that App could be SyncService and that would make for a nice parallel).
Maybe NotificationResolver isn't the worst?
There was a problem hiding this comment.
I think the Client suffix is fine, it's a Matrix client the notification process should use. As such NotificationClient or NotificationServiceClient sounds fine to me.
Agree with @jplatte that Notification is too ambiguous and already used.
There was a problem hiding this comment.
Alright, let's keep NotificationClient for that one then!
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2235 +/- ##
==========================================
- Coverage 76.87% 76.71% -0.17%
==========================================
Files 170 171 +1
Lines 17944 18027 +83
==========================================
+ Hits 13795 13829 +34
- Misses 4149 4198 +49
☔ View full report in Codecov by Sentry. |
…failed the first time
Dead code is dead.
… event decryption
bnjbvr
force-pushed
the
notification-client-api
branch
from
e8070df to
20a4749
Compare
|
This was tested and approved by the iOS folks over at #2204, which stacks on top of this PR. |
| /// | ||
| /// In particular, it takes care of running a full decryption sync, in case the | ||
| /// event in the notification was impossible to decrypt beforehands. | ||
| pub struct NotificationClient { |
There was a problem hiding this comment.
I don't like it, we'd be using the same type name for different things in different places.
This is extracted from #2204, and this implements a proper
NotificationClient, that will take care of handling event decryption for notification events. It's also more logical to have something like this in the UI crate, instead of having custom code at the FFI layer.For now, the FFI code has been moved, and the encryption sync usage for the notification decryption has been included there. In the future, I'm planning to add more, like properly handling invites, and more goodness to address #2179.
I'd like to include tests too, but might be nice to do as a follow-up to not inflate this PR too much.
Fixes #1962.