jupyter-widgets / ipyleaflet

A Jupyter - Leaflet.js bridge
https://ipyleaflet.readthedocs.io
MIT License
1.47k stars 359 forks source link

feat: introduce leaflet-geoman as an alternative to leaflet-draw #1181

Closed iisakkirotko closed 2 months ago

iisakkirotko commented 4 months ago

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 commented 4 months ago

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

maartenbreddels commented 3 months ago

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

giswqs commented 3 months ago

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?

maartenbreddels commented 3 months ago

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

giswqs commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

iisakkirotko commented 3 months ago

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

iisakkirotko commented 3 months ago

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!

maartenbreddels commented 3 months ago

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

martinRenou commented 3 months ago

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.

iisakkirotko commented 3 months ago

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 commented 3 months ago

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

martinRenou commented 3 months ago

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