Bug 70983 - Add implementation of Square and Circle annotations
Summary: Add implementation of Square and Circle annotations
Status: RESOLVED FIXED
Alias: None
Product: poppler
Classification: Unclassified
Component: glib frontend (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: poppler-bugs
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 70901
Blocks:
  Show dependency treegraph
 
Reported: 2013-10-29 07:17 UTC by Germán Poo-Caamaño
Modified: 2013-12-05 17:28 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
glib: Add implementation of Square and Circle annotations (11.66 KB, patch)
2013-10-29 07:17 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib-demo: Add Square and Circle annotations demo (3.08 KB, patch)
2013-10-29 07:18 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib: Add implementation of Square and Circle annotations (17.75 KB, patch)
2013-11-18 23:39 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib-demo: Add Square and Circle annotations demo (1.95 KB, patch)
2013-11-18 23:39 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib: Add implementation of Square and Circle annotations (16.38 KB, patch)
2013-11-28 07:26 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib-demo: Add Square and Circle annotations demo (1.77 KB, patch)
2013-11-28 07:29 UTC, Germán Poo-Caamaño
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Germán Poo-Caamaño 2013-10-29 07:17:14 UTC
Created attachment 88270 [details] [review]
glib: Add implementation of Square and Circle annotations

glib: Add implementation of Square and Circle annotations

Square and Circle only differ in the constructor which defines
the subtype.  Therefore, it uses the same name than Poppler's
Geometry class.
Comment 1 Germán Poo-Caamaño 2013-10-29 07:18:04 UTC
Created attachment 88271 [details] [review]
glib-demo: Add Square and Circle annotations demo
Comment 2 Carlos Garcia Campos 2013-11-03 11:39:21 UTC
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.
Comment 3 Germán Poo-Caamaño 2013-11-18 23:37:32 UTC
(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.
Comment 4 Germán Poo-Caamaño 2013-11-18 23:39:16 UTC
Created attachment 89433 [details] [review]
glib: Add implementation of Square and Circle annotations

Updated patch.
Comment 5 Germán Poo-Caamaño 2013-11-18 23:39:43 UTC
Created attachment 89434 [details] [review]
glib-demo: Add Square and Circle annotations demo

Updated patch for the demo.
Comment 6 Carlos Garcia Campos 2013-11-22 08:21:05 UTC
(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.
Comment 7 Carlos Garcia Campos 2013-11-26 20:16:00 UTC
(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).
Comment 8 Germán Poo-Caamaño 2013-11-28 07:26:25 UTC
Created attachment 89941 [details] [review]
glib: Add implementation of Square and Circle annotations

Updated patch removing the method poppler_annot_set_border_with
Comment 9 Germán Poo-Caamaño 2013-11-28 07:29:31 UTC
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 10 Carlos Garcia Campos 2013-12-05 17:28:13 UTC
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
Comment 11 Carlos Garcia Campos 2013-12-05 17:28:39 UTC
(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.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.