[Poppler-bugs] [Bug 70983] Add implementation of Square and Circle annotations

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=70983

--- Comment #2 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 88270
  --> https://bugs.freedesktop.org/attachment.cgi?id=88270
glib: Add implementation of Square and Circle annotations

Review of attachment 88270:
-----------------------------------------------------------------

::: glib/poppler-annot.cc
@@ +118,4 @@
>    PopplerAnnotMarkupClass parent_class;
>  };
>  
> +struct _PopplerAnnotGeometry

I think we should expose circle and square as different objects, the spec
doesn't mention Geometry annots at all, it's an internal class used for the
implementation. We could use a base class for them for the common API
(everything except the constructors), but I'm not sure geometry is the best
name, a polygon is also a geometric shape, but polygon annotations don't
inherit from geometry annotations (same for lines, etc.). Since the API is
quite simple and the common implementation is in the core in the end, maybe we
can just duplicate the API for circle and square. What do you think?

@@ +406,5 @@
> +{
> +}
> +
> +/**
> +* poppler_annot_geometry_new_circle:

This line is not correctly indented.

@@ +418,5 @@
> +* Creates a new empty #PopplerAnnotGeometry circle.
> +*
> +* Return value: a newly #PopplerAnnotGeometry annotation
> +*
> +* Since: 0.25

0.26

@@ +433,5 @@
> +  border = new AnnotBorderArray ();
> +  border->setWidth (1.5);
> +
> +  annot = new AnnotGeometry (doc->doc, &pdf_rect, Annot::typeCircle);
> +  annot->setBorder (border);

Why do we set a border by default? The BS entry is optional.

@@ +449,5 @@
> + * poppler_page_add_annot()
> + *
> + * Return value: a newly #PopplerAnnotGeometry annotation
> + *
> + * Since: 0.25

0.26

@@ +464,5 @@
> +  border = new AnnotBorderArray ();
> +  border->setWidth (1.5);
> +
> +  annot = new AnnotGeometry (doc->doc, &pdf_rect, Annot::typeSquare);
> +  annot->setBorder (border);

Same comments here about the border.

@@ +1718,5 @@
> + *
> + * Return value: a new allocated #PopplerColor with the color values of
> + *               @poppler_annot, or %NULL. It must be freed with g_free() when done.
> + *
> + * Since: 0.25

0.26

@@ +1752,5 @@
> +        poppler_color->red = (guint16) (values[0] * 65535);
> +        poppler_color->green = (guint16) (values[1] * 65535);
> +        poppler_color->blue = (guint16) (values[2] * 65535);
> +
> +	break;

Do not copy paste all this code, factor it out in a helper method.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/poppler-bugs/attachments/20131103/5b490bd3/attachment.html>