Bug 81665 - Extend support for free text annotations.
Summary: Extend support for free text annotations.
Status: NEW
Alias: None
Product: poppler
Classification: Unclassified
Component: glib frontend (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: poppler-bugs
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-23 10:19 UTC by Anuj Khare
Modified: 2014-07-27 12:55 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
1/4. glib: Add PopplerAnnotAppearance boxed type (5.24 KB, patch)
2014-07-23 10:19 UTC, Anuj Khare
Details | Splinter Review
2/4. core: Expose AnnotFreeText::parseAppearanceString (1.22 KB, patch)
2014-07-23 10:22 UTC, Anuj Khare
Details | Splinter Review
glib: Extend FreeText annotation support (9.59 KB, patch)
2014-07-23 10:23 UTC, Anuj Khare
Details | Splinter Review
4/4. Core: Fix issue when adding annot without contents (873 bytes, patch)
2014-07-23 10:24 UTC, Anuj Khare
Details | Splinter Review
[PATCH 1/4] glib: Extend FreeText annotation support (13.28 KB, patch)
2014-07-25 08:02 UTC, Anuj Khare
Details | Splinter Review
[PATCH 2/4] core: Fix issue when adding annot without contents (1.18 KB, patch)
2014-07-25 08:04 UTC, Anuj Khare
Details | Splinter Review
[PATCH 3/4] core: Expose AnnotFreeText::parseAppearanceString (1.23 KB, patch)
2014-07-25 08:14 UTC, Anuj Khare
Details | Splinter Review
[PATCH 4/4] glib-demo: Extend demo for FreeText annotations (3.43 KB, patch)
2014-07-25 08:15 UTC, Anuj Khare
Details | Splinter Review
core: Expose AnnotFreeText::parseAppearanceString (1.23 KB, patch)
2014-07-27 12:52 UTC, Anuj Khare
Details | Splinter Review
glib: Add PopplerFontDescription boxed type (5.12 KB, patch)
2014-07-27 12:54 UTC, Anuj Khare
Details | Splinter Review
[PATCH 3/4] glib: Extend FreeText annotation support (12.94 KB, patch)
2014-07-27 12:54 UTC, Anuj Khare
Details | Splinter Review
[PATCH 4/4] glib-demo: Extend demo for FreeText annotations (3.54 KB, patch)
2014-07-27 12:55 UTC, Anuj Khare
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Anuj Khare 2014-07-23 10:19:20 UTC
Created attachment 103325 [details] [review]
1/4. glib: Add PopplerAnnotAppearance boxed type

I have attached patches that allow adding new freetext annotations, and implement the intent, callout line, and quadding properties.

The PopplerAnnotAppearance type is used to store the font name, color and size of the free text annotation.
Comment 1 Anuj Khare 2014-07-23 10:22:25 UTC
Created attachment 103326 [details] [review]
2/4. core: Expose AnnotFreeText::parseAppearanceString
Comment 2 Anuj Khare 2014-07-23 10:23:15 UTC
Created attachment 103327 [details] [review]
glib: Extend FreeText annotation support
Comment 3 Anuj Khare 2014-07-23 10:24:12 UTC
Created attachment 103328 [details] [review]
4/4. Core: Fix issue when adding annot without contents
Comment 4 Carlos Garcia Campos 2014-07-23 11:37:09 UTC
Comment on attachment 103325 [details] [review]
1/4. glib: Add PopplerAnnotAppearance boxed type

Review of attachment 103325 [details] [review]:
-----------------------------------------------------------------

I'm not sure about exposing annotation appearances as a boxed type in the public API. It's a bit confusing because all annotations have an appearance, that is more than just font name, size and color. This is to support the default appearance (DA) of free text annotations, so maybe we can add specific API for free text annotations. The main problem is that the default appearance is a required parameter when creating the annot, so if we add parameters to the constructor we won't be able to extend them in the future if needed without breaking the API.
Comment 5 Carlos Garcia Campos 2014-07-23 13:12:51 UTC
Comment on attachment 103327 [details] [review]
glib: Extend FreeText annotation support

Review of attachment 103327 [details] [review]:
-----------------------------------------------------------------

::: 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
Comment 6 Carlos Garcia Campos 2014-07-23 13:21:45 UTC
Comment on attachment 103328 [details] [review]
4/4. Core: Fix issue when adding annot without contents

Review of attachment 103328 [details] [review]:
-----------------------------------------------------------------

::: poppler/Annot.cc
@@ +3015,5 @@
>      fontsize = 10;
>    if (fontcolor == NULL)
>      fontcolor = new AnnotColor(0, 0, 0); // Black
> +  if (!contents)
> +         contents = new GooString ("");

hmm, the thing is a bit inconsistent with regards to contents. In Annot::initialize we set it to NULL when the entry is not present in the dict, but in Annot::setContents() we create an empty GooString when the new contents are NULL. I think we should either set it to NULL in Annot::setContents() or create an empty string in Annot::initialize. If we decide to use NULL for no contents, it would be better to check it here and not add any text operator to the appearance stream when it's NULL.
Comment 7 Anuj Khare 2014-07-25 08:02:45 UTC
Created attachment 103426 [details] [review]
[PATCH 1/4] glib: Extend FreeText annotation support

I have updated the patches according to the comments. The free text annotations do not use a boxed type for appearance anymore.

Core does not parse the font name as of yet, so I have omitted it, but it will need to be added in the future, breaking the API (poppler_annot_free_text_new).
Comment 8 Anuj Khare 2014-07-25 08:04:25 UTC
Created attachment 103427 [details] [review]
[PATCH 2/4] core: Fix issue when adding annot without contents

I have modified it such that we set a new empty GooString for contents everywhere.
Comment 9 Anuj Khare 2014-07-25 08:14:21 UTC
Created attachment 103428 [details] [review]
[PATCH 3/4] core: Expose AnnotFreeText::parseAppearanceString
Comment 10 Anuj Khare 2014-07-25 08:15:07 UTC
Created attachment 103429 [details] [review]
[PATCH 4/4] glib-demo: Extend demo for FreeText annotations

This patch adds the new free text annotation features to glib demo.
Comment 11 Jose Aliste 2014-07-25 08:53:33 UTC
Comment on attachment 103426 [details] [review]
[PATCH 1/4] glib: Extend FreeText annotation support

Review of attachment 103426 [details] [review]:
-----------------------------------------------------------------

This looks much better. Although, see the comment bellow

::: glib/poppler-annot.cc
@@ +1896,5 @@
> +  annot = static_cast<AnnotFreeText *>(POPPLER_ANNOT (poppler_annot)->annot);
> +  annot->setAppearanceString (da);
> +  delete da;
> +}
> +

