Merge lp:~smspillaz/compiz-animation-plugin/compiz-animation-plugion.fix_940603 into lp:compiz-animation-plugin

Proposed by Sam Spilsbury on 2012-04-10
Status: Merged
Merged at revision: 397
Proposed branch: lp:~smspillaz/compiz-animation-plugin/compiz-animation-plugion.fix_940603
Merge into: lp:compiz-animation-plugin
Diff against target: 27 lines (+5/-5)
1 file modified
src/animation.cpp (+5/-5)
To merge this branch: bzr merge lp:~smspillaz/compiz-animation-plugin/compiz-animation-plugion.fix_940603
Reviewer Review Type Date Requested Status
Daniel van Vugt 2012-04-10 Approve on 2012-04-10
Review via email:

Description of the change

See LP #940603

== Problem ==

There is a corner case where a window, when it receives a ReparentNotify, would have its destroy reference incremented and then never decremented because an animation was never playing for it.

== Solution ==

  Don't increment the destroy ref count of a window unless an animation is active.

  It doesn't make any sense to increase the destroy reference count of a window
  on CompWindowNotifyBeforeDestroy based on whether or not an an animation for
  that window "could" be allowed - CompWindowNotifyBeforeDestroyed is called
  on CompWindow::destroy (which under sane circumstances, should only ever be called
  when the window is either reparented away from its parent or the root window
  or when the window is destroyed).

  In this case, we deregister all events on the window so it isn't possible to determine
  through any normal means that a close animation is going to activate on the window. And
  in any event, the unmap which causes close animations always comes before the DestroyNotify
  or ReparentNotify anyways.

  Even if the CompWindowNotifyClose were to somehow come after the window received a ReparentNotify
  or DestroyNotify, the tradeoff then comes between not playing animations for those corner case
  windows or keeping ghost windows on screen because we kept a destroy reference and never
  got rid of it. In this case, it is more sane to do the former.

== Tests ==

None ......

To post a comment you must log in.
Daniel van Vugt (vanvugt) wrote :

Looks good and seems to work OK.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/animation.cpp'
2--- src/animation.cpp 2012-03-20 10:36:39 +0000
3+++ src/animation.cpp 2012-04-10 06:25:23 +0000
4@@ -1631,7 +1631,8 @@
5 {
6 if (!mLockedPaintListCnt)
7 {
8- mLockedPaintList = &cScreen->getWindowPaintList ();
9+ const CompWindowList &pl = cScreen->getWindowPaintList ();
10+ mLockedPaintList = &pl;
12 if (!mGetWindowPaintListEnableCnt)
13 {
14@@ -2776,10 +2777,9 @@
15 if (mPAScreen->shouldIgnoreWindowForAnim (mWindow, true))
16 break;
18- if (AnimEffectNone ==
19- mPAScreen->getMatchingAnimSelection (mWindow,
20- AnimEventClose,
21- &duration))
22+ /* Don't increment the destroy reference count unless
23+ * the window is already animated */
24+ if (!mCurAnimation)
25 break;
27 mDestroyCnt++;


People subscribed via source and target branches