Add event propagation system to Evented #3244

lucaswoj · 2016-09-22T19:27:14Z

Adding a new Map event that needs to be fired from another class requires a lot of GL JS boilerplate. Before tackling #1715, I built an event propogation system that...

adds a Evented#setEventedParent method which propagates events
uses Evented#setEventedParent to eliminate repetitive binding / unbinding of _forward*Event methods
uses Evented#setEventedParent allow event names to remain constant from Evented#fire to Map listeners, making it easier to add new events and follow the path of existing events
improves test coverage of Evented
eliminates the ability to call Evented#off with less than two arguments, removing all listeners (this could break GL JS core)
eliminates lines of code

I'm still not 💯 on the name Evented#setEventedParent. Any other ideas?

cc @jfirebaugh @mourner @mollymerp @lbud @mapsam

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
document any changes to public APIs
post benchmark scores
manually test the debug page

Benchmarks

map-load

master 594248c: 132 ms
datastart 102807b: 114 ms

style-load

master 594248c: 113 ms
datastart 102807b: 110 ms

buffer

master 594248c: 984 ms
datastart 102807b: 1,092 ms

fps

master 594248c: 60 fps
datastart 102807b: 60 fps

frame-duration

master 594248c: 8.1 ms, 1% > 16ms
datastart 102807b: 8 ms, 4% > 16ms

query-point

master 594248c: 1.15 ms
datastart 102807b: 1.23 ms

query-box

master 594248c: 92.81 ms
datastart 102807b: 92.15 ms

geojson-setdata-small

master 594248c: 15 ms
datastart 102807b: 13 ms

geojson-setdata-large

master 594248c: 131 ms
datastart 102807b: 133 ms

jfirebaugh · 2016-09-22T19:50:08Z

This looks great. We can apply these ideas to mapbox/mapbox-gl-native#6383 as well.

I'm still not 💯 on the name Evented#setEventedParent. Any other ideas?

I think it's fine, but if you are looking for something more descriptive, I'd suggest setParentForEventBubbling.

jfirebaugh · 2016-09-22T19:53:44Z

js/style/style.js

