Repository navigation
Fix a bug with the Qt5 backend with mixed resolution displays - #8931
Conversation
e143f62 to
05bdbc4
Compare
When mixed-resolution displays are present, the _dpi_ratio attribute on the canvas may change between paintEvents, which means that we need to change the size of the canvas. However, the underlying canvas is only resized if a resizeEvent is emitted, so we check whether _dpi_ratio has changed between paintEvents, and if so we emit a fake resizeEvent for the widget.
05bdbc4 to
f8cb5c9
Compare
| # We use self.resizeEvent here instead of QApplication.postEvent | ||
| # since the latter doesn't guarantee that the event will be emitted | ||
| # straight away, and this causes visual delays in the changes. | ||
| self.resizeEvent(event) |
There was a problem hiding this comment.
Does this also trigger a paint event?
There was a problem hiding this comment.
It might do - I'm investigating
There was a problem hiding this comment.
Apparently not - at least the paintEvent method does not get called again as a result.
|
@tacaswell @efiring - I think I've got things behaving as they should: with mixed resolution displays, the plots look the same on all displays! There were fundamentally two issues here:
My approach is to keep track in paintEvent of what the last dpi_ratio was when the canvas was drawn. If it has changed, the figure DPI is updated, then the canvas size. However, note that changing the DPI normally causes the canvas to be resized, which we deal with ourselves already, so I've added a new
This is ready for review. |
776c940 to
f3c54a0
Compare
Similarly to resizeEvent, we tell _set_dpi to not change the canvas size since we are doing it ourselves.
b510f19 to
7f4a73a
Compare
… change when _dpi_ratio changes
|
I think the AppVeyor failure is unrelated? (other PRs seem to suffer from it) |
| self.figure._original_dpi = self.figure.dpi | ||
| self.figure.dpi = self._dpi_ratio * self.figure._original_dpi | ||
|
|
||
| def _update_figure_dpi(self): |
There was a problem hiding this comment.
Can you use this in the __init__ above as well (and get rid of the hasattr)?
|
@tacaswell - change implemented, and all tests are passing |
dstansby
left a comment
dstansby
left a comment
There was a problem hiding this comment.
👍 can confirm the original bug, and that this fixes it (using a retina Macbook and non-retina external screen)
QuLogic
left a comment
QuLogic
left a comment
There was a problem hiding this comment.
This test is not passing for me.
Plus a few minor tweaks.
| # mixed resolution displays if dpi_ratio is changing between painting | ||
| # events. | ||
| if (self._dpi_ratio_prev is None or | ||
| self._dpi_ratio != self._dpi_ratio_prev): |
| assert size.width() == 200 | ||
| assert size.height() == 80 | ||
| assert_equal(qt_canvas.get_width_height(), (600, 240)) | ||
| assert_equal(fig.get_size_inches(), (5, 2)) |
There was a problem hiding this comment.
A regular assert would be fine here; you didn't need to import assert_equal.
| qApp.processEvents() | ||
|
|
||
| # The DPI and the renderer width/height change | ||
| assert fig.dpi == 240 |
There was a problem hiding this comment.
This isn't passing for me here; I think you need to do a little more to trigger an update.
There was a problem hiding this comment.
Interesting, this seems to pass on the CI though - or does the test not get run on the CI? What platform and exact Qt version are you using?
There was a problem hiding this comment.
I don't see any obvious skips in the CI, but you can confirm with @anntzer that the tests should be running. The odd thing is I don't see any Qt4 skips and one would normally preclude the other; maybe they just luckily ran on different processes?
I am running on Fedora+conda using the latest defaults Qt package (conda-forge's Qt seems to be broken) 5.6.2-5 with Python 3.6.
There was a problem hiding this comment.
Almost certainly related to #8659 (causing some of the tests not being run).
When mixed-resolution displays are present, the _dpi_ratio attribute on the canvas may change between paintEvents, which means that we need to change the size of the canvas. However, the underlying canvas is only resized if a resizeEvent is emitted, so we check whether _dpi_ratio has changed between paintEvents, and if so we emit a fake resizeEvent for the widget.
Fixes #8061
Note that while this fixes the issue described in #8061 which was that one could end up with weird visual issues such as:
the plot for the lower DPI display still has too large fonts/linewidths, but I think that is a separate bug, which I'll try and fix in another PR.
cc @tacaswell @efiring