Bug 70901

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

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.

(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);

(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 */

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

Created attachment 88800 [details] [review]
glib: Add getter and setter for annotation's rectangle

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

Created attachment 89305 [details] [review]
glib: Add getter and setter for annotation's rectangle

Updated patch

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.