Compiz

Merge lp://qastaging/~compiz-team/compiz/compiz.fix_1195522 into lp://qastaging/compiz/0.9.10

compiz.fix_1195522
Merge into 0.9.10

Proposed by Sam Spilsbury on 2013-06-28

Status:

Merged

Approved by:

Brandon Schaefer on 2013-06-30

Approved revision:

3755

Merged at revision:

3751

Proposed branch:

lp://qastaging/~compiz-team/compiz/compiz.fix_1195522

Merge into:

lp://qastaging/compiz/0.9.10

Diff against target:

2357 lines (+758/-645)

24 files modified

.bzrignore (+1/-0)
plugins/decor/src/decor.cpp (+47/-132)
plugins/decor/src/decor.h (+5/-1)
plugins/decor/tests/acceptance/xorg-gtest/CMakeLists.txt (+1/-1)
plugins/decor/tests/acceptance/xorg-gtest/compiz_decor_acceptance_tests.cpp (+74/-212)
plugins/move/src/move.cpp (+19/-9)
plugins/opengl/src/paint.cpp (+1/-1)
plugins/place/src/constrain-to-workarea/include/constrain-to-workarea.h (+3/-6)
plugins/place/src/constrain-to-workarea/src/constrain-to-workarea.cpp (+25/-49)
plugins/place/src/constrain-to-workarea/tests/constrain-to-workarea/src/test-place-constrain-to-workarea.cpp (+4/-57)
plugins/place/src/place.cpp (+15/-16)
src/event.cpp (+7/-2)
src/plugin.cpp (+15/-2)
src/plugin/tests/test-plugin.cpp (+40/-0)
src/window.cpp (+71/-58)
src/window/extents/src/windowextents.cpp (+10/-0)
tests/manual/README.txt (+9/-0)
tests/manual/plugins/decor.txt (+40/-0)
tests/system/xorg-gtest/tests/compiz_xorg_gtest_configure_window.cpp (+91/-73)
tests/xorg-gtest/CMakeLists.txt (+2/-1)
tests/xorg-gtest/include/compiz-xorg-gtest.h (+79/-3)
tests/xorg-gtest/plugins/testhelper/src/testhelper.cpp (+5/-8)
tests/xorg-gtest/src/compiz-xorg-gtest-config.h.in (+2/-1)
tests/xorg-gtest/src/compiz-xorg-gtest.cpp (+192/-13)

To merge this branch:

bzr merge lp://qastaging/~compiz-team/compiz/compiz.fix_1195522

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2013-06-30
MC Return		2013-06-28	Approve on 2013-06-30
Brandon Schaefer (community)		2013-06-28	Approve on 2013-06-29
Andrea Azzarone		2013-06-28	Pending
Compiz Maintainers		2013-06-28	Pending
Review via email: mp+171944@code.qastaging.launchpad.net

Commit message

Unrevert 3728, fix failing tests.

Change the behaviour of undecorating windows.

Previously when a window was undecorated, we would shift it back to an appropriate position according to its gravity member. That behaviour was problematic because in the StaticGravity case the window has to just stay in the same place. But then if you had a window with StaticGravity which then did get a decoration and later removed it, it would be placed as though it was decorated and appear to be in the wrong place.

The correct behaviour is to place all windows as though they have decorations, and then when decorations are removed, to move the window back to the corner as indicated in its gravity and then expand its size to cover the obscured regions no longer hidden because the decorations went away.

