fix: Qt5Agg support darkmode icon by using svg #30565
Conversation
…Painter to avoid crash
LangQi99
changed the title
|
Not sure, but would using a QIconEngine remove the need for a manual refresh? As far as I read it, the engine could provide the right-sized image on demand (pull principle), instead of explicitly updating on a dpi-change event (push principle). |
LangQi99
force-pushed
the
qt5icon
branch
2 times, most recently
from
7c1c620 to
fb0c6df
Compare
| def _render_svg(self, svg_path, pixmap, size, dpr): | ||
| """Render SVG content to pixmap.""" | ||
| QSvgRenderer = qt_compat.get_qsvg_renderer() | ||
| if QSvgRenderer is None: | ||
| return QtGui.QPixmap() |
There was a problem hiding this comment.
I find the logic not quite clean. You pass pixmap and sometimes return that pixmap with something rendered into, and sometimes you return a different pixmap. I'd rather have that we either only draw into the pixmap (and then do not need to return anything, or that we change to _create_pixmap_from_* and not pass a pixmap in.
| # Get the current device pixel ratio | ||
| dpr = (self.toolbar.devicePixelRatioF() or 1) if self.toolbar else 1 |
There was a problem hiding this comment.
Idea for further simplification:
move this into a private function _devicePixelRatio().
Then ...
| return self._create_pixmap_from_svg(svg_path, size, dpr) | ||
| else: | ||
| large_path = self.image_path.with_name(self.image_path.stem + '_large.png') | ||
| path = large_path if large_path.exists() else self.image_path | ||
| scaled_size = QtCore.QSize( | ||
| int(size.width() * dpr), | ||
| int(size.height() * dpr) | ||
| ) | ||
| return self._create_pixmap_from_png(path, scaled_size, dpr) |
There was a problem hiding this comment.
Interpret size as logical display size for the scope of this function and hide the calculation of the physical size fully inside the _create_pixmap_from_* functions, using self._devicePixelRatio() in there.
This reduces here to
| return self._create_pixmap_from_svg(svg_path, size, dpr) | |
| else: | |
| large_path = self.image_path.with_name(self.image_path.stem + '_large.png') | |
| path = large_path if large_path.exists() else self.image_path | |
| scaled_size = QtCore.QSize( | |
| int(size.width() * dpr), | |
| int(size.height() * dpr) | |
| ) | |
| return self._create_pixmap_from_png(path, scaled_size, dpr) | |
| return self._create_pixmap_from_svg(svg_path, size) | |
| else: | |
| return self._create_pixmap_from_png(self.image_path, size) |
All the type-dependent implementation details (dpi handling, using a possibly available _large PNG image) are then hidden in the respective function.
There was a problem hiding this comment.
Thanks for your patient feedback and guidance. I've really learned a lot from it.💡
There was a problem hiding this comment.
@LangQi99 thanks as well for the patience to go through the sequence of review cycles! We try to keep them low, but in this case, the proper solution only revealed itself step-by-step. But maybe seeing the evolution of thoughts is also instructive 😃.
PR summary
Addresses issue #27590
I noticed in a screenshot from another PR that out of the six backends displayed, only the Qt5Agg icon fails to adapt to dark mode. It remains black and blends in with the background, making it difficult to distinguish.
Old behavior
New behavior
Old behavior
New behavior
PR checklist