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

Register plotly event handlers only once, at mount #2505

Merged

Conversation

bmaranville
Copy link
Contributor

All the event handlers in plotly were being registered on every update, which was causing a lot of issues. Event handlers should only be registered once. This PR moves the registration of the event handlers to a one-time operation in the mounted() function.

Fixes #2504, I think

register event listeners only once in plotly, rather than on every update
@platinops
Copy link

platinops commented Feb 3, 2024

I think this also fixes #2435, thanks for the PR!

@bmaranville
Copy link
Contributor Author

This may not be quite right... it looks like when calling Plotly.newPlot() the listeners are dropped. A simply fix might be to move the set_handlers() function call from mounted() to right after Plotly.newPlot(), inside the update method. I'm trying that out right now.

@falkoschindler falkoschindler linked an issue Feb 4, 2024 that may be closed by this pull request
Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

Thanks for this pull request, @bmaranville!

I looks like it fixes a really st*pid mistake. 😄 Of course, we shouldn't re-assign event handlers on every update!

But why do you think there's something wrong? Do you have an example? It tested with the following code and it seems to work nicely:

fig = go.Figure()
plot = ui.plotly(fig).classes('w-full h-96').on('plotly_click', ui.notify)

def add_trace():
    fig.add_trace(go.Scatter(x=[1, 2, 3], y=[random(), random(), random()]))
    plot.update()

ui.button('Add trace', on_click=add_trace)

@falkoschindler falkoschindler added the question Further information is requested label Feb 4, 2024
@falkoschindler falkoschindler added this to the 1.4.14 milestone Feb 4, 2024
@falkoschindler falkoschindler added the bug Something isn't working label Feb 4, 2024
@bmaranville
Copy link
Contributor Author

bmaranville commented Feb 4, 2024

What I forgot is that changing the config causes newPlot to be triggered on update(), after which the events are no longer firing. I actually ran in to this during testing of my app and was confused again.
So moving this.set_handlers(); right after Plotly.newPlot(...) seems to do the right thing - it registers the event listeners once per newPlot (lifetime of the plot) instead of once per lifetime of the component.

EDIT: I also found that the event listeners were unregistered by toggling the visibility of the ui.plotly element (using bind_visibility_from) - I don't know enough about how the visibility attributes work to know why that is happening.

@falkoschindler
Copy link
Contributor

Well, I still can't reproduce newPlot dropping event registrations (see my above-mentioned test code).

But I can confirm that toggling the visibility stops events from firing. 😕

@bmaranville
Copy link
Contributor Author

bmaranville commented Feb 5, 2024

I don't think your code above triggers another newPlot after the first one - I think the only way to do that is to use the dict form of figure and then alter fig['config'] between plot updates. (the guard logic around newPlot only passes if the config changes, not from adding traces or changing layout.) Here is code that reproduces the problem:

from nicegui import ui
from numpy.random import random
import plotly.graph_objects as go

fig = { "data": [], "config": {} }
plot = ui.plotly(fig).classes('w-full h-96').on('plotly_click', ui.notify)

def add_trace():
    fig['data'].append(go.Scatter(x=[1, 2, 3], y=[random(), random(), random()]).to_plotly_json())
    plot.update()

def add_drawrect():
    fig['config']['modeBarButtonsToAdd'] = ['drawrect']
    plot.update()

ui.button('Add trace', on_click=add_trace)
ui.button('Enable drawrect', on_click=add_drawrect)

ui.run()

@falkoschindler
Copy link
Contributor

Oh right, thanks! That makes sense.
I think we should merge this fix, even though there's still a problem with toggling the visibility. That's probably a separate issue and hasn't been introduced with this PR.

@falkoschindler falkoschindler removed the question Further information is requested label Feb 5, 2024
@falkoschindler falkoschindler merged commit 02376e9 into zauberzeug:main Feb 5, 2024
1 check passed
@falkoschindler
Copy link
Contributor

I created issue #2519 to investigate the problem with toggled visibility.

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
Development

Successfully merging this pull request may close these issues.

multiplying events Warn appear in multiple ui.plotly
3 participants