Merge ~kondor-dani/compiz:annotate_fix2 into compiz:master

[Daniel Kondor]

Make annotate plugin D-Bus interface working, and clean up code.

Second iteration of fix for bug #1878545

Changes moved to separate commits.

Cause of bug (in plugins/annotate/src/annotate.cpp):
 -- "tool" variable in AnnoScreen::draw() gets overwritten between lines 305 and 322 (is it undefined behavior to save the c_str() of the value returned by CompOption::getStringOptionNamed()?)
 -- AnnoScreen::draw() needs to call handleEventSetEnabled() and glPaintOutputSetEnabled() to actually draw anything on the screen

Changes:
 -- store tool in a local std::string
 -- add missing calls to ensure result is drawn on the screen

[Dmitry Shachnev (mitya57)]

I am setting it to Needs Information per Alberts' review.

review: Needs Information

[Alberts Muktupāvels (muktupavels)]

Your example - "dbus-send --print-reply --type=method_call --dest=org.freedesktop.compiz /org/freedesktop/compiz/annotate/screen0/draw org.freedesktop.compiz.activate string:tool" looks incomplete or wrong. From dbus plugin code - "Activate can be used to trigger any existing action. Arguments should be a pair of { string, bool|int32|double|string }."

[Daniel Kondor (kondor-dani)]

The example works (on this branch), since the draw() function fills in everything with default values.

Full example would be e.g.:

dbus-send --print-reply --type=method_call --dest=org.freedesktop.compiz /org/freedesktop/compiz/annotate/screen0/draw org.freedesktop.compiz.activate string:root int32:416 string:tool string:line string:x1 double:100.0 string:y1 double:200.0 string:x2 double:200.0 string:y2 double:300.0 string:fill_color string:"#ff00ffff"

[Alberts Muktupāvels (muktupavels)]

Please use separate commits for separate problems

One for enabling painting - that seems main problem why nothing are drawn on screen, right?

Then fix tool problem as next commit. Looks like draw function has one more similar problem - in text drawing case.

And please remove unneeded empty line removal.

[Daniel Kondor (kondor-dani)]

> Please use separate commits for separate problems
>
> One for enabling painting - that seems main problem why nothing are drawn on
> screen, right?
draw() does nothing without fixing the issue with tool.

I'll create separate commits for these.

>
> Then fix tool problem as next commit. Looks like draw function has one more
> similar problem - in text drawing case.
Indeed, I haven't tested this.

E.g. calling:

dbus-send --print-reply --type=method_call --dest=org.freedesktop.compiz /org/freedesktop/compiz/annotate/screen0/draw org.freedesktop.compiz.activate string:tool string:text string:text string:aaaa string:x double:100.0 string:y double:100.0

prints "Sans" on the screen instead of "aaaa", which is given as a parameter. I'll add a fix for this as well.

>
> And please remove unneeded empty line removal.

[Alberts Muktupāvels (muktupavels)]

- Please next time just force push to existing merge request. :)

- Looks like you forgot cython3 change, just use separate commit for it.

- Could you check if adding cScreen->damageRegion would be enough? I still think that handleEventSetEnabled should not be needed.

Thanks!

[Dmitry Shachnev (mitya57)]

> - Looks like you forgot cython3 change, just use separate commit for it.

A separate merge proposal for cython3 change will be even better, as this will add a separate changelog entry for it.

[Daniel Kondor (kondor-dani)]

> - Could you check if adding cScreen->damageRegion would be enough? I still
> think that handleEventSetEnabled should not be needed.

Yes, it works that way as well! I've added a commit -- note the extra logic needed to calculate the region to pass. I've also noticed that these extents are already calculated by all the draw*() functions and seem to be not used later.

[Daniel Kondor (kondor-dani)]

I've added one more commit removing the code that seems to be unused in the draw*() functions.

[Dmitry Shachnev (mitya57)]

Have you seen my inline comment about simplifying the comparisons?

[Daniel Kondor (kondor-dani)]

Yes, sure, I can easily add a commit for that, but I'm concerned since this will change the behavior from case-insensitve to case-sensitive comparison (the point of strcasecmp() is that it's case-insensitve).

Based on the fact that no one reported this bug for years, my guess is that it's very unlikely that any code relies on this behavior :) -- still it might be better to have a separate merge for that. Let me know what you think.

[Dmitry Shachnev (mitya57)]

Ok, looks good to me now.

review: Approve

[Alberts Muktupāvels (muktupavels)]

- Please prefix commit message title with annotate:.

- "need to call handleEventSetEnabled() to draw on screen" and "use damageRegion() instead of handleEventSetEnabled() in draw()" should be one commit.

- See diff comments.

[Daniel Kondor (kondor-dani)]

Ok, I'm trying to fix remaining issues.