(LP: #1165343).

  1. Completely remove decorOffsetMove and other related code from
     decor.cpp. Put the logic to handle the window->input () - window->border ()
     placement offset inside of setWindowFrameExtents instead. Now the window
     will always be offset from its original non-decorated position to the new
     decorated position, rather than having to guess between decoration sizes.
  2. Make saveGeometry and restoreGeometry work relative to window->border ()
     as opposed to including it in the saved geometry. It is possible that the
     border size might change during maximization, as such, we don't want to
     save the position with the border before maximizing. Instead save the position
     as if it were never decorated so that when the window is restored it can be
     restored to its original position and then adjusted for its new border size.
  3. Fix a few typoes in the tests.
  4. Moved some commonly used matchers into compiz::testing
  5. Make COMPIZ_PLUGIN_DIR accept multiple directories and look in each one
     of them for the plugin
  6. Set COMPIZ_PLUGIN_DIR appropriately for each plugin that we wish to load
     on startup so that we load locally built plugins as opposed to installed
     ones.
  7. Uncomment compiz_discover_tests for the acceptance tests. Now they are
     run by default.

(LP: #1195522)

Description of the change

Unrevert 3728, fix failing tests.

Change the behaviour of undecorating windows.

(LP: #1165343).

(LP: #1195522)

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2013-06-28:

368 + int GetEventMask ();

Const?

The following tests FAILED:
1481 - PixmapDecoratedWindowAcceptance.MaximizeBorderExtentsOnMaximize (Failed)
1482 - PixmapDecoratedWindowAcceptance.MaximizeBorderExtentsOnVertMaximize (Failed)
1483 - PixmapDecoratedWindowAcceptance.MaximizeBorderExtentsOnHorzMaximize (Failed)
1484 - PixmapDecoratedWindowAcceptance.MaximizeFrameWindowSizeEqOutputSize (Failed)
1485 - PixmapDecoratedWindowAcceptance.VertMaximizeFrameWindowSizeEqOutputYHeight (Failed)
1486 - PixmapDecoratedWindowAcceptance.HorzMaximizeFrameWindowSizeEqOutputXWidth (Failed)
1487 - PixmapDecoratedWindowAcceptance.VertMaximizeFrameWindowSizeSameXWidth (Failed)
1488 - PixmapDecoratedWindowAcceptance.HorzMaximizeFrameWindowSizeSameYHeight (Failed)
1489 - PixmapDecoratedWindowAcceptance.UndecoratedWindowExpandToOrigSize (Failed)
1490 - PixmapDecoratedWindowAcceptance.UndecorateStaticGravityWindow (Failed)

Note this is just with a compiled branch...though it shoudn't depend on if I have the branch running or not if they are unit tests.

Start 1451: AdjustmentExtents/PixmapDecorationAdjustment.AdjustRestoredWindowBorderShrinkClient/0

This test hangs sometimes forever. (At lease as far as im willing to let it sit :)

Start 1485: PixmapDecoratedWindowAcceptance.VertMaximizeFrameWindowSizeEqOutputYHeight

Then this wont hung...as well as some others :)...not entirely sure why.

Still have some more reviewing to go :)

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2013-06-28:

> 368 + int GetEventMask ();
>
> Const?

Yeah, I can fix that, cheers.

>
> The following tests FAILED:
> 1481 - PixmapDecoratedWindowAcceptance.MaximizeBorderExtentsOnMaximize
> (Failed)
> 1482 -
> PixmapDecoratedWindowAcceptance.MaximizeBorderExtentsOnVertMaximize (Failed)
> 1483 -
> PixmapDecoratedWindowAcceptance.MaximizeBorderExtentsOnHorzMaximize (Failed)
> 1484 -
> PixmapDecoratedWindowAcceptance.MaximizeFrameWindowSizeEqOutputSize (Failed)
> 1485 -
> PixmapDecoratedWindowAcceptance.VertMaximizeFrameWindowSizeEqOutputYHeight
> (Failed)
> 1486 -
> PixmapDecoratedWindowAcceptance.HorzMaximizeFrameWindowSizeEqOutputXWidth
> (Failed)
> 1487 -
> PixmapDecoratedWindowAcceptance.VertMaximizeFrameWindowSizeSameXWidth (Failed)
> 1488 -
> PixmapDecoratedWindowAcceptance.HorzMaximizeFrameWindowSizeSameYHeight
> (Failed)
> 1489 -
> PixmapDecoratedWindowAcceptance.UndecoratedWindowExpandToOrigSize (Failed)
> 1490 - PixmapDecoratedWindowAcceptance.UndecorateStaticGravityWindow
> (Failed)
>
> Note this is just with a compiled branch...though it shoudn't depend on if I
> have the branch running or not if they are unit tests.
>
> Start 1451: AdjustmentExtents/PixmapDecorationAdjustment.AdjustRestore
> dWindowBorderShrinkClient/0
>
> This test hangs sometimes forever. (At lease as far as im willing to let it
> sit :)
>
> Start 1485:
> PixmapDecoratedWindowAcceptance.VertMaximizeFrameWindowSizeEqOutputYHeight
>
> Then this wont hung...as well as some others :)...not entirely sure why.
>
> Still have some more reviewing to go :)

I've had some trouble with llvmpipe hanging on the tests. My plan is to make decor not depend on opengl and drop in a "fakecompositor" plugin instead which allows us to use the pixmap decorations without drawing anything.

... try running them when your system has cooled off a bit. I'll remove my local installation and check if there's some kind of hidden dependency but there shouldn't be.

> 368     + int GetEventMask ();
> 
> Const?

Yeah, I can fix that, cheers.

