refactor(plugin): simplify marker and popup handling in OMS

swtormy · swtormy · commit bcd59485e3d1 · 2024-11-18T10:16:41.000+03:00
diff --git a/docs/user_guide/plugins/overlapping_marker_spiderfier.md b/docs/user_guide/plugins/overlapping_marker_spiderfier.md
@@ -1,24 +1,30 @@
 # OverlappingMarkerSpiderfier
 
+The `OverlappingMarkerSpiderfier` is a plugin for Folium that helps manage overlapping markers by "spiderfying" them when clicked, making it easier to select individual markers.
+
 ```{code-cell} ipython3
 import folium
-from folium import plugins
+from folium.plugins import OverlappingMarkerSpiderfier
 
 # Create a map
-m = folium.Map(location=[45.05, 3.05], zoom_start=14)
-
-# Generate some markers
-markers = [folium.Marker(location=[45.05 + i * 0.0001, 3.05 + i * 0.0001], options={'desc': f'Marker {i}'}) for i in range(10)]
+m = folium.Map(location=[45.05, 3.05], zoom_start=13)
 
 # Add markers to the map
-for marker in markers:
-    marker.add_to(m)
+for i in range(20):
+    folium.Marker(
+        location=[45.05 + i * 0.0001, 3.05 + i * 0.0001],
+        popup=f"Marker {i}"
+    ).add_to(m)
 
-# Add OverlappingMarkerSpiderfier
-oms = plugins.OverlappingMarkerSpiderfier(
-    markers=markers,
-    options={'keepSpiderfied': True, 'nearbyDistance': 20}
-).add_to(m)
+# Add the OverlappingMarkerSpiderfier plugin
+oms = OverlappingMarkerSpiderfier(options={
+    "keepSpiderfied": True,  # Markers remain spiderfied after clicking
+    "nearbyDistance": 20,    # Distance for clustering markers in pixel
+    "circleSpiralSwitchover": 10,  # Threshold for switching between circle and spiral
+    "legWeight": 2.0         # Line thickness for spider legs
+})
+oms.add_to(m)
 
-# Display the map
 m
+```
+
diff --git a/folium/plugins/overlapping_marker_spiderfier.py b/folium/plugins/overlapping_marker_spiderfier.py
@@ -1,100 +1,67 @@
 from jinja2 import Template
-
-from folium.elements import JSCSSMixin
-from folium.map import Layer
+from folium.elements import JSCSSMixin, MacroElement
 from folium.utilities import parse_options
 
-
-class OverlappingMarkerSpiderfier(JSCSSMixin, Layer):
-    """A plugin that handles overlapping markers by spreading them into a spider-like pattern.
-
-    This plugin uses the OverlappingMarkerSpiderfier-Leaflet library to manage markers
-    that are close to each other or overlap. When clicked, the overlapping markers
-    spread out in a spiral pattern, making them easier to select individually.
-
+class OverlappingMarkerSpiderfier(JSCSSMixin, MacroElement):
+    """
+    A plugin that handles overlapping markers on a map by spreading them out in a spiral or circle pattern when clicked.
+    
+    This plugin is useful when you have multiple markers in close proximity that would otherwise be difficult to interact with.
+    When a user clicks on a cluster of overlapping markers, they spread out in a 'spider' pattern, making each marker 
+    individually accessible.
+
+    Markers must be added to the map **before** calling `oms.add_to(map)`. 
+    The plugin identifies and manages all markers already present on the map.
+      
     Parameters
     ----------
-    markers : list, optional
-        List of markers to be managed by the spiderfier
-    name : string, optional
-        Name of the layer control
-    overlay : bool, default True
-        Whether the layer will be included in LayerControl
-    control : bool, default True
-        Whether the layer will be included in LayerControl
-    show : bool, default True
-        Whether the layer will be shown on opening
     options : dict, optional
-        Additional options to be passed to the OverlappingMarkerSpiderfier instance
-        See https://github.com/jawj/OverlappingMarkerSpiderfier-Leaflet for available options
-
+        The options to configure the spiderfier behavior:
+        - keepSpiderfied : bool, default True
+            If true, markers stay spiderfied after clicking
+        - nearbyDistance : int, default 20
+            Pixels away from a marker that is considered overlapping
+        - legWeight : float, default 1.5
+            Weight of the spider legs
+        - circleSpiralSwitchover : int, optional
+            Number of markers at which to switch from circle to spiral pattern
+    
     Example
     -------
