Merge lp:~vanvugt/compiz/fix-1046661 into lp:compiz/0.9.8

Proposed by Daniel van Vugt on 2012-09-06
Status: Merged
Approved by: Sam Spilsbury on 2012-09-06
Approved revision: 3349
Merged at revision: 3346
Proposed branch: lp:~vanvugt/compiz/fix-1046661
Merge into: lp:compiz/0.9.8
Diff against target: 171 lines (+77/-36)
5 files modified
plugins/opengl/src/fsregion/fsregion.cpp (+2/-2)
plugins/opengl/src/fsregion/fsregion.h (+6/-4)
plugins/opengl/src/fsregion/tests/test-fsregion.cpp (+19/-0)
plugins/opengl/src/paint.cpp (+34/-30)
tests/manual/Unredirect.txt (+16/-0)
To merge this branch: bzr merge lp:~vanvugt/compiz/fix-1046661
Reviewer Review Type Date Requested Status
Sam Spilsbury 2012-09-06 Approve on 2012-09-06
jenkins (community) continuous-integration Approve on 2012-09-06
Review via email: mp+123026@code.launchpad.net

Commit message

Fixed: Windows with an alpha-channel, like gnome-terminal, were not being
considered as possibly covering fullscreen windows. But they most certainly
can. This ensures such RGBA windows are visible if they're stacked above a
fullscreen window. (LP: #1046661)
.

Description of the change

Fixed: Windows with an alpha-channel, like gnome-terminal, were not being
considered as possibly covering fullscreen windows. But they most certainly
can. This ensures such RGBA windows are visible if they're stacked above a
fullscreen window. (LP: #1046661)

To post a comment you must log in.
jenkins (martin-mrazik+qa) wrote :
review: Approve (continuous-integration)
Sam Spilsbury (smspillaz) wrote :

!(flags & (Desktop | Alpha)

That can be expressed as a temporary rather than in a compound if statement

const bool ignoreWindow = (flags & (Desktop | Alpha)

119 + /*
120 + * Alpha windows (status == false) can cover/cancel fullscreen
121 + * unredirected windows. But they can't be unredirected themselves.
122 + * I wonder if this is sensible as some games may use fullscreen
123 + * RGBA (alpha) windows that are fully opaque and need unredirect.
124 + */

This comment is confusing. We're checking whether or not the window has an rgba visual (eg, w->alpha () not whether or not it failed to paint (as status == false) would indicate to me.

Also, the default visual for most windows would be an RGB24 visual, so I don't think the concern is too much. (And we wouldn't be able to detect it anyways, as an RGBA visual just means a client is at liberty to have alpha values in its backing pixmap, which we must blend).

I'm also a bit confused as to why it matters that the window has an alpha channel as to whether or not a fullscreen window underneath it is unredirected. Shouldn't the fullscreen window underneath always be unredirected if there is some other window that is occluding it?

review: Needs Information
Sam Spilsbury (smspillaz) wrote :

> Shouldn't the fullscreen window underneath always be
> unredirected if there is some other window that is occluding it?

Correction: That should read

"Shouldn't the fullscreen window underneath always be redirected if there is some other window that is occluding it"

Daniel van Vugt (vanvugt) wrote :

!(flags & (Desktop | Alpha) is clearer, I think. Programmers think in logic, not words. We don't need to be translating clear logic into extraneous English. It complicates the thought process. Also, "ignoreWindow" is not accurate because it's not the only condition for ignoring a window.

Yeah, the comment is slightly out-dated still. It belongs next to some code that's not there any more. But technically it's still true. status is false for alpha windows (and other cases too).

Alpha "matters" for FullscreenRegion, only because we don't want to accidentally unredirect a translucent fullscreen window. You are right that alpha does not matter for the non-fullscreen windows.

lp:~vanvugt/compiz/fix-1046661 updated on 2012-09-06
3349. By Daniel van Vugt on 2012-09-06

Clarify comment per smspillaz suggestion.

Sam Spilsbury (smspillaz) wrote :

Okay, I'm good with it now after discussing.

Noted that this proposal doesn't deal with the issue of "not counting" windows that are fully-transparent or not painted at all on top of fullscreen windows. This case should be dealt with later.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/opengl/src/fsregion/fsregion.cpp'
2--- plugins/opengl/src/fsregion/fsregion.cpp 2012-09-04 10:19:19 +0000
3+++ plugins/opengl/src/fsregion/fsregion.cpp 2012-09-06 09:57:22 +0000
4@@ -35,11 +35,11 @@
5 }
6
7 bool
8-FullscreenRegion::isCoveredBy (const CompRegion &region, WinType type)
9+FullscreenRegion::isCoveredBy (const CompRegion &region, WinFlags flags)
10 {
11 bool fullscreen = false;
12
13- if (!covered && type == Normal && region == untouched)
14+ if (!covered && !(flags & (Desktop | Alpha)) && region == untouched)
15 {
16 covered = true;
17 fullscreen = true;
18
19=== modified file 'plugins/opengl/src/fsregion/fsregion.h'
20--- plugins/opengl/src/fsregion/fsregion.h 2012-09-04 10:19:19 +0000
21+++ plugins/opengl/src/fsregion/fsregion.h 2012-09-06 09:57:22 +0000
22@@ -36,14 +36,16 @@
23 public:
24 typedef enum
25 {
26- Normal,
27- Desktop
28- } WinType;
29+ Desktop = 1,
30+ Alpha = 2
31+ } WinFlag;
32+
33+ typedef unsigned int WinFlags;
34
35 FullscreenRegion (const CompRect &rect);
36
37 // isCoveredBy is called for windows from TOP to BOTTOM
38- bool isCoveredBy (const CompRegion &region, WinType type = Normal);
39+ bool isCoveredBy (const CompRegion &region, WinFlags flags = 0);
40
41 private:
42 bool covered;
43
44=== modified file 'plugins/opengl/src/fsregion/tests/test-fsregion.cpp'
45--- plugins/opengl/src/fsregion/tests/test-fsregion.cpp 2012-09-04 09:51:05 +0000
46+++ plugins/opengl/src/fsregion/tests/test-fsregion.cpp 2012-09-06 09:57:22 +0000
47@@ -49,6 +49,25 @@
48 EXPECT_TRUE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768)));
49 }
50
51+TEST (OpenGLFullscreenRegion, AlphaFullscreen)
52+{
53+ FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
54+ EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
55+ FullscreenRegion::Alpha));
56+ EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
57+ FullscreenRegion::Desktop));
58+}
59+
60+TEST (OpenGLFullscreenRegion, AlphaOverFullscreen)
61+{
62+ FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
63+ EXPECT_FALSE (monitor.isCoveredBy (CompRegion (50, 60, 70, 80),
64+ FullscreenRegion::Alpha));
65+ EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768)));
66+ EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
67+ FullscreenRegion::Desktop));
68+}
69+
70 TEST (OpenGLFullscreenRegion, NormalWindows)
71 {
72 FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
73
74=== modified file 'plugins/opengl/src/paint.cpp'
75--- plugins/opengl/src/paint.cpp 2012-09-05 10:20:20 +0000
76+++ plugins/opengl/src/paint.cpp 2012-09-06 09:57:22 +0000
77@@ -337,36 +337,40 @@
78 }
79 else
80 tmpRegion -= w->region ();
81-
82- /* unredirect top most fullscreen windows. */
83- FullscreenRegion::WinType type =
84- w->type () & CompWindowTypeDesktopMask ?
85- FullscreenRegion::Desktop :
86- FullscreenRegion::Normal;
87-
88- if (unredirectFS &&
89- !(mask & PAINT_SCREEN_TRANSFORMED_MASK) &&
90- fs.isCoveredBy (w->region (), type))
91- {
92- fullscreenWindow = w;
93- }
94- else
95- {
96- CompositeWindow *cw = CompositeWindow::get (w);
97- if (!cw->redirected ())
98- {
99- // 1. GLWindow::release to force gw->priv->needsRebind
100- gw->release ();
101-
102- // 2. GLWindow::bind, which redirects the window,
103- // rebinds the pixmap, and then rebinds the pixmap
104- // to a texture.
105- gw->bind ();
106-
107- // 3. Your window is now redirected again with the
108- // latest pixmap contents.
109- }
110- }
111+ }
112+
113+ FullscreenRegion::WinFlags flags = 0;
114+ if (w->type () & CompWindowTypeDesktopMask)
115+ flags |= FullscreenRegion::Desktop;
116+ if (w->alpha ())
117+ flags |= FullscreenRegion::Alpha;
118+
119+ /*
120+ * Windows with alpha channels can partially occlude windows
121+ * beneath them and so neither should be unredirected in that case.
122+ */
123+ if (unredirectFS &&
124+ !(mask & PAINT_SCREEN_TRANSFORMED_MASK) &&
125+ fs.isCoveredBy (w->region (), flags))
126+ {
127+ fullscreenWindow = w;
128+ }
129+ else
130+ {
131+ CompositeWindow *cw = CompositeWindow::get (w);
132+ if (!cw->redirected ())
133+ {
134+ // 1. GLWindow::release to force gw->priv->needsRebind
135+ gw->release ();
136+
137+ // 2. GLWindow::bind, which redirects the window,
138+ // rebinds the pixmap, and then rebinds the pixmap
139+ // to a texture.
140+ gw->bind ();
141+
142+ // 3. Your window is now redirected again with the
143+ // latest pixmap contents.
144+ }
145 }
146 }
147 }
148
149=== modified file 'tests/manual/Unredirect.txt'
150--- tests/manual/Unredirect.txt 2012-09-05 10:20:20 +0000
151+++ tests/manual/Unredirect.txt 2012-09-06 09:57:22 +0000
152@@ -59,3 +59,19 @@
153 Every time you triggered expo mode, expo mode should have become visible and
154 you should see the video still playing in the expo view.
155
156+
157+Unredirect cancelation by alpha windows on top (LP: #1046661)
158+-------------------------------------------------------------
159+Setup:
160+#. Install Chromium or Chrome browser.
161+
162+Actions:
163+#. Open a Chrome/Chromium window.
164+#. Open a gnome-terminal window (Ctrl+Alt+T)
165+#. Move the gnome-terminal to the centre of the screen, above the Chrome
166+ window.
167+#. Click on the Chrome window and make it fullscreen (F11).
168+#. Alt+Tab to the gnome-terminal window.
169+
170+Expected Result:
171+ The gnome-terminal window should be visible on top.

Subscribers

People subscribed via source and target branches