Move event management from deckgl.js React component into LayerManager. #738
Conversation
Begin to remove drag event handling. Remove unused event handler props from webgl-renderer.js.
…ct deck.gl component and LayerManager. This enables a non-react app to opt into event handling. This commit is a quick pass for exposition and needs cleanup + tests.
Move all input event handling logic into LayerManager.
|
Thanks @shaojingli, though I'd still like @ibgreen and possibly @Pessimistress to take another pass, since we've had a lot of back-and-forth here. |
src/lib/layer-manager.js
Outdated
| * @param {Function} onLayerClick A handler to be called when any layer is clicked. | ||
| * @param {Function} onLayerHover A handler to be called when any layer is hovered over. | ||
| */ | ||
| setEventParams({ |
There was a problem hiding this comment.
setEventHandlingParameters
| * {Object} srcEvent: native JS Event object | ||
| */ | ||
| _onClick(event) { | ||
| const pos = event.offsetCenter; |
There was a problem hiding this comment.
I have asked this before:
Is offsetCenter a standard field in the Event? My understanding is that it is NOT. if that is true, this code is implicitly coupled to the EventManager because it will only work with events generated by that class.
To break this coupling, position parameter to this function and call as
layerManager._onClick({position: event.offsetPosition, event});
There was a problem hiding this comment.
It's standard now for all events generated by EventManager, but you're correct that neither offsetCenter nor srcEvent are guaranteed to be present on events generated by any other event handling solution.
We're never calling _onClick nor _onPointerMove from LayerManager, those are handlers registered on the eventManager passed into LayerManager.setEventParams(). Therefore, the shim you suggest is not really applicable here.
We can do more to increase the flexibility of LayerManager's implementation of these handlers, but ultimately there is a contract between LayerManager and the passed eventManager that either needs to be explicit (and documented, which I did in the header comment for setEventParams and the _onClick & _onPointerMove handlers), or shimmed with e.g. an accessor that needs to be passed into setEventParams() as well.
In the latter case, the API gets more complex, and we're well into territory that IMO we don't need to venture into right now -- passing a custom eventManager into LayerManager. This feels to me like an edge case, and I think supporting it via more than clear documentation is something we should hold off on until a clear use case arises.
src/lib/layer-manager.js
Outdated
| @@ -446,6 +496,86 @@ export default class LayerManager { | |||
| } | |||
| return error; | |||
| } | |||
|
|
|||
| _validateEventHandling() { | |||
| // Check if a deck-level mouse event has been specified | |||
There was a problem hiding this comment.
Move this comment into a proper function header.
| * {Object} srcEvent: native JS Event object | ||
| */ | ||
| _onPointerMove(event) { | ||
| const pos = event.offsetCenter; |
src/react/deckgl.js
Outdated
| this.layerManager = null; | ||
| this.effectManager = null; | ||
| autobind(this); | ||
| } | ||
|
|
||
| componentWillReceiveProps(nextProps) { | ||
| this.layerManager.setEventParams({ |
There was a problem hiding this comment.
Move this duplicate code into updateLayers or a new separate function?
updateLayers looks like it is already ripe for being renamed updateLayerManager
Separate initialization of LayerManager event handling from param updating, and add documentation Address other comments.
Accidentally merged as part of the rebase; this PR picks up where #708 left off, with a clean history.
This commit does away with LayerEventManager and instead leaves layer event handling in control of LayerManager.
To keep diffs manageable, I stopped short of fixing the picking-on-every-event bug (#634), since it will require adding/removing handlers to/from EventManager every time new top-level handlers are passed to setEventParams() or layers are updated with new per-layer handlers.
For the same reason, I'm also leaving for another PR the work to pull the per-layer handler calls out of draw-and-pick.js.