Merge lp:~smspillaz/compiz/compiz..merge-fix-1075578-workspacenames-flickering-during-display.damage_correctly into lp:compiz/0.9.9

[Sam Spilsbury]

Damage the text area correctly. Extract the method used to determine where the text area was into a separate function and use that to determine where our damage area should be. Also fix a few errors that happened on the last frame of animation.

Damage the text area correctly. Extract the method used to determine where the text area was into a separate function and use that to determine where our damage area should be. Also fix a few errors that happened on the last frame of animation.

[Sam Spilsbury (smspillaz)]

I'll explain again what's going on here:

8 +namespace
9 +{
10 + const float TEXT_BORDER = 10.0f;
11 +}
12 +

I pulled "border" out of drawText and made it a constant. I figure it must be for the damage border around the text itself.

23 +CompPoint
24 +WSNamesScreen::getTextPlacementPosition ()
...

All I've done here is pull out the code which determines where the text actually goes on screen into a separate function. We can use it again later in both the paint and damage code, eg:

62 +void
63 +WSNamesScreen::damageTextArea ()
64 +{
65 + const CompPoint pos (getTextPlacementPosition ());
66 +

and

77 +void
78 +WSNamesScreen::drawText (const GLMatrix &matrix)
79 +{
80 + GLfloat alpha = 0.0f;
81 +
82 + /* assign y (for the lower corner!) according to the setting */
83 + const CompPoint p = getTextPlacementPosition ();

In the damage function here:

62 +void
63 +WSNamesScreen::damageTextArea ()
64 +{
65 + const CompPoint pos (getTextPlacementPosition ());
66 +
67 + /* The placement position is from the lower corner, so we
68 + * need to move it back up by height */
69 + CompRect area (pos.x () - TEXT_BORDER,
70 + pos.y () - TEXT_BORDER - textData.getHeight () ,
71 + textData.getWidth () + TEXT_BORDER * 2,
72 + textData.getHeight () + TEXT_BORDER * 2);
73 +
74 + cScreen->damageRegion (area);
75 +}

The only gotcha was that the workspacenames plugin is a little unconventional and gives the co-ords for the text area relative to its bottom-left corner as opposed to its top-left corner. So we have to subtract the height of the text area in order to get the top-left corner to pass it to damageRegion.

127 + /* Only damage when the */
128 + if (shouldDrawText ())
129 + damageTextArea ();
130 +
131 + cScreen->donePaint ();
132
133 - cScreen->donePaint ();
134 + /* Clear text data if done with fadeout */
135 + if (!timer && !timeoutHandle.active ())
136 + textData.clear ();

We don't want to be calling damageScreen or damageRegion on every frame. Its bad for power usage and performance. So, we only call it if we should be drawing text to the screen on this frame.

I also moved the call to textData.clear () from:

115 -
116 - if (!timer)
117 - textData.clear ();

to:

135 + if (!timer && !timeoutHandle.active ())
136 + textData.clear ();

In donePaint. I don't know why it was like that in this plugin, but the best practice for this is to always clean up paint-related data after animations have completed in donePaint, so that you paint at least one frame at the end as though there was no text object.

Unmerged revisions

=== modified file 'plugins/workspacenames/src/workspacenames.cpp'
--- plugins/workspacenames/src/workspacenames.cpp	2013-03-30 14:35:37 +0000
+++ plugins/workspacenames/src/workspacenames.cpp	2013-03-30 14:35:38 +0000
@@ -24,6 +24,12 @@
 
 #include "workspacenames.h"
 
+namespace
+{
+    const float TEXT_BORDER = 10.0f;
+}
+
+
 CompString
 WSNamesScreen::getCurrentWSName ()
 {
@@ -81,16 +87,14 @@
     textData.renderText (name, attrib);
 }
 
-void
-WSNamesScreen::drawText (const GLMatrix &matrix)
+CompPoint
+WSNamesScreen::getTextPlacementPosition ()
 {
-    GLfloat  alpha;
-    float    x, y, border = 10.0f;
     CompRect oe = screen->getCurrentOutputExtents ();
-
-    x = oe.centerX () - textData.getWidth () / 2;
-
-    /* assign y (for the lower corner!) according to the setting */
+    float x = oe.centerX () - textData.getWidth () / 2;
+    float y = 0;
+    const float border = TEXT_BORDER;
+
     switch (optionGetTextPlacement ())
     {
 	case WorkspacenamesOptions::TextPlacementCenteredOnScreen:
@@ -103,7 +107,7 @@
 
 		if (optionGetTextPlacement () ==
 		    WorkspacenamesOptions::TextPlacementTopOfScreen)
-    		    y = oe.y1 () + workArea.y () +
+		    y = oe.y1 () + workArea.y () +
 			(2 * border) + textData.getHeight ();
 		else
 		    y = oe.y1 () + workArea.y () +
@@ -111,16 +115,49 @@
 	    }
 	    break;
 	default:
-	    return;
+	    return CompPoint (floor (x),
+			      oe.centerY () - textData.getHeight () / 2);
 	    break;
     }
 
+    return CompPoint (floor (x), floor (y));
+}
+
+void
+WSNamesScreen::damageTextArea ()
+{
+    const CompPoint pos (getTextPlacementPosition ());
+
+    /* The placement position is from the lower corner, so we
+     * need to move it back up by height */
+    CompRect        area (pos.x () - TEXT_BORDER,
+			  pos.y () - TEXT_BORDER - textData.getHeight () ,
+			  textData.getWidth () + TEXT_BORDER * 2,
+			  textData.getHeight () + TEXT_BORDER * 2);
+
+    cScreen->damageRegion (area);
+}
+
+void
+WSNamesScreen::drawText (const GLMatrix &matrix)
+{
+    GLfloat  alpha = 0.0f;
+
+    /* assign y (for the lower corner!) according to the setting */
+    const CompPoint p = getTextPlacementPosition ();
+
     if (timer)
 	alpha = timer / (optionGetFadeTime () * 1000.0f);
-    else
+    else if (timeoutHandle.active ())
 	alpha = 1.0f;
 
-    textData.draw (matrix, floor (x), floor (y), alpha);
+    textData.draw (matrix, p.x (), p.y (), alpha);
+}
+
+bool
+WSNamesScreen::shouldDrawText ()
+{
+    return textData.getWidth () && textData.getHeight ();
 }
 
 bool
@@ -132,7 +169,7 @@
 {
     bool status = gScreen->glPaintOutput (attrib, transform, region, output, mask);
 
-    if (textData.getWidth () && textData.getHeight ())
+    if (shouldDrawText ())
     {
 	GLMatrix sTransform (transform);
 
@@ -151,9 +188,6 @@
     {
 	timer -= msSinceLastPaint;
 	timer = MAX (timer, 0);
-
-	if (!timer)
-	    textData.clear ();
     }
 
     cScreen->preparePaint (msSinceLastPaint);
@@ -162,20 +196,27 @@
 void
 WSNamesScreen::donePaint ()
 {
-    /* FIXME: better only damage paint region */
-    cScreen->damageScreen ();
+    /* Only damage when the */
+    if (shouldDrawText ())
+	damageTextArea ();
+ 
+     cScreen->donePaint ();
 
-    cScreen->donePaint ();
+    /* Clear text data if done with fadeout */
+    if (!timer && !timeoutHandle.active ())
+	textData.clear ();
 }
 
 bool
 WSNamesScreen::hideTimeout ()
 {
     timer = optionGetFadeTime () * 1000;
+
+    /* Clear immediately if there is no fadeout */
     if (!timer)
 	textData.clear ();
-
-    cScreen->damageScreen ();
+ 
+    damageTextArea ();
 
     timeoutHandle.stop ();
 
@@ -201,7 +242,7 @@
 	renderNameText ();
 	timeoutHandle.start (timeout, timeout + 200);
 
-	cScreen->damageScreen ();
+	damageTextArea ();
     }
 }
 

=== modified file 'plugins/workspacenames/src/workspacenames.h'
--- plugins/workspacenames/src/workspacenames.h	2012-06-22 12:27:42 +0000
+++ plugins/workspacenames/src/workspacenames.h	2013-03-30 14:35:38 +0000
@@ -79,6 +79,16 @@
 
 	void
 	handleEvent (XEvent *);
+
+	CompPoint
+	getTextPlacementPosition ();
+
+	void
+	damageTextArea ();
+
+    private:
+
+	bool shouldDrawText ();
 };
 
 class WorkspacenamesPluginVTable :