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

Fix user_key and wid conflation bugs, rename arguments for clarity #3827

Merged
merged 1 commit into from Sep 22, 2021

Conversation

AnOctopus
Copy link
Contributor

A chunk of the session state api had ambiguous argument names, from back when we were trying to make user keys and widget ids interchangeable rather than being clear and explicit. Renaming them revealed an issue with the checks in __setitem__, which is now fixed, and the prompting issue of not checking the right key for setting the frontend value is also fixed by adding a user_key argument.

I don't know how these two bugs were in there, because I thought we had tests for these cases, but maybe we do and those tests are just too narrow.

@AnOctopus
Copy link
Contributor Author

Something I considered but didn't immediately decide to do is renaming the generic key argument to something that more clearly indicates that it could be one or the other, like lookup_key. Incidentally this would be a good candidate for an ADT, but that would be a larger refactor and keeping to a clear naming convention gives much of the benefit.

It is also worth considering totally excising the _get_widget_id method and replacing it with more explicit checks in the few places it is used, which might fit as part of this change.

Copy link
Collaborator

@vdonato vdonato left a comment

Choose a reason for hiding this comment

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

I haven't figured out exactly what's going on here yet, but this currently (surprisingly to me) introduces a regression to behavior similar to that described in #3534. The difference now is that it's every other click that gets reset unexpectedly

@AnOctopus
Copy link
Contributor Author

I think the script in #3534 is inherently going to have that kind of issue with the transient widget model, because it sets one of the widget function's arguments to a changing value, which has the effect of making the code setting the "default" also reset the widget frequently, which will cause changes from the web app to be dropped (because they're for a no longer used widget).

import streamlit as st

st.title("Experimental: Play with Query Strings!")

app_state = st.experimental_get_query_params()
app_state = {
    k: v[0] if isinstance(v, list) else v for k, v in app_state.items()
}  # fetch the first item in each query string as we don't have multiple values for each query string key in this example

# [2] Radio
radio_list = ["Eat", "Sleep", "Both"]
default_radio = int(app_state["radio"]) if "radio" in app_state else 0
if "what_to_do" not in st.session_state:
    st.session_state.what_to_do = radio_list[default_radio]

radio = st.radio(
    "What are you doing at home during quarantine?",
    radio_list,
    key="what_to_do",
)

app_state["radio"] = radio_list.index(radio)
st.experimental_set_query_params(**app_state)

print("")

This is a modified version of the code using session state to only set the value if it doesn't exist.

I don't know why it would have been working before this PR, because it seems to me that it shouldn't have been.

@AnOctopus
Copy link
Contributor Author

Actually I just tried the original script from 3534 and it has the same behavior on both develop and this branch

Copy link
Collaborator

@vdonato vdonato left a comment

Choose a reason for hiding this comment

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

It turns out the revival of #3534 currently exists on develop, so that isn't a blocker to get this PR in, and we can figure out what to do about #3534 regressing in a followup change

(edit: the behavior being revived here actually existed pre-session_state, so its revival is essentially a consequence of us choosing to revert widget identity semantics to what they were in the pre-state world)

@AnOctopus AnOctopus merged commit f1da0f4 into streamlit:develop Sep 22, 2021
@AnOctopus AnOctopus deleted the widget-setting branch September 22, 2021 01:02
kmcgrady pushed a commit that referenced this pull request Sep 22, 2021
tconkling added a commit to tconkling/streamlit that referenced this pull request Sep 22, 2021
* develop:
  mypy: ignore git package entirely
  Update submodule reference for component-template (streamlit#3831)
  Update component-lib version
  Update change log
  Update change log
  Update changelog with docs link
  Add advanced widget docs page (streamlit#3820)
  Updates Components API with theme.base support (streamlit#3826)
  Adds memo and singleton docstrings to API reference (streamlit#3825)
  Add advanced widget docs page (streamlit#3820)
  Fix user_key and wid conflation bugs (streamlit#3827)
  Fix user_key and wid conflation bugs (streamlit#3827)
  (Hopefully) Fix CI, and also suppress some warnings (streamlit#3828)
  Bugfix/3580 (streamlit#3808)
  Up version to 0.89.0
  Update change log
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.

None yet

2 participants