Compiz

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

  1. compiz.fix_1165343
  2. Merge into 0.9.10
Proposed by Sam Spilsbury
Status: Superseded
Proposed branch: lp://qastaging/~compiz-team/compiz/compiz.fix_1165343
Merge into: lp://qastaging/compiz/0.9.10
Diff against target: 642 lines (+178/-158)
9 files modified
plugins/decor/src/decor.cpp (+78/-38)
plugins/decor/src/decor.h (+6/-0)
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 (+3/-8)
src/window/extents/src/windowextents.cpp (+10/-0)
tests/manual/README.txt (+9/-0)
tests/manual/plugins/decor.txt (+40/-0)
To merge this branch: bzr merge lp://qastaging/~compiz-team/compiz/compiz.fix_1165343
Reviewer Review Type Date Requested Status
MC Return Approve
PS Jenkins bot (community) continuous-integration Approve
Compiz Maintainers Pending
Review via email: mp+164762@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2013-04-18.

This proposal has been superseded by a proposal from 2013-05-26.

Commit message

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)

Description of the change

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.

The spec is unfortunately not so clear what to do in this case. But KWin does it this way
and it seems to be the sanest way of doing it.

(LP: #1165343)

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:3660
http://jenkins.qa.ubuntu.com/job/compiz-ci/137/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/175
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-pbuilder/./build=pbuilder,distribution=raring,flavor=amd64/526

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

review: Approve (continuous-integration)
Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

The commit message looks a bit strange...

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

606 +# Install any sun-awt application

You could give some concrete example here.

596 +Actions:
597 +# Start and launch friends-app
598 +
599 +Expected Result:
600 + The QML window should have its decorations visible and
601 + be contained in the top right hand corner of the work area

It starts placed in the top-left corner here...

Note: I was not able to fully test this branch yet, just tested the changes
      to the place and decor plugins, but not core, so I have to "Abstain"
      for now.

review: Abstain
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

On Mon, Apr 22, 2013 at 6:04 PM, MC Return <email address hidden> wrote:
> Review: Abstain
>
> 606 +# Install any sun-awt application
>
> You could give some concrete example here.
>
> 596 +Actions:
> 597 +# Start and launch friends-app
> 598 +
> 599 +Expected Result:
> 600 + The QML window should have its decorations visible and
> 601 + be contained in the top right hand corner of the work area
>
> It starts placed in the top-left corner here...

Ah, that's a typo. Thanks.

>
> Note: I was not able to fully test this branch yet, just tested the changes
> to the place and decor plugins, but not core, so I have to "Abstain"
> for now.
> --
> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1165343/+merge/159540
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~compiz-team/compiz/compiz.fix_1165343 into lp:compiz.

--
Sam Spilsbury

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:3661
http://jenkins.qa.ubuntu.com/job/compiz-ci/142/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/180
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-pbuilder/./build=pbuilder,distribution=raring,flavor=amd64/531

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

review: Approve (continuous-integration)
Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

The commit message here still looks a bit strange ;)

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

LGTM now. +1

review: Approve
Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

I am still hitting bug #1159324 in current trunk... :(

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

With our without this branch?
On 20/05/2013 1:39 AM, "MC Return" <email address hidden> wrote:

> I am still hitting bug #1159324 in current trunk... :(
> --
>
> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1165343/+merge/159540
> Your team Compiz Maintainers is requested to review the proposed merge of
> lp:~compiz-team/compiz/compiz.fix_1165343 into lp:compiz.
>

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

> With our without this branch?

Without it.

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Okay. Please comment on the bug reports then to raise that issue. I'm still
waiting on someone from canonical to review this branch and posting
comments here can get confusing.
On 20/05/2013 9:27 AM, "MC Return" <email address hidden> wrote:

> > With our without this branch?
>
> Without it.
> --
>
> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1165343/+merge/159540
> Your team Compiz Maintainers is requested to review the proposed merge of
> lp:~compiz-team/compiz/compiz.fix_1165343 into lp:compiz.
>

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

Guake is still broken, even with this MP :(

The strange thing is -> frist it broke, then you fixed it, then it broke again and now it is already broken for quite a while...

review: Needs Fixing
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

I'll have a quick look into it now I guess.

On Mon, May 20, 2013 at 6:15 PM, MC Return <email address hidden> wrote:
> Review: Needs Fixing
>
> Guake is still broken, even with this MP :(
>
> The strange thing is -> frist it broke, then you fixed it, then it broke again and now it is already broken for quite a while...
> --
> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1165343/+merge/159540
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~compiz-team/compiz/compiz.fix_1165343 into lp:compiz.

--
Sam Spilsbury

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

> I'll have a quick look into it now I guess.
>
Thx. +1

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Should work as expected now.

This reminds me that the decor plugin really needs to be a priority in terms of getting an acceptance test framework up and running - the amount of regressions we get here is *stupidly* high because the behaviour is so tightly coupled with the rest of the geometry-changing plugins.

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

> Should work as expected now.
>
Hmm, did you run the manual tests ?

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

Yes, I did "run" the manual tests. They all verify just fine. friends-app and guake both sit flush with the panels upon placement.

Unless I missed something?

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:3663
http://jenkins.qa.ubuntu.com/job/compiz-ci/181/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/222
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-raring-amd64-ci/38
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-raring-armhf-ci/38
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-raring-i386-ci/36

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

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:3663
http://jenkins.qa.ubuntu.com/job/compiz-ci/182/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/223
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-raring-amd64-ci/39
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-raring-armhf-ci/39
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-raring-i386-ci/37

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

review: Approve (continuous-integration)
Revision history for this message
MC Return (mc-return) wrote :

> Yes, I did "run" the manual tests. They all verify just fine. friends-app and
> guake both sit flush with the panels upon placement.
>
> Unless I missed something?

+1. Thanks.

(I missed the change in src/window/extents/src/windowextents.cpp ;))

review: Approve
Revision history for this message
MC Return (mc-return) wrote :

Note: I've linked your branch to the Guake bug report.

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: