Merge lp:~smspillaz/compiz-core/compiz-core.fix_969101 into lp:compiz-core
Status: | Merged |
---|---|
Merged at revision: | 3086 |
Proposed branch: | lp:~smspillaz/compiz-core/compiz-core.fix_969101 |
Merge into: | lp:compiz-core |
Diff against target: |
1083 lines (+784/-123) 8 files modified
plugins/decor/CMakeLists.txt (+5/-1) plugins/decor/src/clip-groups/CMakeLists.txt (+62/-0) plugins/decor/src/clip-groups/include/clip-groups.h (+100/-0) plugins/decor/src/clip-groups/src/clip-groups.cpp (+76/-0) plugins/decor/src/clip-groups/tests/CMakeLists.txt (+14/-0) plugins/decor/src/clip-groups/tests/clip-groups/src/test-decor-clip-groups.cpp (+395/-0) plugins/decor/src/decor.cpp (+92/-121) plugins/decor/src/decor.h (+40/-1) |
To merge this branch: | bzr merge lp:~smspillaz/compiz-core/compiz-core.fix_969101 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Daniel van Vugt | Approve on 2012-04-05 | ||
Alan Griffiths | 2012-03-31 | Approve on 2012-04-02 | |
Review via email:
|
This proposal supersedes a proposal from 2012-03-30.
Description of the change
== Problem ==
See LP#969101 - DecorWindow:
== Solution ==
Refactor shadow clipping into a clip group and clippable interfaces. Clip groups represent a single "layer" for shadowing purposes where all the shadows are clipped against the union of the input rects of the windows belonging to that layer. Only update items in the clip group when a relevant one has changed.
MatchedDecorCli
== Tests ==
Unit tests included.
Daniel van Vugt (vanvugt) wrote : | # |
Tests OK. No obvious regressions.
However it does terrify me to be changing such a vast (and important) region of code so close to release. I can't retain confidence in the release if we merge this before precise, so I'll leave the decision to someone else.
Daniel van Vugt (vanvugt) wrote : | # |
Approved, on the condition that it doesn't get merged till 0.9.8.
Sam Spilsbury (smspillaz) wrote : | # |
1) Its fully tested
2) It vastly improves performance
3) The vast majority of the diff size is in the tests
4) It improves code design
5) Its blocking another very important piece of code from landing
I think those are plenty good reasons to merge this /before/ precise
Took it for a test spin here and it works very well. Tried all sorts of weird window combinations with maximaized, semi-maximized, etc. All worked.
This branch seems to have as a side effect also fixing the bug where lingering shadow outlines would be briefly left behind when scrubbing panel app/indicator menus very fast. Only tested this on fast (sandybridge) hardware though, and not on my slower netbook. Is this correct or am I dreaming things up?
Alan Griffiths (alan-griffiths) wrote : | # |
I agree with Sam that the "after" code is cleaner and that the majority of the diff is low risk (adding tests & build script).
I'd quibble with making destructors pure, and NVPI but it seems to run fine.
Cleaner, unit tested code... what's not to like?
Sam Spilsbury (smspillaz) wrote : | # |
> Took it for a test spin here and it works very well. Tried all sorts of weird
> window combinations with maximaized, semi-maximized, etc. All worked.
>
> This branch seems to have as a side effect also fixing the bug where lingering
> shadow outlines would be briefly left behind when scrubbing panel
> app/indicator menus very fast. Only tested this on fast (sandybridge) hardware
> though, and not on my slower netbook. Is this correct or am I dreaming things
> up?
That is a bug in the animation plugin. Not sure what to do about it really.
Daniel van Vugt (vanvugt) wrote : | # |
Confirmed this performance improvement seems to unblock the issue with:
lp:~smspillaz/compiz-core/compiz-core.work_923683
The killer: tests are not added to "make test"
Also, pure virtual functions should be private (not protected).
And there's no point to making destructors pure => "virtual ~DecorClippable Interface () {}"
Comment only: I (and almost everyone except Herb) don't see the point of NVPI.