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