First draft of new custom pane functionality. #1094

lpkirwin · 2019-03-14T06:44:22Z

This is a rough sketch to agree the API before I flesh it out and add docs. Right now I see two options for the API:

Add custom pane object to map, add kwarg to other layers:

CustomPane("name", ...).add_to(m)
OtherLayer(..., pane="name).add_to(m)

Closest to leaflet API
Would require 'pane' argument or **kwargs for layers (raster layers already pass kwargs, and leaflet layers all take 'pane' as an option
I haven't changed any other class yet, but the example in the notebook works because TileLayer does pass kwargs on to leaflet

Add custom pane object to map, add other layers as child of pane:

p = CustomPane(...).add_to(m)
OtherLayer(...).add_to(p)

Don't have to mess around with extra arguments to the other layers
Further from the leaflet API, but perhaps (?) closer to folium?

What I have so far is the basis for 1. Happy to flip to 2 if that's preferred.

Closes #1088

lpkirwin · 2019-03-14T06:46:01Z

#1088

Conengmo · 2019-03-16T10:41:51Z

Hi @lpkirwin, thanks for taking this up. Good sketch you made. It is clear to me how option 1 would work out.

I have a preference for option 2 because:

it's more like folium
we can do more checks on the Python side and raise exceptions there

But I'm not sure if we can implement that in a sensible way. Leaflet has no methods to move an object to another pane, it has to be done on creation as far as I can tell. So we would have to rummage about in the templates or options of folium objects anyway.

I'd be interested to hear your ideas about how we could implement option 2.

If that seems not workable, option 1 is also a good option. Passing all **kwargs as Leaflet options for most folium objects is something we wanted to do anyway. Perhaps we can find another way to raise meaningful exception on the Python side when users implement this functionality wrong.

- Layers now added as the child of a pane - Options and parent of child layers overridden at render time - Avoids the need to add a kwarg to other layers

lpkirwin · 2019-03-16T15:58:32Z

Hi @Conengmo, no problem.

Added a new commit with an idea for option 2. It follows the second API above, adding other layers as children to the custom pane. I've added a render argument to the CustomPane class that

Adds the 'pane' option to each child's options dict
Switches the parent of each child to the pane's parent (i.e. the map)

and then calls super(). Works fine with TileLayers, e.g. here. I can see not all folium classes have options, so not sure how well this would extend past TileLayers (and unsure how many use-cases there are outside TileLayers...)

What do you think?

Conengmo · 2019-03-16T16:55:14Z

folium/map.py

+            options = json.loads(element.options)
+            options.update({'pane': self.get_name()})
+            element.options = json.dumps(options, sort_keys=True, indent=8)
+            element._parent = self._parent


When you move the element up the tree it will not get rendered, because the parent of CustomPane has already been rendered...

It rendered OK in my test notebook, see screenshot below. I haven't properly moved the element up the tree -- just swapped out the parent, but the tile layer still exists in the pane's children list.

Ah right, it's in the list of children so it will get rendered. Good one.

But then why change its parent?

Changed the parent just so that, when render is called on the layer, the javascript ends up as

element().add_to(map_name)

rather than

element().add_to(pane_name)

which leaflet wouldn't understand

That makes sense, thanks for explaining.

Conengmo · 2019-03-16T16:57:31Z

Thanks for the quick test. I added a comment about a possible problem with the child elements not being rendered. That's probably fixable by calling render manually. In general I think it best to avoid messing with the object tree. But sometimes it has to be done to get cool features :)

What do you think is the best approach of these two?

lpkirwin · 2019-03-16T17:11:08Z

I like the API better on the second, but same as you I worry that hacking around in the object tree could have some unintended consequences, e.g. when interacting with other objects or features that might not expect this behavior. After calling render() on the pane, there's some inconsistent state (element has the map as a _parent, but is in the pane's _children and not the map's _children).

But I don't have other concrete ideas at the moment. Maybe it would be possible for the tile layer to check at render time whether its parent was a map or a pane, and then use its parent/grandparent name as appropriate.

Conengmo · 2019-03-16T17:57:11Z

Hmm given your exploration I now lean towards option 1. It seems less fragile than mucking around with the object tree.

I'm currently working on making sure each object passes **kwargs as options, so that should work together nicely.

lpkirwin · 2019-03-16T18:01:04Z

Sounds good, and I agree. I'll revert back to option 1 and flesh out the documentation

lpkirwin · 2019-03-21T22:16:41Z

Hi @Conengmo, have switched back to the earlier implementation and added a docstring and a bit more to the notebook. Couple questions for you:

Should I do anything about kwargs, e.g. adding 'pane' as a specific kwarg in other classes, or adding **kwargs to classes that don't accept them? You mentioned that this was being developed elsewhere, so if so I can hold off.
A straightforward test to add would be the equivalent of test_popup_show() in test_map.py. Do you think any other tests would be helpful?

Conengmo

This is looking good! I added some smaller comments to put the dots on the i so to say, hope you can have a look at them.

Should I do anything about kwargs, e.g. adding 'pane' as a specific kwarg in other classes, or adding **kwargs to classes that don't accept them? You mentioned that this was being developed elsewhere, so if so I can hold off.

That PR has been merged, so most classes accept options in kwargs. We could consider adding pane as an explicit argument to some classes, but given that this is an advanced feature I reckon we can keep it as it is now.

A straightforward test to add would be the equivalent of test_popup_show() in test_map.py. Do you think any other tests would be helpful?

Yes, something like test_popup_show() would be fine! We want to make more functional tests later, but for now having a test that verifies stuff didn't got broken basically is already very useful.

folium/map.py

Conengmo · 2019-03-31T19:01:50Z

folium/map.py

+        (625) corresponds to between markers and tooltips. Default
+        panes and z-indexes can be found at
+        https://github.com/Leaflet/Leaflet/blob/v1.0.0/dist/leaflet.css#L87
+    pointer_events: bool, default False


What are the consequences of setting the default here to False?

Maybe good to add a small section in the example notebook on how this option can be used.

For my particular use case (labels on top of choropleth), setting this to False ensures that the new layer won't mask mouse events from lower layers, e.g. if you want a tooltip to pop up when you hover over a choropleth.

Right now I'm struggling to imagine a case where you'd want this to be True! Have googled around for how others are using custom panes, and none of them involve interactivity. I could remove the keyword from the class -- I put it in thinking it would be better to keep flexibility even if I can't anticipate how it would be used.

Thanks for explaining, sounds like False is a good default. I agree on leaving it in.

folium/map.py

… into custom-panes Pulling latest changes from master

- Cleaning up the jinja template - Changing link in the docstring - Adding a test for correct script representation - Adding a couple more tile templates to the standard set

- Changing python version to match other notebooks - Removing TOC section from nbextension

Conengmo · 2019-04-14T15:46:51Z

Just had a look at your changes and I think this is ready! The code looks nice, you wrote a test, the example notebook is good.

We fixed the issue with the notebook tests on Travis before, so I took the liberty of updating your branch from master. When the tests are passed I'll go ahead and merge this.

lpkirwin · 2019-04-15T14:00:50Z

Just had a look at your changes and I think this is ready! The code looks nice, you wrote a test, the example notebook is good.

We fixed the issue with the notebook tests on Travis before, so I took the liberty of updating your branch from master. When the tests are passed I'll go ahead and merge this.

Thanks Frank!

jmaralc · 2019-08-12T14:28:59Z

Great feature!! thanks a lot 👍

First draft of new custom pane functionality.

dae22b2

Conengmo added the in discussion This PR or issue is being discussed label Mar 16, 2019

Trying new API for the CustomPane element.

84e4bd2

- Layers now added as the child of a pane - Options and parent of child layers overridden at render time - Avoids the need to add a kwarg to other layers

Removed the (now unneeded) 'name' arg from CustomPane.

53bebb2

Conengmo reviewed Mar 16, 2019

View reviewed changes

lpkirwin added 2 commits March 21, 2019 21:41

Switch back to first proposed API, added docstring

2b58876

pull from remote master, fixed conflict in map.py

2e29884

Conengmo requested changes Mar 31, 2019

View reviewed changes

Conengmo added waiting for changes This PR has been reviewed and changes are needed before merging and removed in discussion This PR or issue is being discussed labels Mar 31, 2019

lpkirwin added 3 commits April 5, 2019 22:08

Merge branch 'master' of https://github.com/python-visualization/folium…

9981eff

… into custom-panes Pulling latest changes from master

Implementing changes suggested in the PR.

3db1314

- Cleaning up the jinja template - Changing link in the docstring - Adding a test for correct script representation - Adding a couple more tile templates to the standard set

Travis CI failed on last commit, trying:

6dc63a8

- Changing python version to match other notebooks - Removing TOC section from nbextension

Conengmo approved these changes Apr 14, 2019

View reviewed changes

Merge remote-tracking branch 'remotes/upstream/master' into custom-panes

787bcfd

Conengmo added ready PR is ready for merging and removed waiting for changes This PR has been reviewed and changes are needed before merging labels Apr 14, 2019

Conengmo merged commit 3ad7a28 into python-visualization:master Apr 14, 2019

Conengmo removed the ready PR is ready for merging label Apr 14, 2019

lpkirwin deleted the custom-panes branch April 15, 2019 14:01

First draft of new custom pane functionality. #1094

First draft of new custom pane functionality. #1094

Uh oh!

Conversation

lpkirwin commented Mar 14, 2019 • edited by Conengmo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpkirwin commented Mar 14, 2019

Uh oh!

Conengmo commented Mar 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpkirwin commented Mar 16, 2019

Uh oh!

Conengmo Mar 16, 2019

Choose a reason for hiding this comment

Uh oh!

lpkirwin Mar 16, 2019

Choose a reason for hiding this comment

Uh oh!

Conengmo Mar 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpkirwin Mar 16, 2019

Choose a reason for hiding this comment

Uh oh!

Conengmo Mar 16, 2019

Choose a reason for hiding this comment

Uh oh!

Conengmo commented Mar 16, 2019

Uh oh!

lpkirwin commented Mar 16, 2019

Uh oh!

Conengmo commented Mar 16, 2019

Uh oh!

lpkirwin commented Mar 16, 2019

Uh oh!

lpkirwin commented Mar 21, 2019

Uh oh!

Conengmo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Conengmo Mar 31, 2019

Choose a reason for hiding this comment

Uh oh!

lpkirwin Apr 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Conengmo Apr 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Conengmo commented Apr 14, 2019

Uh oh!

lpkirwin commented Apr 15, 2019

Uh oh!

jmaralc commented Aug 12, 2019

Uh oh!

Uh oh!

lpkirwin commented Mar 14, 2019 •

edited by Conengmo

Loading

Conengmo commented Mar 16, 2019 •

edited

Loading

Conengmo Mar 16, 2019 •

edited

Loading

lpkirwin Apr 5, 2019 •

edited

Loading