[xwl] Drag and drop between Xwayland and Wayland native clients
ClosedPublic

Authored by romangg on Sep 20 2018, 9:28 PM.

Details

Summary

Building upon the generic X Selection support this patch establishes another
selection class representing the XDND selection and provides interfaces
to communicate drags originating from Xwayland windows to the Wayland
server KWin and drags originating from Wayland native drags to Xwayland.

For Wayland native drags KWin will claim the XDND selection as owner and
will simply translate all relevant events to the XDND protocol and receive
alike messages by X clients.

When an X client claims the XDND selection KWin is notified via the X protocol
and it decides if it allows the X drag to transcend into the Wayland protocol.
If this is the case the mouse position is tracked and on entering a Wayland
native window a proxy X Window is mapped to the top of the window stack. This
proxy window acts as a drag destination for the drag origin window and again
X messages will be translated into respective Wayland protocol calls. If the
cursor leaves the Wayland window geometry before a drop is registered, the
proxy window is unmapped, what triggers a subsequent drag leave event.

In both directions the necessary core integration is minimal. There is a single
call to be done in the drag and drop event filter through the Xwayland
interface class.

From my tests this patch facilitates drags between any Qt/KDE apps. What needs
extra care are the browsers, which use target formats, that are not directly
compatible with the Wayland protocol's MIME representation. For Chromium an
additional integration step must be done in order to provide it with a net
window stack containing the proxy window.

Test Plan

Manually. Auto tests planned.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
romangg requested review of this revision.Sep 20 2018, 9:28 PM
romangg created this revision.
alexde added a subscriber: alexde.Oct 26 2018, 9:55 AM
romangg updated this revision to Diff 48528.Jan 2 2019, 12:29 PM

Rebase on master/changes.

Restricted Application added a project: KWin. · View Herald TranscriptJan 2 2019, 12:29 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag added a subscriber: zzag.Jan 2 2019, 2:21 PM

Some coding style nitpicks. Please ignore them for now.

input.cpp
1475

Unrelated whitespace change.

2102

The name is confusing. Currently, it implies that this method looks for a managed client, which seams not the case.

wayland_server.h
270

* compositor

xwl/clipboard.cpp
138

nullptr

141

Isn't qobject_cast preferred over bool inherits(const char *className) const?

xwl/databridge.h
77

Please follow coding style.

xwl/dnd.cpp
44–45

Please follow coding style.

136

nullptr

xwl/dnd.h
26–27

Please follow coding style.

79

According to the coding style, we should put a whitespace before "*", though both variants are okay.

xwl/drag.cpp
25–26

Please follow coding style.

31

KWin uses C++14, not C++20.

44

Please don't use C-style casts.

57

Delete it?

71

Delete it?

xwl/drag.h
39–42

/**

xwl/drag_wl.cpp
48

We don't need this variable.

68

Please use qobject_instead. afaik, inherits() is slower than qobject_cast.

119–120

nullptr

208

Please don't use c-style casts.

298

Please follow coding style, no short names.

xwl/drag_wl.h
32–33

Please follow coding style.

56

Wouldn't it be better to use DndActions instead of CActions?

139

m_dataOffer?

xwl/drag_x.cpp
28–31

Please follow coding style. Includes have to be sorted.

111–117

In order to be consistent, we should use single-line comments, e.g.

// Start drag with serial of last left pointer button press.
// This means X to Wl drags can only be executed ....
127

Seems to be redundant.

348–350

Use std::any_of instead.

399

style

499

Please don't use c-style casts.

xwl/drag_x.h
83

m_dataSource

romangg updated this revision to Diff 48866.Jan 7 2019, 2:38 PM
romangg marked 19 inline comments as done.
  • Review comments
romangg added inline comments.Jan 7 2019, 2:39 PM
input.cpp
2102

Why is it not the case? In the stacking_order only the managed ones are, not?

xwl/clipboard.cpp
138

It takes an xcb_xfixes_selection_notify_event_t struct pointer included from xcb/xfixes.h. When I reuse components from X libraries I use the legacy NULL, because I remember that it can lead to problem if one doesn't and then gives it back to a function from such a library.

141

Could be. Qt doc is not clear on it. I will change it though.

xwl/dnd.h
79

Yea, I just find it looks weird, when there is no variable name after it, so when I just write it together.

xwl/drag.cpp
57

Would leave it as a reminder for now. If we don't have any clients making use of it, we can delete it till end of year.

xwl/drag.h
39–42

Thanks, looks better in my IDE. 8) I changed it at other similar places as well. In the other patches are other ones as well. But let us redo these after the merge.

xwl/drag_wl.cpp
298

cnt is used as an indexing variable. In this case short names are fine in my opinion. There are even people who use i for that I heard.

xwl/drag_wl.h
56

Yea, In the beginning there was also an SAction for the Server part of it, that's why the C. ;)

xwl/drag_x.cpp
127

True, I still think one should do it always, even in a destructor.

romangg updated this revision to Diff 48868.Jan 7 2019, 2:58 PM
  • Drag filter pos argument instead of event (we need only the position)
romangg updated this revision to Diff 48870.Jan 7 2019, 3:05 PM
  • Remove unneeded pointer position setters
zzag added inline comments.Jan 7 2019, 3:19 PM
input.cpp
2102

Because we don't have concept of managed windows for wayland clients.

xwl/clipboard.cpp
138

it can lead to problem if one doesn't

When?

Given that NULL has to evaluate to 0, I still suggest to use nullptr.

http://c-faq.com/null/machnon0.html

xwl/drag_wl.cpp
298

From coding style:

Single character variable names can denote counters and temporary variables whose purpose is obvious
zzag added inline comments.Jan 7 2019, 3:21 PM
input.cpp
2102

s/managed windows for wayland clients/managed clients on Wayland/

zzag added inline comments.Jan 7 2019, 3:26 PM
xwl/databridge.h
77

Please follow coding style. It should be

Dnd *dnd
romangg added inline comments.Jan 7 2019, 3:26 PM
input.cpp
2102

We also want to find managed X windows by that, not only Wayland windows. And a Wayland window is always managed in a sense.

Other way to say it would be findWindowsThatAreNotUnmanaged. ;)

zzag added inline comments.Jan 7 2019, 3:30 PM
input.cpp
2102

I'd just use an enum, e.g.

Toplevel *findToplevel(const QPoint &pos, FindToplevelOptions options);

or something like that.

zzag added inline comments.Jan 7 2019, 3:35 PM
input.cpp
1480

Stupid question: why only managed?

romangg added inline comments.Jan 7 2019, 3:52 PM
input.cpp
1480

It's an X workaround: a drag can have a Pixmap as drag icon. Then, if the drag icon is below the cursor, we would identify this one as target instead of the real window below it.

With this workaround we solve this problem with the downside that we can't drop anymore on unmanaged windows. but there are probably not many / any X clients with unmanaged windows accepting drags anyway.

zzag added a comment.Jan 8 2019, 3:43 PM

Would it be feasible to use some helpers from xcbutils.h to simplify code a little bit, e.g. create windows, raise windows, etc?

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Feb 19, 12:09 PM
This revision was automatically updated to reflect the committed changes.