@@ -67,12 +62,12 @@ function Style(stylesheet, animationLoop, options) {

        if (stylesheet.sprite) {
            this.sprite = new ImageSprite(stylesheet.sprite);
-            this.sprite.on('load', this.fire.bind(this, 'change'));


Is there some other mechanism now for triggering a repaint when the sprite finishes loading?

Yes. SpriteImage fires a style.change event which is propagated to Style then Map. The same flow occurs without this on -> fire interaction within Style.

mourner

This looks great! I did the same thing in Leaflet some time ago, with addEventParent and removeEventParent methods, and it fit in really nicely. The only difference is that Leaflet needed the ability to have multiple event parents (hence add instead of set), and also ability to fire an event without propagation (so it's fire(type, data, propagate) where propagate can be false), but these seem not to be necessary in GL JS. 👍

mourner · 2016-09-23T08:42:35Z

js/style/style.js

@@ -420,7 +399,7 @@ Style.prototype = util.inherit(Evented, {
        }
        this._validateLayer(layer);

-        layer.on('error', this._forwardLayerEvent);
+        layer.setEventedParent(this, {layer: {id: layer.id}});


Is it necessary to have nested {layer: {id: ...} data for some compatibility reasons, or could we just have {layerId: ...}?

As this is a public event, I suspect that changing the structure would impact our users.

The intent behind this structure is to give us the flexibility to make StyeLayers part of the public API. Think of {id: ...} as the public interface of StyleLayer. 😅

I am working on adding custom layer types now, which bring these classes further into the public API.

mourner · 2016-09-23T08:45:27Z

js/ui/map.js

-    this.on('move', this._update.bind(this, false));
-    this.on('zoom', this._update.bind(this, true));
+    this.on('move', this._update);
+    this.on('zoom', this._update);


Did you intentionally remove binds here? They set the updateStyle argument of _update (e.g. so that move doesn't trigger zoom recalc as far as I remember and zoom does). Without bind, updateStyle will always be truthy because move and zoom pass event data to listeners.

This was an oversight. I saw that _update was bound to this but did not appreciate the bound arguments. Reverting.

#3244 (comment)

lucaswoj · 2016-09-23T16:27:49Z

Glad to hear this is in line with what we've done in other projects! Excited to 🚢 and keep digging on #1715

* Minor refactoring of Evented * Refactor Evented tests * Add Evented#pipe * Fix flaky map test * Use Evented#pipe in SourceCache * Rename "Evented#pipe" to "Evented#forwardEvents" * Add "passes original 'target' to forwarded listeners" test * Use event forwarding for source -> style events * Use event forwarding for layer -> style events * Use event forwarding for sprite -> style events * Use event forwarding for all style -> map events * Get rid of typed error events * Replace "forwardEvents" with "setEventedParent" * Minor refactoring * Make Evented#setEventedParent private * Fix benchmarks * Eliminate remaining ImageSprite#load events * Fix removed bound arguments per PR comment #3244 (comment)

Lucas Wojciechowski added 15 commits September 21, 2016 13:17

Minor refactoring of Evented

f40f7dd

Refactor Evented tests

399aeb1

Add Evented#pipe

2783f5e

Fix flaky map test

55ff64b

Use Evented#pipe in SourceCache

8081d0b

Rename "Evented#pipe" to "Evented#forwardEvents"

17b10f0

Add "passes original 'target' to forwarded listeners" test

718111b

Use event forwarding for source -> style events

312f4d4

Use event forwarding for layer -> style events

430319d

Use event forwarding for sprite -> style events

2664bed

Use event forwarding for all style -> map events

662b1ae

Get rid of typed error events

5532483

Replace "forwardEvents" with "setEventedParent"

c6c1875

Minor refactoring

5469843

Make Evented#setEventedParent private

102807b

jfirebaugh mentioned this pull request Sep 22, 2016

Replace MapChange enum and Backend::notifyMapChange with dedicated observer interface mapbox/mapbox-gl-native#6383

Closed

jfirebaugh reviewed Sep 22, 2016

View reviewed changes

Lucas Wojciechowski added 2 commits September 22, 2016 13:31

Fix benchmarks

a10b3a3

Eliminate remaining ImageSprite#load events

62ad37a

lucaswoj force-pushed the datastart branch from 469ff6e to 62ad37a Compare September 22, 2016 20:38

mourner approved these changes Sep 23, 2016

View reviewed changes

mourner reviewed Sep 23, 2016

View reviewed changes

Fix removed bound arguments per PR comment

2693ad5

#3244 (comment)

lucaswoj merged commit 24f2135 into master Sep 23, 2016

lucaswoj deleted the datastart branch September 23, 2016 16:27

lucaswoj mentioned this pull request Sep 23, 2016

Document or remove any event that can be fired by "Map" #2232

Closed

jfirebaugh mentioned this pull request Oct 28, 2016

map.off(event) doesn't works anymore #3484

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add event propagation system to Evented #3244

Add event propagation system to Evented #3244

lucaswoj commented Sep 22, 2016

jfirebaugh commented Sep 22, 2016

jfirebaugh Sep 22, 2016

lucaswoj Sep 22, 2016

mourner left a comment

mourner Sep 23, 2016

lucaswoj Sep 23, 2016

mourner Sep 23, 2016 •

edited

Loading

lucaswoj Sep 23, 2016

lucaswoj commented Sep 23, 2016

Add event propagation system to Evented #3244

Add event propagation system to Evented #3244

Conversation

lucaswoj commented Sep 22, 2016

Launch Checklist

Benchmarks

map-load

style-load

buffer

fps

frame-duration

query-point

query-box

geojson-setdata-small

geojson-setdata-large

jfirebaugh commented Sep 22, 2016

jfirebaugh Sep 22, 2016

Choose a reason for hiding this comment

lucaswoj Sep 22, 2016

Choose a reason for hiding this comment

mourner left a comment

Choose a reason for hiding this comment

mourner Sep 23, 2016

Choose a reason for hiding this comment

lucaswoj Sep 23, 2016

Choose a reason for hiding this comment

mourner Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

lucaswoj Sep 23, 2016

Choose a reason for hiding this comment

lucaswoj commented Sep 23, 2016

mourner Sep 23, 2016 •

edited

Loading