Rewrite ARTEventEmitter per the spec and adjust Connection. #179

tcard · 2016-02-01T15:38:10Z

Previously, ARTConnection managed state subscriptions and
ARTEventEmitter was just a shell. Now the heavy-lifting of
passing events around is done by ARTEventEmitter, which is
generic and can be reused by others, and ARTConnection just uses
it to emit events and expose its listening interface.

This also introduces ARTConnectionStateChange, used in connection
state events' callbacks, as the spec says.

Something that went away is that starting listening to events
dispatched the current event to the callback. Now it doesn't. It
starts listening to changes from that point on. I think this
makes more sense and is consistent with what you expect from an
event emitter.

Fixes #133.

tcard · 2016-02-01T16:53:49Z

I didn't run the Objective-C test suite. Sorry. Working on that now.

tcard · 2016-02-01T18:52:07Z

Passing:

ricardopereira · 2016-02-02T07:18:10Z

ably-ios/ARTEventEmitter.h

+// This way you can automatically "implement the EventEmitter pattern" for a class
+// as the spec say. It's supposed to be used together with ART_EMBED_IMPLEMENTATION_EVENT_EMITTER
+// in the implementation of the class.
+#define ART_EMBED_INTERFACE_EVENT_EMITTER(EventType, ItemType) - (__GENERIC(ARTEventListener, ItemType) *)on:(EventType)event call:(void (^)(ItemType))cb;\


Why did you not use a protocol?

Well, I'm thinking about it and maybe it is not possible using objc protocols with generics?

Can protocols implement methods and have properties? I think they were just interfaces that implementors had to, well, implement themselves.

Yes, you can have properties. I guess you can have the protocol for the interface and have the macro for the implementation.

In any case, a protocol suggests that classes can (should?) bring their own implementation for the protocol, when in fact that's not the case here. In this case, they should just provide access to the ARTEventEmitter implementation.

I'd prefer not to overcomplicate the thing with a protocol besides the class.

mattheworiordan · 2016-02-02T09:24:14Z

starts listening to changes from that point on. I think this
makes more sense and is consistent with what you expect from an
event emitter.

@tcard yes, that is correct. FYI, we have an unspecified equivalent of how this used to work though, see https://github.com/ably/ably-js/blob/master/common/lib/util/eventemitter.js#L160-L176 and https://github.com/ably/ably-ruby/blob/master/lib/ably/modules/state_emitter.rb#L61-L103. Obviously this is out of spec so not needed

mattheworiordan · 2016-02-02T09:26:15Z

ably-ios/ARTEventEmitter.h

- (instancetype)initWithRealtime:(ARTRealtime *)realtime;
- (id<ARTSubscription>)on:(ARTRealtimeConnectionStateCb)cb;
- (void)removeEvents;
+- (__GENERIC(ARTEventListener, ItemType) *)onAll:(void (^)(ItemType))cb;


Based on our previous discussions about method overloading, I thought we had preferred to use overloading in the end. If so, why do we have onAll as opposed to on(void (^)(ItemType))cb?

Overloading is problematic when there is a different number of arguments. Plus, I think we agreed on doing the idiomatic thing... See #158 (comment)

I thought we agreed on something quite different, see #121 (comment). I thought #158 was an exception because it was an initializer / constructor?

Hmm, you're right, I didn't consider that Objective-C allows overloading with different number of arguments. My previous comment makes no sense. Will change.

Fixed at 8aa38f6.

tcard · 2016-02-04T21:14:54Z

Rebased, plus two changes: 5cba6ca and 43f6833.

@ricardopereira @mattheworiordan PTAL

mattheworiordan · 2016-02-08T11:08:09Z

ably-ios/ARTEventEmitter.h

+- (__GENERIC(ARTEventListener, ItemType) *)once:(void (^)(ItemType __art_nullable))cb;\
+\
+- (void)off:(EventType)event listener:(__GENERIC(ARTEventListener, ItemType) *)listener;\
+- (void)off:(__GENERIC(ARTEventListener, ItemType) *)listener;


Is there not also meant to be another overloaded off without any params?

(RTE5) EventEmitter#off deregisters a listener for the specified event if given, else it is deregisters that listener for all registrations (global and event-specific)

I understand from that that there is at least a listener parameter.

Fixed at e9f6153997cd248376bd0db611c8e77fad733e08.

mattheworiordan · 2016-02-08T11:18:43Z

LGTM, only thing missing from what I can see is an off without any args to remove all listeners

Previously, ARTConnection managed state subscriptions and ARTEventEmitter was just a shell. Now the heavy-lifting of passing events around is done by ARTEventEmitter, which is generic and can be reused by others, and ARTConnection just uses it to emit events and expose its listening interface. This also introduces ARTConnectionStateChange, used in connection state events' callbacks, as the spec says. Something that went away is that starting listening to events dispatched the current event to the callback. Now it doesn't. It starts listening to _changes_ from that point on. I think this makes more sense and is consistent with what you expect from an event emitter. Fixes #133.

