[Poppler-bugs] [Bug 51487] [PATCH] Add support for TextMarkup Annotations in glib frontend

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=51487

--- Comment #42 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 89566
  --> https://bugs.freedesktop.org/attachment.cgi?id=89566
glib: Add PopplerAnnotTextMarkup class and subtypes

Review of attachment 89566:
-----------------------------------------------------------------

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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/poppler-bugs/attachments/20140119/f123586d/attachment-0001.html>