Merge lp:~smspillaz/compiz-core/compiz-core.more-sound-reparenting-behaviour into lp:compiz-core

Proposed by Sam Spilsbury on 2012-04-04
Status: Merged
Merged at revision: 3085
Proposed branch: lp:~smspillaz/compiz-core/compiz-core.more-sound-reparenting-behaviour
Merge into: lp:compiz-core
Diff against target: 110 lines (+10/-45)
3 files modified
src/event.cpp (+2/-1)
src/privatescreen.h (+0/-2)
src/window.cpp (+8/-42)
To merge this branch: bzr merge lp:~smspillaz/compiz-core/compiz-core.more-sound-reparenting-behaviour
Reviewer Review Type Date Requested Status
Daniel van Vugt 2012-04-04 Approve on 2012-04-04
Review via email: mp+100720@code.launchpad.net

Description of the change

Stab in the dark.

I think whats going on for bug 940603 is that windows aren't being removed from the window list if they are unreparented by the application itself in very special circumstances (not sure what because I can't reproduce the bug).

This code adds some more sound checks to see where the window is going and destroys it based on that, rather than using the same logic for both reparented and unreparented windows.

Also removed the whole destroyedFrameWindows tracking code which was more or ineffectual and could potentially result in a frame window leak.

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

In case you're wondering, I just corrected the bug number in the description.

Sam Spilsbury (smspillaz) wrote :

thanks

Daniel van Vugt (vanvugt) wrote :

I was optimistically hoping this branch would fix bug 865672. But it doesn't.

Daniel van Vugt (vanvugt) wrote :

Seems to work perfectly without any visible regressions.

I cannot confirm it fixes any particular bug at all. And I don't claim to fully understand the code. Based on recent history, that would lead me to abstain. However I strongly support any changes that make the code significantly smaller and easier to maintain. And if there is a chance this fixes bug 940603 then we really want to commit it for the sake of testing at least.

Less importantly, I think it's bad practice to assume None == 0 even though it is true. So this:
  if (priv->serverFrame)
should be written as:
  if (priv->serverFrame != None)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/event.cpp'
2--- src/event.cpp 2012-03-24 09:49:30 +0000
3+++ src/event.cpp 2012-04-04 03:23:19 +0000
4@@ -1355,7 +1355,8 @@
5
6 if (w)
7 {
8- if (event->xreparent.parent != w->priv->wrapper)
9+ if ((w->priv->wrapper && event->xreparent.parent != w->priv->wrapper) ||
10+ (!w->priv->wrapper && event->xreparent.parent != priv->root))
11 {
12 w->moveInputFocusToOtherWindow ();
13 w->destroy ();
14
15=== modified file 'src/privatescreen.h'
16--- src/privatescreen.h 2012-03-29 16:54:46 +0000
17+++ src/privatescreen.h 2012-04-04 03:23:19 +0000
18@@ -523,8 +523,6 @@
19 CompWindow::Map windowsMap;
20 std::list<CompGroup *> groups;
21
22- std::map <CompWindow *, CompWindow *> detachedFrameWindows;
23-
24 CompWindowVector clientList; /* clients in mapping order */
25 CompWindowVector clientListStacking; /* clients in stacking order */
26
27
28=== modified file 'src/window.cpp'
29--- src/window.cpp 2012-03-30 11:43:20 +0000
30+++ src/window.cpp 2012-04-04 03:23:19 +0000
31@@ -1441,8 +1441,11 @@
32 windowNotify (CompWindowNotifyBeforeDestroy);
33
34 /* Don't allow frame windows to block input */
35- XUnmapWindow (screen->dpy (), priv->serverFrame);
36- XUnmapWindow (screen->dpy (), priv->wrapper);
37+ if (priv->serverFrame)
38+ XUnmapWindow (screen->dpy (), priv->serverFrame);
39+
40+ if (priv->wrapper)
41+ XUnmapWindow (screen->dpy (), priv->wrapper);
42
43 oldServerNext = serverNext;
44 oldServerPrev = serverPrev;
45@@ -6461,21 +6464,6 @@
46 dw->serverPrev = this->serverPrev;
47 }
48
49- /* If this window has a detached frame, destroy it, but only
50- * using XDestroyWindow since there may be pending restack
51- * requests relative to it */
52-
53- std::map <CompWindow *, CompWindow *>::iterator it =
54- screen->priv->detachedFrameWindows.find (this);
55-
56- if (it != screen->priv->detachedFrameWindows.end ())
57- {
58- CompWindow *fw = (it->second);
59-
60- XDestroyWindow (screen->dpy (), fw->id ());
61- screen->priv->detachedFrameWindows.erase (it);
62- }
63-
64 if (!priv->destroyed)
65 {
66 StackDebugger *dbg = StackDebugger::Default ();
67@@ -6934,22 +6922,6 @@
68 attr.colormap = cmap;
69 attr.override_redirect = true;
70
71- /* Look for existing detached frame windows and reattach them
72- * in case this window as reparented again after being withdrawn */
73- std::map <CompWindow *, CompWindow *>::iterator it =
74- screen->priv->detachedFrameWindows.find (window);
75-
76- if (it != screen->priv->detachedFrameWindows.end ())
77- {
78- /* Trash the old frame window
79- * TODO: It would be nicer if we could just
80- * reparent back into it, but there are some
81- * problems with that */
82-
83- XDestroyWindow (dpy, (it->second)->id ());
84- screen->priv->detachedFrameWindows.erase (it);
85- }
86-
87 /* We need to know when the frame window is created
88 * but that's all */
89 XSelectInput (dpy, screen->root (), SubstructureNotifyMask);
90@@ -7192,17 +7164,11 @@
91
92 /* Put the frame window "above" the client window
93 * in the stack */
94- CompWindow *fw = PrivateWindow::createCompWindow (id, attrib, serverFrame);
95-
96- /* Put this window in the list of "detached frame windows"
97- * so that we can reattach it or destroy it when we are
98- * done with it */
99-
100- screen->priv->detachedFrameWindows[window] = fw;
101+ PrivateWindow::createCompWindow (id, attrib, serverFrame);
102 }
103
104- /* Safe to destroy the wrapper but not the frame */
105- XUnmapWindow (screen->dpy (), serverFrame);
106+ /* Issue a DestroyNotify */
107+ XDestroyWindow (screen->dpy (), serverFrame);
108 XDestroyWindow (screen->dpy (), wrapper);
109
110 window->windowNotify (CompWindowNotifyUnreparent);

Subscribers

People subscribed via source and target branches