Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multiple markers with one icon #2053

Closed

Conversation

Conengmo
Copy link
Member

@Conengmo Conengmo commented Dec 14, 2024

This is an exploration of an idea.

Allow using one Icon object for multiple markers by:

  • separating the icon Javascript object creation from setting it on the marker
    • by having a method that allows registering a marker to an icon
  • making sure setting it on the markers happens after all markers are created

Test it out with:

m = Map((0, 0))
icon = Icon(icon='music')
Marker((0, 0), icon=icon).add_to(m)
Marker((0, 1), icon=icon).add_to(m)

We could extend this to all marker and icon like classes. This does not apply to popups and tooltips, since those are not exclusively used markers, which icons are.

self.marker = marker
self.icon = icon

def register_marker(self, marker: "Marker") -> None:
Copy link
Collaborator

@hansthen hansthen Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it would feel more natural to have a setIcon method on Marker (which generates the same code).

if icon is not None:
# here we make sure the icon is rendered after the last marker that needs it
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels more natural to make SetIcon a child of Marker and add the Icon to the map instead of to the Marker.

if icon is not None:
# here we make sure the icon is rendered after the last marker that needs it
if icon._parent is not None:
Copy link
Collaborator

@hansthen hansthen Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two lines feel a bit awkward. Also they are not necessary if we make Icon a child of Map like in my earlier comments.

if icon is not None:
# here we make sure the icon is rendered after the last marker that needs it
if icon._parent is not None:
del icon._parent._children[icon.get_name()]
self.add_child(icon)
Copy link
Collaborator

@hansthen hansthen Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my proposal, the Icon could be added to the map (or to the parent of the Marker). To ensure backwards compatibility we can add the icon from the marker. We'd have to move it either to add_to(m) or to the render() method from the marker.

I am not sure if add_child is idempotent. At least for Folium it can do no harm to make add_child ensure that children are added only once. Otherwise, we'd have to ensure the child is added only once from here.

if icon is not None:
# here we make sure the icon is rendered after the last marker that needs it
if icon._parent is not None:
del icon._parent._children[icon.get_name()]
self.add_child(icon)
self.icon = icon
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you follow my setIcon proposal, we could call it here.

@hansthen
Copy link
Collaborator

hansthen commented Dec 19, 2024

This is an exploration of an idea.

This is a great idea. I really like this proposal. It is very pragmatic and solves the immediate reported issue. In the review are a few suggestions, mainly to add the generated icon to the map, instead of the Marker. I think it will make the resulting code simpler.

User code would be like this:

m = Map((0, 0))
icon = Icon(icon='music').add_to(m)
Marker((0, 0), icon=icon).add_to(m)
Marker((0, 1), icon=icon).add_to(m)

However, in the review there are also suggestions to make it backwards compatible.

m = Map((0, 0))
icon = Icon(icon='music')
Marker((0, 0), icon=icon).add_to(m)
Marker((0, 1), icon=icon).add_to(m)

@Conengmo
Copy link
Member Author

Thanks for your review! I'll take a look at it next!

@Conengmo
Copy link
Member Author

I've been messing around with your suggestions, but I haven't figured it out so far. When I was working on this two weeks ago, I actually began with trying to solve this in the Marker class, but couldn't figure it out. Maybe you'd like to play around with your idea a bit, see if you can get it to work?

We have make sure the setIcon JS call is always called after both the Icon and the Marker have been initialized in JS. This should be true for multiple cases:

  • Creating an Icon, creating a Marker with the icon argument
  • Creating a Marker, creating an Icon, adding the icon to the marker with icon.add_to(marker).
  • Creating a Marker, creating an Icon, adding the icon to the marker with marker.add_child(icon).

Currently, Branca/Folium don't check if an element was already added to another element. That means we'll get duplicates if multiple markers add the same icon object to their parent/map/figure.

@Conengmo
Copy link
Member Author

Conengmo commented Dec 29, 2024

Alright let's simplify a bit and only support icon reuse when the Marker(icon=icon) argument is used.

  • If you try to solve it in the Marker.__init__(), you get the issue that the marker object itself is not yet added to a parent.
  • If you want to do it in Marker.add_to, you get the issue that you would also have to do it in parent.add_child(), which we can't because that could be anything.
  • If you do it in Marker.render(), you get the issue that the tree is already being evaluated. If you add the icon to a higher level, I get the issue that it's not being evaluated, probably because the loop already skipped that part.

@Conengmo
Copy link
Member Author

It could actually work the same as Layer. Where in the render method, we add an Element to Figure.script that combines the marker and icon. If we do that after all children of Marker are rendered, we are guaranteed both icon and marker JS objects exist. Additional component is we have to make sure the icon is rendered only once, we can because Figure.script contains key names that we can check in.

@Conengmo
Copy link
Member Author

Conengmo commented Jan 1, 2025

Close in favor of #2068

@Conengmo Conengmo closed this Jan 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants