Bug 70901

Summary: Add getter and setter for annotation's rectangle
Product: poppler Reporter: Germán Poo-Caamaño <gpoo+bfdo>
Component: glib frontendAssignee: poppler-bugs <poppler-bugs>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 51487, 70981, 70982, 70983    
Attachments: glib: Add getter and setter for annotation's rectangle
glib: Add getter and setter for annotation's rectangle
glib: Add getter and setter for annotation's rectangle
glib: Add getter and setter for annotation's rectangle

Description Germán Poo-Caamaño 2013-10-27 00:10:34 UTC
Created attachment 88161 [details] [review]
glib: Add getter and setter for annotation's rectangle

Annotation objects contain at least two keys, Rect and Subtype.
The former has the coordinates where the annotation is placed.
The getter and setter allows to obtain and modify the position
of a given annotation.

See Page 606 of the PDF 1.7 reference.
Comment 1 Carlos Garcia Campos 2013-11-03 10:38:12 UTC
Comment on attachment 88161 [details] [review]
glib: Add getter and setter for annotation's rectangle

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

::: 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(...);
Comment 2 Germán Poo-Caamaño 2013-11-05 19:33:03 UTC
Comment on attachment 88161 [details] [review]
glib: Add getter and setter for annotation's rectangle

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

::: glib/poppler-annot.cc
@@ +719,5 @@
> + * Since: 0.25
> + **/
> +void
> +poppler_annot_set_rectangle (PopplerAnnot    *poppler_annot,
> +			     PopplerRectangle poppler_rect)

We still need to call setRect with the coordinates.  PopplerRectangle (struct) does not map to PDFRectangle (class).
Comment 3 Germán Poo-Caamaño 2013-11-05 19:36:20 UTC
Created attachment 88728 [details] [review]
glib: Add getter and setter for annotation's rectangle

Updated patch.

Instead of using a reference in poppler_annot_get_rectangle, I think it is better to just return a PopplerRectangle. I am unsure of the default values
for g_return_val_if_fail.  Either 0,0,0,0 or -1,-1,-1,-1.
Comment 4 Carlos Garcia Campos 2013-11-06 08:02:23 UTC
(In reply to comment #2)
> Comment on attachment 88161 [details] [review] [review]
> glib: Add getter and setter for annotation's rectangle
> 
> Review of attachment 88161 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: glib/poppler-annot.cc
> @@ +719,5 @@
> > + * Since: 0.25
> > + **/
> > +void
> > +poppler_annot_set_rectangle (PopplerAnnot    *poppler_annot,
> > +			     PopplerRectangle poppler_rect)
> 
> We still need to call setRect with the coordinates.  PopplerRectangle
> (struct) does not map to PDFRectangle (class).

Yes, I mean to use a pointer for the parameter so that the caller can do:

PopplerRectangle rect = { 1, 2, 3, 4 };
poppler_annot_set_rectangle (annot, &rect);
Comment 5 Carlos Garcia Campos 2013-11-06 08:04:41 UTC
(In reply to comment #3)
> Created attachment 88728 [details] [review] [review]
> glib: Add getter and setter for annotation's rectangle
> 
> Updated patch.
> 
> Instead of using a reference in poppler_annot_get_rectangle, I think it is
> better to just return a PopplerRectangle. I am unsure of the default values
> for g_return_val_if_fail.  Either 0,0,0,0 or -1,-1,-1,-1.

Same idea here. Instead of returning the struct, you can return a pointer as an out parameter. To avoid the problem of how to represent an invalid rectangle, make the function return a boolean. When the function returns FALSE the value of the rectangle is undefined, whatever the user passed.

PopplerRectangle rect;
if (!poppler_annot_get_rect (annot, &rect))
    /* Do whatever with the rect */
Comment 6 Germán Poo-Caamaño 2013-11-07 01:34:55 UTC
(In reply to comment #5)
> (In reply to comment #3)
> > Created attachment 88728 [details] [review] [review] [review]
> > glib: Add getter and setter for annotation's rectangle
> > 
> > Updated patch.
> > 
> > Instead of using a reference in poppler_annot_get_rectangle, I think it is
> > better to just return a PopplerRectangle. I am unsure of the default values
> > for g_return_val_if_fail.  Either 0,0,0,0 or -1,-1,-1,-1.
> 
> Same idea here. Instead of returning the struct, you can return a pointer as
> an out parameter. To avoid the problem of how to represent an invalid
> rectangle, make the function return a boolean. When the function returns
> FALSE the value of the rectangle is undefined, whatever the user passed.
> 
> PopplerRectangle rect;
> if (!poppler_annot_get_rect (annot, &rect))
>     /* Do whatever with the rect */

This is what the first patch did.
Comment 7 Germán Poo-Caamaño 2013-11-07 02:00:27 UTC
Created attachment 88800 [details] [review]
glib: Add getter and setter for annotation's rectangle
Comment 8 Carlos Garcia Campos 2013-11-07 08:12:26 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > Created attachment 88728 [details] [review] [review] [review] [review]
> > > glib: Add getter and setter for annotation's rectangle
> > > 
> > > Updated patch.
> > > 
> > > Instead of using a reference in poppler_annot_get_rectangle, I think it is
> > > better to just return a PopplerRectangle. I am unsure of the default values
> > > for g_return_val_if_fail.  Either 0,0,0,0 or -1,-1,-1,-1.
> > 
> > Same idea here. Instead of returning the struct, you can return a pointer as
> > an out parameter. To avoid the problem of how to represent an invalid
> > rectangle, make the function return a boolean. When the function returns
> > FALSE the value of the rectangle is undefined, whatever the user passed.
> > 
> > PopplerRectangle rect;
> > if (!poppler_annot_get_rect (annot, &rect))
> >     /* Do whatever with the rect */
> 
> This is what the first patch did.

Yes, and I only asked when can this return FALSE, because I still think that from the user point of view, this should never be false. The rectangle is a required field of any annotation, so it should always return a valid rectangle, otherwise it's a bug because we are discarding invalid annots in the core. So, I think we should just make it void and add g_return_if_fail if the passed in pointer is not an annot. It's a programmer error to pass an invalid annot. Sorry for the misunderstanding.
Comment 9 Germán Poo-Caamaño 2013-11-16 08:08:23 UTC
Created attachment 89305 [details] [review]
glib: Add getter and setter for annotation's rectangle

Updated patch
Comment 10 Carlos Garcia Campos 2013-11-17 09:08:47 UTC
Comment on attachment 89305 [details] [review]
glib: Add getter and setter for annotation's rectangle

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

I've pushed it to git master with some modifications. Thanks.

::: glib/poppler-annot.cc
@@ +686,5 @@
> + *
> + * Retrieves the rectangle representing the page coordinates where the
> + * annotation @poppler_annot is placed.
> + *
> + * If @poppler_annot is an invalid annotation, then @poppler_rect is not set.

I've removed this, because passing an invalid annot is not something expected, and the sentence is not true if g_return macros are disabled, in that case it will crash.

@@ +696,5 @@
> +                             PopplerRectangle *poppler_rect)
> +{
> +  PDFRectangle *annot_rect;
> +
> +  g_return_if_fail (POPPLER_IS_ANNOT (poppler_annot));

We should also check here that poppler_rect passed is not NULL.

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

I still think this should receive a pointer, so I've changed this too.

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.