D11069: [kdecoration] Refine shadows

[Vlad Zagorodniy]

This revision was automatically updated to reflect the committed changes.
Closed by commit R31:54b75015eda8: [kdecoration] Refine shadows (authored by 
zzag).

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11069?vs=35489=37267

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

AFFECTED FILES
  kdecoration/CMakeLists.txt
  kdecoration/breezedecoration.cpp
  kdecoration/breezesettingsdata.kcfg

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11069: [kdecoration] Refine shadows

[Vlad Zagorodniy]

zzag updated this revision to Diff 35489.
zzag added a comment.
This revision is now accepted and ready to land.


  Drop HiDPI support.

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11069?vs=35488=35489

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

AFFECTED FILES
  kdecoration/CMakeLists.txt
  kdecoration/breezedecoration.cpp
  kdecoration/breezesettingsdata.kcfg

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11069: [kdecoration] Refine shadows

[Vlad Zagorodniy]

zzag planned changes to this revision.
zzag added a comment.


  OK, the issue above is still present. Need to fix this somehow.

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11069: [kdecoration] Refine shadows

[Vlad Zagorodniy]

zzag updated this revision to Diff 35488.
zzag added a comment.


  Wrong comparison

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11069?vs=35487=35488

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

AFFECTED FILES
  kdecoration/CMakeLists.txt
  kdecoration/breezedecoration.cpp
  kdecoration/breezesettingsdata.kcfg

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11069: [kdecoration] Refine shadows

[Vlad Zagorodniy]

zzag updated this revision to Diff 35487.
zzag added a comment.


  Do not allow dpr < 1.

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11069?vs=34594=35487

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

AFFECTED FILES
  kdecoration/CMakeLists.txt
  kdecoration/breezedecoration.cpp
  kdecoration/breezesettingsdata.kcfg

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11069: [kdecoration] refine shadows

[Vlad Zagorodniy]

zzag updated this revision to Diff 34594.
zzag added a comment.


  Rebase

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11069?vs=31809=34594

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

AFFECTED FILES
  kdecoration/CMakeLists.txt
  kdecoration/breezedecoration.cpp
  kdecoration/breezesettingsdata.kcfg

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11069: [kdecoration] refine shadows

[Vlad Zagorodniy]

zzag updated this revision to Diff 31809.
zzag added a comment.


  Rebase.

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11069?vs=29540=31809

BRANCH
  refine-shadows-kdecoration

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

AFFECTED FILES
  kdecoration/CMakeLists.txt
  kdecoration/breezedecoration.cpp
  kdecoration/breezesettingsdata.kcfg

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11069: [kdecoration] refine shadows

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  First, I am now running this set of patch routinely and think these new 
shadows are gorgeous on a dayly usage. (I think the default is too large to my 
taste, but using a 70% shadow strenght and medium size, I'm quite happy with 
the result). So no regret with accepting it. 
  Second, concerning code review: 
  I now see that you kept the door opened for horizontal offsets in your 
shadowParams definition and in the code implementation. I think this is quite 
superfluous, and could be dropped. Not sure that the current shadow model works 
for non centered shadows anyway. 
  This would result in changing .offset frop QPoint to int, and modifying the 
rendering code accordingly. IMHO there is no point being too generic here.

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11069: [kdecoration] refine shadows

[Vlad Zagorodniy]

zzag added a comment.


  Hey, everyone!
  
  Even though the patches are accepted I won't land them because KWin patches 
haven't landed yet(I'm waiting for the code to be reviewed).

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11069: [kdecoration] refine shadows

[Hugo Pereira Da Costa]

hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.


  Ship it. Thanks !

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11069: [kdecoration] refine shadows

[Vlad Zagorodniy]

zzag added a comment.


  In D11069#226041 , @ngraham wrote:
  
  > Right, and I can accept that. Visually, I feel like the bottom shadow is a 
bit too stark and eye-catching. There's a very dark strip right under the 
bottom of the window that I find a bit distracting.
  
  
  Well, according to the lighting model that's totally correct. So, I don't 
want to change that.
  
  If bottom shadows and side shadows were equally sized, then they would give 
impression like it's an aura or neon or whatever. That would be totally fine in 
Oxygen but here.. I dunno.

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11069: [kdecoration] refine shadows

[Nathaniel Graham]

ngraham added a comment.


  In D11069#226037 , @zzag wrote:
  
  > Oh, sorry, I misunderstood you. Bottom shadows should be bigger than side 
shadows because shadows are casted from north. :)
  >
  > For example, raise a light source above the title bar and look at the 
window from infinity(I wish I could have such sight).
  
  
  Right, and I can accept that. Visually, I feel like the bottom shadow is a 
bit too stark and eye-catching. There's a very dark strip right under the 
bottom of the window that I find a bit distracting.

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11069: [kdecoration] refine shadows

[Vlad Zagorodniy]

zzag added a comment.


  In D11069#226036 , @abetts wrote:
  
  > What is there left to be done in this implementation?
  
  
  If you're happy about current look of the shadows than nothing. :)
  
  I'm still waiting for KWin patches to be reviewed. (see 'Depends on' in the 
revision's summary)

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11069: [kdecoration] refine shadows

[Andres Betts]

abetts added a comment.


  What is there left to be done in this implementation? Are we close to 
shipping?

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11069: [kdecoration] refine shadows

[Vlad Zagorodniy]

zzag added a comment.


  Oh, sorry, I misunderstood you. Bottom shadows should be bigger than side 
shadows because shadows are casted from north. :)
  
  For example, raise a light source above the title bar and look at the window 
from infinity(I wish I could have such sight).

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11069: [kdecoration] refine shadows

[Vlad Zagorodniy]

zzag added a comment.


  In D11069#226026 , @ngraham wrote:
  
  > I still think the light source is a bit too far up on the Y axis, and the 
bottom shadow is a bit too big compared to the side shadows. But I won't -1 it, 
since these new shadows are just so awesome in the aggregate!
  
  
  I haven't implemented spread property, sorry. I don't have too much time for 
this. :(

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11069: [kdecoration] refine shadows

[Nathaniel Graham]

ngraham added a comment.


  I still think the light source is a bit too far up on the Y axis, and the 
bottom shadow is a bit too big compared to the side shadows. But I won't -1 it, 
since these new shadows are just so awesome in the aggregate!

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11069: [kdecoration] refine shadows

[Vlad Zagorodniy]

zzag added a subscriber: abetts.
zzag added a comment.


  Hugo, could you please review this patch?

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: abetts, fabianr, hpereiradacosta, ngraham, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart