D5106: Add support for activating screenedges through touch swipe gestures

[David Edmundson]

davidedmundson added a comment.


  mostly looks fine. Only one major comment about the return after handling client edges.

INLINE COMMENTS

> gestures.cpp:61
> +    case Direction::Right:
> +        return std::min(std::abs(delta.width()) / std::abs(m_minimumDelta.width()), 1.0);
> +    default:

having negatives in minimumDelta would make no sense in the first place..

> screenedge.cpp:76
>      , m_client(nullptr)
> +    , m_gesture(new SwipeGesture)
>  {

who owns SwipeGesture?

> screenedge.cpp:84
> +                m_client->showOnScreenEdge();
> +                unreserve();
> +            }

equivalent ::handle() code returns after this.

> screenedge.cpp:92
> +    connect(m_gesture, &SwipeGesture::cancelled, this, &Edge::stopApproaching);
> +    connect(m_gesture, &SwipeGesture::triggered, this, &Edge::stopApproaching);
> +    connect(m_gesture, &SwipeGesture::progress, this,

maybe we want to do this inside the triggered lambda above, otherwise we call stopApproaching, before handling the trigger which is queued.

If our wayland side ever gets a doStopApproaching, it might result in being a bit out of order

> screenedge.cpp:473
>  {
> +    if (isScreenEdge() && !m_edges->isDesktopSwitching()) {
> +        m_edges->gestureRecognizer()->registerGesture(m_gesture);

Note that the only subclass of Edge doesn't call the super class for any of the virtual methods: doGeom/activate/deactivate.

To fit the current pattern this should be in setGeometry / reserve / unreserve respectively

(and yes, I know the current only subclass of Edge isn't relevant in this case)

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

To: graesslin, #kwin, #plasma_on_wayland
Cc: davidedmundson, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20170319/79a9a9ed/attachment.html>