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

Current implementation of ChoiceElement.set_options() calls self.update() twice if a value is set, causing bugs in code that relies on data coming from on_changed() to be accurate. #3733

Closed
sSasha-uwu opened this issue Sep 15, 2024 · 2 comments · Fixed by #3736
Labels
bug Something isn't working
Milestone

Comments

@sSasha-uwu
Copy link
Contributor

sSasha-uwu commented Sep 15, 2024

Description

Just ran into this issue while working with the ui.radio() element. When I call set_options() on the radio element with a value set, the on_changed() callback is triggered twice due to the duplicated self.update() calls within the set_options() function. Not only is it strange to call update() twice for no reason, it causes bugs. If the current value of the radio is not within the new set of options, it will call on_changed() once with a None, and then a second time with the correct value. This causes seemingly impossible NoneType errors downstream that now need to be accounted for.

Fortunately, this is incredibly simple to fix.

Original code from nicegui/elements/choice_element.py > ChoiceElement() > set_options():

def set_options(self, options: Union[List, Dict], *, value: Any = ...) -> None:
    """Set the options of this choice element.

    :param options: The new options.
    :param value: The new value. If not given, the current value is kept.
    """
    self.options = options
    self.update()
    if value is not ...:
        self.value = value
        self.update()

Fixed code:

def set_options(self, options: Union[List, Dict], *, value: Any = ...) -> None:
    """Set the options of this choice element.

    :param options: The new options.
    :param value: The new value. If not given, the current value is kept.
    """
    self.options = options
    if value is not ...:
        self.value = value
    self.update()

If someone specifically wanted this functionality for whatever reason, they could call set_options() and set_value() separately, achieving the duplicated call regardless, so nothing is lost by making this change.

@rodja
Copy link
Member

rodja commented Sep 16, 2024

Looks good to me. Would you like to create a pull request @sSasha-uwu? We would be happy to have you as a contributor. Ideally you would also add a test to verify correct behaviour.

@falkoschindler
Copy link
Contributor

Interesting. I guess I added the if-block later and didn't think about self.update() doing more than just updating the frontend. And since consecutive updates get eliminated when enqueued in the outbox, it didn't seem like a problem.
Anyway, the solution looks totally reasonable to me. 👍

@falkoschindler falkoschindler added this to the 2.2 milestone Sep 23, 2024
@falkoschindler falkoschindler added the bug Something isn't working label Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants