dc.events.trigger losses events when two different sources are triggering

[kum-deepak]

Currently dc.events.trigger keeps only one variable to store the pending event. This causes no problem when only one logical source is firing the events. However if there are more than one sources which are firing events, all but one will discarded.

To fix this I am planning to create a class that can be instantiated for each logical event source. I will submit a PR when completed.

[kum-deepak]

@gordonwoodhull a question:

When the new version is done all usage of dc.events.trigger will need to be updated to the new semantics. Most of the current usage do not pass delay, which as I understand would cause the closure to be executed immediately. Can these instances be converted inline code?

[gordonwoodhull]

Ugh! This is super frustrating. I guess it is a "cargo cult" situation where everyone used trigger because everyone else was, even though it doesn't do anything except run the closure immediately, if there is no delay.

And it's not like the behavior changed - this behavior goes back to 2012 (3a85b4c).

I think that we unfortunately need to retain the semantics of trigger since it's documented and I am sure that people use it outside of the library. (For some reason the documentation is failing to be published but the function is marked up correctly and is used in many of the examples.)

[kum-deepak]

:)

In that case should we keep the new semantics as a new function (anyway it is going to be a class like thing) and leave the current dc.events.trigger.

Then we cleanup code within dc.js and deprecate dc.events.trigger, with a message containing link back to documentation on how to upgrade.

[gordonwoodhull]

I agree - let's keep the function broken and upgrade internally to something better.

A couple more design considerations:

• we probably want a real throttle, not debounce (and I'm not sure that the old code is even correct as a debounce - it's really "delay and stop any other triggers from happening during that delay") (Better throttling #630)
• debounce and throttle are usually implemented as function wrappers (see e.g. underscore's throttle and debounce), which is more elegant because the state is associated directly with the handler, not global, class, or object.

The disadvantage of a function wrapper is that we'll need to move these "closures" out and make them independent functions. But it looks like the only local state used is a filter or a range, so I don't think we lose anything by making those parameters.

I counted 10 uses that have no delay, and 4 that have delay. I'd start by just changing those 4 and seeing if everything still works. Eventually I think you are right that the other 10 should be inlined, but we have to determine if throttling was intended or if it's just cargo-cult.

[kum-deepak]

The underscore's model may not work easily in our case. Consider, _chart._brushing - there are two invocations of dc.events.trigger - but both needs to be part of the same queue.

[gordonwoodhull]

But those can be one throttled function with different arguments for each of the places it is called. There's no difference between replaceFilter and filter in this usage - they could both be replaceFilter.

The scatter plot one is similar (one trigger is missing a delay for no good reason).

And the zoom one is alone.

[kum-deepak]

Thanks @gordonwoodhull!

I am moving on to fixing the zoom behavior.

[kum-deepak]

During my little experience with Angular, I realized that Angular internally uses RxJS which has quite interesting concepts. I am definitely not suggesting to include RxJS. However their API may be worth looking at - https://www.learnrxjs.io/operators/filtering/debouncetime.html

I have started believing that if we have good implementations for both debounce and throttle (https://www.learnrxjs.io/operators/filtering/throttletime.html), we can make more responsive (as in fluid) brushing and zooming.

[kum-deepak]

While considering reducing global footprint, it seems following will reduce this problem quite a lot:

• Inline trigger calls that do have a delay.
• Make trigger a method in BaseMixin, so that the state is confined to a chart. This will reduce the likelihood of an event getting lost.
• The existing behaviour will be available through the compat layer.