Also, I've tried squashing the previous commits, but the result doesn't look right :( -- I'll try to do it again.

[Daniel Kondor (kondor-dani)]

I've fixed the git history and added commits based on the review comments.

[Dmitry Shachnev (mitya57)]

> revert change of return type of AnnoScreen::drawText()

Maybe you can squash this into the commit that changed the return type?

Otherwise, looks good to me. Alberts, what do you think?

[Alberts Muktupāvels (muktupavels)]

There are indentation problems that would be nice to fix. See inline/diff comments.

Also please prefix commit messages with "annotate:". You can use `git rebase -i a9d598a` to do that. Choose r or reword to edit commit messages.

Otherwise looks good. Thanks!

[Daniel Kondor (kondor-dani)]

Ok, I think I was able to fix the indentation -- I didn't realize before that arguments are supposed to line up with the opening parentheses :)

Let me know if you see any other issue.

[Alberts Muktupāvels (muktupavels)]

Thanks!

review: Approve

diff --git a/plugins/annotate/src/annotate.cpp b/plugins/annotate/src/annotate.cpp
index f415d6c..4f8285f 100644
--- a/plugins/annotate/src/annotate.cpp
+++ b/plugins/annotate/src/annotate.cpp
@@ -169,13 +169,10 @@ AnnoScreen::drawRectangle (double         x,
 
     if (cr)
     {
-	double  ex1, ey1, ex2, ey2;
-
 	setSourceColor (cr, fillColor);
 	cairo_rectangle (cr, x, y, w, h);
 	cairo_fill_preserve (cr);
 	cairo_set_line_width (cr, strokeWidth);
-	cairo_stroke_extents (cr, &ex1, &ey1, &ex2, &ey2);
 	setSourceColor (cr, strokeColor);
 	cairo_stroke (cr);
 
@@ -195,12 +192,9 @@ AnnoScreen::drawLine (double         x1,
 
     if (cr)
     {
-	double ex1, ey1, ex2, ey2;
-
 	cairo_set_line_width (cr, width);
 	cairo_move_to (cr, x1, y1);
 	cairo_line_to (cr, x2, y2);
-	cairo_stroke_extents (cr, &ex1, &ey1, &ex2, &ey2);
 	cairo_set_operator (cr, CAIRO_OPERATOR_SOURCE);
 	setSourceColor (cr, color);
 	cairo_stroke (cr);
@@ -219,9 +213,9 @@ AnnoScreen::drawText (double              x,
 		      cairo_font_weight_t fontWeight,
 		      unsigned short      *fillColor,
 		      unsigned short      *strokeColor,
-		      double              strokeWidth)
+		      double              strokeWidth,
+		      CompRect&           damageRect)
 {
-    REGION reg;
     cairo_t *cr = cairoContext ();
 
     if (cr)
@@ -240,14 +234,10 @@ AnnoScreen::drawText (double              x,
 	setSourceColor (cr, strokeColor);
 	cairo_stroke (cr);
 	cairo_restore (cr);
-
-	reg.rects    = &reg.extents;
-	reg.numRects = 1;
-
-	reg.extents.x1 = x;
-	reg.extents.y1 = y + extents.y_bearing - 2.0;
-	reg.extents.x2 = x + extents.width + 20.0;
-	reg.extents.y2 = y + extents.height;
+	
+	damageRect.setGeometry (x, y + extents.y_bearing - 2.0,
+				extents.width + 20.0,
+				extents.height - extents.y_bearing + 2.0);
 
 	content = true;
     }
@@ -299,10 +289,11 @@ AnnoScreen::draw (CompAction         *action,
 
     if (cr)
     {
-	const char	*tool;
+	CompString	tool;
 	unsigned short	*fillColor, *strokeColor;
+	CompRect	damageRect;
 
-	tool = CompOption::getStringOptionNamed (options, "tool", "line").c_str ();
+	tool = CompOption::getStringOptionNamed (options, "tool", "line");
 
 	cairo_set_operator (cr, CAIRO_OPERATOR_OVER);
 	cairo_set_line_cap (cr, CAIRO_LINE_CAP_ROUND);
@@ -319,7 +310,7 @@ AnnoScreen::draw (CompAction         *action,
 	strokeWidth = CompOption::getFloatOptionNamed (options, "stroke_width",
 						       strokeWidth);
 
-	if (strcasecmp (tool, "rectangle") == 0)
+	if (strcasecmp (tool.c_str (), "rectangle") == 0)
 	{
 	    double x = CompOption::getFloatOptionNamed (options, "x", 0);
 	    double y = CompOption::getFloatOptionNamed (options, "y", 0);
@@ -328,8 +319,10 @@ AnnoScreen::draw (CompAction         *action,
 
 	    drawRectangle (x, y, w, h, fillColor, strokeColor,
 			   strokeWidth);
+	    damageRect.setGeometry (x, y, w, h);
+	    
 	}
-	else if (strcasecmp (tool, "ellipse") == 0)
+	else if (strcasecmp (tool.c_str (), "ellipse") == 0)
 	{
 	    double xc = CompOption::getFloatOptionNamed (options, "xc", 0);
 	    double yc = CompOption::getFloatOptionNamed (options, "yc", 0);
@@ -338,35 +331,38 @@ AnnoScreen::draw (CompAction         *action,
 
 	    drawEllipse (xc, yc, xr, yr, fillColor, strokeColor,
 			 strokeWidth);
+	    damageRect.setGeometry (xc - xr, yc - yr, 2 * xr, 2 * yr);
 	}
-	else if (strcasecmp (tool, "line") == 0)
+	else if (strcasecmp (tool.c_str (), "line") == 0)
 	{
 	    double x1 = CompOption::getFloatOptionNamed (options, "x1", 0);
 	    double y1 = CompOption::getFloatOptionNamed (options, "y1", 0);
 	    double x2 = CompOption::getFloatOptionNamed (options, "x2", 100);
 	    double y2 = CompOption::getFloatOptionNamed (options, "y2", 100);
 
-	    drawLine (x1, y1, x2, y2, strokeWidth, fillColor);
+	    drawLine (x1, y1, x2, y2, strokeWidth, strokeColor);
+	    damageRect.setGeometry (MIN (x1, x2), MIN (y1, y2),
+				    abs (x1 - x2), abs (y1 - y2) );
 	}
-	else if (strcasecmp (tool, "text") == 0)
+	else if (strcasecmp (tool.c_str (), "text") == 0)
 	{
-	    const char          *text, *family;
+	    CompString          text, family;
 	    cairo_font_slant_t  slant;
 	    cairo_font_weight_t weight;
-	    const char          *str;
+	    CompString          str;
 
-	    str = CompOption::getStringOptionNamed (options, "slant", "").c_str ();
+	    str = CompOption::getStringOptionNamed (options, "slant", "");
 
-	    if (strcasecmp (str, "oblique") == 0)
+	    if (strcasecmp (str.c_str (), "oblique") == 0)
 		slant = CAIRO_FONT_SLANT_OBLIQUE;
-	    else if (strcasecmp (str, "italic") == 0)
+	    else if (strcasecmp (str.c_str (), "italic") == 0)
 		slant = CAIRO_FONT_SLANT_ITALIC;
 	    else
 		slant = CAIRO_FONT_SLANT_NORMAL;
 
-	    str = CompOption::getStringOptionNamed (options, "weight", "").c_str ();
+	    str = CompOption::getStringOptionNamed (options, "weight", "");
 
-	    if (strcasecmp (str, "bold") == 0)
+	    if (strcasecmp (str.c_str (), "bold") == 0)
 		weight = CAIRO_FONT_WEIGHT_BOLD;
 	    else
 		weight = CAIRO_FONT_WEIGHT_NORMAL;
@@ -374,16 +370,27 @@ AnnoScreen::draw (CompAction         *action,
 	    double x = CompOption::getFloatOptionNamed (options, "x", 0);
 	    double y = CompOption::getFloatOptionNamed (options, "y", 0);
 
-	    text = CompOption::getStringOptionNamed (options, "text", "").c_str ();
+	    text = CompOption::getStringOptionNamed (options, "text", "");
 
 	    family = CompOption::getStringOptionNamed (options, "family",
-						       "Sans").c_str ();
+						       "Sans");
 
 	    double size = CompOption::getFloatOptionNamed (options, "size", 36.0);
 
-	    drawText (x, y, text, family, size, slant, weight,
-		      fillColor, strokeColor, strokeWidth);
+	    drawText (x, y, text.c_str (), family.c_str (), size, slant,
+		      weight, fillColor, strokeColor, strokeWidth, damageRect);
 	}
+	else
+	    return true;
+	
+	/* Add border width to the damage region */
+	damageRect.setGeometry (damageRect.x () - (strokeWidth / 2),
+				damageRect.y () - (strokeWidth / 2),
+				damageRect.width () + strokeWidth + 1,
+				damageRect.height () + strokeWidth + 1);
+	
+	cScreen->damageRegion (damageRect);
+	gScreen->glPaintOutputSetEnabled (this, true);
     }
 
     return true;
diff --git a/plugins/annotate/src/annotate.h b/plugins/annotate/src/annotate.h
index 0df450c..273557a 100644
--- a/plugins/annotate/src/annotate.h
+++ b/plugins/annotate/src/annotate.h
@@ -136,7 +136,8 @@ class AnnoScreen :
 		  cairo_font_weight_t fontWeight,
 		  unsigned short      *fillColor,
 		  unsigned short      *strokeColor,
-		  double              strokeWidth);
+		  double              strokeWidth,
+		  CompRect&           damageRect);
 
         /* Actions */