make shadows work for windows 100%width or height
AbandonedPublic

Authored by mart on Jun 7 2017, 5:22 PM.

Details

Reviewers
graesslin
Group Reviewers
Plasma
KWin
Summary

when a window has 100% width or height (common in the panel),
it disables the shadows on the sides (or top/bottom) and
since they are used to compute width/height, if all 4 corner
shadows are disabled, it will think the resulting shadow pixmap
is null having width or height == 0
in this case, use the width of left and right pixmaps for the width
and height of top and bottom for the height, as they will be the only elements present

BUG: 380825

Test Plan

100% width panels now have a shadow in wayland, kickoff also in
the windowd kwin when it doesn't fit in vertical resolution

Diff Detail

Repository
R108 KWin
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
mart created this revision.Jun 7 2017, 5:22 PM
Restricted Application added a project: KWin. · View Herald TranscriptJun 7 2017, 5:22 PM
Restricted Application added subscribers: kwin, plasma-devel. · View Herald Transcript

Investigation sounds sensible, the fix is off.

Old code is:
max(topLeft,topRight)
+
max (left, right)
+
max(bottomLeft, bottomRight)


What you're saying we want is:

max(topLeft,topRight,top)
+
max (left, right)
+
max(bottomLeft, bottomRight, bottom)


you're written
max(top, bottom, topRight, topLeft)
+
max (left, right)
+
max(bottomLeft, bottomRight, bottom)

mart added a comment.EditedJun 7 2017, 5:50 PM

you're written
max(top, bottom, topRight, topLeft)
+
max (left, right)
+
max(bottomLeft, bottomRight, bottom)

no, the intention is
max(left+right, (max(topLeft,bottomLeft) + max(top,bottom) + max(topright,bottomright))

that is, if topleft+top+topright ==0 and bottomleft+bottom+bottomright == 0,
then use as pixmap width the width of left+right

(i may even make it more explicit by not doing a max, but just if(width==0) width = left+right

mart added a comment.Jun 7 2017, 7:10 PM

yes, that's him, thanks

mart edited the summary of this revision. (Show Details)Jun 7 2017, 7:13 PM
mart updated this revision to Diff 15310.Jun 9 2017, 3:30 PM
  • more explicit checking on width/height==0
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJun 9 2017, 3:30 PM

You're fixing the one bug at hand as though it's a special case, rather than fixing the original code.
This leaves it still broken in the case that you have 3 images for the top and only one for the bottom (for whatever reason)

mart added a comment.Jun 9 2017, 3:45 PM

You're fixing the one bug at hand as though it's a special case, rather than fixing the original code.
This leaves it still broken in the case that you have 3 images for the top and only one for the bottom (for whatever reason)

i think it should work correctly? the width would be calculated as the 3 ones on the top as they would "win" and all should work..

I think what David has in mind is a shadow constellation like

o----o
|    |
 ----

with the lower edges deliberately being omitted - this will cause glitchy rendering (because the bottom edge will be stretched over the edges) - thus he's looking for an entirely casual quad handling (instead of the present one that expects a regular shape with at best some sides being entirely omitted)

Marco "just" wants to make cases legal, where a window has only edges on one or two opposing sides - there should maybe an additional sanity check to ensure this: only width OR height should be zero at some point and the other value should match the sum of the other axis of the invoked left/right resp. top/bottom edges.

scene_opengl.cpp
2262

This is uncaught here (and in several other occasions but is caught in the other function) and apparently (at least has been) a somewhat legal condition ...

mart added a comment.Jun 12 2017, 9:37 AM

I like the approach david has taken in D6164

mart updated this revision to Diff 15468.Jun 15 2017, 4:27 PM
  • more explicit checking on width/height==0
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJun 15 2017, 4:27 PM
graesslin added inline comments.Jun 15 2017, 4:56 PM
geometry.cpp
2010 ↗(On Diff #15468)

why is the signal moved?

mart added inline comments.Jun 15 2017, 5:25 PM
geometry.cpp
2010 ↗(On Diff #15468)

hmm, this actually got mixed with D6229, but was not supposed to end up in this one

mart added a comment.Jun 15 2017, 5:32 PM

note: this is mutually exclusive with D6164

mart abandoned this revision.Jun 21 2017, 12:01 PM