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

[MC Return]

Workspacenames:

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.

This also fixes the flickering during display.

Other minor code refactoring.

(LP: #1075578, LP: #1162246)

Note:
Sophisticated damage handling is now implemented and fully working.
Credits and thanks for this work go to Sam !
Once again a *top job*.

This is fully tested and works perfectly :)

Now this plugin is ready for daily use.

[Daniel van Vugt (vanvugt)]

I think we should fix the FIXME. damageScreen is too expensive in some cases, and always when unityshell is loaded.

Please try using damageRegion instead. Similar to:
https://code.launchpad.net/~vanvugt/compiz/fix-1068518/+merge/131552

review: Needs Fixing

[MC Return (mc-return)]

> I think we should fix the FIXME. damageScreen is too expensive in some cases,
> and always when unityshell is loaded.
>

I agree. Using the show repaint plug-in even shows that visually. This should be improved indeed.

> Please try using damageRegion instead. Similar to:
> https://code.launchpad.net/~vanvugt/compiz/fix-1068518/+merge/131552

I will try to do it.

[MC Return (mc-return)]

Somebody (Sam ? ;)) needs to help me with the implementation of the damage rectangle handling as I have no idea how to correctly implement it.

Nevertheless I would suggest to merge this branch for now as it fixes the flickering during display of the workspacename reliably.
Sophisticated damage handling can be implemented at a later stage as well, fixing the bug, every user of workspace naming experiences currently, should have priority.

[Sam Spilsbury (smspillaz)]

> Sophisticated damage rectangle calculation and thus the FIXME is still missing.

Huh, surely it can't be that difficult.

Talk to me on IRC about it later today.

[MC Return (mc-return)]

> > Sophisticated damage rectangle calculation and thus the FIXME is still
> missing.
>
> Huh, surely it can't be that difficult.
>
For me it still is ;)

Sam, please take over this branch and implement the correct damage rectangle handling.
I will study your .diff then and try to learn from that ;)

[Sam Spilsbury (smspillaz)]

I'll have a look into this as soon as I get time to. I'm pretty busy
with uni and stuff at the moment.

[MC Return (mc-return)]

> I'll have a look into this as soon as I get time to. I'm pretty busy
> with uni and stuff at the moment.

Thanks :)
No stress with that, but IMHO this MP can also be merged *as is* and the TODO
can also be implemented at a later stage...
This MP reliably fixes bug # #1075578, no more flickering.

[Sam Spilsbury (smspillaz)]

> > I'll have a look into this as soon as I get time to. I'm pretty busy
> > with uni and stuff at the moment.
>
> Thanks :)
> No stress with that, but IMHO this MP can also be merged *as is* and the TODO
> can also be implemented at a later stage...
> This MP reliably fixes bug # #1075578, no more flickering.

Well, not quite. It does fix the bug yes, but the whole point of patch review is to ensure that we don't put things into the codebase that we know are bad. In this case, damageScreen on every frame (note: it really is on *every* *frame* regardless of whether the text box is active or not) is a pretty bad thing to go into the code, and currently causes other plugins using the damage region to go into serious overdrive.

So I think this one is at least worth looking into.

[MC Return (mc-return)]

> > > I'll have a look into this as soon as I get time to. I'm pretty busy
> > > with uni and stuff at the moment.
> >
> > Thanks :)
> > No stress with that, but IMHO this MP can also be merged *as is* and the
> TODO
> > can also be implemented at a later stage...
> > This MP reliably fixes bug # #1075578, no more flickering.
>
> Well, not quite. It does fix the bug yes, but the whole point of patch review
> is to ensure that we don't put things into the codebase that we know are bad.
> In this case, damageScreen on every frame (note: it really is on *every*
> *frame* regardless of whether the text box is active or not) is a pretty bad
> thing to go into the code, and currently causes other plugins using the damage
> region to go into serious overdrive.
>
> So I think this one is at least worth looking into.

