workaround for correct wayland positioning
ClosedPublic

Authored by mart on May 7 2017, 7:18 PM.

Details

Summary

as showEvent is too soon, create the plasma surface on
:Expose event, and reposition the surface

Test Plan

krunner is correctly positioned with correct blur and shadows

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mart created this revision.May 7 2017, 7:18 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMay 7 2017, 7:18 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

I assume this are adjustments for Qt 5.8 or later. How does this work with Qt 5.7? I fear if we change like this we are going to break Qt 5.7.

krunner/view.cpp
217

what?

I assume this are adjustments for Qt 5.8 or later. How does this work with Qt 5.7? I fear if we change like this we are going to break Qt 5.7.

Yes, in combination with the other two. There's some notes on T6064.
It's a valid fear, I also have a Qt 5.7 machine to test against and we won't be merging anything whilst it has a negative side effect.

davidedmundson added inline comments.May 12 2017, 2:59 PM
krunner/view.cpp
236

return retval.

otherwise you're processsing it twice.

247

When we get here we've already run Dialog::event()

won't that have already done the positioning?

There's a virtual geometry method in dialog to allow krunner and such to be different

mart updated this revision to Diff 14556.May 15 2017, 4:00 PM
  • don't execute event two times
Restricted Application edited projects, added Plasma on Wayland; removed Plasma. · View Herald TranscriptMay 15 2017, 4:00 PM
mart marked 2 inline comments as done.May 15 2017, 4:04 PM
mart added inline comments.
krunner/view.cpp
247

no, it doesn-t seem.
at that point it executed expose of dialog::event,
this calls dialog::updateVisibility which moves the dialog only when it has a visualparent.
i don't think the virtual popupposition may be repurposed for this case.
may be worth a deeper look on what's the exact event sequence tough

Is it tested on X, Qt 5.7 as well as Qt 5.9

If you want me to do any of those, just ask.

krunner/view.cpp
250

we need m_plasmaShellSurface = nullptr;

this leaves a dangling pointer, which is both dangerous but also means we won't re-create it in ExposeEvent, I would have assumed that would mean the second show won't work.

edit, ignore that last comment. I hadn't seen it was a QPointer.

Just my comment about checking on all other plaforms remains.

mart added a comment.May 22 2017, 12:31 PM

edit, ignore that last comment. I hadn't seen it was a QPointer.

Just my comment about checking on all other plaforms remains.

i could check on wayland Qt 5.9, X Qt 5.8, both seemed fine

hein added a subscriber: hein.May 24 2017, 4:09 PM

Did we do the 5.7 test?

mart added a comment.May 24 2017, 4:16 PM
In D5748#111625, @hein wrote:

Did we do the 5.7 test?

ah, i understood David did, sorry

davidedmundson accepted this revision.May 24 2017, 5:28 PM
This revision is now accepted and ready to land.May 24 2017, 5:28 PM
This revision was automatically updated to reflect the committed changes.