Use QWindow overload of QIcon::pixmap
Needs RevisionPublic

Authored by davidre on Apr 24 2020, 3:09 PM.

Details

Summary

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.

Test Plan

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
davidre created this revision.Apr 24 2020, 3:09 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 24 2020, 3:09 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidre requested review of this revision.Apr 24 2020, 3:09 PM
davidre retitled this revision from Use QWindow overload of icon.pixmap to Use QWindow overload of QIcon::pixmap.
davidre updated this revision to Diff 81107.Apr 24 2020, 3:15 PM

Correct code style when I'm touching these lines either way

cblack requested changes to this revision.Apr 24 2020, 3:22 PM
cblack added a subscriber: cblack.

Icon handling looks somewhat funky with this patch applied when a window changes DPI.

This revision now requires changes to proceed.Apr 24 2020, 3:22 PM
apol added a comment.Apr 24 2020, 5:07 PM

+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.

apol added a comment.Apr 24 2020, 5:09 PM

Icon handling looks somewhat funky with this patch applied when a window changes DPI.

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.

In D29154#656585, @apol wrote:

+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.

I guess how you tested your patches? Looking at icons on all the dpis?

apol accepted this revision.Apr 24 2020, 6:40 PM

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.

In D29154#656690, @apol wrote:

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

Actually everything in the sidebad is very blurry at 2x

apol added a comment.Apr 25 2020, 2:38 PM

Yeah, I guess that phabricator scaled the pictures to take less space.

My impression was that the patch improved things.

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

ndavis accepted this revision.Jun 17 2020, 5:45 AM

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.