ipyleaflet icon indicating copy to clipboard operation
ipyleaflet copied to clipboard

feat: move from leaflet-draw to leaflet-geoman

Open iisakkirotko opened this issue 11 months ago • 1 comments

This replaces the old leaflet-draw implementation with a leaflet-geoman based one, in accordance with https://github.com/jupyter-widgets/ipyleaflet/issues/1165. The change should be backwards compatible in that even though leaflet-draw is no longer used on the front end, the functionality is available identically from the python side.

iisakkirotko avatar Mar 01 '24 13:03 iisakkirotko

The remaining visual test failure seems unrelated, see the reported diffs: dc964591a90a9bc7445db0487ddaea75ac69c5aa e7db508791c7ed51a8cc959d715b88037391c0b8

iisakkirotko avatar Mar 04 '24 09:03 iisakkirotko

@giswqs do you use DrawControls a lot? I wonder if you are willing to test this PR. We have to break things.

maartenbreddels avatar Mar 25 '24 12:03 maartenbreddels

Yes, I can give it a try. Do I Just pip install from the branch? Anything else I need to do to make sure the JS part is included?

giswqs avatar Mar 25 '24 12:03 giswqs

I think that should work fine. Installation instructions have changed a bit btw https://github.com/jupyter-widgets/ipyleaflet?tab=readme-ov-file#installation-from-sources

maartenbreddels avatar Mar 25 '24 14:03 maartenbreddels

It seems the position of the draw control is not working properly. In the example below, the DrawControl is supposed to be placed beneath the LayersControl, but instead it is being added above the ZoomControl. Same thing happens when adding the draw control to other position, e.g., topright. In other words, the draw control will always be placed at the top or bottom corner, ignoring other controls that have already been placed in the corner.

from ipyleaflet import Map, LayersControl, FullScreenControl, DrawControl, GeomanDrawControl

m = Map(center=[40, -100], zoom=4)
m.layout.height='600px'
m.scroll_wheel_zoom=True
m.add(FullScreenControl())
m.add(LayersControl(position='topleft'))
draw_control = DrawControl(position='topleft')
m.add(draw_control)
m

New DrawControl image

GeomanDrawControl image

Old DrawControl image

The UI tests also has the same issue. image

giswqs avatar Mar 26 '24 12:03 giswqs

When a drawn polygon has no more than four vertices, it returns a polygon type. When it has more than four vertices, it returns a Polygon type. The old draw control returns a Polygon type no matter how many vertices the polygon has.

New DrawControl image

image

Old DrawControl image image

giswqs avatar Mar 26 '24 12:03 giswqs

Can't enter text when the text layer is initially created. Need to click on the edit layer button in order to add text.

The JavaScript version can add enter text when the layer is added. No need to click on the edit layer button.

https://www.geoman.io/docs/text-layer

from ipyleaflet import Map, GeomanDrawControl

m = Map(center=(50, 354), zoom=5)

draw_control = GeomanDrawControl()
draw_control.text = {
    "textOptions": {
        "text":  "Geoman is fantastic! 🚀"
    }
}

m.add(draw_control)
m.layout.height = '600px'
m.scroll_wheel_zoom = True
m

Peek 2024-03-26 09-26

giswqs avatar Mar 26 '24 13:03 giswqs

How to add the marker tool to the draw control? The following code does not work.

from ipyleaflet import Map, GeomanDrawControl

m = Map(center=(50, 354), zoom=5)
draw_control = GeomanDrawControl()
draw_control.marker = {
    "markerStyle": {
        "draggable": True
    }}
draw_control.polyline =  {
    "pathOptions": {
        "color": "#6bc2e5",
        "weight": 8,
        "opacity": 1.0
    }
}
m.add(draw_control)
m.layout.height = '600px'
m.scroll_wheel_zoom = True
m

image

giswqs avatar Mar 26 '24 14:03 giswqs

Thanks a lot for your feedback @giswqs! I'll take a look at the issues later this week.

iisakkirotko avatar Mar 26 '24 15:03 iisakkirotko

Hey @giswqs!

b8863cc4d8a1d056ae8c8729f37ac71ac5dba1ab should address all of your comments, with the exception of the issue with the differing capitalisation of "polygon". The type you mention there is consistent, but admittedly confusingly named - one instance is under "feature.geometry.type", while the other is under "feature.properties.type". The second one is used, among other things, to differentiate between different types of Marker layers (TextMarker, CircleMarker, etc). Maybe renaming this one is in order to avoid confusion. I'd love to hear your thoughts on what you think a suitable name would be!

iisakkirotko avatar Mar 28 '24 12:03 iisakkirotko

Hi all,

first of all, some credits, and thank you's: Thanks to @mangecoeur from Planeto SA for sponsoring this feature. Planeto develops geospatial solutions for district energy planning (planeto-energy.ch) and use ipyleaflet a lot. It's great to see companies funding open source projects which they use, kudos to them.

Iisakki: Thank you for the hard work you put into building this feature. A lot of energy went into it.

@giswqs Thank you for the thorough review, it has improved the quality and backwards compatibility a lot.

I talked to @martinRenou privately and expressed my concerns about breaking backward compatibility despite my best efforts to avoid that. Given that leaflet.draw is still working, there is no strong reason to get rid (e.g. a security issue, or strong maintenance issue), we thought it would be better for now to:

  1. Keep the old DrawControl, nothing changes or breaks for existing users.
  2. Introduce the new GeomanDrawControl (this is done in this PR) and DrawControlCompat (rename what is now DrawControl in this PR).

In the future, when needed, if we do a major release, we can then:

  1. remove the old DrawControl
  2. rename DrawControlCompat to DrawControl and add DrawControlCompat as an alias for backward compatibility.

I hope everyone agrees that's a safe and frictionless path forward.

cheers,

Maarten

maartenbreddels avatar Apr 10 '24 07:04 maartenbreddels

Thanks a lot for that work! ipyleaflet is really missing contributors right now. I personally don't have time/energy to continue any maintenance on it, but I'm happy to make releases from time to time.

Is the PR ready? Happy to give it a final review/merge.

martinRenou avatar Apr 10 '24 08:04 martinRenou

Hi @martinRenou!

I'll make this PR reflect the approach @maartenbreddels outlined in https://github.com/jupyter-widgets/ipyleaflet/pull/1181#issuecomment-2046786537 today, and open a draft PR for the future breaking change. So I expect this to be fully ready for review later today!

iisakkirotko avatar Apr 11 '24 11:04 iisakkirotko

@martinRenou PR should be ready for final review! The installation errors seem to be due to an upgrade to @types/[email protected], which should be fixed in https://github.com/jupyter-widgets/ipyleaflet/pull/1186

iisakkirotko avatar Apr 11 '24 17:04 iisakkirotko

Thanks! I just rebased from the UI, you'll need to pull again

martinRenou avatar Apr 12 '24 07:04 martinRenou