Fix WMS layer control

[ocefpaf]

Commit #152 introduced a bug in the add_layers_to_map (The top control to select the layers).

In this PR:

• I removed the image_overlay from the added_layers. I could not make it work due to a bug I was not able to find.
• I reverted add_layers_to_map to its original code and things are working now.

TODO: Re-add image_overlay to the added_layers without breaking add_layers_to_map for other layers.

@andrewgiessel Let me know your thoughts on this since it was your original PR.

@BibMartin Can you review and merge? One it is done I will port it to master.

[andrewgiessel]

I'll take a look later this week

ag

On Aug 24, 2015, at 13:41, Filipe notifications@github.com wrote:

Commit #152 introduced a bug in the add_layers_to_map (The top control to select the layers).

In this PR:

I removed the image_overlay from the added_layers. I could not make it work due to a bug I was not able to find.
I reverted add_layers_to_map to its original code and things are working now.
TODO: Re-add image_overlay to the added_layers without breaking add_layers_to_map for other layers.

@andrewgiessel Let me know your thoughts on this since it was your original PR.

@BibMartin Can you review and merge? One it is done I will port it to master.

You can view, comment on, or merge this pull request online at:

#200

Commit Summary

Fix WMS layer control
File Changes

M folium/folium.py (20)
Patch Links:

https://github.com/python-visualization/folium/pull/200.patch
https://github.com/python-visualization/folium/pull/200.diff
—
Reply to this email directly or view it on GitHub.

[andrewgiessel]

Thanks for the ping @ocefpaf. Just getting back online after some travels.

I remember asking about this during while I was building my first PR (#152 (comment)) but I think a review fell through the cracks and our tests didn't catch it. My apologizes for introducing a bug- but maybe it's just exposing something that needs to be fixed.

Code Reference

original code
my refactor (in master)
this pr

The original (and the code my refactored version mimicked) builds a string and uses this template to render it, which is simply var layer_list = { {{ layers }} };

You can see that the original code didn't actually use the rendered string and instead used the constructed string. I refactored the string construction for clarity (which I see you kept) and assumed that we needed to use the result from the template. IMO this is the key issue- do we need the template? Do some layers assume a rendering and some don't? It seems as though this could be changed to do all the string stuff in the template, instead of this two stage rendering.

I will look more carefully at what the different layers expect - I probably am doing something non-standard in image_layers, despite my intentions.

@ocefpaf
@BibMartin

[ocefpaf]

I remember asking about this during while I was building my first PR (#152 (comment)) but I think a review fell through the cracks and our tests didn't catch it. My apologizes for introducing a bug- but maybe it's just exposing something that needs to be fixed.

The only thing "more lacking" than our tests are our reviewer free time 😜

Do not worry! As you said we ended up exposing something that needs some re-thinking. The code that created the list of layers was hackish. We need to think how are we going to add this functionality for v0.2.0.

Right now this PR return the status quo of broken-but-working.

[BibMartin]

Sorry, I've been long to answer ; it seems good to me.

@ocefpaf : I have in mind something to fix this in #203's mindset. I'll try to do it in the coming days.

[ocefpaf]

I have in mind something to fix this in #203's mindset. I'll try to do it in the coming days.

Great I do not want this hack to leave v0.1.6 😉

[andrewgiessel]

What's the current status/who's assigned? are we waiting on #203 before
tackling this?

On Thu, Aug 27, 2015 at 5:52 AM Filipe notifications@github.com wrote:

I have in mind something to fix this in #203
#203 mindset. I'll
try to do it in the coming days.

Great I do not want this hack to leave v0.1.6 [image: 😉]

—
Reply to this email directly or view it on GitHub
#200 (comment)
.

[ocefpaf]

What's the current status/who's assigned?

Can I "volunteer" you? 😉

Waiting on #203 before tackling this?

Yes. I really want all new code to be in the light of #203.

[andrewgiessel]

Sure thing
On Thu, Sep 3, 2015 at 17:35 Filipe notifications@github.com wrote:

What's the current status/who's assigned?

Can I "volunteer" you? [image: 😉]

Waiting on #203 #203
before tackling this?

Yes. I really want all new code to be in the light of #203
#203.

—
Reply to this email directly or view it on GitHub
#200 (comment)
.