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