> 
> The following tests FAILED:
>         1481 - PixmapDecoratedWindowAcceptance.MaximizeBorderExtentsOnMaximize
> (Failed)
>         1482 -
> PixmapDecoratedWindowAcceptance.MaximizeBorderExtentsOnVertMaximize (Failed)
>         1483 -
> PixmapDecoratedWindowAcceptance.MaximizeBorderExtentsOnHorzMaximize (Failed)
>         1484 -
> PixmapDecoratedWindowAcceptance.MaximizeFrameWindowSizeEqOutputSize (Failed)
>         1485 -
> PixmapDecoratedWindowAcceptance.VertMaximizeFrameWindowSizeEqOutputYHeight
> (Failed)
>         1486 -
> PixmapDecoratedWindowAcceptance.HorzMaximizeFrameWindowSizeEqOutputXWidth
> (Failed)
>         1487 -
> PixmapDecoratedWindowAcceptance.VertMaximizeFrameWindowSizeSameXWidth (Failed)
>         1488 -
> PixmapDecoratedWindowAcceptance.HorzMaximizeFrameWindowSizeSameYHeight
> (Failed)
>         1489 -
> PixmapDecoratedWindowAcceptance.UndecoratedWindowExpandToOrigSize (Failed)
>         1490 - PixmapDecoratedWindowAcceptance.UndecorateStaticGravityWindow
> (Failed)
> 
> Note this is just with a compiled branch...though it shoudn't depend on if I
> have the branch running or not if they are unit tests.
> 
>         Start 1451: AdjustmentExtents/PixmapDecorationAdjustment.AdjustRestore
> dWindowBorderShrinkClient/0
> 
> This test hangs sometimes forever. (At lease as far as im willing to let it
> sit :)
> 
>         Start 1485:
> PixmapDecoratedWindowAcceptance.VertMaximizeFrameWindowSizeEqOutputYHeight
> 
> Then this wont hung...as well as some others :)...not entirely sure why.
> 
> Still have some more reviewing to go :)

... try running them when your system has cooled off a bit. I'll remove my local installation and check if there's some kind of hidden dependency but there shouldn't be.

Revision history for this message

MC Return (mc-return) wrote on 2013-06-28:

Thanks a lot for this, Sam.
Your last version in r3278 already brought massive improvements, especially when running Emerald.

I am quite sure that this one will finally get rid of all decor problems.
Will test this ASAP.

Revision history for this message

MC Return (mc-return) wrote on 2013-06-28:

First review:

726 + /* As the window is shrunk to accomadate the border size, we must subtract

should be (typo):

726 + /* As the window is shrunk to accommodate the border size, we must subtract

===

738 + ct::WINDOW_WIDTH - shrink.width () + size.width (),
739 + ct::WINDOW_HEIGHT - shrink.height () + size.height ());

795 + ct::WINDOW_WIDTH - shrink.width () + size.width (),
796 + ct::WINDOW_HEIGHT - shrink.height () + size.height (),

I would align those (just a suggestion, no real need to fix):

738 + ct::WINDOW_WIDTH - shrink.width () + size.width (),
739 + ct::WINDOW_HEIGHT - shrink.height () + size.height ());

===

You could remove the brackets here:

832 + xwc.x = xRoot - (width / 2);

855 + xwc.x = xRoot - (width / 2);

===

I guess this was just an indentation fix ?

871 - gw->priv->configureLock->release ();
872 + gw->priv->configureLock->release ();

===

The rest LGTM also.
As this touches many source-files it is not really easy to manually test though, but the old version of this was already great and now this additionally has massive test coverage, so "Approve" from me.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-06-28:

FAILED: Continuous integration, rev:3751
http://jenkins.qa.ubuntu.com/job/compiz-ci/218/
Executed test runs:
    None: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/259/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-saucy-amd64-ci/31/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-armhf-ci/31
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-saucy-i386-ci/31/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/218/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2013-06-28:

> First review:
>
> 726 + /* As the window is shrunk to accomadate the border size, we must
> subtract
>
> should be (typo):
>
> 726 + /* As the window is shrunk to accommodate the border size, we
> must subtract

Fixed.

>
> I guess this was just an indentation fix ?
>
> 871 - gw->priv->configureLock->release ();
> 872 + gw->priv->configureLock->release ();
>
> ===

Pretty much

>
> The rest LGTM also.
> As this touches many source-files it is not really easy to manually test
> though, but the old version of this was already great and now this
> additionally has massive test coverage, so "Approve" from me.

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2013-06-28:

Note: I've removed the acceptance tests from the CI target as llvmpipe does not work correctly there. We'll re-enable them once we can run the tests without llvmpipe.

Revision history for this message

MC Return (mc-return) wrote on 2013-06-28:

I would say -> let's give it a try.
With the PPAs in place we will notice possible regressions early and can combat them then, if any will occur, which I doubt to happen...

The final decision to merge is yours. +1 from me. :)

review: Approve

Revision history for this message

MC Return (mc-return) wrote on 2013-06-28:

One note:
If r3747 of this MP ( http://bazaar.launchpad.net/~compiz-team/compiz/compiz.fix_1195522/revision/3747 ) fixes LP: #1165343, shouldn't we link this branch to the bug report then and add the bug number to the main commit message ?

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2013-06-28:

Your branch hates me:

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2013-06-28:

I need to fix my system up though...

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2013-06-29:

After talking with Sam its turning out that the tests failures are due to llvmpipe and some timeout problems. Since this branch fixes some blockers, and each failing tests passes when ran individually, Im approving this branch.

The tests do need to be fixed up, but lets get the blockers fixed first :)

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-06-29:

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/compiz-autolanding/110/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-saucy-amd64-autolanding/22/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-armhf-autolanding/22
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-saucy-i386-autolanding/22/console
    SUCCESS: http://s-jenkins:8080/job/generic-cleanup-mbs/540