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

Trigger for ui.checkbox and ui.toggle not triggered on unselect #94

Closed
srini-x opened this issue Sep 9, 2020 · 19 comments
Closed

Trigger for ui.checkbox and ui.toggle not triggered on unselect #94

srini-x opened this issue Sep 9, 2020 · 19 comments
Assignees
Labels
bug Bug in code ui Related to UI
Milestone

Comments

@srini-x
Copy link
Contributor

srini-x commented Sep 9, 2020

Q SDK Version, OS
Q 0.3.1
OS: macOS 10.15.6

Actual behavior

When using trigger=True on ui.checkbox and ui.toggle, selecting an option creates trigger with the component name in q.args. However, when an option is unselected, q.args is empty.

Expected behavior

Expected behavior is have the q.args with the name of the component that is unselected and the value to be False.

Here is the log from my app. checking the checkbox created green_zone but unchecking it did not create any args

initialized: True
route: before:dashboard, now:None
args:
green_zone

initialized: True
route: before:dashboard, now:None
args:

@srini-x srini-x added the bug Bug in code label Sep 9, 2020
@lo5 lo5 added the ui Related to UI label Sep 9, 2020
@lo5 lo5 added this to the 2020 Q3 milestone Sep 9, 2020
@mturoci mturoci self-assigned this Sep 10, 2020
@mturoci
Copy link
Collaborator

mturoci commented Sep 10, 2020

Thanks for the issue @srini-x! I checked, but I couldn't even make the checkbox change the value with trigger set to True:
Screen Recording 2020-09-10 at 9 28 56 AM
This is related to #44 as we discussed with @lo5.

Can you confirm this is the behavior you are observing so that I can mark this issue as a duplicate?

@srini-x
Copy link
Contributor Author

srini-x commented Sep 10, 2020

I was able to check and uncheck. Not sure why it is acting differently for you. Let me try to get a gif.

@srini-x
Copy link
Contributor Author

srini-x commented Sep 10, 2020

@mturoci
Screen-Recording-2020-09-10-at-1

ui.checkbox(
    name="green_zone",
    label="Green Zone",
    value=True,
    trigger=True,
),
ui.checkbox(
    name="yellow_zone",
    label="Yellow Zone",
    value=True,
    trigger=True,
),

@mturoci
Copy link
Collaborator

mturoci commented Sep 10, 2020

Interesting, will investigate further.

@mturoci
Copy link
Collaborator

mturoci commented Sep 10, 2020

Tried your code, but still not clickable:
ezgif com-video-to-gif

Will wait till #44 gets resolved and see then.

@lo5
Copy link
Member

lo5 commented Sep 11, 2020

@mturoci This issue seems to be unrelated to #44.

I'm curious why you and @srini-x are observing different behaviors.

In my case, the checkboxes are working as expected, and they do send true/false correctly to the backend.

To reproduce, I ran the todo.py example, and checked/unchecked the third checkbox. This is what I observe in the socket channel (note todo_3: false and todo_3: true):

Screenshot from 2020-09-10 22-07-18

@srini-x, @mturoci - Can you please try the todo.py example and report back what's the behavior you are observing?

@mturoci
Copy link
Collaborator

mturoci commented Sep 11, 2020

@lo5 Got same behavior for todo.py as you.

@mturoci
Copy link
Collaborator

mturoci commented Sep 11, 2020

Also observed that when tweaking checkbox with trigger and value it also works as expected:

ui.checkbox(name='checkbox_unchecked', label='Not checked', value=q.args.checkbox_unchecked, trigger=True)

@mturoci
Copy link
Collaborator

mturoci commented Sep 11, 2020

@lo5 I understand that trigger submits form, but should it rerender the whole card as well?

@lo5
Copy link
Member

lo5 commented Sep 11, 2020

@mturoci does the whole form re-render when a checkbox is changed?

@srini-x
Copy link
Contributor Author

srini-x commented Sep 11, 2020

@lo5 todo.py works as expected. I am reviewing my code for hidden bugs.

@mturoci
Copy link
Collaborator

mturoci commented Sep 11, 2020

@lo5 the page is marked as dirty within sync call. I guess that's why it rerenders the whole page and might be a possible bug.

We have it discussed a bit in this PR already - #11

@lo5
Copy link
Member

lo5 commented Sep 11, 2020

sync() itself doesn't mark the page as dirty. That is probably happening after the round-trip if the app decides to modify the page. To be clear, calling page.add() or setting page['foo']= will mark the page as dirty.

@lo5
Copy link
Member

lo5 commented Sep 11, 2020

The behavior you observed in #11 might also be related to the above comment.

@mturoci
Copy link
Collaborator

mturoci commented Sep 11, 2020

Correct. My point is I am not sure if it should be dirty at that point since no cards were added / deleted.

Sync's round-trip to server wants to override example card in checkbox.py example and replace it with the initial state. That's why the page is marked dirty.

Callstack after clicking on checkbox with trigger:
image

@lo5
Copy link
Member

lo5 commented Sep 11, 2020

For simplicity, checkbox.py modifies the page using page['example']=, so the page is marked as dirty.

To optimize this, it could simply modify the .items of the form_card, like in wizard.py.

@mturoci
Copy link
Collaborator

mturoci commented Sep 11, 2020

@srini-x The problem is here: https://github.com/h2oai/q-dai-performance/blob/5bf98871b6a0cf87ba4fd53f56c544a9596423d4/q_dai_performance/dashboard_ui.py#L355

As @lo5 pointed out, you should not set whole card to a page, but only items. Please let me know if that fixed the issue for you so I can close this one afterwards.

@srini-x
Copy link
Contributor Author

srini-x commented Sep 11, 2020

@lo5
I had a bug. I fixed it. but now I am getting what @mturoci is getting.

checkbox_bug

I am redrawing the entire form_card now. It looks like if I just update the items, it will work as expected.

@mturoci
Copy link
Collaborator

mturoci commented Sep 14, 2020

For anyone stumbling upon this issue in future: Simply update items instead of redrawing the whole card.

@mturoci mturoci closed this as completed Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug in code ui Related to UI
Projects
None yet
Development

No branches or pull requests

3 participants