Previously only the combobox used this overload but that dropped in
0eaf762705d84fab5c70a8934ffa7cfeadeeebde because of the changes in
582f5ebad1686d47168a3246e2aff5beefb59121. However we should prefer that
overload because it takes the screen into account on which the icon is rendered.
This reinstates the code removed in 0eaf762 as a helper function to retrieve the
window from a window or styleObject.
Details
Icons look correct
(I don't have a high dpi screen myself but this should be straightforward)
Diff Detail
- Repository
- R31 Breeze
- Branch
- window
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 25832 Build 25850: arc lint + arc unit
Icon handling looks somewhat funky with this patch applied when a window changes DPI.
+1
I did the same thing for kirigami @ D29100 and plasma-framework @ D29102 yesterday after looking at Qt's code and assessing it was the right thing to do.
I am not familiar with Breeze's code but the patch is sound.
I do have all of the dpis so if you tell me how I'll test it.
This should probably fix the BUG 418869.
Right, should re-render the pixmaps when the window changes (we do so in QtQuick).
That said, I don't think that this is reason enough to keep this patch. I guess the alternative would be to render at the maximum dpi of all the outputs we have which is odd.
This is what it looks like for me with the patch applied.
scale 1x:
scale 2x:
master:
scale 1x:
scale 2x:
I can see a noticeable wonky-ness on 1x master, so I'd say this patch helps.
Is master scale 1x the correct image? Also notice the network icon looks bad at 2x in both images
Yeah, I guess that phabricator scaled the pictures to take less space.
My impression was that the patch improved things.
I did some digging and think that things should be repainted when they change screen
https://code.woboq.org/qt5/qtbase/src/widgets/kernel/qwidgetwindow.cpp.html#_ZN13QWidgetWindow18handleScreenChangeEv
In my testing dragging a window to a screen with a different scalefactor has no issue and @apol said for him it's better than the status quo. So I would like to land this
Giving this a +1 just for consistency with the kirigami and plasma-framework changes. I can't really test this since I don't have any screens with different DPIs though.