SDK: UISI AutoReporting #1386
SDK: UISI AutoReporting #1386
Conversation
…angleyd/5017_uisi_autoreporter
langleyd
requested review from
a team,
gileluard and
BillCarsonFr
and removed request for
a team
…angleyd/5017_uisi_autoreporter
| @param onComplete the block called when the operation completes. | ||
| */ | ||
| - (void)handleRoomKeyEvent:(MXEvent*)event onComplete:(void (^)(void))onComplete; | ||
| - (void)handleToDeviceEvent:(MXEvent*)event onComplete:(void (^)(void))onComplete; |
There was a problem hiding this comment.
This may be an important breaking change. Is it OK for you @manuroe ?
There was a problem hiding this comment.
Just for a bit of context. The function becomes a bit more general, so not just handling RoomKeyEvent but handles toDeviceEvents generally. The code to handle RoomKeyEvent is unchanged. The naming also made some of the code read better like here. It also matches the android implementation now.
There was a problem hiding this comment.
From chatting to Gil I think functionally we are happy with the changes, it was just the breaking change. I think I'm happy with the breaking changed based off previous conversations with @manuroe . But will catch up with him when he's back.
There was a problem hiding this comment.
I am not against the change of this API. Plus, it seems it is used only internally.
But I wonder if we really this method. See my review comment.
| /// Manages the adding, removing MXLiveEventListeners and dispatching of events to those listeners. | ||
| @objcMembers public class MXEventStreamService: NSObject { | ||
|
|
||
| var listeners = [MXLiveEventListener]() |
There was a problem hiding this comment.
If MXLiveEventListener subclasses NSObject we could use then NSHashTable<ObjectType>.weakObjectsHashTable() and that could be even safer.
There was a problem hiding this comment.
Oh yea nice and MXMulticastDelegate looks like it is a perfect fit here.
…k references to the listeners.
| onComplete(); | ||
| break; | ||
| } | ||
| [self.mxSession.eventStreamService dispatchOnLiveToDeviceWithEvent:event]; |
There was a problem hiding this comment.
mxSession is already a hard dependency in crypto, i just re-used it.
Android handles toDeviceEvents in crypto like this and it made the caller site clearer in some instances as we were passing a toDevice variable.
But I really don't feel strong about it, so happy to move it back, Will create a new PR.
Accompanies app PR element-hq/element-ios#5743
To address element-hq/element-ios#5017