Event dispatching needs to account for adding new listeners while iterating over the same list

[Draknek]

Related to #436.

Some listeners were getting called multiple times if getting added from an event listener of the same type.

Test case:

import flash.display.*;
import flash.events.*;
import openfl.Lib;

class Main extends Sprite
{
    public function new ()
    {
        super();

        Lib.current.stage.addEventListener(KeyboardEvent.KEY_DOWN, A, 100);
        Lib.current.stage.addEventListener(KeyboardEvent.KEY_DOWN, B, 0);
        Lib.current.stage.addEventListener(KeyboardEvent.KEY_DOWN, C, -100);
    }

    function A (e:KeyboardEvent)
    {
        trace("A");
    }

    function B (e:KeyboardEvent)
    {
        trace("B");

        Lib.current.stage.addEventListener(KeyboardEvent.KEY_DOWN, before1, 300);
        Lib.current.stage.addEventListener(KeyboardEvent.KEY_DOWN, before2, 200);
    }

    function C (e:KeyboardEvent)
    {
        trace("C");
    }

    function before1 (e:KeyboardEvent)
    {
        trace("before1");
    }

    function before2 (e:KeyboardEvent)
    {
        trace("before2");
    }
}

Expected behaviour (first key press):

Main.hx:18: A
Main.hx:23: B
Main.hx:31: C

Actual behaviour (first key press), A & B called twice:

Main.hx:18: A
Main.hx:23: B
Main.hx:36: before2
Main.hx:18: A
Main.hx:23: B
Main.hx:31: C

Expected & actual behaviour (second key press):

Main.hx:41: before1
Main.hx:36: before2
Main.hx:18: A
Main.hx:23: B
Main.hx:31: C

[Draknek]

Just a reminder that this code is demonstrably broken and this is an easy patch to apply :)

[jgranick]

I am sorry for the delay, event dispatching is a bit of a "fine science", and I'm cautious about integrating changes to it (since it's so easy to accidentally break things elsewhere)

I think the real thing is that the event dispatch needs to be isolated from changes. I'm worried that a .copy() (though an easy fix) would hurt the standard performance of event dispatching, so I am trying a __dispatching map (true or false) to know if we're in a loop, and to use a __newEventMap to isolate changes to the list (if we are in a related dispatch loop already) and we apply them after

Hopefully this keeps the performance up, while also taking care of this scenario. Your test works fine here 😄

[Draknek]

This code looks dodgy to me: you're checking if (__dispatching.exists(type)) but that will be true if __dispatching has any mapping for type, even false.

You need to do __dispatching.remove(type) instead of __dispatching.set(type, false).

It also looks like you're doing more work than necessary: you say you're wary of calling copy(), but this code does that potentially once a frame I think. My patch #454 never copies any lists, because that's not necessary: you just need to be careful to update the loop index when you modify the list you're currently looping over.

Do you care about being compatible with Flash event-dispatching? Because IMO that should be a goal and I don't think this code is there yet - I think I can provide examples that run differently when compiled for Flash vs. Neko.

[jgranick]

Ah, great catch, it should be removed or at least checked for true

Although event dispatching happens ALL THE TIME, I'm not sure how often the list is modified with the dispatch loop. Aren't they usually set about once, then left for the remainder of the app?

Honestly, I'd love more test cases, as you know, it's a tricky thing to get quite right, but I think we're getting closer, thanks to your help 😄