search

IITC-CE / ingress-intel-total-conversion

intel.ingress.com total conversion user script with some new features. Should allow easier extension of the intel map.
https://iitc.app
ISC License
290 stars 109 forks source link

Adopt Leaflet.Evented instead of current hooks system. #217

Open johnd0e opened 5 years ago

johnd0e commented 5 years ago

IITC currently is pretty integrated with Leaflet. And Leaflet event system is powerfull, and definitely more comprehent than IITC custom homebrew hooks.

But then we need some additional compatibility layer to support old plugins.

johnd0e commented 4 years ago

  1. Currently we already have entities represented as leaflet objects : portal, link, field. They all are inherited from L.Evented, and able to fire/process events:

    • either themselves

      Details Events: `add`, `remove`, `move`, and also tooltip/popup events. Arbitrary extra events can be added for iitc-specific purposes (`select`,`detailsLoad`,`detailsUpdate`)

      (Note: ATM individual portals are not consistestent: #325)

    • or propagate events to parents (map object, or any L.FeatureGroup)
      Details Events: `layeradd`, `layerremove`.

  2. We also need iitc global object to provide general events. This object also may become a new base for whole iitc API (instead of polluting window) See https://github.com/IITC-CE/ingress-intel-total-conversion/issues/108#issuecomment-509034195

johnd0e commented 3 years ago

Leaflet event system may be not directly applicable:

But as other part of our code also heavily use Leaflet events - perhaps we could to make changes to L.Evented.

johnd0e commented 3 years ago
  • There is no arguments control, so such errors will not be reported:

Can be fixed in https://github.com/Leaflet/Leaflet/pull/7518

  • When firing event - handler call is not wrapped in try/catch

We need some ideas how to refactor fire to be able to add try/catch there in compatible way.

johnd0e commented 3 years ago

One more thing that is not supported by Leaflet: interruption of runHooks: https://github.com/IITC-CE/ingress-intel-total-conversion/blob/c9d6b98f4cd62b64b1eeea85233fc59f34dcf954/core/code/hooks.js#L67-L81

Well, I'm not sure that it is important, may be we should just discard it. Opinions?

le-jeu commented 3 years ago

Related commit : 1a8d15d1c59efe4f5ddb7a9d7e1e08e5c06110a1 and https://github.com/IITC-CE/ingress-intel-total-conversion/blob/3cab5faae663eefae3447d87630ddb10bbb86075/plugins/player-activity-tracker.js#L509-L514

Interruption depends on the order of registered hook and doesn't seem used, but the return value of runHooks is revelant here (true if any hook returns false)

johnd0e commented 3 years ago

@le-jeu I can imagine some use cases for runHooks return value, but I do not see anything relevant in your player-tracker sample

Edit: found single occurrence (in all iitc code): https://github.com/IITC-CE/ingress-intel-total-conversion/blob/3cab5faae663eefae3447d87630ddb10bbb86075/core/code/chat.js#L315-L317

johnd0e commented 3 years ago

but the return value of runHooks is revelant here

So I agree that this may be useful feature (though not essential). It is possible to simulate it even with current leaflet events, by inspecting data object after fire.

And I have some ideas how to improve that in leaflet itself: https://github.com/Leaflet/Leaflet/issues/6787