Leaflet control #1902

hansthen · 2024-03-12T19:03:39Z

Based on the original draft by @Conengmo

Create a new base class Control, which can be used to more easily create new control based plugins.
Update the original CustomControl so that it also accepts a style parameter.
Change all the plugins that create L.Control subclasses to use Control as a base class.

This is to some extent a work-in-progress. Once #1898 is merged I can make Control inherit from JSCSSMixin and remove that class from all the plugins and just use the inherited method from JSCSSMixin inheritance to add stylesheets and javascript.

Furthermore, many of the plugins could be simplified. In the current implementation options are very often coded in the _Template. By using the Control base class and JsCode parameters as part of the specified options we can avoid that. Or, at least, we can move flow code from the Jinja templates into the __init__ method, which is cleaner.

Also, I tried to make map.LayerControl as subclass of Control, but that resulted in circular dependencies.

I did not do everything at once, because I want to take small steps and check if everything stays backward compatible.

Conengmo

This PR contains three separate ideas that I think we should handle separately.

CustomControl is useful I think, and something requested by users. It inherits from Control, but doesn't use anything from it, so it might as well be separate.

I'm not sure what Control is supposed to do. It's a base class for many other classes in this PR, but none of the now child classes use anything from it. No arguments are passed in super().__init__() and the template is overwritten. So it might as well not be there. In which case is shouldn't. Long chains of object inheritance make things difficult to grok.

Then there are also some changes in Realtime that on first glance are not linked to the control change.

So my suggestion would be to take the first and third point and deal with those in separate PRs. And then have a discussion about the second point, how the class structure of Folium should be.

hansthen · 2024-05-19T04:16:03Z

Thanks for the review. I will rework it as requested.

1. Created a root control class which can load any Leaflet control 2. Made all items that generate a leaflet Control in plugins inherit from Control instead of MacroElement. 3. Still TODO: It would be nice if Control itself inherited from JSCSSMixin. But for that we'd need to revamp loading of css and js. 4. Probably many of the plugins could be rewritten more simply by using features from Control and JsCode instead of using custom templates. 5. map.LayerControl does not inherit from Control due to issues with circular imports. (features import map, so map cannot import features). We'd need to move LayerControl for that to work, but that is breaking change.

Most of the code of mouse_position has been removed. Instead it simply calls the constructor of Control with the typed parameters.

hansthen · 2024-06-02T18:55:26Z

@Conengmo I removed the parts that could be handled as separate Pull Requests. I also rewrote the MousePosition control plugin. I think it clarifies why adding Control allows us to simplify a large part of the code base. Let me know what you think.

I would prefer to rewrite the other plugins incrementally. However, that means that for a while there will be inconsistent approaches in the code.

Conengmo

The MousePosition plugin is indeed a good motivation for the new Control class.

The Control class does two things:

it's a template for creating any Leaflet control class
it's used in the inheritance tree to indicate something is a control.

What I find difficult is that in that second case the template and the init of Control should be skipped. But the init it ran, and creates some self.options that are not used. That sounds like it could cause issues, running extra code in the tree that's not needed.

I'd appreciate it if you could keep the PR's minimal, these style changes with all these trailing comma's make it hard to review. If you want to make style changes, please do these in a separate PR.

And just to be sure, I'm not proposing major changes or something, but I'm not convinced on this PR yet, so I want to understand better.

Conengmo · 2024-06-14T15:31:54Z

folium/plugins/beautify_icon.py

