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

removeLayerGroup throws "Layer was not found" for valid layers #190

Closed McBen closed 5 years ago

McBen commented 5 years ago

it's a bug in the old iitc code. Never arises because the "_leaflet_id" was equal to the entry in the layerchooser. This is no longer the case in leaflet 1+

Bug Fix:

diff --git a/code/utils_misc.js b/code/utils_misc.js
index 39865ce3..7d5fbdb1 100644
--- a/code/utils_misc.js
+++ b/code/utils_misc.js
@@ -409,9 +409,11 @@ window.addLayerGroup = function(name, layerGroup, defaultDisplay) {
 }

 window.removeLayerGroup = function(layerGroup) {
-  if(!layerChooser._layers[layerGroup._leaflet_id]) throw('Layer was not found');
+  const layerEntry = layerChooser._layers.find( entry => entry.layer === layerGroup);
+  if(!layerEntry) throw('Layer was not found');
+
   // removing the layer will set it's default visibility to false (store if layer gets added again)
-  var name = layerChooser._layers[layerGroup._leaflet_id].name;
+  var name = layerEntry.name;
   var enabled = isLayerGroupDisplayed(name);
   map.removeLayer(layerGroup);
   layerChooser.removeLayer(layerGroup);
johnd0e commented 5 years ago

Do you know any real plugin using this function?

P.S. The issue is known https://github.com/IITC-CE/ingress-intel-total-conversion/issues/163#issuecomment-482785398, but I am going to refactor the whole layerChooser, in order to get rid of all these supplementary functions (still keeping stubs for compatibility).

McBen commented 5 years ago

Do you know any real plugin using this function?

My private build. I already replaced that code for myself but maybe other authors will face the same problem.

Function is working in iitc but broken in iitc-ce. (even when the bug already exists in the old code)

The issue is known #163 (comment)

Good to hear you'll refactor that part too. Maybe you should change the label of the issue to be more informative.

modos189 commented 5 years ago

Thanks for fix. I think we can accept it until johnd0e rewrites layerChooser.

johnd0e commented 5 years ago

@modos189 Please note that we avoid ES6 in our code. This function is unused, I suppose we don't have heavy need to fix it separately.

modos189 commented 5 years ago

Not used by us, but if used by private plugins, we can fix it. But, of course, we need to remake, don't use ES6. Anyway, it's not hard

johnd0e commented 5 years ago

Well, I am almost sure that nobody uses it, even in private plugins) But I may be wrong, need to check McBen's Layers control.

McBen commented 5 years ago

Well, I am almost sure that nobody uses it, even in private plugins)

atleast me was using it and got reports of user that my plugin won't work with iitc-ce if noone (else) uses it: remove it instead of leave unfunctional code inside.

But I may be wrong, need to check McBen's Layers control.

Thats a complete other topic. I did my layer-control long time ago and if I remember right it's a complete replacement (with still bugs inside)

johnd0e commented 5 years ago

atleast me was using it and got reports of user that my plugin won't work with iitc-ce

That is another matter. What exactly plugin do you mean?

McBen commented 5 years ago

anyway...you want it, you got it heres a amazing plugins which doesnt work -> https://github.com/McBen/johnd0eExample

works in every iitc version. except iitc-ce

johnd0e commented 5 years ago

@McBen

108: IITC 'api' is very extensive, as every minor internal function is (potentially) available to plugins. But in fact there is relatively small subset of functions that are (really) used by plugins.

In process of refactoring IITC internals inevitable changes, and I do not see any sense to keep every old function interface 'just in case'.

But real plugins using it - another matter, as we trying to maintain compatibility. I have downloaded a lot of plugins for exact this purpose: check real API usage. So my question was about real plugins, not hypothetical ones.

~~BTW, your sample plugin has error in function usage, and unable to work in your own fork. (And if your have layer instance then you do not need IITC API to remove it, just use leaflet's removeLayer)~~

McBen commented 5 years ago

sad to here...so I've to keep writing iitc-ce workaround to keep my users happy. 'cause iitc-ce will increase incompabilty with old iitc code.

you don't even mind how many code is out there you will never see. Bad for us all some of these aren't maintained anymore.

johnd0e commented 5 years ago

I'd be happy to fix any reported incompatibility.

McBen commented 5 years ago

I did here

McBen commented 5 years ago

BTW, your sample plugin has error in function usage, and unable to work in your own fork. (And if your have layer instance then you do not need IITC API to remove it, just use leaflet's removeLayer)

as said -5min code- quickly checked. should work (if initialized in correct order) removeLayerand removelayergroupare doing different things

modos189 commented 5 years ago

I found use of removelayergroup() function in this secret resistance plugin reswue.user.js

McBen commented 5 years ago

could you please add s.t.r.i.k.e code too

McBen commented 5 years ago

and maybe drunkenfrog or riot or what ever none plublic code you know

modos189 commented 5 years ago

I have no idea what you're talking about)

McBen commented 5 years ago

yeah..me too..seems totally wired to request fixing a function which is totally bugged in iitc-ce

modos189 commented 5 years ago

What? I don't see reason not to fix