Differential D13456

[wayland] Make it possible to reach screen edges
AbandonedPublic
Actions

Authored by zzag on Jun 9 2018, 11:23 PM.

Details

Reviewers
poboiko
Group Reviewers
KWin
Summary

This patch aims to fix bug 374867 in a way described at my comment #3 there:

It behaves in a following manner: when I'm moving cursor, each time it jumps by some vector (dx, dy), which depends on the speed of cursor movement. It seems like there is a
following check: if (x+dx, y+dy) is outside the visible area, just ignore that move, and thus make the cursor stuck at some position away from the border.
If I move slowly, then (dx, dy) is small, which allows me to come closer to the border and eventually touch it (thus triggering the edge action). But if I keep moving fast, the cursor is
stuck.
The proper behavior would be to move cursor exactly at the border instead.

Example: if I move cursor fast towards i.e. upper-left angle, it generates bunch of updatePosition() calls with position being somewhere around (-100, -100).
Current code just ignores those events; new code just moves it to (0, 0) instead.

Test Plan

Screen edges & auto-hiding panels now work fine for me on wayland.
Also tested with external display attached.

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
poboiko created this revision.Jun 9 2018, 11:23 PM
Restricted Application added a project: KWin. · View Herald TranscriptJun 9 2018, 11:23 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
poboiko requested review of this revision.Jun 9 2018, 11:23 PM
graesslin added a subscriber: graesslin.Jun 11 2018, 4:29 AM

Great work. Could you please extend the unit test for this area. Did you run them? As this is a behavior change I could imagine that it breaks the existing tests.

poboiko added a comment.Jun 11 2018, 4:25 PM

No, I didn't run unit tests.
Actually, I might need some help with that, because I just tried to build KWin, run "make test", and actually lots of tests fail (both with and without this patch), see https://paste.kde.org/pnqk86drj (I didn't manage to test further - it hangs after "kwin-testActivities")
Seems like I am doing something wrong :(

I can look at autotests/integration/pointer_input.cpp and add some tests as soon as I figure out how to make it work.

davidedmundson added a subscriber: davidedmundson.Jun 11 2018, 4:27 PM

run "dbus-launch ./test-whatever"

zzag added a subscriber: zzag.Jun 11 2018, 4:28 PM
In D13456#277135, @davidedmundson wrote:

run "dbus-launch ./test-whatever"

I usually go to the build directory and run

bin/testWhateverIWantToTest
graesslin added a comment.Jun 11 2018, 7:30 PM
In D13456#277137, @zzag wrote:
In D13456#277135, @davidedmundson wrote:

run "dbus-launch ./test-whatever"

I usually go to the build directory and run

bin/testWhateverIWantToTest

the dbus-launch is quite important. Otherwise the tests might mess with your running session. make test should do that part automatically.

For the failing tests in the paste: check https://blog.martin-graesslin.com/blog/2017/06/running-kwins-auto-test-suite/

zzag added a comment.Jun 11 2018, 8:08 PM
In D13456#277207, @graesslin wrote:

the dbus-launch is quite important. Otherwise the tests might mess with your running session. make test should do that part automatically.

Thank you for the explanation. I'm afraid that make test still messes my session.

davidedmundson added a comment.Jul 5 2018, 11:57 AM

What's the status of this?

I can help with anything if needed

graesslin added a comment.Jul 5 2018, 5:15 PM
In D13456#287253, @davidedmundson wrote:

What's the status of this?

I can help with anything if needed

I think the only thing missing is adjusting unit test.

poboiko added a comment.Jul 5 2018, 5:50 PM
In D13456#287253, @davidedmundson wrote:

What's the status of this?

I can help with anything if needed

Sorry, I'm having pretty busy month, didn't really have time for coding.
Martin is right, need to implement unit tests for that. I would appreciate if you could do that!

zzag added a comment.Jul 10 2018, 8:26 AM

Also, it looks like this code doesn't handle a case when user moves pointer really-really fast. In that case, the pointer can bypass whole screens and end up in offscreen area.

pointer_input.cpp
749–757

I believe it can be simplified, e.g.

p.setX(qBound(qreal(curScreen.left()), p.x(), qreal(curScreen.right())));
if (!screenContainsPos(p)) {
    p.setY(qBound(qreal(curScreen.top()), p.y(), qreal(curScreen.bottom())));
    if (!screenContainsPos(p)) {
        // Can it happen?
        return;
    }
}
zzag added a comment.EditedJul 10 2018, 9:45 AM

I believe updatePosition should do the following:

...

QPointF p = pos;

if (!screenContainsPos(p)) {
    const QRect unitedScreenGeometry = screens()->geometry();
    p.setX(qBound(qreal(unitedScreenGeometry.left()), p.x(), qreal(unitedScreenGeometry.right())));
    p.setY(qBound(qreal(unitedScreenGeometry.top()), p.y(), qreal(unitedScreenGeometry.bottom()));
}

if (!screenContainsPos(p)) {
    // TODO: maybe move upwards.
    const QRect currentScreenGeometry = screens()->geometry(screens()->number(m_pos.toPoint()));
    p.setX(qBound(qreal(currentScreenGeometry.left()), p.x(), qreal(currentScreenGeometry.right())));
    p.setY(qBound(qreal(currentScreenGeometry.top()), p.y(), qreal(currentScreenGeometry.bottom())));
}

p = applyPointerConfinement(p);

...

I haven't tested it so maybe it's wrong. Anyway, I could work on this problem if you don't mind.

Edit: also another possible solution would be

const QRect curScreen(screens()->geometry(screens()->number(pos.toPoint())));

Another fancy solution, which is more precise, would be to do ray-tracing.

zzag added a comment.Jul 10 2018, 1:19 PM

So, it seems to be a little bit complicated: if there are gaps between screens, p should be bounded by screens()->geometry(). Otherwise, p should be bounded by geometry of the current screen. The problem is the case when pointer moves really fast.

FWIW, with the following setup


that's not possible to move pointer to the screen on the right hand side.

graesslin added a comment.Jul 10 2018, 5:13 PM
In D13456#289923, @zzag wrote:

FWIW, with the following setup


that's not possible to move pointer to the screen on the right hand side.

That setup should not be allowed. If you were able to create it, that would be a bug. One of the basic design decisions I did for the input and output handling was to disallow space between screens.

zzag added a comment.Jul 10 2018, 5:39 PM
In D13456#290013, @graesslin wrote:

That setup should not be allowed. If you were able to create it, that would be a bug. One of the basic design decisions I did for the input and output handling was to disallow space between screens.

Okay, that simplifies things quite a lot. So, here's how I suggest to fix 374867:

  • try to confine p to screens()->geometry()
  • if some screen contains p, we're done
  • otherwise, confine p to the current screen

If you don't have objections, I'd like to start work on it.

graesslin added a comment.Jul 10 2018, 5:47 PM

go for it :-)

zzag commandeered this revision.Jul 11 2018, 7:58 AM
zzag abandoned this revision.
zzag added a reviewer: poboiko.

See D14036

Revision Contents
Changeset List

PathPackages
Mpointer_input.cpp (8 lines)

Diff 35923

View Options

pointer_input.cpp

Loading...