D15061: [xwl] Generic X selections translation mechanism with Clipboard support

zzag added inline comments.

INLINE COMMENTS

> clipboard.cpp:98
> +    };
> +    /*
> +     * Wayland source gets created when:

Please use single line comments.

> clipboard.h:25-26
>  
> -class QProcess;
> -
> -class xcb_screen_t;
> +namespace KWayland {
> +namespace Server {
> +class DataDeviceInterface;

Please follow coding style.

> clipboard.h:36
>  
> -class Xwayland : public QObject
> +/*
> + * Represents the X clipboard, which is on Wayland side just called

/**

> clipboard.h:44
>  public:
> -    Xwayland(ApplicationWaylandAbstract *app, QObject *parent = nullptr);
> -    virtual ~Xwayland();
> -
> -    void init();
> -
> -Q_SIGNALS:
> -    void criticalError(int code);
> +    explicit Clipboard(xcb_atom_t atom, QObject *parent);
>  

Why explicit?

> romangg wrote in databridge.cpp:95-101
> See D15627 <https://phabricator.kde.org/D15627>

What exactly is the issue?

> databridge.h:57
> +public:
> +    static DataBridge* self();
> +

Please follow coding style.

> databridge.h:59
> +
> +    DataBridge(QObject *parent = nullptr);
> +    ~DataBridge();

Missing explicit?

> selection.cpp:39
> +{
> +    if (mimeType == "text/plain;charset=utf-8") {
> +        return atoms->utf8_string;

Please compare against QLatin1String

> selection.cpp:98
> +    if (m_disownPending) {
> +        /* notify of our own disown - ignore it */
> +        m_disownPending = false;

Please use a single line comment.

> selection.cpp:130
> +    case XCB_SELECTION_NOTIFY:
> +        if (handleSelNotify((xcb_selection_notify_event_t *)event)) {
> +            return true;

Please don't use c-style casts.

> selection.cpp:308-310
> +//        if (wasActive && !m_wlToXTransfers.isEmpty()) {
> +//            m_wlToXTransfers[0]->startTransferFromSource();
> +//        }

Either add a TODO comment or delete it.

> romangg wrote in selection.cpp:132
> Spills out warnings. It's similar to how Weston and wlroots does it though, so I don't want to increase difference to them here in this first version. Let's revisit it later though.
> 
> Or do you have directly a simple solution for it?

> Or do you have directly a simple solution for it?

http://doc.qt.io/qt-5/qtglobal.html#Q_FALLTHROUGH

> selection_source.h:119
> +
> +    /* @param ds must exist.
> +     *

/**

> selection_source.h:135
> +
> +    bool handleSelNotify(xcb_selection_notify_event_t *event);
> +

No short names

REPOSITORY
  R108 KWin

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

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