<html> <head> <base href="https://bugs.freedesktop.org/" /> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW --- - [PATCH] Add support for TextMarkup Annotations in glib frontend" href="https://bugs.freedesktop.org/show_bug.cgi?id=51487#c42">Comment # 42</a> on <a class="bz_bug_link bz_status_NEW " title="NEW --- - [PATCH] Add support for TextMarkup Annotations in glib frontend" href="https://bugs.freedesktop.org/show_bug.cgi?id=51487">bug 51487</a> from <a class="email" href="mailto:carlosgc@gnome.org" title="Carlos Garcia Campos <carlosgc@gnome.org>"> Carlos Garcia Campos</a> <pre>Comment on <a href="attachment.cgi?id=89566" name="attach_89566" title="glib: Add PopplerAnnotTextMarkup class and subtypes">attachment 89566</a> <a href="attachment.cgi?id=89566&action=edit" title="glib: Add PopplerAnnotTextMarkup class and subtypes">[details]</a> <a href='page.cgi?id=splinter.html&bug=51487&attachment=89566'>[review]</a> glib: Add PopplerAnnotTextMarkup class and subtypes Review of <a href="attachment.cgi?id=89566" name="attach_89566" title="glib: Add PopplerAnnotTextMarkup class and subtypes">attachment 89566</a> <a href="attachment.cgi?id=89566&action=edit" title="glib: Add PopplerAnnotTextMarkup class and subtypes">[details]</a> <a href='page.cgi?id=splinter.html&bug=51487&attachment=89566'>[review]</a>: ----------------------------------------------------------------- Looks good. I've just pushed it with the modifications I suggested. Thanks! ::: glib/poppler-annot.cc @@ +276,5 @@ > + sizeof (AnnotQuadrilaterals::AnnotQuadrilateral *), > + quadrilaterals->len); > + > + for (i = 0; i < quadrilaterals->len; i++) { > + quadrilateral = g_array_index (quadrilaterals, PopplerQuadrilateral, i); You can use &g_array_index (quadrilaterals, PopplerQuadrilateral, i) to get the pinter and avoid copying the struct. @@ +286,5 @@ > + quadrilateral.p4.x, quadrilateral.p4.y); > + } > + > + back_quads = new AnnotQuadrilaterals (quads_array, i); > + annot->setQuadrilaterals (back_quads); back_quads is leaked here, because setQuadrilaterals creates a new AnnotQuadrilaterals for the given one. @@ +289,5 @@ > + back_quads = new AnnotQuadrilaterals (quads_array, i); > + annot->setQuadrilaterals (back_quads); > + > + return _poppler_create_annot (POPPLER_TYPE_ANNOT_TEXT_MARKUP, annot); > +} The content of this function is mostly duplicated in poppler_annot_text_markup_set_quadrilaterals, it could be factored out. We could add create_annot_quads_from_poppler_quads and create_poppler_quads_from_annot_quads @@ +330,5 @@ > + return _poppler_annot_text_markup_new (annot); > +} > + > +/** > + * poppler_annot_text_markup_new_highlight_with_quadrilaterals: As I commented already, I think we should enforce text markup annot to be created with quads. @@ +352,5 @@ > + AnnotTextMarkup *annot; > + PDFRectangle pdf_rect(rect->x1, rect->y1, > + rect->x2, rect->y2); > + > + g_return_val_if_fail (quadrilaterals != NULL, NULL); You should aso check that the array is not empty. @@ +1748,5 @@ > + quadrilateral.p3.y = back_quads->getY3(i); > + quadrilateral.p4.x = back_quads->getX4(i); > + quadrilateral.p4.y = back_quads->getY4(i); > + > + g_array_append_val (quads_array, quadrilateral); We can avoid the memmove dancing here. Since we are creating a sized array and we know the size is not going to change, we can set the size right after creating the array and access its contents with g_array_index to fill the already allocated PopplerQuadrilateral directly.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>