Add EventHandler to layer object

[hansthen]

In combination with JsCode this makes it easier for users to add on method calls for event handling without extending Folium itself.

The functionality was inspired by PR #1866 by @yschopfer19. The PR was not accepted yet, because of concerns with code duplication. In the approach taken in the current PR, #1866 would not be necessary anymore, as the requested changes could be added completely in client code space.

[hansthen]

@Conengmo Can you have a look at this Pull Request? I would like to have this functionality for Realtime as well. Currently it is not possible to set the bounds for a Map based on Realtime data. If I can attach Event Handlers, I can reset the map's bounds when new data arrives.

[hansthen]

Thanks for the review @ocefpaf. I do have one question. On second thought, maybe it is better to not use a Mixin, but simply define a superclass for Layer Evented with the same functionality. This follows the Leaflet class hierarchy more closely.

Any thoughts on this?

[ocefpaf]

Any thoughts on this?

I don't have a strong opinion on either options. It is mostly a design pattern that we need to choose and stick with it for consistency. With that said, I'd like for you and @Conengmo to agree on one b/c you are the ones maintaining folium the most. I can go with the flow you two dictate here without a problem.

[hansthen]

I don't have a strong opinion on either options. It is mostly a design pattern that we need to choose and stick with it for consistency. With that said, I'd like for you and @Conengmo to agree on one b/c you are the ones maintaining folium the most. I can go with the flow you two dictate here without a problem.

Makes sense. I will try to contact Frank to discuss. We are actually from the same country, so I am sure we should be able to come up with something. I will convert this one to a draft for the moment.

[ocefpaf]

Makes sense. I will try to contact Frank to discuss. We are actually from the same country, so I am sure we should be able to come up with something. I will convert this one to a draft for the moment.

Cool. I'd love to restore some dev channel communication. Frank and I used gitter in the past but that is gone now. I'd be ok with direct email or something similar.

Mostly I'd like to go over two ideas I have for folium as we move forward with you two.

[Conengmo]

Let's hijack this PR to talk about communication :) Would using Github Discussions in this repo work? It's all public, so we can't discuss private things, but I don't mind talking about Folium in the open.

[Conengmo]

I do like the idea in this PR!

I'm thinking though, where in the code we would use this? Or is it a convenience for users who might want to apply additional event handlers?

I'm also wondering where or how we should apply this. Not sure yet what's best.

• Having a mixin that we or that users add where it's needed makes sense. Especially since it only adds an on method, so the inheritance complexity is quite low.
• If we stick with Folium flavour, we would only provide the EventHandler class, and let users add that as a child directly, without an on method.
• If this is generally useful, maybe this should be applied to the source, to the MacroElement in Branca.

[hansthen]

Let's hijack this PR to talk about communication :) Would using Github Discussions in this repo work? It's all public, so we can't discuss private things, but I don't mind talking about Folium in the open.

Discussions would be fine for most things.

@Conengmo We do work / live very near each other. I would like to meet in person if you are open to that. My e-mail address is in my profile. If you want we can setup a meeting via mail.

[hansthen]

I'm thinking though, where in the code we would use this? Or is it a convenience for users who might want to apply additional event handlers?

This is indeed meant as an extra capability for users. In my specific case, I would like to use it to autozoom Realtime layers. In the Leaflet.Realtime code examples, this code is used:

realtime.on('update', function() {
    map.fitBounds(realtime.getBounds(), {maxZoom: 3});
});

I don't think this is currently possible to do without changing or extending the Realtime plugin.

I chose the on method name, because it is also used in Leaflet itself. Also it is slightly less work than adding a separate child element.

[ocefpaf]

Let's hijack this PR to talk about communication :)

I guess I started it 🙈

Would using Github Discussions in this repo work? It's all public, so we can't discuss private things, but I don't mind talking about Folium in the open.

Sure. There are some ideas that I want to pass by you two. We can do that using GD.

[Conengmo]

I opened the Discussions feature and added a Collaborators category: https://github.com/python-visualization/folium/discussions/categories/collaborators. It's public, but only maintainers can create posts. I'd love to hear what ideas you have Filipe!

[Conengmo]

We do work / live very near each other. I would like to meet in person if you are open to that. My e-mail address is in my profile. If you want we can setup a meeting via mail.

That does sound fun, but I don't know if I have time for that to be honest :) I'll email you, let's see where that leads!

[Conengmo]

This is indeed meant as an extra capability for users. In my specific case, I would like to use it to autozoom Realtime layers.

Fair enough, sounds good to add!

I chose the on method name, because it is also used in Leaflet itself. Also it is slightly less work than adding a separate child element.

Both arguments make sense to me, but my thinking is going a bit in another direction.

Looking at the three ideas for implementation I posted earlier, I don't think we should go with the mixin. Having it as an optional mixin is confusing I think, because it's not transparent to users when that on method is available and when not. And it adds lots of extra changes over time as we inevitably slowly add it to all classes.

If possible, I'd like to have Folium be as consistent as possible. Having a single way of working is part of that, and also part of Python. Just having the EventHandler class would fit conceptually best I think. And it's also the simplest change.

If you feel strongly about having an on method on elements, I don't want to say no to that. But I'm now thinking maybe best to add that to Element in Branca. That way it's available in any object we make, which makes sense since it's such a generic Javascript feature.

What would be your preference?

[hansthen]

I will rewrite it as you suggested. I will also let your comments about on simmer for a while. If I still feel adding on would be desirable later, I will open a new PR.

[hansthen]

Mostly I'd like to go over two ideas I have for folium as we move forward with you two.

We now have github discussions 🥂

[Conengmo]

Looks good, you can merge it if you want!

Two suggestions for your consideration, one small, one bigger:

• Give the class a docstring first line, which is now missing.
• We could consider moving this class as well as JsCode to Branca, since they are not Leaflet-specific, but help with any HTML/JS templating. If you agree, we might consider not merging this PR but add the class in Branca directly, as well as adding JsCode there.

[hansthen]

Thanks for the review! I added the extra docstring line.

About your other suggestion, I would go the other direction (but for the same rationale).

In my view, the on method that is generated in the EventHandler is not generic javascript, but specific to Leaflet. It is added by the L.Evented class. L.Evented is a parent to L.Map and L.Layer, but not to L.Control for instance. It generates an error to add an EventHandler to anything that does not generate an L.Map or an L.Layer.

If we moved it to Branca, I would be afraid that people would be encouraged to add EventHandlers to elements that do not support it, generating questions and increasing our maintenance burden.

It is confusing because so many packages (jquery and node) use on for adding event handlers that it feels like javascript should have it.

[Conengmo]

My bad, I didn’t realize the ‘on’ method is Leaflet specific :) totally agree then, it shouldn’t go to Branca.

[Conengmo]

It generates an error to add an EventHandler to anything that does not generate an L.Map or an L.Layer.

Hmm this sounds like something to be mindful of. I guess that’s why you made it a mixin to begin with? To only add it to certain applicable classes?

[hansthen]

It generates an error to add an EventHandler to anything that does not generate an L.Map or an L.Layer.

Hmm this sounds like something to be mindful of. I guess that’s why you made it a mixin to begin with? To only add it to certain applicable classes?

Indeed, but now I think it would be better to follow the Leaflet class hierarchy and create an L.Evented equivalent and make it the parent of Map and Layer.

[Conengmo]

Alright, I see your point better now. Can you give me a few days? I want to look at this a bit better now that I get it more.

[Conengmo]

I see you made #1959, I'll add a review there 👍

[hansthen]

Closing since #1959 supersedes this one.