Compiz

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

  1. compiz.fix_1165343.1
  2. Merge into 0.9.10
Proposed by Sam Spilsbury
Status: Merged
Approved by: Brandon Schaefer
Approved revision: 3726
Merged at revision: 3728
Proposed branch: lp://qastaging/~compiz-team/compiz/compiz.fix_1165343.1
Merge into: lp://qastaging/compiz/0.9.10
Diff against target: 648 lines (+179/-158)
10 files modified
.bzrignore (+1/-0)
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.1
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Brandon Schaefer (community) Approve
MC Return Approve
Review via email: mp+165768@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2013-05-20.

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 : Posted in a previous version of this proposal

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 : Posted in a previous version of this proposal

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 : Posted in a previous version of this proposal

> 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 : Posted in a previous version of this proposal

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

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

How often do I need to approve this ?

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

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

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

review: Approve (continuous-integration)
Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

Sorry its taken so long to review this... but my gnome-terminals lose their decor when running this branch :(

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

gnome-terminals seem to have their own will sometimes :(

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

Brandon could you post some steps to reproduce that?

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

Built this branch, then built unity trunk and on a compiz --replace ccp my decor was just missing from terminals. Possibly had to do with going to maximized gnome terminal to a normal window? Ill try to get some more detail tomorrow!

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

Were your terminals aleady maximized when you replaced the running
compiz instance? In such a case they would already be undecorated and
they wouldn't be redecorated once they destroyed and re-created
themselves because they preserve their decoration state.

In any case, gnome-terminal is particularly badly behaved whenever the
compositing state changes. As mentioned it destroys and re-creates its
own window whilst the window manager is being restarted. This gives
rise to a number of window management related race conditions. As
such, I generally ignore all the misbehavior that happens with
gnome-terminal when the running compiz instance is replaced. This is
probably just an example of that.

On Tue, May 28, 2013 at 11:30 PM, Brandon Schaefer
<email address hidden> wrote:
> Built this branch, then built unity trunk and on a compiz --replace ccp my decor was just missing from terminals. Possibly had to do with going to maximized gnome terminal to a normal window? Ill try to get some more detail tomorrow!
> --
> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1165343.1/+merge/165768
> Your team Compiz Maintainers is subscribed to branch lp:compiz.

--
Sam Spilsbury

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

As an aside, I believe this misbehavior had a lot to do with
transparency being completely removed in 3.8. (gnome-terminal needs to
change its colormap whenever the compositing state changes - ARGB is
required for compositing and RGB is required for fake transparency.)

On Tue, May 28, 2013 at 11:34 PM, Sam Spilsbury <email address hidden> wrote:
> Were your terminals aleady maximized when you replaced the running
> compiz instance? In such a case they would already be undecorated and
> they wouldn't be redecorated once they destroyed and re-created
> themselves because they preserve their decoration state.
>
> In any case, gnome-terminal is particularly badly behaved whenever the
> compositing state changes. As mentioned it destroys and re-creates its
> own window whilst the window manager is being restarted. This gives
> rise to a number of window management related race conditions. As
> such, I generally ignore all the misbehavior that happens with
> gnome-terminal when the running compiz instance is replaced. This is
> probably just an example of that.
>
> On Tue, May 28, 2013 at 11:30 PM, Brandon Schaefer
> <email address hidden> wrote:
>> Built this branch, then built unity trunk and on a compiz --replace ccp my decor was just missing from terminals. Possibly had to do with going to maximized gnome terminal to a normal window? Ill try to get some more detail tomorrow!
>> --
>> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1165343.1/+merge/165768
>> Your team Compiz Maintainers is subscribed to branch lp:compiz.
>
>
>
> --
> Sam Spilsbury

--
Sam Spilsbury

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

Hmm interesting, as I do keep my gnome-terminals in transparent mode. Soo let me give it some more testing to see. The big reason I marked this as needs fixing because I reverted your changes and the decor came back.

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

Sweet, things seem to be working fine now? (Must have messed up the installed and possibly broke the decor plugin?)

Either way, code looks good and the unit tests/manual tests are working for me :)

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

This seems to be wrong:

37 + int dwidth = sizeDelta.width () - lastSizeDelta.height ();

and seems to be responsible for bug #1186723 -> Regression: Now Guake, Terra and other drop-down terminals are too large and extend to the right side into the next screen

I'll propose a fix...

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: