Closed Mattijah closed 6 years ago
Hey @Mattijah, thanks for bringing this to my attention! 😄
I'm thinking we'll fix this with a doubly linked list.
_listeners
property from Event
_first
property to Event
that's a weak reference to the first active EventListener
_last
property to Event
that's a weak reference to the last active EventListener
_prev
property to EventListener
that's a weak reference to the listener before it_next
property to EventListener
that's a DynamicPointer
to the listener after it_next
, use the strongPointer
property for one-time listeners. Otherwise, use the weakPointer
property. If you have time to help, it should be relatively simple to change the implementation. And you can ask me anything if there's any confusing bits. If you can't afford to help, I might be able to fix this sometime next week.
Hi @aleclarson
I'm not sure if I will find time to work on this before you. If so, I will definitely let you know. I have also one more question about listeners. Shouldn't the property listenerCount
return the actual number of listeners that were added to the Event
? Currently, for instance in the example above it returns 1
as it counts listeners using the targetID
.
Thanks
@Mattijah Yeah, that's definitely another bug. We'll add a _listenerCount: Int
property to the Event
class to fix that. Thanks again! 👍
We'll also have to figure out how to maintain support for targets when switching to the doubly linked list approach. Maybe we'll keep a linked list for each target. So the proposed _first
and _last
properties should be in some new class and we'll store those instances in a dictionary where the keys are target identifiers. But there could be a more elegant solution.
@aleclarson I've just read your idea (steps) mentioned here https://github.com/aleclarson/emitter-kit/issues/43#issuecomment-332831493 on how to fix this issue. I'm not really sure about that. It sounds quite complex to me. I'm also concern there could be some problems (e.g. the referencing could get even more complicated) after certain listeners were deallocated.
I think a listener (EventListener
) shouldn't know or deal with any other listeners in any way. That should be up to the Event
. We only need to know the order in which the listeners were added to the event and that could be achieved simply just by storing them in an array.
_listeners = [String: [DynamicPointer<Listener>]]()
_listeners[targetID].append(newListener)
_listeners[targetID].remove(listener)
_listeners[targetID].count
Or is there something I've missed out and so this solution can not be used? Thanks
***Well, yeah. Support for targets changes things a bit.
@Mattijah Good points. The only question is what happens when a listener is removed from the array while an event is being emitted. For example, if a one-time listener removes itself, will the listener after it get skipped? I don't know without testing it. If a listener gets skipped, we could either create a "removal queue" or do the linked list approach.
Another benefit of the linked list is O(1) removal, compared to O(n) for a Swift array. Though, that may not matter to most users.
If we would go with a standard for-in
loop we'd then iterate through a copy of the array so we wouldn't have a problem with missing elements. Obviously in this case we need to check whether the listener that is about to be triggered is still listening or not.
We could also iterate through the original array if that's necessary but I think the option mentioned above is better (definitely more elegant).
Yep I was thinking about it before posting my comment. The things is, we will iterate the array every time we emit an event. Is one time remove
iteration a big problem? On top of that in most cases this array will probably not contain many elements (listeners) anyway. 1? I don't know.
Still, if this need to be improved for some reason. We could definitely find some better solution ;)
If we're going to copy the listener array, we should use filter
and change Listener._trigger
to return false
if it should be removed. Since the emit phase is synchronous, manually removed listeners shouldn't cause any skips.
Let me know if you decide to get started on this. You can open a pull request before you're done, and if you don't have enough time, I can wrap it up.
I'm not completely sure what you mean now. We will not manually copy the array. This is something that is done in the background by Swift. We don't really need to use filter
too. Listener would be removed from the Event._listeners
in the same way as they are being removed now but since we started to iterate through the array before the listener was removed the copy still contains it.
Are you happy with the proposed solution?
Yes, I understand that Swift arrays are value types. But since we're (implicitly) creating a new array, it doesn't hurt to use filter
AFAIK. This way, you will traverse the array while it's being copied. Also, one-time listeners wouldn't have to do an O(n) removal during the emit phase. Instead, the _trigger
method would return false and the listener would simply not be included in the new array of listeners.
Fixed in v5.2.0 🎉
a57ccd8f0197be844b57080c65bfd7aecc25a9e9
Listeners should be called in the order they were added. Currently they are stored in a
Dictionary
(with no defined order).Example:
The output may be:
or
This may be a pretty big problem when actions need to be done in the certain order. E.g. we may listen to an event in the same class where it's also located. The listener would be set in the constructor and I'd expect that this is the first listener that would get called. After that would be call other listeners (that were set let's say out of this class).