-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Update background in SpanSelector._press whenever useblit=True. #9660
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
Conversation
Previously, the canvas would only get redrawn when useblit=True only if span_stays is set. This causes e.g. import sys from PyQt5.QtWidgets import * from matplotlib.backends.backend_qt5agg import * from matplotlib.figure import Figure from matplotlib.widgets import SpanSelector class App(QMainWindow): def __init__(self): super().__init__() self.setCentralWidget(QWidget()) layout = QVBoxLayout() self.centralWidget().setLayout(layout) self._fig = Figure() self._fig.subplots() layout.addWidget(FigureCanvas(self._fig)) layout.addWidget(QPushButton("span", clicked=self._span_cb)) def _span_cb(self): self._span = SpanSelector( self._fig.axes[0], print, "horizontal", useblit=True) qapp = QApplication(sys.argv) app = App() app.show() qapp.exec_() to use an incorrect background(?) when clicking and starting a span for the first time. To be honest I'm not sure this is exactly the correct solution, although it does fix the issue...
That is because self.background is None before any draw is being made. The red patch you see is actually the dragging line because background (None) is not restored. The "correct" way perhaps to fix this is to add a draw() and copy the background in onmove event. This is because the benefit of blitting is used here. Adding a moving flag to just update the background before blitting. That is what I do in my implementation. However, with the current implementation I would suggest to add
|
I think @lkjell 's suggestion is more correct. The background is updated automatically in a callback on Might be worth adding a check if the figure is stale and forcing a redraw and if not just grabbing the background. |
I believe that as suggested by @tacaswell that checking for staleness is probably the correct thing to do. |
Actually you cannot check for staleness because the figure will mostly be staled anyway. The current implementation is very clever and too complex if you do not understand what should be done thus making it very prone for bugs. The problem occur because of blitting. And blitting is only useful during move event. For blitting to function properly it needs a clean background. Thus to make it sane and guard against retarded code that change the background one should before a move event force a redraw with span invisible, copy the background and turn span visible on. The question is where to put this redraw on. Should it be in press event or move event. If you put it in press then it is a bit redundant because you do not need it when there is no movement. Putting it in move event will let you have a move flag to fix some inconsistency with span_stay is visible but the next time you only click and do not move. The old span will still be visible but select callback gives different number. I only suggest to update the background in update because of the fancy implementation but it does not guard against people make strange code like change the background or deactivate spanselector do something with the figure without redrawing and activate the spanselector. |
Why is it most likely stale? Changes to animated artists should not be marking the figure as stale and for people in IPython should end up with a non-stale figure everytime they get the prompt back. We should be biased towards putting logic in |
If I add a Anyway, I do believe that my initial suggestion on just updating the background in |
Perhaps a new bug. Tested in ipython and python. Jupyter does not seem to have interactive.
|
With |
TBH I sort of lost track of the discussion above. I can confirm that this PR still works and still fixes the originally mentionned issue (which is still present), but if you have a better solution, go for it :) |
Previously, the canvas would only get redrawn when useblit=True only if
span_stays is set. This causes e.g.
to use an incorrect background(?) when clicking and starting a span for
the first time.
To be honest I'm not sure this is exactly the correct solution, although
it does fix the issue...
PR Summary
PR Checklist