@@ -95,7 +95,7 @@ def __init__(
        inner_icon_style="",
        spin=False,
        number=None,
-        **kwargs
+        **kwargs,


why are all these trailing commas in this PR? Nothing can follow a **kwargs entry so a trailing comma is not necessary. Black or ruff doesn't add these if I'm not mistaken, so why add them?

They were added by ruff format, so don't blame me :-). I agree they do not make sense in this case. As far as I know there is no option to remove these trailing commas. I can manually undo these kind of changes, but its a hassle. I'd prefer to just blindly follow ruff.

We don't run ruff format on this repo yet though. We use Black to autoformat and have Ruff as linter. So it's better not to run ruff format, since it will create a lot of changes.

We could switch over to ruff format though and ditch Black. Maybe that's good to do anyway, and if we merge that before this PR that also solves the issue.

My bad, I thought we used ruff as a formatter as well. I can create an extra PR to ditch black if you want, but I have no strong opinion either way. If not I will undo the ruff formatting changes.

Conengmo · 2024-06-14T15:32:26Z

folium/plugins/realtime.py

@@ -42,7 +42,7 @@ class Realtime(JSCSSMixin, MacroElement):
    remove_missing: bool, default False
        Should missing features between updates been automatically
        removed from the layer
-    container: Layer, default GeoJson
+    container: FeatureGroup, default GeoJson


this is in a separate PR already

Will rebase once the other PR is accepted.

Conengmo · 2024-06-14T15:38:06Z

folium/plugins/mouse_position.py

+        self.add_js_link(
+            "Control_MousePosition_js",
+            "https://cdn.jsdelivr.net/gh/ardhi/Leaflet.MousePosition/src/L.Control.MousePosition.min.js",
+        )
+        self.add_css_link(
+            "Control_MousePosition_css",
+            "https://cdn.jsdelivr.net/gh/ardhi/Leaflet.MousePosition/src/L.Control.MousePosition.min.css",


please use the JSCSSMixin class attributes for these

Why? The method calls seem cleaner to me compared to the static attributes?

the static attributes allow users that want to create offline maps to bundle these dependencies and then update the values in the static attributes. I know they might also use these new methods, but it's breaking behavior to define these in the init for this use case.

Also, I think it's better to have a consistent way of working. So not have two different ways of defining dependencies.

Conengmo · 2024-06-14T15:43:02Z

folium/plugins/timestamped_geo_json.py

 from folium.folium import Map
 from folium.utilities import get_bounds, parse_options


-class TimestampedGeoJson(JSCSSMixin, MacroElement):
+class TimestampedGeoJson(JSCSSMixin, Control):


just as an example, my issue with using the 'correct' Leaflet classes for Python inheritance is that here we don't only make a control, we also make a geojson layer. So which should it be? Inheriting from Control is not really helping with anything here, only making it more confusing IMO.

I agree with you that it makes it confusing here, but my remedy would go the other direction.

My feeling here is that adding both a Layer and a Control is a design mistake. There are tickets open in which users have requested to be able to add multiple Layers and control them using a single control. Like the Timeline plugin does. If I find the courage I would split the different Timestamped into seperate Control and Layer implementations (or rewrite them using the TimelineControl plugin).

That's fair, that they should be separate. But until we rebuild this we have the current situation, where the object is not a control or a layer, but a Folium specific something.

Okay, I will remove the Control base class for these classes for now.

Conengmo · 2024-06-14T15:43:25Z

folium/features.py

@@ -1973,3 +1985,65 @@ def __init__(
            out.setdefault(cm(color), []).append([[lat1, lng1], [lat2, lng2]])
        for key, val in out.items():
            self.add_child(PolyLine(val, color=key, weight=weight, opacity=opacity))
+
+
+class Control(MacroElement):


how would the CustomControl thing we also want use this?

Good point and the answer is I don't know yet.

I noted that the CustomControl is like a Control that is linked to a layer, something I did not realize at first. In pure Leaflet this is something of an anomaly, as both Controls and Layers are contained in a Map and there is no structural association between Control and Layer. Typically it is the other way round and a Control determines the visibility of the Layer.

My guess is it would be fine as you are implementing it now. It would probably be the same as other Controls that are associated with a set of Layers.

I noted that the CustomControl is like a Control that is linked to a layer, something I did not realize at first. In pure Leaflet this is something of an anomaly, as both Controls and Layers are contained in a Map and there is no structural association between Control and Layer. Typically it is the other way round and a Control determines the visibility of the Layer.

That's interesting... So I guess the custom control is a bit of a weird one, and it's okay not to consider it here.

hansthen · 2024-06-16T15:34:18Z

First off, thanks for looking at the Pull Request. I know you are not enthusiastic about making major changes to Folium, so it is appreciated that you are willing to consider this.

it's used in the inheritance tree to indicate something is a control.
Indeed. I also have the vague hope it will in the future help the developers of streamlit-folium. This package allows for dynamic changes to the generated Leaflet map. They do fairly complicated stuff now with regular expressions to separate Layers and Controls. Having a simple marker in the python object hierarchy would make their life simpler.

What I find difficult is that in that second case the template and the init of Control should be skipped. But the init it ran, and creates some self.options that are not used. That sounds like it could cause issues, running extra code in the tree that's not needed.

Just to clarify, you have no difficulties with how I rewrote the mouse_position? It is rather that in all the other instances, there is an unused parent class? I agree with you there, that the situation is not ideal. I would rewrite all the plugins (and native Controls) similarly, but it is a lot of work. Not all of it will be as straightforward as for the mouse_position plugin. I would prefer to approach this incrementally after (and if) we come to an agreement on the basic approach.

Would it help if I added the Control class also incrementally whenever I update the a plugin to use the new approach?

Conengmo · 2024-06-16T16:03:21Z

I also have the vague hope it will in the future help the developers of streamlit-folium. This package allows for dynamic changes to the generated Leaflet map. They do fairly complicated stuff now with regular expressions to separate Layers and Controls. Having a simple marker in the python object hierarchy would make their life simpler.

That's interesting! I'd like to know more about that.

I gotta run, but just as a quick thought for now. Does it make sense to split this class into a Control class that's basically empty but indicates the object name in Javascript is a control, and a ControlTemplate or whatever (bad name) that can be used to create any control object? Where Control is inherited from by whichever class it applies to, and ControlTemplate inherits from Control and is used to build control objects?

I understand a bit better know to have our Python classes only implement one Javascript class. Question I have now is how we then bundle these in a way that's convenient for our Python users?

Conengmo · 2024-06-16T16:05:26Z

Just to clarify, you have no difficulties with how I rewrote the mouse_position?

No, I like the abstraction. Only doubt would be whether the abstraction is common enough to warrant abstracting it.

Would it help if I added the Control class also incrementally whenever I update the a plugin to use the new approach?

Not sure if I fully understand, but if you're talking about not adding Control as a parent to a class that implements multiple Javascript classes, then yes, doing that incrementally sounds like a good plan.

hansthen · 2024-06-16T19:02:18Z

Not sure if I fully understand, but if you're talking about not adding Control as a parent to a class that implements multiple Javascript classes, then yes, doing that incrementally sounds like a good plan.

I meant that in the current PR, I added Control already to several python classes that generate a Leaflet Control without adapting their implementation. These classes just have a different base class, but do not use their parent's template of constructor. This should not break anything, but it is maybe not the cleanest solution. I wanted to rewrite all the plugins later to cleanup the code, calling the parent constructor and probably removing their template code.

My alternative would be to not add Control as base class for the other plugins now, but only after I have adapted the code.

Conengmo added the waiting for review PR is waiting to be reviewed label Apr 3, 2024

hansthen mentioned this pull request Apr 30, 2024

Make Folium follow the Leaflet class hierarchy more closely #1941

Open

hansthen requested a review from Conengmo May 4, 2024 11:48

Conengmo requested changes May 6, 2024

View reviewed changes

Conengmo removed the waiting for review PR is waiting to be reviewed label May 6, 2024

hansthen marked this pull request as draft May 19, 2024 04:16

hansthen added 6 commits June 2, 2024 20:36

WIP: add a css parameter to add styling

feb0930

Add documentation and fix small errors

d20567e

Rewrite mouse_position to demonstrate Control

d020cc5

Most of the code of mouse_position has been removed. Instead it simply calls the constructor of Control with the typed parameters.

Removed CustomControl from this PR

c5ce381

Undo typing changes in realtime

1f55089

hansthen force-pushed the leaflet-control branch from 7c15ac2 to 1f55089 Compare June 2, 2024 18:36

hansthen requested a review from Conengmo June 2, 2024 18:50

hansthen marked this pull request as ready for review June 2, 2024 18:57

Conengmo reviewed Jun 14, 2024

View reviewed changes

hansthen marked this pull request as draft October 6, 2024 07:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leaflet control #1902

Leaflet control #1902

hansthen commented Mar 12, 2024 •

edited

Loading

Conengmo left a comment

hansthen commented May 19, 2024

hansthen commented Jun 2, 2024

Conengmo left a comment •

edited

Loading

Conengmo Jun 14, 2024

hansthen Jun 16, 2024 •

edited

Loading

Conengmo Jun 16, 2024

Conengmo Jun 16, 2024

hansthen Jun 16, 2024

Conengmo Jun 14, 2024

hansthen Jun 16, 2024

Conengmo Jun 14, 2024

hansthen Jun 16, 2024

Conengmo Jun 16, 2024 •

edited

Loading

Conengmo Jun 14, 2024

hansthen Jun 16, 2024

Conengmo Jun 16, 2024

hansthen Jun 16, 2024

Conengmo Jun 14, 2024

hansthen Jun 16, 2024

Conengmo Jun 16, 2024

hansthen commented Jun 16, 2024

Conengmo commented Jun 16, 2024

Conengmo commented Jun 16, 2024

hansthen commented Jun 16, 2024

Leaflet control #1902

Are you sure you want to change the base?

Leaflet control #1902

Conversation

hansthen commented Mar 12, 2024 • edited Loading

Conengmo left a comment

Choose a reason for hiding this comment

hansthen commented May 19, 2024

hansthen commented Jun 2, 2024

Conengmo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hansthen Jun 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Conengmo Jun 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hansthen commented Jun 16, 2024

Conengmo commented Jun 16, 2024

Conengmo commented Jun 16, 2024

hansthen commented Jun 16, 2024

hansthen commented Mar 12, 2024 •

edited

Loading

Conengmo left a comment •

edited

Loading

hansthen Jun 16, 2024 •

edited

Loading

Conengmo Jun 16, 2024 •

edited

Loading