search

CartoDB / torque

Temporal mapping for CARTO
http://cartodb.github.com/torque/
BSD 4-Clause "Original" or "Old" License
397 stars 131 forks source link

Repair broken reference #279

Closed donflopez closed 7 years ago

donflopez commented 7 years ago

Fix #278 and #1528

alonsogarciapablo commented 7 years ago

In theory, Torque layers should work as standalone layers (eg: someone should be able to add a Torque layer to an existing Google Maps map, as in this example). OverlayView implements a setMap method, but I'm not sure that'll set this.map (it's being used in that example too). What do you think @donflopez? How would someone "set" the map that is then being used internally in GMapsTorqueLayer?

Also, I traveled back in time to find out the bug we're seeing in CartoDB.js was introduced in https://github.com/CartoDB/cartodb.js/commit/77449e90e7e7e4667a38755cee6affdbae30b3ea, where we removed the map option we're passing as an option.

donflopez commented 7 years ago

Yep, that's the problem with the inheritance and mutability; you don't know where or who is changing the state of your class, here is pretty hard to find out where this.map or this.options.maps come from.

CanvasLayer prototype is inheriting from OverlayView instance from GMaps library, so we don't know what is happening exactly and the code is obfuscated.

Here, in the examples we pass the map instance as an option; but later we set the map using a GMaps method that seems to be the real handler for the map instance because the map passed as an options have a blind end, it isn't used.

So I've removed it to avoid confusions, and I think that this.map is the correct way because GMaps library set it and it's available when we use it on torque.js. It is too confusing, so maybe I'm missing something.

What do you think @alonsogarciapablo?

alonsogarciapablo commented 7 years ago

Here, in the examples we pass the map instance as an option; but later we set the map using a GMaps method that seems to be the real handler for the map instance because the map passed as an options have a blind end, it isn't used.

I'm ok with it as long as:

  1. setMap is actually setting this.map.
  2. CanvasLayer doens't actually need the map option you removed.

I guess we can remove the map option from this example (and the rest of examples) and see if it works... 👍 ?

donflopez commented 7 years ago

Yeah! I tried it, and it works. I've committed the change.

Is there any other example using GMaps with torque?

alonsogarciapablo commented 7 years ago

Not sure if there're more examples. I'm not very familiar with this repo.

:+1: Thanks! 👏

donflopez commented 7 years ago

I checked, there are two more, but the examples are broken :( If you want I can change the map as an option, but they don't work for other reasons.

Thanks to you!