[Poppler-bugs] [Bug 70901] Add getter and setter for annotation's rectangle

[bugzilla-daemon at freedesktop.org]

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

--- Comment #1 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 88161
  --> https://bugs.freedesktop.org/attachment.cgi?id=88161
glib: Add getter and setter for annotation's rectangle

Review of attachment 88161:
-----------------------------------------------------------------

::: glib/poppler-annot.cc
@@ +682,5 @@
> + *
> + * Retrieves the rectangle representing the page coordinates where the
> + * annotation @poppler_annot is placed.
> + *
> + * Return value: %TRUE if #PopplerRectangle was correctly filled, %FALSE otherwise

How can this return false? Rect is required field in annot dict, if it is not
present the annot is not even created.

@@ +684,5 @@
> + * annotation @poppler_annot is placed.
> + *
> + * Return value: %TRUE if #PopplerRectangle was correctly filled, %FALSE otherwise
> + *
> + * Since: 0.25

This should be 0.26, the next stable release.

@@ +696,5 @@
> +
> +  g_return_val_if_fail (POPPLER_IS_ANNOT (poppler_annot), FALSE);
> +  g_return_val_if_fail (poppler_rect != NULL, FALSE);
> +
> +  annot = static_cast<Annot *>(POPPLER_ANNOT (poppler_annot)->annot);

You don't need to casts, poppler_annot is already a PopplerAnnot * and
poppler_annot->annot is already Annot *

@@ +704,5 @@
> +  poppler_rect->x2 = annot_rect->x2;
> +  poppler_rect->y1 = annot_rect->y1;
> +  poppler_rect->y2 = annot_rect->y2;
> +
> +  return TRUE;

This should be void, we are unconditionally returning TRUE.

@@ +715,5 @@
> + *
> + * Move the annotation to the rectangle representing the page coordinates where the
> + * annotation @poppler_annot should be placed.
> + *
> + * Since: 0.25

0.26

@@ +719,5 @@
> + * Since: 0.25
> + **/
> +void
> +poppler_annot_set_rectangle (PopplerAnnot    *poppler_annot,
> +			     PopplerRectangle poppler_rect)

Use a pointer here, to avoid copying the whole struct. The caller can use a
stack allocated struct and pass the stack memory address.

@@ +721,5 @@
> +void
> +poppler_annot_set_rectangle (PopplerAnnot    *poppler_annot,
> +			     PopplerRectangle poppler_rect)
> +{
> +  Annot        *annot;

Extra spaces there.

@@ +728,5 @@
> +
> +  annot = static_cast<Annot *>(POPPLER_ANNOT (poppler_annot)->annot);
> +
> +  annot->setRect (poppler_rect.x1, poppler_rect.y1,
> +                  poppler_rect.x2, poppler_rect.y2);

You don't need the casts here either, this could be just:

poppler_annot->annot->setRect(...);

-- 
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/81fe2dd1/attachment-0001.html>