D29154: Use QWindow overload of QIcon::pixmap

[Noah Davis]

ndavis accepted this revision.
ndavis added a comment.


  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.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D29154

To: davidre, apol, broulik, ndavis, #breeze, davidedmundson, cblack
Cc: cblack, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29154: Use QWindow overload of QIcon::pixmap

[David Redondo]

davidre added a comment.


  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

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D29154

To: davidre, apol, broulik, ndavis, #breeze, davidedmundson, cblack
Cc: cblack, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29154: Use QWindow overload of QIcon::pixmap

[David Redondo]

davidre added a comment.


  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

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D29154

To: davidre, apol, broulik, ndavis, #breeze, davidedmundson, cblack
Cc: cblack, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29154: Use QWindow overload of QIcon::pixmap

[Aleix Pol Gonzalez]

apol added a comment.


  Yeah, I guess that phabricator scaled the pictures to take less space.
  
  My impression was that the patch improved things.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D29154

To: davidre, apol, broulik, ndavis, #breeze, davidedmundson, cblack
Cc: cblack, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29154: Use QWindow overload of QIcon::pixmap

[David Redondo]

davidre added a comment.


  Actually everything in the sidebad is very blurry at 2x

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D29154

To: davidre, apol, broulik, ndavis, #breeze, davidedmundson, cblack
Cc: cblack, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29154: Use QWindow overload of QIcon::pixmap

[David Redondo]

davidre added a comment.


  In D29154#656690 , @apol wrote:
  
  > This is what it looks like for me with the patch applied.
  >
  > scale 1x: F8258193: Screenshot_20200424_203628.png 

  >  scale 2x: F8258201: Screenshot_20200424_203729.png 

  >
  > master:
  >  scale 1x: F8258205: Screenshot_20200424_203923.png 

  >  scale 2x:F8258207: Screenshot_20200424_203959.png 

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

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D29154

To: davidre, apol, broulik, ndavis, #breeze, davidedmundson, cblack
Cc: cblack, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29154: Use QWindow overload of QIcon::pixmap

[Aleix Pol Gonzalez]

apol accepted this revision.
apol added a comment.


  This is what it looks like for me with the patch applied.
  
  scale 1x: F8258193: Screenshot_20200424_203628.png 

  scale 2x: F8258201: Screenshot_20200424_203729.png 

  
  master:
  scale 1x: F8258205: Screenshot_20200424_203923.png 

  scale 2x:F8258207: Screenshot_20200424_203959.png 

  
  I can see a noticeable wonky-ness on 1x master, so I'd say this patch helps.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D29154

To: davidre, apol, broulik, ndavis, #breeze, davidedmundson, cblack
Cc: cblack, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29154: Use QWindow overload of QIcon::pixmap

[David Redondo]

davidre added a comment.


  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?

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D29154

To: davidre, apol, broulik, ndavis, #breeze, davidedmundson, cblack
Cc: cblack, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29154: Use QWindow overload of QIcon::pixmap

[Aleix Pol Gonzalez]

apol added a comment.


  In D29154#656516 , @cblack wrote:
  
  > Icon handling looks somewhat funky with this patch applied when a window 
changes DPI.
  >  F8257898: ksnip_20200424-111720.png 
  
  
  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.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D29154

To: davidre, apol, broulik, ndavis, #breeze, davidedmundson, cblack
Cc: cblack, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29154: Use QWindow overload of QIcon::pixmap

[Aleix Pol Gonzalez]

apol added a comment.


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

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D29154

To: davidre, apol, broulik, ndavis, #breeze, davidedmundson, cblack
Cc: cblack, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29154: Use QWindow overload of QIcon::pixmap

[Carson Black]

cblack requested changes to this revision.
cblack added a comment.
This revision now requires changes to proceed.


  Icon handling looks somewhat funky with this patch applied when a window 
changes DPI.
  F8257898: ksnip_20200424-111720.png 

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D29154

To: davidre, apol, broulik, ndavis, #breeze, davidedmundson, cblack
Cc: cblack, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29154: Use QWindow overload of QIcon::pixmap

[David Redondo]

davidre updated this revision to Diff 81107.
davidre added a comment.


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

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29154?vs=81106&id=81107

BRANCH
  window

REVISION DETAIL
  https://phabricator.kde.org/D29154

AFFECTED FILES
  kstyle/breezehelper.cpp
  kstyle/breezehelper.h
  kstyle/breezestyle.cpp

To: davidre, apol, broulik, ndavis, #breeze, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart