Leaflet.Editable icon indicating copy to clipboard operation
Leaflet.Editable copied to clipboard

Target specific polygon or polyline in a multi

Open yohanboniface opened this issue 9 years ago • 3 comments

In Leaflet master branch, multis are now managed directly by Polygon and Polyline classes. This refactor comes with a much simple and clear structure of the Path family, but also some limitations when dealing with editing the mulitpolygons or multipolylines. There is no more mapping between every single shape of a polygon and a Javascript object. So for example the events of every shape are managed by the same javascript object, without a native way to know which shape has really triggered it. There are some use cases where this missing information give us a bit of hard time:

  • when we need to create a new hole in a polygon by clicking on it
  • when we want to delete a polygon or a polyline by clicking on it (but not deleting the multipolygon or multipolyline itself)
  • when we want to drag a whole polygon or polyline

I've come up with a solution, which basically consists in taking the latlng of the event, then looping over all the shapes of the multi, and trying to determine which of them includes the latlng. This works, and it's unittested, but, firstly, it's a bit fragile, and, secondly, it duplicates a bit of code from Leaflet.

Now that we are very close the release of Leaflet 1.0, I would like to be sure I'm not missing a stronger scenario, and maybe take the opportunity to do a bit of refactor the Leaflet side to avoid those bits of code duplicated.

@mourner, @tmcw, @danzel, @jfirebaugh, I would love your feedback on this. :)

Thanks in advance! :)

yohanboniface avatar Mar 03 '15 21:03 yohanboniface

Maybe we could expand the event from leaflet to include which of the shapes in the polygon was the one that was clicked?

danzel avatar Mar 05 '15 18:03 danzel

Yeah, that's the tradeoff — multipolygons are a first-class citizens in Leaflet now, but they're phisically one object, so you have to do point-in-polygon tests to determine relationships with individual rings.

How would you imagine this to be handled on the Leaflet side? I'm up for some refactoring if it makes things simper and doesn't add much code.

mourner avatar Mar 05 '15 19:03 mourner

Maybe we could expand the event from leaflet to include which of the shapes in the polygon was the one that was clicked?

This would be an ideal situation, but given that there is not easy and costless way of determining this shape, I'm not sure we should compute this at every event. But maybe we could provide a public API to do that, when needed.

I'm up for some refactoring if it makes things simper and doesn't add much code.

Here is a suggestion. As a reminder, what I'm doing right is looping among the shapes to determine which one contains the event LatLng (and one of my initial questions was: do you think of a better pattern? so I'm totally open to better suggestions :) ).

  • my code here is duplicating some part of L.Polygon.prototype._containsPoint and L.Polyline.prototype._containsPoint, because those methods are dealing with all the path parts in once, while in this situation we need to check every part separately, so I'd suggest we split those methods in two, where there is one that deals with one part and this other one that loops over all parts calling the first
  • I think this need will raise from other users too (Leaflet.Draw?), so I'd suggest to have a public API in Leaflet itself to deal with that, so moving (and refactoring) the shapeFromLatLng from Leaflet.Editable to Leaflet core

Of course, if we all agree on a scenario, I'll take care of a PR. :)

yohanboniface avatar Mar 06 '15 08:03 yohanboniface