Urgh, you are right. :( I am sorry - I did not notice it was that bad (reproduced now with showrepaint).
I 100% agree that it is worth looking into (and I tried)...

review: Needs Fixing

[MC Return (mc-return)]

Sam, I've merged your branch into this one here now.
Thanks a lot (again) :)

I can confirm it works perfectly (have tested it with showrepaint).
The border could even be smaller I guess, as the damage rectangle
is bigger than actually needed, but that is a minor thingy ;)...

Or do you want me to fix that also ?

review: Approve

[MC Return (mc-return)]

A minor problem still remains:

It seems the text is not fading in anymore (just out).
Investigating...

review: Needs Fixing

[MC Return (mc-return)]

Nope - never was fading in -> Sorry ;)

review: Approve

[MC Return (mc-return)]

One minor improvement maybe:

We could change

+ const float TEXT_BORDER = 10.0f;

to

+ const unsigned short TEXT_BORDER = 2;

Did that in r3469.

[Sam Spilsbury (smspillaz)]

The change to TEXT_BORDER is fine.

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'plugins/workspacenames/src/workspacenames.cpp'
--- plugins/workspacenames/src/workspacenames.cpp	2012-12-04 15:06:12 +0000
+++ plugins/workspacenames/src/workspacenames.cpp	2013-03-30 17:00:28 +0000
@@ -24,21 +24,23 @@
 
 #include "workspacenames.h"
 
+namespace
+{
+    const unsigned short TEXT_BORDER = 2;
+}
+
+
 CompString
 WSNamesScreen::getCurrentWSName ()
 {
-    int		currentVp;
-    int		listSize;
     CompString	ret;
-    CompOption::Value::Vector names;
-    CompOption::Value::Vector vpNumbers;
-
-    vpNumbers = optionGetViewports ();
-    names     = optionGetNames ();
-
-    currentVp = screen->vp ().y () * screen->vpSize ().width () +
+
+    CompOption::Value::Vector vpNumbers = optionGetViewports ();
+    CompOption::Value::Vector names     = optionGetNames ();
+
+    int currentVp = screen->vp ().y () * screen->vpSize ().width () +
 		screen->vp ().x () + 1;
-    listSize  = MIN (vpNumbers.size (), names.size ());
+    int listSize  = MIN (vpNumbers.size (), names.size ());
 
     for (int i = 0; i < listSize; i++)
 	if (vpNumbers[i].i () == currentVp)
@@ -51,11 +53,10 @@
 WSNamesScreen::renderNameText ()
 {
     CompText::Attrib attrib;
-    CompString	     name;
 
     textData.clear ();
 
-    name = getCurrentWSName ();
+    CompString name = getCurrentWSName ();
 
     if (name.empty ())
 	return;
@@ -86,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:
@@ -108,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 () +
@@ -116,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
@@ -135,11 +167,9 @@
 			      CompOutput		*output,
 			      unsigned int		mask)
 {
-    bool status;
-
-    status = gScreen->glPaintOutput (attrib, transform, region, output, mask);
-
-    if (textData.getWidth () && textData.getHeight ())
+    bool status = gScreen->glPaintOutput (attrib, transform, region, output, mask);
+
+    if (shouldDrawText ())
     {
 	GLMatrix sTransform (transform);
 
@@ -158,9 +188,6 @@
     {
 	timer -= msSinceLastPaint;
 	timer = MAX (timer, 0);
-
-	if (!timer)
-	    textData.clear ();
     }
 
     cScreen->preparePaint (msSinceLastPaint);
@@ -169,21 +196,27 @@
 void
 WSNamesScreen::donePaint ()
 {
-    /* FIXME: better only damage paint region */
-    if (timer)
-	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 ();
 
@@ -209,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 17:00:28 +0000
@@ -79,6 +79,16 @@
 
 	void
 	handleEvent (XEvent *);
+
+	CompPoint
+	getTextPlacementPosition ();
+
+	void
+	damageTextArea ();
+
+    private:
+
+	bool shouldDrawText ();
 };
 
 class WorkspacenamesPluginVTable :