D15627: [xwl] Drag and drop between Xwayland and Wayland native clients

zzag added a comment.


  Some coding style nitpicks. Please ignore them for now.

INLINE COMMENTS

> input.cpp:1475
>          case QEvent::MouseMove: {
> +
>              const auto pos = input()->globalPointer();

Unrelated whitespace change.

> input.cpp:2095
> +
> +Toplevel *InputRedirection::findManagedToplevel(const QPoint &pos)
> +{

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

> wayland_server.h:271
>          KWayland::Client::Registry *registry = nullptr;
> +        KWayland::Client::Compositor *comp = nullptr;
>          KWayland::Client::Seat *seat = nullptr;

* compositor

> clipboard.cpp:139
>  {
> +    createX11Source(NULL);
> +

nullptr

> clipboard.cpp:142
> +    const auto *ac = workspace()->activeClient();
> +    if (!ac || !ac->inherits("KWin::Client")) {
> +        // clipboard is only allowed to be acquired when Xwayland has focus

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

> databridge.h:76
>      }
> +    Dnd* dnd() const {
> +        return m_dnd;

Please follow coding style.

> dnd.cpp:43-44
> +
> +namespace KWin {
> +namespace Xwl {
> +

Please follow coding style.

> dnd.cpp:135
> +    }
> +    createX11Source(NULL);
> +    const auto *seat = waylandServer()->seat();

nullptr

> dnd.h:25-26
> +
> +namespace KWayland {
> +namespace Client {
> +class Surface;

Please follow coding style.

> dnd.h:78
> +    Drag *m_currentDrag = nullptr;
> +    QVector<Drag*> m_oldDrags;
> +

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

> drag.cpp:24-25
> +
> +namespace KWin {
> +namespace Xwl {
> +

Please follow coding style.

> drag.cpp:30
> +    xcb_client_message_event_t event = {
> +        .response_type = XCB_CLIENT_MESSAGE,
> +        .format = 32,

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

> drag.cpp:43
> +                   XCB_EVENT_MASK_NO_EVENT,
> +                   (const char *)&event);
> +    xcb_flush(xcbConn);

Please don't use C-style casts.

> drag.cpp:56
> +        return CAction::None;
> +//        return CAction::Ask;
> +    }

Delete it?

> drag.cpp:70
> +        return XCB_ATOM_NONE;
> +//        return atoms->xdnd_action_ask;
> +    }

Delete it?

> drag.h:39
>  
> -class KWIN_EXPORT XwaylandIface : public QObject
> +/*
> + * An ongoing drag operation.

/**

> drag_wl.cpp:47
> +{
> +    auto * seat = waylandServer()->seat();
> +    m_dsi = seat->dragSource()->dragSource();

We don't need this variable.

> drag_wl.cpp:67
> +    }
> +    if (!ac || !ac->inherits("KWin::Client")) {
> +        // no target or wayland native target,

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

> drag_wl.cpp:118-119
> +                                                        0, 1);
> +    auto *reply = xcb_get_property_reply(xcbConn, cookie, NULL);
> +    if (reply == NULL) {
> +        doFinish();

nullptr

> drag_wl.cpp:207
> +    const xcb_atom_t usedActionAtom = m_version > 4 ? data->data32[2] :
> +                                                      (uint32_t)XCB_ATOM_NONE;
> +    Q_UNUSED(success);

Please don't use c-style casts.

> drag_wl.cpp:297
> +    size_t cnt = 0;
> +    size_t totalCnt = 0;
> +    for (const auto mimeName : mimeTypesNames) {

Please follow coding style, no short names.

> drag_wl.h:31-32
> +
> +namespace KWayland {
> +namespace Client {
> +class Surface;

Please follow coding style.

> drag_wl.h:55
> +
> +using CActions = KWayland::Client::DataDeviceManager::DnDActions;
> +

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

> drag_wl.h:138
> +    // might delete it.
> +    QPointer<KWayland::Client::DataOffer> m_do;
> +

m_dataOffer?

> drag_x.cpp:27-30
> +#include "atoms.h"
> +#include "wayland_server.h"
> +#include "workspace.h"
> +#include "abstract_client.h"

Please follow coding style. Includes have to be sorted.

> drag_x.cpp:110-116
> +    /*
> +     * Start drag with serial of last left pointer button press.
> +     * This means X to Wl drags can only be executed with the left pointer button being pressed.
> +     * For touch and (maybe) other pointer button drags we have to revisit this.
> +     *
> +     * Until then we accept the restriction for Xwayland clients.
> +     */

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 ....

> drag_x.cpp:126
> +    delete m_ds;
> +    m_ds = nullptr;
> +}

Seems to be redundant.

> drag_x.cpp:347-349
> +    const auto mimeIt = std::find_if(mimes.begin(), mimes.end(),
> +                            [name](const Mime &m) { return m.first == name; });
> +    return mimeIt != mimes.end();

Use std::any_of instead.

> drag_x.cpp:398
> +    if (reply == NULL) {
> +            return;
> +    }

style

> drag_x.cpp:498
> +    data.data32[1] = flags;
> +    data.data32[4] = flags & (1 << 0) ? m_actionAtom : (uint32_t)XCB_ATOM_NONE;
> +    Drag::sendClientMessage(m_srcWindow, atoms->xdnd_status, &data);

Please don't use c-style casts.

> drag_x.h:82
> +
> +    KWayland::Client::DataSource *m_ds;
> +

m_dataSource

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D15627

To: romangg, #kwin
Cc: zzag, kwin, alexde, GB_2, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, ngraham, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart