Bug 70983

Created attachment 88271 [details] [review]
glib-demo: Add Square and Circle annotations demo

Comment on attachment 88270 [details] [review]
glib: Add implementation of Square and Circle annotations

Review of attachment 88270 [details] [review]:
-----------------------------------------------------------------

::: 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.

(In reply to comment #2)
> @@ +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.

If we don't set the border, we get an "invisible" square/circle.
One way to enable it is good a sensible default.  The alternatives
are:
- A hard coded sensible default
- Receive *line_width, if NULL then not set anything. Otherwise,
  set the width.
- Add in the documentation that to create a visible circle/square it is
  required to call an additional method (set_border_width) afterwards.
- Not do anything at all

I am going to submit an updated patch with last one.

Created attachment 89433 [details] [review]
glib: Add implementation of Square and Circle annotations

Updated patch.

Created attachment 89434 [details] [review]
glib-demo: Add Square and Circle annotations demo

Updated patch for the demo.

(In reply to comment #3)
> (In reply to comment #2)
> > @@ +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.
> 
> If we don't set the border, we get an "invisible" square/circle.

This is a bug in poppler core then. Actually, it seems we are not exactly following the spec for borders, I'll fix it.

(In reply to comment #6)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > @@ +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.
> > 
> > If we don't set the border, we get an "invisible" square/circle.
> 
> This is a bug in poppler core then. Actually, it seems we are not exactly
> following the spec for borders, I'll fix it.

I've just sent a few patches to the list that should fix this problem. With those patches we don't need to set any border width at all to have square and circle annotations, so for now I would not expose set_border_width, because it's not needed and we might expose borders differently (using a PopplerAnnotBorder object or something like that).

Created attachment 89941 [details] [review]
glib: Add implementation of Square and Circle annotations

Updated patch removing the method poppler_annot_set_border_with

Created attachment 89942 [details] [review]
glib-demo: Add Square and Circle annotations demo

Updated patch removing the _set_border_with() invocation.

That is, after applying the patches submitted in http://lists.freedesktop.org/archives/poppler/2013-November/010662.html

I applied only the last 3 ones, otherwise it fails to compile the qt backend. It works as expected.

I think sooner than later we might want to add the API for setting an arbitrary border.

Comment on attachment 89941 [details] [review]
glib: Add implementation of Square and Circle annotations

Review of attachment 89941 [details] [review]:
-----------------------------------------------------------------

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

(In reply to comment #9)
> Created attachment 89942 [details] [review] [review]
> glib-demo: Add Square and Circle annotations demo
> 
> Updated patch removing the _set_border_with() invocation.
> 
> That is, after applying the patches submitted in
> http://lists.freedesktop.org/archives/poppler/2013-November/010662.html
> 
> I applied only the last 3 ones, otherwise it fails to compile the qt
> backend. It works as expected.
> 
> I think sooner than later we might want to add the API for setting an
> arbitrary border.

Pushed this one as well.