[Poppler-bugs] [Bug 70981] glib-demo: Add support for simple line annotations

[bugzilla-daemon at freedesktop.org]

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

--- Comment #2 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 88267
  --> https://bugs.freedesktop.org/attachment.cgi?id=88267
glib: Add support for simple line annotations

Review of attachment 88267:
-----------------------------------------------------------------

::: glib/poppler-annot.cc
@@ +351,5 @@
> +{
> +}
> +
> +/**
> +* poppler_annot_line_new:

This line is not correctly indented.

@@ +359,5 @@
> + * Creates a new Line annotation that will be
> + * located on @rect when added to a page. See
> + * poppler_page_add_annot()
> + *
> + * Return value: A newly #PopplerAnnotLine annotation

newly created

@@ +361,5 @@
> + * poppler_page_add_annot()
> + *
> + * Return value: A newly #PopplerAnnotLine annotation
> + *
> + * Since: 0.25

0.26.

@@ +1439,5 @@
>  }
> +
> +/* PopplerAnnotLine */
> +void
> +poppler_annot_line_set_width (PopplerAnnotLine *poppler_annot, gdouble width)

This should be documented. Use a new line for every parameter-

@@ +1447,5 @@
> +  g_return_if_fail (POPPLER_IS_ANNOT_LINE (poppler_annot));
> +
> +  annot = static_cast<AnnotLine *>(POPPLER_ANNOT (poppler_annot)->annot);
> +  annot->setLeaderLineLength(width);
> +  annot->setLeaderLineExtension(width);

This is confusing, because this only applies to annot lines using leader lines.
Note that simply calling this, changes the meaning of the vertices. In a normal
lines the width is determined but its vertices, since it's a rectangle in the
end. I'm not sure how to expose the leader lines thing, though.

@@ +1452,5 @@
> +  // annot->setOpacitiy(0.5);
> +}
> +
> +void
> +poppler_annot_line_set_vertices (PopplerAnnotLine *poppler_annot,

This should be documented too.

@@ +1454,5 @@
> +
> +void
> +poppler_annot_line_set_vertices (PopplerAnnotLine *poppler_annot,
> +                                 gdouble x1, gdouble y1,
> +                                 gdouble x2, gdouble y2)

I wonder if we should to expose PopplerPoint, I guess it will be required if we
eventually expose Polygon annotations.

@@ +1498,5 @@
> +/**
> + * poppler_annot_line_get_start_style:
> + * @poppler_annot: a #PopplerAnnotLine
> + *
> + * Return value: (transfer none): The ending style located at the start of the line.

You don't need to use transfer annotations for a method returning an enum.

@@ +1524,5 @@
> + *
> + * Since: 0.25
> + **/
> +PopplerAnnotLineEndingStyle
> +poppler_annot_line_get_end_style (PopplerAnnotLine *poppler_annot)

I think I would merge both methods into a single
poppler_annot_line_get_ending_style that returns both start and end as out
parameters, allowing to pass NULL if you are not interested in any of them.

@@ +1556,5 @@
> +
> +  annot = static_cast<AnnotLine *>(POPPLER_ANNOT (poppler_annot)->annot);
> +
> +  annot->setStartEndStyle ((AnnotLineEndingStyle)start_style,
> +			   (AnnotLineEndingStyle)end_style);

Why are we just casting the enums here, but using a convert method in the
getters? We can either always cast (adding a compile assert to make sure we
break the build if the enums are not in sync) or use converter methods, but not
mix it.

-- 
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/6c375e24/attachment.html>