[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 #10 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 89941
  --> https://bugs.freedesktop.org/attachment.cgi?id=89941
glib: Add implementation of Square and Circle annotations

Review of attachment 89941:
-----------------------------------------------------------------

Pushed the patch after addressing my comments below. Thank you very much.

::: glib/poppler-annot.cc
@@ +425,5 @@
> + *
> + * Creates a new Circle annotation that will be
> + * located on @rect when added to a page. See
> + * poppler_page_add_annot()
> +*

This is wrongly indented.

@@ +426,5 @@
> + * Creates a new Circle annotation that will be
> + * located on @rect when added to a page. See
> + * poppler_page_add_annot()
> +*
> +* Creates a new empty #PopplerAnnotCircle circle.

This line looks redundant

@@ +428,5 @@
> + * poppler_page_add_annot()
> +*
> +* Creates a new empty #PopplerAnnotCircle circle.
> +*
> +* Return value: a newly #PopplerAnnotCircle annotation

newly created

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

newly created

@@ +691,5 @@
>    poppler_annot->annot->setFlags ((guint) flags);
>  }
>  
> +static PopplerColor *
> +_poppler_new_poppler_color (AnnotColor *color)

We don't use the _ prefix for private static functions, but only for private
functions declared in a header file. The name is a bit confusing, it could be
called create_poppler_color_from_annot_color

@@ +727,5 @@
>    return poppler_color;
>  }
>  
> +static AnnotColor *
> +_poppler_new_annot_color (PopplerColor *poppler_color)

And this create_annot_color_from_poppler_color

@@ +729,5 @@
>  
> +static AnnotColor *
> +_poppler_new_annot_color (PopplerColor *poppler_color)
> +{
> +  AnnotColor *color = NULL;

We don't need the local variable if we return early when passed in pointer is
NULL and then we can just return new AnnotColor()

@@ +1635,5 @@
> +{
> +  AnnotGeometry *annot;
> +
> +  g_return_val_if_fail (POPPLER_IS_ANNOT_CIRCLE (poppler_annot) ||
> +                        POPPLER_IS_ANNOT_SQUARE (poppler_annot), NULL);

Do not use g_return macros in private methods. Also these checks are already
done by the callers.

@@ +1649,5 @@
> +{
> +  AnnotGeometry *annot;
> +
> +  g_return_if_fail (POPPLER_IS_ANNOT_CIRCLE (poppler_annot) ||
> +                    POPPLER_IS_ANNOT_SQUARE (poppler_annot));

Ditto.

@@ +1679,5 @@
> +}
> +
> +/**
> + * poppler_annot_circle_set_interior_color:
> + * @poppler_annot: a #PopplerAnnot

a #PopplerAnnotCircle

@@ +1717,5 @@
> +}
> +
> +/**
> + * poppler_annot_square_set_interior_color:
> + * @poppler_annot: a #PopplerAnnot

a #PopplerAnnotSquare

-- 
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/20131205/87fce584/attachment.html>