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.
Created attachment 103326 [details] [review] 2/4. core: Expose AnnotFreeText::parseAppearanceString
Created attachment 103327 [details] [review] glib: Extend FreeText annotation support
Created attachment 103328 [details] [review] 4/4. Core: Fix issue when adding annot without contents
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 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 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.
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).
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.
Created attachment 103428 [details] [review] [PATCH 3/4] core: Expose AnnotFreeText::parseAppearanceString
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 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 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 on attachment 103325 [details] [review] 1/4. glib: Add PopplerAnnotAppearance boxed type I guess this one is obsolete now.
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.
Created attachment 103539 [details] [review] glib: Add PopplerFontDescription boxed type The boxed type used to store font attributes.
Created attachment 103540 [details] [review] [PATCH 3/4] glib: Extend FreeText annotation support
Created attachment 103541 [details] [review] [PATCH 4/4] glib-demo: Extend demo for FreeText annotations
Hi, I am working on this feature with WYSIWYG live editing support as a part of GSoC 2018 project: https://summerofcode.withgoogle.com/projects/#6053164340477952 You are welcome to comment and share your experience regarding the same.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/poppler/poppler/issues/570.
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.