[Poppler-bugs] [Bug 81665] Extend support for free text annotations.

[bugzilla-daemon at freedesktop.org]

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

--- Comment #5 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 103327
  --> https://bugs.freedesktop.org/attachment.cgi?id=103327
glib: Extend FreeText annotation support

Review of attachment 103327:
-----------------------------------------------------------------

::: glib/poppler-annot.cc
@@ +472,5 @@
>  
> +static GooString *create_appearance_string (PopplerAnnotAppearance *appearance)
> +{
> +    if (!appearance)
> +        return new GooString ("/Invalid_font 12 Tf");

Doesn't this fail? It's using an invalid font

@@ +495,5 @@
> + * poppler_page_add_annot()
> + *
> + * Return value: A newly created #PopplerAnnotFreeText annotation
> + *
> + * Since: 0.27

Use 0.28 instead

@@ +499,5 @@
> + * Since: 0.27
> + */
> +PopplerAnnot *
> +poppler_annot_free_text_new (PopplerDocument  *doc,
> +                             PopplerRectangle *rect)

The DA entry is a required field, so I think we should pass the font name,
color and size as parameters of the constructor

@@ +1703,5 @@
> + * poppler_annot_free_text_set_quadding:
> + * @poppler_annot: a #PopplerAnnotFreeText
> + * @quadding: a #PopplerAnnotFreeTextQuadding
> + *
> + * Retrieves the intent of the @poppler_annot.

Copy paste issue here :-)

@@ +1705,5 @@
> + * @quadding: a #PopplerAnnotFreeTextQuadding
> + *
> + * Retrieves the intent of the @poppler_annot.
> + *
> + * Return value: #PopplerAnnotFreeTextIntent of @poppler_annot.

Ditto, this is a void function

@@ +1723,5 @@
> +
> +/**
> + * poppler_annot_free_text_get_inent:
> + * @poppler_annot: a #PopplerAnnotFreeText
> + * @quadding: a #PopplerAnnotFreeTextIntent

This parameter doesn't exist

@@ +1742,5 @@
> +
> +/**
> + * poppler_annot_free_text_set_intent:
> + * @poppler_annot: a #PopplerAnnotFreeText
> + * @quadding: a #PopplerAnnotFreeTextIntent

This should be @intent

@@ +1806,5 @@
>  
> +/**
> + * poppler_annot_free_text_get_callout_line:
> + * @poppler_annot: a #PopplerAnnotFreeText
> + * @callout: a #PopplerAnnotCalloutLine

Since this can be NULL to unset the callout line, add (allow none) tag and
document that this can be NULL and the effects of passing NULL.

@@ +1834,5 @@
> +
> +  annot = static_cast<AnnotFreeText *>(POPPLER_ANNOT (poppler_annot)->annot);
> +  annot->setCalloutLine (line);
> +  if (!line)
> +    delete line;

if (line)

@@ +1862,5 @@
> +  annot = static_cast<AnnotFreeText *>(POPPLER_ANNOT (poppler_annot)->annot);
> +  da = annot->getAppearanceString();
> +
> +  if (!da || da->getLength() == 0)
> +    g_error ("The free text annotation has no DA");

This can't happen, the core already fails to create the annot in this case (or
it should), so we can safely assume here there's a da. Otherwise, use an assert
instead of g_error

-- 
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/20140723/bb6eeba4/attachment.html>