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

Add return values for selected layers in LayersControl #217

Merged
merged 3 commits into from
Sep 28, 2024

Conversation

reyemb
Copy link
Contributor

@reyemb reyemb commented Sep 26, 2024

Description:

I primarily use st_folium in combination with LayerControl in Streamlit applications. However, one challenge I encountered is that there is no straightforward way to determine which layers are currently selected from the LayerControl. This limits functionality when trying to dynamically update components such as legends for each layer.

Here's a basic example of the issue:

  • The user toggles visibility of multiple layers using LayerControl.
  • There is no direct support to return the currently selected layers, which makes it difficult to update elements like legends that correspond to those layers.

To address this issue, I introduced a modification that allows st_folium to return the currently selected layers when interacting with the LayerControl. Now, when users toggle visibility of layers, the Streamlit app can respond accordingly, such as updating a legend for each selected layer. This greatly enhances interactivity.

@@ -111,6 +113,18 @@ function onDraw(e: any) {
return onLayerClick(e)
}

function removeLayer(e: any) {
const global_data = window.__GLOBAL_DATA__
global_data.selected_layers.delete(e.layer["wmsParams"]["layers"]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this generically apply to any type of map layer added? Or is this just certain types of layers?

@blackary
Copy link
Collaborator

@reyemb this looks like a great addition to me -- there are a couple of formatting issues that github actions caught. Would you mind fixing those? You can fix those manually, or just run pre-commit install and pre-commit run --all-files locally to have them auto-fixed for you.

…st Layer because some provider use the same Layer but different Urls.
@reyemb
Copy link
Contributor Author

reyemb commented Sep 27, 2024

Thank you for the feedback!

Here are the changes I made in response to the issues raised:

Removed the Out-of-Place Comment:

I removed the comment that was flagged as out-of-place in the previous review.

Added an Example File:

I provided an additional example file showcasing how the newly introduced functionality.

Support for All Layer Types:

In response to the feedback about whether the solution applies to different types of map layers: Yes, the solution I've implemented supports all types of layers added to the map, not just specific ones.

Previously, I was only returning the names of the layers, but I found that some providers use layers with identical names while differentiating by URLs. For this reason, the updated implementation now returns a string containing a combination of both the layer's name and URL.
The format is:

  • layer_name1;url1,layer_name2;url2
  • Layers and URLs are separated by ";"
  • Multiple layers are separated by ","

@blackary
Copy link
Collaborator

That way of separating out layers seems OK, but I think it would be better to just return them as json objects, like

[{"name": "foo", "url": "bar"}...]

Is there a good reason not to return them like that?

Also, can you please fix the linting changes? As I suggested here #217 (comment), you can do this locally with pre-commit.

@reyemb
Copy link
Contributor Author

reyemb commented Sep 27, 2024

Hey I thought I go the easy way by returning a string, but yeah this approach does look better. I also ran pre-commit hopefully it is okay now :)

@reyemb
Copy link
Contributor Author

reyemb commented Sep 27, 2024

I dont know why the test fails. It worked nicely on my side :(
image

@randyzwitch
Copy link
Owner

Looks like the tests are being flaky @reyemb with a bunch of timeouts happening. Me or @blackary will try running the tests locally and if they pass, then we'll merge

@randyzwitch
Copy link
Owner

All but one passes locally for me @reyemb

============================================================= FAILURES =============================================================
___________________________________________ test_layer_control_dynamic_update[chromium] ____________________________________________

page = <Page url='http://localhost:8699/dynamic_layer_control'>

    def test_layer_control_dynamic_update(page: Page):
        page.get_by_role("link", name="dynamic layer control").click()
        # page.get_by_text("Show generated code").click()
    
        page.frame_locator('iframe[title="streamlit_folium\\.st_folium"]').get_by_text(
            "Parcels"
        ).click()
        expect(
            page.frame_locator('iframe[title="streamlit_folium\\.st_folium"]').get_by_text(
                "Parcels"
            )
        ).not_to_be_checked()
        expect(
            page.frame_locator('iframe[title="streamlit_folium\\.st_folium"]')
            .get_by_role("img")
            .locator("svg")
        ).to_be_hidden()
        expect(page.get_by_text("dashArray")).to_be_hidden()
    
        page.get_by_test_id("stRadio").get_by_text("Both").click()
        page.frame_locator('iframe[title="streamlit_folium\\.st_folium"]').get_by_text(
            "Parcels"
        ).click()
        expect(
            page.frame_locator('iframe[title="streamlit_folium\\.st_folium"]').get_by_text(
                "Parcels"
            )
>       ).not_to_be_checked()
E       AssertionError: Locator expected not to be checked
E       Actual value: True 
E       Call log:
E       LocatorAssertions.not_to_be_checked with timeout 5000ms
E         - waiting for frame_locator("iframe[title=\"streamlit_folium\\.st_folium\"]").get_by_text("Parcels")
E         -   locator resolved to <span> Parcels</span>
E         -   unexpected value "checked"
E         -   locator resolved to <span> Parcels</span>
E         -   unexpected value "checked"
E         -   locator resolved to <span> Parcels</span>
E         -   unexpected value "checked"
E         -   locator resolved to <span> Parcels</span>
E         -   unexpected value "checked"
E         -   locator resolved to <span> Parcels</span>
E         -   unexpected value "checked"
E         -   locator resolved to <span> Parcels</span>
E         -   unexpected value "checked"
E         -   locator resolved to <span> Parcels</span>
E         -   unexpected value "checked"
E         -   locator resolved to <span> Parcels</span>
E         -   unexpected value "checked"
E         -   locator resolved to <span> Parcels</span>
E         -   unexpected value "checked"

tests/test_frontend.py:384: AssertionError
========================================================= warnings summary =========================================================
tests/test_package.py::test_map
  <frozen importlib._bootstrap>:488: DeprecationWarning: Type google.protobuf.pyext._message.ScalarMapContainer uses PyType_Spec with a metaclass that has custom tp_new. This is deprecated and will no longer be allowed in Python 3.14.

tests/test_package.py::test_map
  <frozen importlib._bootstrap>:488: DeprecationWarning: Type google.protobuf.pyext._message.MessageMapContainer uses PyType_Spec with a metaclass that has custom tp_new. This is deprecated and will no longer be allowed in Python 3.14.

tests/test_package.py::test_map
  /Users/randyzwitch/miniconda3/envs/streamlit-folium/lib/python3.12/site-packages/google/protobuf/internal/well_known_types.py:91: DeprecationWarning: datetime.datetime.utcfromtimestamp() is deprecated and scheduled for removal in a future version. Use timezone-aware objects to represent datetimes in UTC: datetime.datetime.fromtimestamp(timestamp, datetime.UTC).
    _EPOCH_DATETIME_NAIVE = datetime.datetime.utcfromtimestamp(0)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
------------------------- generated xml file: /Users/randyzwitch/github/streamlit-folium/test-results.xml --------------------------
===================================================== short test summary info ======================================================
FAILED tests/test_frontend.py::test_layer_control_dynamic_update[chromium] - AssertionError: Locator expected not to be checked
============================================ 1 failed, 21 passed, 3 warnings in 35.83s =============================================
(streamlit-folium) randyzwitch@Randys-MBP streamlit-folium % 

@randyzwitch
Copy link
Owner

Going to merge this. I believe that #219 from @blackary might fix the test, not sure

@randyzwitch randyzwitch merged commit 0b2a92f into randyzwitch:master Sep 28, 2024
0 of 5 checks passed
randyzwitch added a commit that referenced this pull request Sep 28, 2024
randyzwitch added a commit that referenced this pull request Sep 28, 2024
@randyzwitch
Copy link
Owner

@reyemb ...my apologies, but can you re-submit this PR? I made a logical error about the order I could merge these PRs, and I cannot re-open this one

Should just be hitting the button again from your fork to re-submit

@reyemb reyemb mentioned this pull request Sep 30, 2024
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.

3 participants