See ably/ably-js#227 (comment).

Also, remove "ignored" list for total listeners; `off` will just be a no-op if called with a specific event and a total listener. See: ably/docs#82 #179 (comment)

#179 (comment)

ricardopereira · 2016-02-09T08:26:27Z

ably-ios/ARTEventEmitter.m

+    }
+    @finally {
+        for (ARTEventEmitterEntry *entry in toRemoveFromListenersForEvent) {
+            [listenersForEvent removeObject:entry];


It only removes the occurrence in that array (listenersForEvent).
I think it should use self.listeners.

Hey, good catch. Although I'm not sure what you mean. This should be calling [removeObject:entry fromArrayWithKey:event inDictionary:self.listeners], because that handles the case in which self.listeners[event] is left empty and needs to be deleted to avoid entry leaks. I fixed that at 859e474.

I didn't thought about the special case when I wrote about it but looks great. 👍

mattheworiordan · 2016-02-09T11:58:24Z

LGTM 👍

https://github.com/ably/ably-ios/pull/179/files#r52278212

Rewrite ARTEventEmitter per the spec and adjust Connection.

tcard force-pushed the rewrite-arteventemitter branch from fa9ebd1 to 63d0cc7 Compare February 1, 2016 18:51

ricardopereira reviewed Feb 2, 2016
View reviewed changes

mattheworiordan reviewed Feb 2, 2016
View reviewed changes

tcard mentioned this pull request Feb 2, 2016

EventEmitter spec needs to be defined more clearly ably/docs#82

Closed

tcard force-pushed the rewrite-arteventemitter branch 2 times, most recently from 8aa38f6 to 43f6833 Compare February 4, 2016 21:14

mattheworiordan reviewed Feb 8, 2016
View reviewed changes

tcard added 9 commits February 9, 2016 00:06

Remove some unused variables.

1ab5dd3

EventEmitter: dispose empty lists to avoid leaks.

0a002a3

See ably/ably-js#227 (comment).

Add retry in to ConnectionStateChange's interface.

7f3a51e

Remove 'all' from overloaded EventEmitter methods.

34c6a97

EventEmitter: allow nil data to be emitted.

884c3a3

EventEmitter: disable listening twice from the same Listener.

a757228

Also, remove "ignored" list for total listeners; `off` will just be a no-op if called with a specific event and a total listener. See: ably/docs#82 #179 (comment)

Rename ARTEventEmitter.totalListeners -> anyListeners.

9a7ad19

EventEmitter: add off without arguments.

cacb6eb

#179 (comment)

tcard force-pushed the rewrite-arteventemitter branch from e9f6153 to cacb6eb Compare February 8, 2016 23:06

ricardopereira reviewed Feb 9, 2016
View reviewed changes

EventEmitter: fix entry leak when emitting from self.listeners.

859e474

https://github.com/ably/ably-ios/pull/179/files#r52278212

tcard added a commit that referenced this pull request Feb 9, 2016

Merge pull request #179 from ably/rewrite-arteventemitter

f5b2fe2

Rewrite ARTEventEmitter per the spec and adjust Connection.

tcard merged commit f5b2fe2 into master Feb 9, 2016

tcard deleted the rewrite-arteventemitter branch February 9, 2016 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite ARTEventEmitter per the spec and adjust Connection. #179

Rewrite ARTEventEmitter per the spec and adjust Connection. #179

tcard commented Feb 1, 2016

tcard commented Feb 1, 2016

tcard commented Feb 1, 2016

ricardopereira Feb 2, 2016

ricardopereira Feb 2, 2016

tcard Feb 2, 2016

ricardopereira Feb 2, 2016

tcard Feb 3, 2016

mattheworiordan commented Feb 2, 2016

mattheworiordan Feb 2, 2016

tcard Feb 2, 2016

mattheworiordan Feb 3, 2016

tcard Feb 3, 2016

tcard Feb 3, 2016

tcard commented Feb 4, 2016

mattheworiordan Feb 8, 2016

tcard Feb 8, 2016

tcard Feb 8, 2016

mattheworiordan commented Feb 8, 2016

ricardopereira Feb 9, 2016

tcard Feb 9, 2016

ricardopereira Feb 9, 2016

mattheworiordan commented Feb 9, 2016

Rewrite ARTEventEmitter per the spec and adjust Connection. #179

Rewrite ARTEventEmitter per the spec and adjust Connection. #179

Conversation

tcard commented Feb 1, 2016

tcard commented Feb 1, 2016

tcard commented Feb 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattheworiordan commented Feb 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcard commented Feb 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattheworiordan commented Feb 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattheworiordan commented Feb 9, 2016