PyQT5 Backend Partial Redraw Fix #14456
Conversation
| rect = event.rect() | ||
| width = rect.width() * self._dpi_ratio | ||
| height = rect.height() * self._dpi_ratio | ||
| left, top = self.mouseEventCoords(rect.topLeft()) |
There was a problem hiding this comment.
Can you leave a comment in the code about why we are calling this?
There was a problem hiding this comment.
Yeah for sure. Good call.
|
Thanks for digging into this! The "simple" scaling for hi-dpi has historically been the source of a surprising a amount of grief. |
|
Do we need this change on 3.1.x? This prob needs someone to check that it works on different operating systems? |
|
This should get interactive testing across several OSs and I think should be backported to both 3.1.x and 2.2.x |
|
@tacaswell happy to do it! @jklymak FWIW I've tested this change on Ubuntu 18.04 and OSX Mojave 10.14.3, though I'm sure more extensive testing is in order. |
|
Is there anything else I can do here. More testing or tweaks to style? |
|
Just checking in on this. Anything I can do? |
|
@pshriwise Pinging to get this back on our radar is exactly the right thing to do! |
| left, top = self.mouseEventCoords(rect.topLeft()) | ||
| # shift the "top" by the height of the image to get the | ||
| # correct corner for our coordinate system | ||
| top -= height |
There was a problem hiding this comment.
I guess the point is that this is really bottomRight(), but with the off-by-1 correction? perhaps move the comment about the off-by-1 there, if I got this correctly... and add a new variable called bottom, otherwise it's quite confusing.
There was a problem hiding this comment.
Good point! I'll add a right variable too to make the Bbox construction more clear.
|
just a small clarification needed but otherwise looks good, approving conditional on it (... assuming my understanding is correct :)) |
|
It looks like CI is failing in the setup step. Is it worth a restart? Or maybe should I rebase this branch? |
|
Doc build failure is clearly spurious (this doesn't touch any docstrings). |
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon! If these instruction are inaccurate, feel free to suggest an improvement. |
…456-on-v3.1.x Backport PR #14456 on branch v3.1.x (PyQT5 Backend Partial Redraw Fix)
|
This code path has diverged significantly from 2.2.x, going to defer backporting this unless there is a champion for it. |
PR Summary
This PR is related to partial redraw buffers for QT5 on screens with
devicePixelRatio() > 1from issue #14160.At first, this appeared to be a problem with the PyQT5/PySide2
QRectangles being passed toFigureCanvasQT's, but a deeper look showed that there was some confusion about when theFigureCanvasQT._dpi_ratioshould be applied. This PR ensures that it is only applied when creating a partial buffer for redraw and that the original rectangle locations from PyQT5/Pyside2 are used to place redrawn region. It still addresses the off-by-one issue by using the bottom coordinate of the rectangle and shifting it by the rectangle's height.