We already know that we want the font type to be changed accessed, so the API should have some PangoFontDescription or so to get and set the font + the color. Even if Poppler Core does not support different fonts in FreeText Annot, I believe we want the API to be right in glib right away.
Comment 12 Jose Aliste 2014-07-25 08:53:35 UTC
Comment on attachment 103426 [details] [review]
[PATCH 1/4] glib: Extend FreeText annotation support

Review of attachment 103426 [details] [review]:
-----------------------------------------------------------------

This looks much better. Although, see the comment bellow

::: glib/poppler-annot.cc
@@ +1896,5 @@
> +  annot = static_cast<AnnotFreeText *>(POPPLER_ANNOT (poppler_annot)->annot);
> +  annot->setAppearanceString (da);
> +  delete da;
> +}
> +

We already know that we want the font type to be changed accessed, so the API should have some PangoFontDescription or so to get and set the font + the color. Even if Poppler Core does not support different fonts in FreeText Annot, I believe we want the API to be right in glib right away.
Comment 13 Carlos Garcia Campos 2014-07-25 09:30:15 UTC
Comment on attachment 103427 [details] [review]
[PATCH 2/4] core: Fix issue when adding annot without contents

Review of attachment 103427 [details] [review]:
-----------------------------------------------------------------

Thanks for the patch, I've just pushed it to master and stable branch with some minor modifications.
Comment 14 Carlos Garcia Campos 2014-07-25 09:31:03 UTC
Comment on attachment 103325 [details] [review]
1/4. glib: Add PopplerAnnotAppearance boxed type

I guess this one is obsolete now.
Comment 15 Anuj Khare 2014-07-27 12:52:55 UTC
Created attachment 103538 [details] [review]
core: Expose AnnotFreeText::parseAppearanceString

I have attached new patches, which now use a PopplerFontDescription type to creaet new Free Text annotations.
Comment 16 Anuj Khare 2014-07-27 12:54:07 UTC
Created attachment 103539 [details] [review]
glib: Add PopplerFontDescription boxed type

The boxed type used to store font attributes.
Comment 17 Anuj Khare 2014-07-27 12:54:49 UTC
Created attachment 103540 [details] [review]
[PATCH 3/4] glib: Extend FreeText annotation support
Comment 18 Anuj Khare 2014-07-27 12:55:28 UTC
Created attachment 103541 [details] [review]
[PATCH 4/4] glib-demo: Extend demo for FreeText annotations


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.