Merge lp:~vanvugt/unity/fix-1021541 into lp:unity

Proposed by Daniel van Vugt
Status: Merged
Approved by: Łukasz Zemczak
Approved revision: no longer in the source branch.
Merged at revision: 2473
Proposed branch: lp:~vanvugt/unity/fix-1021541
Merge into: lp:unity
Diff against target: 160 lines (+37/-33)
3 files modified
manual-tests/Dash.txt (+15/-0)
plugins/unityshell/src/unityshell.cpp (+20/-32)
plugins/unityshell/src/unityshell.h (+2/-1)
To merge this branch: bzr merge lp:~vanvugt/unity/fix-1021541
Reviewer Review Type Date Requested Status
Łukasz Zemczak Approve
Review via email: mp+113685@code.launchpad.net

Commit message

Ensure the unity shell gets drawn before (below) windows stacked above it,
like DnD icons (LP: #1021541)

Description of the change

Ensure the unity shell gets drawn before (below) windows stacked above it,
like DnD icons (LP: #1021541)

UNBLOCK

To post a comment you must log in.
Revision history for this message
Sam Spilsbury (smspillaz) wrote :

This looks okay.

If you're having trouble with override redirect windows not visually obscuring the dash and launcher, but logically doing so and thus causing the dash and launcher to flicker, I'd suggest using CompWindow::region () instead of CompWindow::geometry (), as the former will include the XShape region.

Either that or don't include override redirect windows in the calculation when the cursor is grabbed.

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

A second note here, while it is not appropriate to enforce autotesting at this point since we're unblocking a release, however I'd strongly suggest using automated unit and integration testing as a means to test this code in future, or at least make it a priority to get it covered by autotest. People running trunk don't manually go through all the manual tests and the feedback loop that we have on the manual tests is quite (this is a fact of life).

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

Oh wow, language fail, "while ..., I'd" ...

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

Code looks fine and tested branch fixes the issue, +1

review: Approve
Revision history for this message
Unity Merger (unity-merger) wrote :

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-unity/898/console reported an error when processing this lp:~vanvugt/unity/fix-1021541 branch.
Not merging it.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'manual-tests/Dash.txt'
--- manual-tests/Dash.txt 2012-06-21 06:55:53 +0000
+++ manual-tests/Dash.txt 2012-07-06 05:55:25 +0000
@@ -204,6 +204,21 @@
204Outcome:204Outcome:
205 The Dash should close and the music file should open up in Firefox.205 The Dash should close and the music file should open up in Firefox.
206206
207
208DnD stacking
209------------
210
211Setup:
212#. Open the Dash (Super).
213
214Action:
215#. Drag any icon from the dash and wave it over the dash and launcher area.
216
217Outcome:
218 The icon being dragged is drawn on top of the dash and launcher. Never
219 below it.
220
221
207Lens Tests222Lens Tests
208============223============
209224
210225
=== modified file 'plugins/unityshell/src/unityshell.cpp'
--- plugins/unityshell/src/unityshell.cpp 2012-07-04 02:37:23 +0000
+++ plugins/unityshell/src/unityshell.cpp 2012-07-06 05:55:25 +0000
@@ -916,7 +916,8 @@
916bool UnityScreen::forcePaintOnTop ()916bool UnityScreen::forcePaintOnTop ()
917{917{
918 return !allowWindowPaint ||918 return !allowWindowPaint ||
919 ((switcher_controller_->Visible() || launcher_controller_->IsOverlayOpen())919 ((switcher_controller_->Visible() ||
920 PluginAdapter::Default()->IsExpoActive())
920 && !fullscreen_windows_.empty () && (!(screen->grabbed () && !screen->otherGrabExist (NULL))));921 && !fullscreen_windows_.empty () && (!(screen->grabbed () && !screen->otherGrabExist (NULL))));
921}922}
922923
@@ -1241,14 +1242,14 @@
1241#endif1242#endif
12421243
1243 // CompRegion has no clear() method. So this is the fastest alternative.1244 // CompRegion has no clear() method. So this is the fastest alternative.
1244 aboveShell = CompRegion();1245 fullscreenRegion = CompRegion();
1245 nuxRegion = CompRegion();1246 nuxRegion = CompRegion();
12461247
1247 /* glPaintOutput is part of the opengl plugin, so we need the GLScreen base class. */1248 /* glPaintOutput is part of the opengl plugin, so we need the GLScreen base class. */
1248 ret = gScreen->glPaintOutput(attrib, transform, region, output, mask);1249 ret = gScreen->glPaintOutput(attrib, transform, region, output, mask);
12491250
1250#ifndef USE_MODERN_COMPIZ_GL1251#ifndef USE_MODERN_COMPIZ_GL
1251 if (doShellRepaint && !force && aboveShell.contains(*output))1252 if (doShellRepaint && !force && fullscreenRegion.contains(*output))
1252 doShellRepaint = false;1253 doShellRepaint = false;
12531254
1254 if (doShellRepaint)1255 if (doShellRepaint)
@@ -1313,6 +1314,7 @@
1313 compizDamageNux(cScreen->currentDamage());1314 compizDamageNux(cScreen->currentDamage());
13141315
1315 didShellRepaint = false;1316 didShellRepaint = false;
1317 firstWindowAboveShell = NULL;
1316}1318}
13171319
1318void UnityScreen::donePaint()1320void UnityScreen::donePaint()
@@ -2286,13 +2288,17 @@
2286 /*2288 /*
2287 * The occlusion pass tests windows from TOP to BOTTOM. That's opposite to2289 * The occlusion pass tests windows from TOP to BOTTOM. That's opposite to
2288 * the actual painting loop.2290 * the actual painting loop.
2291 *
2292 * Detect uScreen->fullscreenRegion here. That represents the region which
2293 * fully covers the shell on its output. It does not include regular windows
2294 * stacked above the shell like DnD icons or Onboard etc.
2289 */2295 */
2290 if (isNuxWindow(window))2296 if (isNuxWindow(window))
2291 {2297 {
2292 if (mask & PAINT_WINDOW_OCCLUSION_DETECTION_MASK)2298 if (mask & PAINT_WINDOW_OCCLUSION_DETECTION_MASK)
2293 {2299 {
2294 uScreen->nuxRegion += window->geometry();2300 uScreen->nuxRegion += window->geometry();
2295 uScreen->nuxRegion -= uScreen->aboveShell;2301 uScreen->nuxRegion -= uScreen->fullscreenRegion;
2296 }2302 }
2297 return false; // Ensure nux windows are never painted by compiz2303 return false; // Ensure nux windows are never painted by compiz
2298 }2304 }
@@ -2302,14 +2308,17 @@
2302 PAINT_WINDOW_TRANSLUCENT_MASK |2308 PAINT_WINDOW_TRANSLUCENT_MASK |
2303 PAINT_WINDOW_TRANSFORMED_MASK |2309 PAINT_WINDOW_TRANSFORMED_MASK |
2304 PAINT_WINDOW_NO_CORE_INSTANCE_MASK;2310 PAINT_WINDOW_NO_CORE_INSTANCE_MASK;
2305 if (!(mask & nonOcclusionBits))2311 if (!(mask & nonOcclusionBits) &&
2312 (window->state() & CompWindowStateFullscreenMask))
2306 // And I've been advised to test other things, but they don't work:2313 // And I've been advised to test other things, but they don't work:
2307 // && (attrib.opacity == OPAQUE)) <-- Doesn't work; Only set in glDraw2314 // && (attrib.opacity == OPAQUE)) <-- Doesn't work; Only set in glDraw
2308 // && !window->alpha() <-- Doesn't work; Opaque windows often have alpha2315 // && !window->alpha() <-- Doesn't work; Opaque windows often have alpha
2309 {2316 {
2310 uScreen->aboveShell += window->geometry();2317 uScreen->fullscreenRegion += window->geometry();
2311 uScreen->aboveShell -= uScreen->nuxRegion;2318 uScreen->fullscreenRegion -= uScreen->nuxRegion;
2312 }2319 }
2320 if (uScreen->nuxRegion.isEmpty())
2321 uScreen->firstWindowAboveShell = window;
2313 }2322 }
23142323
2315 GLWindowPaintAttrib wAttrib = attrib;2324 GLWindowPaintAttrib wAttrib = attrib;
@@ -2370,38 +2379,17 @@
2370 }2379 }
2371 }2380 }
23722381
2373 /*
2374 * Paint the shell in *roughly* the compiz stacking order. This is only
2375 * approximate because we're painting all the nux windows as soon as we find
2376 * the bottom-most nux window (from bottom to top).
2377 * But remember to avoid painting the shell if it's within the aboveShell
2378 * region.
2379 */
2380 if (uScreen->doShellRepaint &&2382 if (uScreen->doShellRepaint &&
2381 !uScreen->forcePaintOnTop () &&2383 !uScreen->forcePaintOnTop () &&
2382 !uScreen->aboveShell.contains(window->geometry())2384 window == uScreen->firstWindowAboveShell &&
2385 !uScreen->fullscreenRegion.contains(window->geometry())
2383 )2386 )
2384 {2387 {
2385 std::vector<Window> const& xwns = nux::XInputWindow::NativeHandleList();
2386 unsigned int size = xwns.size();
2387
2388 for (CompWindow* w = window; w && uScreen->doShellRepaint; w = w->prev)
2389 {
2390 auto id = w->id();
2391
2392 for (unsigned int i = 0; i < size; ++i)
2393 {
2394 if (xwns[i] == id)
2395 {
2396#ifdef USE_MODERN_COMPIZ_GL2388#ifdef USE_MODERN_COMPIZ_GL
2397 uScreen->paintDisplay();2389 uScreen->paintDisplay();
2398#else2390#else
2399 uScreen->paintDisplay(region, matrix, mask);2391 uScreen->paintDisplay(region, matrix, mask);
2400#endif2392#endif
2401 break;
2402 }
2403 }
2404 }
2405 }2393 }
24062394
2407 if (window->type() == CompWindowTypeDesktopMask)2395 if (window->type() == CompWindowTypeDesktopMask)
24082396
=== modified file 'plugins/unityshell/src/unityshell.h'
--- plugins/unityshell/src/unityshell.h 2012-07-04 02:37:23 +0000
+++ plugins/unityshell/src/unityshell.h 2012-07-06 05:55:25 +0000
@@ -283,7 +283,8 @@
283 CompOutput* _last_output;283 CompOutput* _last_output;
284284
285 CompRegion nuxRegion;285 CompRegion nuxRegion;
286 CompRegion aboveShell;286 CompRegion fullscreenRegion;
287 CompWindow* firstWindowAboveShell;
287288
288 nux::Property<nux::Geometry> primary_monitor_;289 nux::Property<nux::Geometry> primary_monitor_;
289290