-    >>> markers = [marker1, marker2, marker3]  # Create some markers
-    >>> spiderfier = OverlappingMarkerSpiderfier(
-    ...     markers=markers, keepSpiderfied=True, nearbyDistance=20
-    ... )
-    >>> spiderfier.add_to(m)  # Add to your map
+    >>> oms = OverlappingMarkerSpiderfier(options={
+    ...     "keepSpiderfied": True,
+    ...     "nearbyDistance": 30,
+    ...     "legWeight": 2.0
+    ... })
+    >>> oms.add_to(map)
     """
-
     _template = Template(
         """
         {% macro script(this, kwargs) %}
-        var {{ this.get_name() }} = (function () {
-            var layerGroup = L.layerGroup();
-
+        (function () {
             try {
                 var oms = new OverlappingMarkerSpiderfier(
                     {{ this._parent.get_name() }},
                     {{ this.options|tojson }}
                 );
 
-                var popup = L.popup({
-                    offset: L.point(0, -30)
+                oms.addListener('spiderfy', function() {
+                    {{ this._parent.get_name() }}.closePopup();
                 });
 
-                oms.addListener('click', function(marker) {
-                    var content;
-                    if (marker.options && marker.options.options && marker.options.options.desc) {
-                        content = marker.options.options.desc;
-                    } else if (marker._popup && marker._popup._content) {
-                        content = marker._popup._content;
-                    } else {
-                        content = "";
-                    }
-
-                    if (content) {
-                        popup.setContent(content);
-                        popup.setLatLng(marker.getLatLng());
-                        {{ this._parent.get_name() }}.openPopup(popup);
+                {{ this._parent.get_name() }}.eachLayer(function(layer) {
+                    if (
+                        layer instanceof L.Marker
+                    ) {
+                        oms.addMarker(layer);
                     }
                 });
 
-                oms.addListener('spiderfy', function(markers) {
-                    {{ this._parent.get_name() }}.closePopup();
-                });
-
-                {% for marker in this.markers %}
-                var {{ marker.get_name() }} = L.marker(
-                    {{ marker.location|tojson }},
-                    {{ marker.options|tojson }}
-                );
-
-                {% if marker.popup %}
-                {{ marker.get_name() }}.bindPopup({{ marker.popup.get_content()|tojson }});
-                {% endif %}
-
-                oms.addMarker({{ marker.get_name() }});
-                layerGroup.addLayer({{ marker.get_name() }});
-                {% endfor %}
             } catch (error) {
-                console.error('Error in OverlappingMarkerSpiderfier initialization:', error);
+                console.error('Error initializing OverlappingMarkerSpiderfier:', error);
             }
-
-            return layerGroup;
         })();
         {% endmacro %}
-
         """
     )
 
@@ -105,27 +72,14 @@ class OverlappingMarkerSpiderfier(JSCSSMixin, Layer):
         )
     ]
 
-    def __init__(
-        self,
-        markers=None,
-        name=None,
-        overlay=True,
-        control=True,
-        show=True,
-        options=None,
-        **kwargs,
-    ):
-        super().__init__(name=name, overlay=overlay, control=control, show=show)
+    def __init__(self, options=None, **kwargs):
+        super().__init__()
         self._name = "OverlappingMarkerSpiderfier"
-
-        self.markers = markers or []
-
         default_options = {
             "keepSpiderfied": True,
             "nearbyDistance": 20,
             "legWeight": 1.5,
         }
         if options:
             default_options.update(options)
-
         self.options = parse_options(**default_options, **kwargs)
diff --git a/tests/plugins/test_overlapping_marker_spiderfier.py b/tests/plugins/test_overlapping_marker_spiderfier.py
@@ -4,14 +4,30 @@
 """
 
 import numpy as np
-from jinja2 import Template
 
-import folium
-from folium import plugins
-from folium.utilities import normalize
+from folium.folium import Map
+from folium.map import Marker
+from folium.plugins.overlapping_marker_spiderfier import OverlappingMarkerSpiderfier
 
 
-def test_overlapping_marker_spiderfier():
+def test_oms_js_inclusion():
+    """
+    Test that the OverlappingMarkerSpiderfier JavaScript library is included in the map.
+    """
+    m = Map([45.05, 3.05], zoom_start=14)
+    oms = OverlappingMarkerSpiderfier().add_to(m)
+
+    rendered_map = m._parent.render()
+    assert (
+        '<script src="https://cdnjs.cloudflare.com/ajax/libs/OverlappingMarkerSpiderfier-Leaflet/0.2.6/oms.min.js"></script>'
+        in rendered_map
+    ), "OverlappingMarkerSpiderfier JS file is missing in the rendered output."
+
+
+def test_marker_addition():
+    """
+    Test that markers are correctly added to the map.
+    """
     N = 10
     np.random.seed(seed=26082009)
     data = np.array(
@@ -21,92 +37,60 @@ def test_overlapping_marker_spiderfier():
         ]
     ).T
 
-    m = folium.Map([45.05, 3.05], zoom_start=14)
-    markers = [
-        folium.Marker(location=loc, popup=f"Marker {i}") for i, loc in enumerate(data)
-    ]
+    m = Map([45.05, 3.05], zoom_start=14)
+    markers = [Marker(location=loc, popup=f"Marker {
+                      i}") for i, loc in enumerate(data)]
 
     for marker in markers:
         marker.add_to(m)
 
-    oms = plugins.OverlappingMarkerSpiderfier(
-        markers=markers, options={"keepSpiderfied": True, "nearbyDistance": 20}
-    ).add_to(m)
-
-    tmpl_for_expected = Template(
-        """
-        var {{this.get_name()}} = (function () {
-            var layerGroup = L.layerGroup();
-            try {
-                var oms = new OverlappingMarkerSpiderfier(
-                    {{ this._parent.get_name() }},
-                    {{ this.options|tojson }}
-                );
-
-                var popup = L.popup({
-                    offset: L.point(0, -30)
-                });
-
-                oms.addListener('click', function(marker) {
-                    var content;
-                    if (marker.options && marker.options.options && marker.options.options.desc) {
-                        content = marker.options.options.desc;
-                    } else if (marker._popup && marker._popup._content) {
-                        content = marker._popup._content;
-                    } else {
-                        content = "";
-                    }
-
-                    if (content) {
-                        popup.setContent(content);
-                        popup.setLatLng(marker.getLatLng());
-                        {{ this._parent.get_name() }}.openPopup(popup);
-                    }
-                });
-
-                oms.addListener('spiderfy', function(markers) {
-                    {{ this._parent.get_name() }}.closePopup();
-                });
-
-                {% for marker in this.markers %}
-                var {{ marker.get_name() }} = L.marker(
-                    {{ marker.location|tojson }},
-                    {{ marker.options|tojson }}
-                );
-
-                {% if marker.popup %}
-                {{ marker.get_name() }}.bindPopup({{ marker.popup.get_content()|tojson }});
-                {% endif %}
-
-                oms.addMarker({{ marker.get_name() }});
-                layerGroup.addLayer({{ marker.get_name() }});
-                {% endfor %}
-            } catch (error) {
-                console.error('Error in OverlappingMarkerSpiderfier initialization:', error);
-            }
-
-            return layerGroup;
-        })();
-        """
+    assert len(m._children) == len(markers) + 1, (
+        f"Expected {len(markers)} markers on the map, but found {
+            len(m._children) - 1}."
     )
-    expected = normalize(tmpl_for_expected.render(this=oms))
 
-    out = normalize(m._parent.render())
 
-    assert (
-        '<script src="https://cdnjs.cloudflare.com/ajax/libs/OverlappingMarkerSpiderfier-Leaflet/0.2.6/oms.min.js"></script>'
-        in out
-    )
+def test_map_bounds():
+    """
+    Test that the map bounds correctly encompass all added markers.
+    """
+    N = 10
+    np.random.seed(seed=26082009)
+    data = np.array(
+        [
+            np.random.uniform(low=45.0, high=45.1, size=N),
+            np.random.uniform(low=3.0, high=3.1, size=N),
+        ]
+    ).T
 
-    assert expected in out
+    m = Map([45.05, 3.05], zoom_start=14)
+    markers = [Marker(location=loc, popup=f"Marker {
+                      i}") for i, loc in enumerate(data)]
+
+    for marker in markers:
+        marker.add_to(m)
 
     bounds = m.get_bounds()
     assert bounds is not None, "Map bounds should not be None"
 
     min_lat, min_lon = data.min(axis=0)
     max_lat, max_lon = data.max(axis=0)
 
-    assert bounds[0][0] <= min_lat
-    assert bounds[0][1] <= min_lon
-    assert bounds[1][0] >= max_lat
-    assert bounds[1][1] >= max_lon
+    assert bounds[0][0] <= min_lat, "Map bounds do not correctly include the minimum latitude."
+    assert bounds[0][1] <= min_lon, "Map bounds do not correctly include the minimum longitude."
+    assert bounds[1][0] >= max_lat, "Map bounds do not correctly include the maximum latitude."
+    assert bounds[1][1] >= max_lon, "Map bounds do not correctly include the maximum longitude."
+
+
+def test_overlapping_marker_spiderfier_integration():
+    """
+    Test that OverlappingMarkerSpiderfier integrates correctly with the map.
+    """
+    m = Map([45.05, 3.05], zoom_start=14)
+    oms = OverlappingMarkerSpiderfier(
+        options={"keepSpiderfied": True, "nearbyDistance": 20})
+    oms.add_to(m)
+
+    assert oms.get_name() in m._children, (
+        f"OverlappingMarkerSpiderfier is not correctly added to the map."
+    )
