Port QPA away from Wayland
ClosedPublic

Authored by zzag on Jul 29 2019, 3:52 PM.

Details

Summary

So far wayland was used by internal clients to submit raster buffers
and position themselves on the screen. While we didn't have issues with
submitting raster buffers, there were some problems with positioning
task switchers. Mostly, because we had effectively two paths that may
alter geometry.

A better approach to deal with internal clients is to let our QPA use
kwin core api directly. This way we can eliminate unnecessary roundtrips
as well make geometry handling much easier and comprehensible.

The last missing piece is shadows. Both Plasma::Dialog and Breeze widget
style use platform-specific APIs to set and unset shadows. We need to
add shadows API to KWindowSystem. Even though some internal clients lack
drop-shadows at the moment, I don't consider it to be a blocker. We can
add shadows back later on.

CCBUG: 386304

Diff Detail

Repository
R108 KWin
Branch
qpa-port-from-wayland
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 15684
Build 15702: arc lint + arc unit
zzag created this revision.Jul 29 2019, 3:52 PM
Restricted Application added a project: KWin. · View Herald TranscriptJul 29 2019, 3:52 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Jul 29 2019, 3:52 PM
zzag retitled this revision from WIP: [plugins/qpa] Port away from Wayland to WIP: qpa: Port away from Wayland.Jul 30 2019, 1:25 PM
zzag updated this revision to Diff 64712.Aug 27 2019, 3:19 AM
zzag retitled this revision from WIP: qpa: Port away from Wayland to [WIP] [plugins/qpa] Port away from Wayland.
zzag edited the summary of this revision. (Show Details)

User actions popup menu and outline seem to work fine.

Autotests pass although geometry handling is still kind of broken.

zzag updated this revision to Diff 64936.Aug 29 2019, 1:06 PM
zzag retitled this revision from [WIP] [plugins/qpa] Port away from Wayland to Port QPA away from Wayland.
zzag edited the summary of this revision. (Show Details)

Update.

zzag added a reviewer: KWin.Aug 29 2019, 1:20 PM

Sorry for the huge patch.

zzag updated this revision to Diff 64988.EditedAug 30 2019, 12:48 PM

Delete unused QPainter include.

I had been experimenting with it for a bit and apparently forgot to cleanup after myself.

+1

Have Roman ack that losing the shadows for now is ok.

zzag updated this revision to Diff 66650.Sep 23 2019, 9:06 AM

Delete unrelated change in input.cpp

I forgot to cleanup some leftover in InputDeviceHandler::updateInternalWindow

romangg added inline comments.
autotests/integration/internal_window.cpp
216

Feels a bit odd to do it in the cleanup. But not critical.

effects.cpp
1099

On X is this not needed anymore? I.e. on X all internal clients now also are of type InternalClient and not anymore Unmanaged? If so, this needs to be mentioned in the commit message.

plugins/scenes/qpainter/scene_qpainter.cpp
406

Why can't it be an X client?

pointer_input.cpp
559

Use full sentences.

workspace.h
656

Is this really necessary?

zzag updated this revision to Diff 66651.Sep 23 2019, 9:43 AM

Reword a comment.

zzag added inline comments.Sep 23 2019, 9:45 AM
effects.cpp
1099

We don't need to call findUnmanaged as findInternal covers internal clients in X11 operation mode.

plugins/scenes/qpainter/scene_qpainter.cpp
406

Are you talking about the case when a Client doesn't have a surface assigned to it yet?

workspace.h
656

I don't see any good reason to keep addInternalClient and removeInternalClient public.

zzag added inline comments.Sep 23 2019, 9:55 AM
workspace.h
656

If it's really that important to keep Workspace class friend-free, I can make those two methods public. However, it'll pollute public interface of Workspace class.

zzag added inline comments.Sep 23 2019, 10:02 AM
workspace.h
656

For example, X11EventFilter does something similar to what InternalClient does. It registers itself in a constructor and unregisters itself in the destructor.

void registerEventFilter(X11EventFilter *filter);

implies that one create X11EventFilter object on the heap, initialize it, and after that call registerEventFilter method. However, that's absolutely not true. Such implementation details have to be hidden.

friend keyword is not a good thing, I don't deny it. However, in some cases it's a necessary measure to keep public stuff clean.

zzag updated this revision to Diff 66652.Sep 23 2019, 10:12 AM

No friend class (I don't want to discuss coding practices anymore)

zzag added inline comments.Sep 23 2019, 10:20 AM
plugins/scenes/qpainter/scene_qpainter.cpp
406

If you are indeed talking about that case, then I'd say we don't need to worry about it because the pixmap won't be valid.

romangg added inline comments.Sep 23 2019, 12:26 PM
effects.cpp
1099

But it didn't before that, right? Before that internal clients in X mode were categorized as Unmanaged clients.

plugins/scenes/qpainter/scene_qpainter.cpp
406

Nah, I was just overlooking that it's the QPainter backend, which is only available on Wayland.

zzag added inline comments.Sep 23 2019, 12:32 PM
effects.cpp
1099

But it didn't before that, right?

That's right, each internal client was/is represented by an instance of InternalClient in Xwayland/Wayland mode,.

Before that internal clients in X mode were categorized as Unmanaged clients.

They are still Unmanaged in X11 mode.

romangg accepted this revision.Sep 23 2019, 1:13 PM
This revision is now accepted and ready to land.Sep 23 2019, 1:13 PM
This revision was automatically updated to reflect the committed changes.