D6591: XdgV6 - Kwin side

[Martin Flöser]

graesslin requested changes to this revision.
graesslin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> shell_client.cpp:222
> +
> +        connect(static_cast<XdgShellInterface *>(m_xdgShellSurface->global()), &XdgShellInterface::pingDelayed,
> +            m_xdgShellSurface, [this](qint32 serial) {

maybe instead of the three casts just store a local variable?

  auto global = (static_cast<XdgShellInterface *>(m_xdgShellSurface->global());

> shell_client.cpp:231-239
> +        connect(static_cast<XdgShellInterface *>(m_xdgShellSurface->global()), &XdgShellInterface::pingTimeout,
> +            m_xdgShellSurface, [this](qint32 serial) {
> +                auto it = m_pingSerials.find(serial);
> +                if (it != m_pingSerials.end()) {
> +                    qCDebug(KWIN_CORE) << "Final ping timeout, asking to kill:" << caption();
> +                    killWindow();
> +                    m_pingSerials.erase(it);

You should only kill if the ping reason is close. This would also ping for focus, wouldn't it?

> shell_client.cpp:277-280
> +        connect(m_xdgShellPopup, &XdgShellPopupInterface::grabbed, this, [this](SeatInterface *seat, quint32 serial) {
> +            Q_UNUSED(serial)
> +            seat->setFocusedPointerSurface(surface());
> +        });

this needs to go through our PointerInputRedirection code, otherwise SeatInterface and PointerInputRedirection get out of sync. From my understanding that should be ::popupGrab?

> shell_client.cpp:608-609
>      if (m_xdgShellSurface && isCloseable()) {
> -        m_xdgShellSurface->close();
> -        return;
> -    }
> -    if (m_qtExtendedSurface && isCloseable()) {
> +        const qint32 pingSerial = static_cast<XdgShellInterface *>(m_xdgShellSurface->global())->ping();
> +        m_pingSerials.insert(pingSerial, CloseWindow);
> +    } else if (m_qtExtendedSurface && isCloseable()) {

just wondering: isn't there a close() missing? It's only pinging...

> shell_client.h:44
>  public:
> +    enum PingReason {
> +        CloseWindow = 0,

please use enum class for new enums.

REPOSITORY
  R108 KWin

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

To: davidedmundson, #plasma, graesslin
Cc: graesslin, kwin, plasma-devel, #kwin, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20170907/af429aac/attachment-0001.html>