make shadows work on wayland
ClosedPublic

Authored by mart on May 18 2017, 11:53 AM.

Details

Summary

since from Qt 5.8 QtWayland destroys its surfaces every time
a window gets hidden and recreates them again when is shown
(that's how the protocol is defined) install the shadows
every time the window is shown, using a map to keep track of surfaces, in order to delete them on window hide and avoid leaks

Test Plan

popup menus have correct shadows on wayland now

Diff Detail

Repository
R31 Breeze
Branch
phab/waylandshadow
Lint
No Linters Available
Unit
No Unit Test Coverage
mart created this revision.May 18 2017, 11:53 AM
Restricted Application added a project: Plasma. · View Herald TranscriptMay 18 2017, 11:53 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

We're going to be leaking both Surface and Shadow objects in installWaylandFilter, no?

kstyle/breezeshadowhelper.cpp
177

we use Expose everywhere else, why not here? It gets called a lot less

185

why not just remove the widget from the list?

469

this is currently leaking?

fromWindow constructs a new QObject

472

this is going to be effectively leaking. on a show/hide/show we'll get now have two of these in memory, just one not attached to anything.

487–488

off topic, but why do we have a commit on the surface? We haven't changed the surface, just attaced something to it.

anthonyfieroni added inline comments.
kstyle/breezeshadowhelper.cpp
174

We make check on every event for what we know what it is, does it better to use polymorphic helper?

kstyle/breezeshadowhelper.cpp
177

Also, instead of using first "contains" then .value, one should really use an iterator (to avoid parsing the map twice):

auto&& iter = _widgets.find( widget );
if( iter == _widgets.end() || iter.value() == 0 )

Something like that.

mart added inline comments.May 19 2017, 11:01 AM
kstyle/breezeshadowhelper.cpp
177

it's a qwidget, doesn't get expose events

469

it should index those by widget as well i guess, then using this to keep track as opposed to repurposing the _widgets map

mart updated this revision to Diff 14839.May 25 2017, 1:03 PM
  • use a different map to keep track of surfaces
mart edited the summary of this revision. (Show Details)May 25 2017, 1:04 PM
mart updated this revision to Diff 15156.Jun 5 2017, 9:19 AM
  • use a const_iterator
This revision was automatically updated to reflect the committed changes.

This breaks builds on Neon and Kubuntu CI (Qt 5.7.1)

http://build.neon.kde.org/job/xenial_unstable_plasma_breeze_bin_amd64/120/console

05:10:59 In file included from /workspace/build/build-qt4/kstyle/moc_breezeshadowhelper.cpp:9:0,
05:10:59 from /workspace/build/build-qt4/kstyle/breeze_automoc.cpp:33:
05:10:59 /workspace/build/build-qt4/kstyle/../../kstyle/breezeshadowhelper.h:162:24: error: ‘KWayland’ was not declared in this scope
05:10:59 QMap<QWidget*, KWayland::Client::Surface *> _widgetSurfaces;
05:10:59 ^
05:10:59 /workspace/build/build-qt4/kstyle/../../kstyle/breezeshadowhelper.h:162:51: error: template argument 2 is invalid
05:10:59 QMap<QWidget*, KWayland::Client::Surface *> _widgetSurfaces;

This breaks builds on Neon and Kubuntu CI (Qt 5.7.1)

http://build.neon.kde.org/job/xenial_unstable_plasma_breeze_bin_amd64/120/console

05:10:59 In file included from /workspace/build/build-qt4/kstyle/moc_breezeshadowhelper.cpp:9:0,
05:10:59 from /workspace/build/build-qt4/kstyle/breeze_automoc.cpp:33:
05:10:59 /workspace/build/build-qt4/kstyle/../../kstyle/breezeshadowhelper.h:162:24: error: ‘KWayland’ was not declared in this scope
05:10:59 QMap<QWidget*, KWayland::Client::Surface *> _widgetSurfaces;
05:10:59 ^
05:10:59 /workspace/build/build-qt4/kstyle/../../kstyle/breezeshadowhelper.h:162:51: error: template argument 2 is invalid
05:10:59 QMap<QWidget*, KWayland::Client::Surface *> _widgetSurfaces;

Working on fixing it.
Just a couple of ifdef missing.

! In D5910#114781, @hpereiradacosta wrote:
Working on fixing it.
Just a couple of ifdef missing.

The fix you just pushed seems to have done the trick. Thank you.

mart added a comment.Jun 7 2017, 8:39 AM

thanks for fixing, sorry about that.