Merge lp:~vanvugt/compiz/fix-201681-wall into lp:compiz/0.9.9

Proposed by Daniel van Vugt on 2012-11-29
Status: Merged
Approved by: Brandon Schaefer on 2012-11-30
Approved revision: 3487
Merged at revision: 3492
Proposed branch: lp:~vanvugt/compiz/fix-201681-wall
Merge into: lp:compiz/0.9.9
Diff against target: 21 lines (+2/-2)
1 file modified
plugins/wall/src/wall.cpp (+2/-2)
To merge this branch: bzr merge lp:~vanvugt/compiz/fix-201681-wall
Reviewer Review Type Date Requested Status
Brandon Schaefer (community) 2012-11-29 Approve on 2012-11-30
PS Jenkins bot (community) continuous-integration Approve on 2012-11-29
Compiz Maintainers 2012-11-29 Pending
Review via email: mp+136879@code.launchpad.net

Commit message

When dragging a window between workspaces, don't warp the pointer by a
mysterious 10 pixels. Instead warp the pointer by 1 so it goes from one edge
to the next without getting ahead of the window drag. (LP: #201681)

Description of the change

This provides a dramatic improvement over the old behaviour but it's still imperfect. In some cases the warp will be a pixel or two off. This is because we're always assuming it should be 1 pixel per flip, but we have no guarantee this is right because we don't have the exact delta from the move plugin. All initateFlip tells you is the direction.

And you can't calculate your own delta "dx = pointerX - lastPointerX". That is almost always zero because core likes to reset "lastPointerX = pointerX" on every event loop.

Yet another reason why all window movement should be absolute and not relative.

To post a comment you must log in.
Sam Spilsbury (smspillaz) wrote :

This looks okay in its current form, the surrounding code feels a little weird but I can't think of anything better.

Have you looked into getting this under test?

Daniel van Vugt (vanvugt) wrote :

The only relevant tests would be: EXPECT_TRUE(1 != 10)
or: EXPECT_TRUE(0 + screen->width() - 1 == screen->width () - 1)

They're kind of pointless statements.

That is unless we fix the whole mouse warping issue _everywhere_. That would be non-trivial and require tests.

Sam Spilsbury (smspillaz) wrote :

Okay, lets put this through now.

On Thu, Nov 29, 2012 at 6:47 PM, Daniel van Vugt
<email address hidden> wrote:
> The only relevant tests would be: EXPECT_TRUE(1 != 10)
> or: EXPECT_TRUE(0 + screen->width() - 1 == screen->width () - 1)
>
> They're kind of pointless statements.
>
> That is unless we fix the whole mouse warping issue _everywhere_. That would be non-trivial and require tests.
> --
> https://code.launchpad.net/~vanvugt/compiz/fix-201681-wall/+merge/136879
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~vanvugt/compiz/fix-201681-wall into lp:compiz.

--
Sam Spilsbury

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Brandon Schaefer (brandontschaefer) wrote :

Haha...I spent part of today figure this out as well! Perfect, looks good.

Though the problem in 12.04 seems...a bit more then 10 px...so Ill take a look at that tomorrow to see if it is.

Looks good to me!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/wall/src/wall.cpp'
2--- plugins/wall/src/wall.cpp 2012-09-05 15:55:13 +0000
3+++ plugins/wall/src/wall.cpp 2012-11-29 09:31:22 +0000
4@@ -864,7 +864,7 @@
5
6 if (dx < 0)
7 {
8- offsetX = screen->width () - 10;
9+ offsetX = screen->width () - 1;
10 warpX = pointerX + screen->width ();
11 }
12 else if (dx > 0)
13@@ -880,7 +880,7 @@
14
15 if (dy < 0)
16 {
17- offsetY = screen->height () - 10;
18+ offsetY = screen->height () - 1;
19 warpY = pointerY + screen->height ();
20 }
21 else if (dy > 0)

Subscribers

People subscribed via source and target branches