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

on_change method is not triggered in Streamlit 1.34 #70

Open
jordi-crespo opened this issue May 5, 2024 · 12 comments
Open

on_change method is not triggered in Streamlit 1.34 #70

jordi-crespo opened this issue May 5, 2024 · 12 comments

Comments

@jordi-crespo
Copy link

I have been working on streamlit 1.32.0 with streamlit-option-menu.

I use the on_call function to modify the cookies and it is working correctly.

When upgrading to streamlit 1.34.0, this function seems not to be executed.

option_menu(
                menu_title=None,
                options=self.menu_options,
                icons=self.menu_icons,
                menu_icon="cast",
                orientation="vertical",  # Cambio a vertical para la barra lateral
                default_index=default_index,  # Usar el índice de la opción guardada
                on_change=self.on_change_callback,
                key='navigation_menu',
                styles={
                    "container": {
                        "background-color": "#fff",
                        "padding": "0px",
                        "margin": "0px",
                        "border-radius": "0px"
                    },
                    "nav-link-selected": {"background-color": "#037efc", "color": "#fff"},
                    "nav-link": {"--hover-color": "#f0f2f6", "color": "##191c1e", "font-family": "'Source Sans Pro', sans-serif"},
                },
            )
            
     def handle_navigation_change(self, navigation_value):
        navigation = st.session_state.get(navigation_value)
        if navigation in self.pages or navigation == "Logout":
            self.cookie_manager.cookies["last_visited_page"] = navigation
@fgdvir
Copy link
Collaborator

fgdvir commented May 5, 2024

I've originally added the callback from this thread. It is a workaround, so it is possible that something broke in the new streamlit.
need to investigate it, but it is possible that the solution will be to remove this feature if a new workaround won't be found

@Socvest
Copy link

Socvest commented May 6, 2024

I posted this here and here too as I noticed this

@raethlein
Copy link

The internal package structure has changed due to a refactoring; if you change the import in the streamlit_callback.py to from streamlit.components.v1 import custom_component as _components, it should work again.

That being written, this is not an official API we are going to guarantee from Streamlit-side 😉 However, we will look into this more closely and try to come up with a clean API for the future so that this patch is not needed anymore. In the meantime, I hope this^ unblocks you (for now).

raethlein added a commit to streamlit/streamlit that referenced this issue May 22, 2024
## Describe your changes

In the past, some custom components have used a
patch (https://gist.github.com/okld/1a2b2fd2cb9f85fc8c4e92e26c6597d5) to
register an on_change callback. Recently, we have done some refactoring
that broke this workaround. This PR is a suggestion to extend our
official API to make the patch redundant.

Note that we only want to pass the `on_change_callback` and not the
`args` and `kwargs`.
The `register_widget` function today uses `args` and `kwargs` as
keywords to pass to the `on_change callback`. Besides the unfortunate
naming - these are special keywords meant for functions themselves and
not for pass-through arguments - we are thinking about deprecating them
entirely, since you can wrap the callback easily to pass the arguments.

## GitHub Issue Link (if applicable)

Closes #3977
Related to victoryhb/streamlit-option-menu#70

## Testing Plan

- Explanation of why no additional tests are needed
- Unit Tests (JS and/or Python)
- A new unit test is added to make sure the on_change callback is called
when the value changes during a ScriptRun
- E2E Tests
  - prepare `on_change` callback test in the `option_menu` function
- Any manual testing needed?
- I manually tested it on the example of
[streamlit-option-menu](https://github.com/victoryhb/streamlit-option-menu)

---

**Contribution License Agreement**

By submitting this pull request you agree that all contributions to this
project are made under the Apache 2.0 license.
@nikatlas
Copy link
Contributor

nikatlas commented May 23, 2024

I published a temporary patched version here if someone is interested from #71:

https://pypi.org/project/streamlit-option-menu-patch/

@raethlein
Copy link

We have also merged a PR (streamlit/streamlit#8633) to allow passing on_change natively to the call of a custom component. This means, for future versions (>= 1.36) of Streamlit, the workaround patch should not be necessary anymore.

@nikatlas
Copy link
Contributor

We have also merged a PR (streamlit/streamlit#8633) to allow passing on_change natively to the call of a custom component. This means, for future versions (>= 1.36) of Streamlit, the workaround patch should not be necessary anymore.

Yeah I noticed that but that requires some refactoring on the streamlit-option-menu as far as I understood from your PR. Is this the case? @raethlein

@raethlein
Copy link

Yeah, you should be able to remove these lines and the register_callback script in general and then pass the on_change callback to the _component_func call here.
Since right now streamlit-option-menu documents to accept a key argument in the passed callback, I think the full snippet would look something like this:

if on_change is not None:
    if key is None:
        st.error("You must pass a key if you want to use the on_change callback for the option menu")
    else:
        _on_change = lambda: on_change(key)

component_value = _component_func(options=options, 
            key=key, defaultIndex=default_index, icons=icons, menuTitle=menu_title, 
            menuIcon=menu_icon, default=options[default_index], 
            orientation=orientation, styles=styles, manualSelect=manual_select, on_change=_on_change)

@victoryhb
Copy link
Owner

Hi @raethlein @nikatlas, sorry for the late reply. I have just merged the PR by @nikatlas (thanks!) and pushed a new PyPi version. I haven't followed the recent API changes too closely. Are there any further changes that we need to make for the upcoming Streamlit releases?

@nikatlas
Copy link
Contributor

Yeah, you should be able to remove these lines and the register_callback script in general and then pass the on_change callback to the _component_func call here. Since right now streamlit-option-menu documents to accept a key argument in the passed callback, I think the full snippet would look something like this:

if on_change is not None:
    if key is None:
        st.error("You must pass a key if you want to use the on_change callback for the option menu")
    else:
        _on_change = lambda: on_change(key)

component_value = _component_func(options=options, 
            key=key, defaultIndex=default_index, icons=icons, menuTitle=menu_title, 
            menuIcon=menu_icon, default=options[default_index], 
            orientation=orientation, styles=styles, manualSelect=manual_select, on_change=_on_change)

@victoryhb I think the changes mentioned here are the proper solution.

@victoryhb
Copy link
Owner

I wonder if you have time to do another PR for that? @nikatlas

@nikatlas
Copy link
Contributor

nikatlas commented May 29, 2024

Cannot promise anything, not much time thats why I went with the patch. Not familiar with streamlit yet, so it will not come out of the box for me.
But as far as I undersood the whole change is the snippet from @raethlein though it should be tested.
If my workload comes by streamlit again I will give it a try. ⛵

@victoryhb
Copy link
Owner

Seems not much change is needed. I will do it myself. Thanks.

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

No branches or pull requests

6 participants