GNOME Bugzilla – Bug 583377
Unimplemented annotation: POPPLER_ANNOT_HIGHLIGHT/POPPLER_ANNOT_STRIKE_OUT
Last modified: 2016-02-17 19:52:07 UTC
While reading a PDF, I got those warnings: ** (evince:62317): WARNING **: Unimplemented annotation: POPPLER_ANNOT_HIGHLIGHT, please post a bug report in Evince bugzilla (http://bugzilla.gnome.org) with a testcase. ** (evince:62317): WARNING **: Unimplemented annotation: POPPLER_ANNOT_STRIKE_OUT, please post a bug report in Evince bugzilla (http://bugzilla.gnome.org) with a testcase. Note that the annotations are more or less displayed correctly (excepted that the highlight prevent the text to be seen). Anyhow, the rendering is probably concerning poppler.
Created attachment 135054 [details] Simple version of the PDF with the two types of unimplemented annotations
I found the same problems (highlight but I do not see the text). In addition, the text bubbles with the comments are not shown neither.
(In reply to comment #0) > Note that the annotations are more or less displayed correctly (excepted that > the highlight prevent the text to be seen). Anyhow, the rendering is probably > concerning poppler. yes, this is already fixed in poppler >= 0.12, you need cairo from git master, though.
(In reply to comment #2) > the text bubbles with the comments are not shown neither. This is because only Text anotations are supported at the moment, not highlight yet.
I'd like to second that bug report, I have the exact same problem (hence I don't provide another test case). It doesn't bother me too much that I can't read the highlighted text (I can if I select it as a work around), but it's a huge drawback that there is no way whats-o-ever of reading the text of the comment.
With the libpoppler version in Ubuntu Maverick (0.14.1), the highlight no longer covers the text behind it, i.e. both types of annotation seem to be rendered fine. Evince emits still all the warnings though.
Hm. I'm using "Document Viewer. Using poppler/cairo (0.14.0)" from Ubuntu Maverick and the highlight does indeed cover the text. Which version of poppler/cairo and evince is supposed to have this issue fixed?
Created attachment 167141 [details] Screenshot of the test document Huh, that's strange. See the attached screenshot with evince showing the test document. I'm running maverick in a VM. poppler is 0.14.1, cairo is 1.9.14.1
(In reply to comment #7) > Hm. I'm using "Document Viewer. Using poppler/cairo (0.14.0)" from Ubuntu > Maverick and the highlight does indeed cover the text. > > Which version of poppler/cairo and evince is supposed to have this issue fixed? cairo >= 1.9.4
To come back to the initial bugreport. It seems that for Evince 3.0.0 and poppler 0.16.3 the drawing errors have been fixed. However, the strikeout highlight is no longer shown. Also, although the highlight annotation is drawn, there still seems to be no way to reach the associated annotation text; this is only possible for the plain annotations (attached to a single point).
Created attachment 200171 [details] page which generate warnings about poppler highlight I still see the warnings in Linux Mint 11 Katya - x64 edition, although it is properly displayed printing does not the highlighted text. In Linux Mint 8 Helena - x64 edition I don't see the highlighted text unless I select it with the mouse.
still there in 3.3.90
Created attachment 216878 [details] [review] add basic highlighting support to pdf backend The following (minimal) patch adds support for highlighting. Note that the only thing that this patch implements is that with the patch you can see the contents of a highlight annotation by hovering the annotation (and waiting enough time for the alert-type popup to appear). Probably we should add support for having a Poup Window also, so people can modify the contents?
*** Bug 659478 has been marked as a duplicate of this bug. ***
For the ones interested, we started a new branch wip/annotation_support in order to keep track of all the work towards having this and other bugs about annotations fixed.
*** Bug 635934 has been marked as a duplicate of this bug. ***
*** Bug 676865 has been marked as a duplicate of this bug. ***
Here is a real-world public document that exhibits these annotations: http://groups.csail.mit.edu/mac/users/hal/misc/cacm-late-draft.pdf
*** Bug 692507 has been marked as a duplicate of this bug. ***
*** Bug 645707 has been marked as a duplicate of this bug. ***
There is another example of a pdf with annotations here: ftp://dante.ctan.org/tex-archive/macros/latex/contrib/pdfcomment/doc/example.pdf and the LaTeX source: ftp://dante.ctan.org/tex-archive/macros/latex/contrib/pdfcomment/doc/example.tex The above links were taken from https://bitbucket.org/kleberj/pdfcomment/wiki/Home, where you can find more examples.
*** Bug 708066 has been marked as a duplicate of this bug. ***
*** Bug 712218 has been marked as a duplicate of this bug. ***
Created attachment 273337 [details] [review] Add bounding-rectangle property to EvAnnotation This allows an EvAnnotation to know its location on the page. This is patch 1 of 6 to implement highlight and strikeout text markup annotations.
Created attachment 273338 [details] [review] Add support for interactive areas in annotations. Some annotation types do not have a rectangular shape and require non-rectangular boundary checks to determine if the user clicked on them. A new method ev_annotation_is_location_in_area() is added that determines whether a point location is actually inside the annotation's interactive area. 2/6
Created attachment 273339 [details] [review] Add EvQuadrilateral type required for text markup annotations. EvQuadrilateral is a simple boxed type containing 4 EvPoints that constitute the corners of a quadrilateral. PDF markup annotations make use of this structure to define an annotation's active area. 3/6
Created attachment 273340 [details] [review] Add EvAnnotationTextMarkup class. This base class should be used for all text markup annotations like text highlighting, underlining, etc.
Created attachment 273341 [details] [review] Add support for POPPLER_ANNOT_HIGHLIGHT
Created attachment 273342 [details] [review] Add support for POPPLER_ANNOT_STRIKE_OUT
I would really appreciate to see some comments on the patches, even if it's just "wait for another n weeks." I would love to work on further annotation features in Evince, but don't want to go on until I know I am not on a completely wrong track.
Thomas, your patches seem to go on the good track and we appreciate your effort. However, I think we might have some Google SoC interns working on annotation support this summer, so it will be great if we could coordinate the work so we don't step on each other. On the patches, I just fixed my laptop, so I will start playing with them asap.
Now that the GSoC accepted interns have been published and Anuj is going to work on overall annotation improvements, I'm looking forward for any improvements on annotations. We sure should coordinate any efforts, please note that I was not aware of a specific GSoC project for evince annotations when I started working on it. Obviously, I won't get in the way of Anuj finishing his internship, but I'm also happy if I can be of any help closing further annotation bugs ;-)
Review of attachment 273340 [details] [review]: Mostly cosmetic comments. Overall, it is missing the gtk-doc strings at the beginning of the public methods. ::: libdocument/ev-annotation.c @@ +85,3 @@ +static void ev_annotation_text_markup_iface_init (EvAnnotationMarkupInterface *iface); +static void ev_annotation_attachment_markup_iface_init (EvAnnotationMarkupInterface *iface); +static void ev_annotation_text_markup_markup_iface_init (EvAnnotationMarkupInterface *iface); This name sound weird to me. Why don't use text_markup_iface instead? It is already in use, but for annotation_text, in such case, wouldn't be better to fix that interface instead? @@ +1381,3 @@ + + if (annot->quadrilaterals) { + g_object_unref(annot->quadrilaterals); Missing space before parenthesis. Shouldn't we use g_array_free instead? @@ +1389,3 @@ +ev_annotation_text_markup_init (EvAnnotationTextMarkup *annot) +{ + annot->quadrilaterals = g_array_new(FALSE, TRUE, sizeof(EvQuadrilateral)); Add space before the opening parenthesis @@ +1398,3 @@ +{ + EvAnnotationTextMarkup *tm_annot = EV_ANNOTATION_TEXT_MARKUP (annot); + EvAnnotationClass *p_class; Maybe name it ev_annotation_class instead, it is more consistent with the rest of the patch and easier to read. @@ +1400,3 @@ + EvAnnotationClass *p_class; + gboolean in_bounding_rect = FALSE; + GArray *quadrilaterals = NULL; Align the variables names @@ +1416,3 @@ + EvQuadrilateral quad = g_array_index(quadrilaterals, + EvQuadrilateral, + i); Space before opening parenthesis @@ +1489,3 @@ + switch (prop_id) { + case PROP_TEXT_MARKUP_QUADRILATERALS: + g_value_take_boxed(value, ev_annotation_text_markup_get_quadrilaterals(annot)); Space before parenthesis @@ +1529,3 @@ +{ + GArray *result = g_array_sized_new (FALSE, FALSE, + sizeof(EvQuadrilateral), Space before opening parenthesis. Why not quads_array instead of result? @@ +1539,3 @@ +gboolean +ev_annotation_text_markup_set_quadrilaterals (EvAnnotationTextMarkup *annot, + GArray *arr) quads_array is more meaningful than arr. @@ +1545,3 @@ + + if (annot->quadrilaterals->len) + g_array_remove_range (annot->quadrilaterals, 0, annot->quadrilaterals->len); Maybe we would be leaking memory by not freeing the quadrilaterals. You might want to set g_array_set_clear_func to the array.
Review of attachment 273339 [details] [review]: This looks good to me. Maybe put the star next to argument instead of the data type (to follow the convention with the rest of the code related to annotations)
Review of attachment 273337 [details] [review]: Just cosmetic comments. ::: backend/pdf/ev-poppler.cc @@ +2933,3 @@ + gdouble page_height; + EvRectangle ev_bound_rect; + PopplerRectangle poppler_rect; Align the variables @@ +2967,3 @@ + g_object_set(ev_annot, + "bounding-rectangle", &ev_bound_rect, + NULL); Space before opening parenthesis @@ +3064,3 @@ annot_mapping = g_new (EvMapping, 1); + ev_annotation_get_bounding_rectangle(ev_annot, + &annot_mapping->area); Space before parenthesis ::: libview/ev-view.c @@ +3121,2 @@ ev_annotation_set_color (annot, &color); + ev_annotation_set_bounding_rectangle(annot, &doc_rect); Space before opening parenthesis
Review of attachment 273341 [details] [review]: Just cosmetic comments. In general, use space before opening parenthesis. Also, add gtk-doc in public methods. ::: backend/pdf/ev-poppler.cc @@ +2990,3 @@ + NULL, &page_height); + + guint i; The declaration of the variable should be at the beginning of the block
*** Bug 729506 has been marked as a duplicate of this bug. ***
Created attachment 281553 [details] [review] Add bound rectangle property to EvAnnotation Hi, I reviewed these patches and have been using them on a branch to add new annotations. Small changes were made, there were some memory issues. Also, there are two patches from myself, one that renames the interface of some annotations to avoid the markup_markup duplication and another that implements the support for actually adding the new annotation to the backend. Germán's cosmetic comments were also addressed.
Created attachment 281554 [details] [review] Implements EvQuadrilateral
Created attachment 281555 [details] [review] Add support for interactive areas
Created attachment 281556 [details] [review] Renaming interface to avoid redundancy
Created attachment 281557 [details] [review] Implements EvAnnotationTextMarkup
Created attachment 281558 [details] [review] Adding backend support for adding new annotations
Created attachment 281559 [details] [review] Support for highlight annotation
Created attachment 281560 [details] [review] Support for strike out annotation
Created attachment 281602 [details] [review] Implements EvAnnotationTextMarkup without interactive areas. At the moment, interactive areas are not needed. This patch implements EvAnnotationTextMarkup without the methods for checking interactive areas. As a result, the patch that adds support for interactive areas is deprecated.
Created attachment 281613 [details] [review] back-end support for highlight annotation The bound rectangle property is already saved in the mapping, therefore we don't need it in the EvAnnotation object. This alters the highlight patch.
Created attachment 281614 [details] [review] back-end support for strike-out annotation Renaming the patch to avoid confusion. This is not full support of the annotation yet.
Review of attachment 281556 [details] [review]: I don't think we should rename these, see below ::: libdocument/ev-annotation.c @@ +74,3 @@ static void ev_annotation_markup_default_init (EvAnnotationMarkupInterface *iface); +static void ev_annotation_text_iface_init (EvAnnotationMarkupInterface *iface); +static void ev_annotation_attachment_iface_init (EvAnnotationMarkupInterface *iface); The pattern is: type_prefix + interface_name + init meaning that this is the function of type that initialize the vmethods of the interface_name. So, in case of text markup annotations, it should be something like: ev_annotation_text_markup_markup_iface_init() and there's no conflict. I know it sounds weird because the interface is EvAnnotationMarkup and the class that implements it in EvAnnotationTextMarkup
Review of attachment 281602 [details] [review]: Looks good in general, I have just a few comments ::: libdocument/ev-annotation.c @@ +1368,3 @@ + annot->quadrilaterals = NULL; + } +} You should chain up here to call the finalize of the parent class. @@ +1374,3 @@ +{ + annot->quadrilaterals = g_array_new (FALSE, TRUE, sizeof(EvQuadrilateral)); + g_array_set_clear_func (annot->quadrilaterals, (GDestroyNotify) g_value_unset); Why g_value_unset? We are not saving GValues but EvQuadrilaterals in the array, no? @@ +1455,3 @@ + * annotation. + * + * Since: 3.13 Use 3.14 instead @@ +1466,3 @@ + annot->quadrilaterals->len); + + return quads_array; I think we can return the internal array as a transfer none value instead of duplicating the array. @@ +1493,3 @@ + sizeof (EvQuadrilateral), + quads_array->len); + g_array_append_vals (quads, quads_array->data, quads_array->len); GArray is refcounted, we can take a reference instead of duplicating the array contents
Review of attachment 281558 [details] [review]: Pushed with the change I comment below. Thanks! ::: backend/pdf/ev-poppler.cc @@ +3142,3 @@ + break; + default: + poppler_annot = NULL; I think this should never happen, and if happens it's a bug, so I've added an assert here instead.
Review of attachment 281613 [details] [review]: Looks good in general, I have several comments, but most of them are cosmetic minor issues ::: backend/pdf/ev-poppler.cc @@ +2828,3 @@ + GArray *ev_quads = g_array_sized_new (FALSE, FALSE, + sizeof(EvQuadrilateral), + poppler_quads->len); Please move the initialization to a different line than the declaration. @@ +2837,3 @@ + PopplerQuadrilateral quad = g_array_index (poppler_quads, + PopplerQuadrilateral, + i); Ditto. @@ +2844,3 @@ + */ + ev_quad.p1.x = quad.p3.x; + ev_quad.p1.y = page_height - quad.p3.y; Do not line up the quads @@ +2855,3 @@ + ev_quad.p4.y = page_height - quad.p1.y; + + g_array_append_val(ev_quads, ev_quad); g_array_append_val( -> g_array_append_val ( @@ +2869,3 @@ + GArray *poppler_quads = g_array_sized_new (FALSE, FALSE, + sizeof(PopplerQuadrilateral), + ev_quads->len); Same here @@ +2878,3 @@ + EvQuadrilateral ev_quad = g_array_index (ev_quads, + EvQuadrilateral, + i); Ditto @@ +2884,3 @@ + * lower-left, lower-right + */ + poppler_quad.p1.x = ev_quad.p4.x; Same comment about lines ups @@ +2896,3 @@ + poppler_quad.p4.y = page_height - ev_quad.p2.y; + + g_array_append_val(poppler_quads, poppler_quad); and the space before the ( @@ +3040,3 @@ + PopplerAnnotTextMarkup *poppler_text_markup; + EvAnnotationTextMarkup *ev_annot_text_markup; + GArray *poppler_quads = NULL; This doesn't need to be initialized to NULL. @@ +3046,3 @@ + + poppler_quads = poppler_annot_text_markup_get_quadrilaterals (poppler_text_markup); + if (poppler_quads && poppler_quads->len) { Maybe this check could be done in ev_quads_from_poppler_quads() with an early return NULL, and then you don't need to initialize ev_quads either. @@ +3050,3 @@ + } + + ev_annot_text_markup = EV_ANNOTATION_TEXT_MARKUP (ev_annot); This cast is unneeded if you use g_object_set, if you use the ev_annot API, you could do the cast in place since it's only used once ev_annot_text_markup_set_quads (EV_ANNOTATION_TEXT_MARKUP (ev_annot), quads); @@ +3052,3 @@ + ev_annot_text_markup = EV_ANNOTATION_TEXT_MARKUP (ev_annot); + + g_object_set (ev_annot_text_markup, Don't we have a ev_annot_text_markup_set_quads method? @@ +3240,3 @@ + GArray *poppler_quads = poppler_quads_from_ev_quads (ev_quads, page); + + poppler_annot = poppler_annot_text_markup_new_highlight (pdf_document->document, &poppler_rect, poppler_quads); You are leaking the poppler_quads here. ::: libdocument/ev-annotation.c @@ +1427,3 @@ + "page", page, + NULL)); + if (annot) This can't be NULL here. @@ +1428,3 @@ + NULL)); + if (annot) + annot->type = EV_ANNOTATION_TYPE_HIGHLIGHT; I'm not sure this is the right place for this, since TextMarkup annotations have their own subtype, I think we could add a construct property type and a new enum for the text markup annot types. When the property is set, it actually sets the type of the parent.
Created attachment 281753 [details] [review] Add EvAnnotationTextMarkup class Thank you Carlos. I updated the patches according to your comments and without the interface renaming.
Created attachment 281754 [details] [review] pdf: add support for POPPLER_ANNOT_HIGHLIGHT
Created attachment 281755 [details] [review] pdf: add support for POPPLER_ANNOT_STRIKEOUT
Created attachment 281804 [details] [review] Add EvAnnotationTextMarkup class Fixed memory problem.
Created attachment 281805 [details] [review] pdf: add support for POPPLER_ANNOT_HIGHLIGHT Fixed memory problem.
Created attachment 281806 [details] [review] pdf: add support for POPPLER_ANNOT_STRIKEOUT Fixed memory problem.
Created attachment 281848 [details] [review] Add EvQuadrilateral type required for text markup annotations Hi Carlos, I rebased the patches on master and split them into more punctual changes. I am posting here the ones that are necessary to read highlight annotations and list them in the sidebar. Now we can select the annotation in the list and the view navigates to it, but highlight annotations are not yet editable. Best, Giselle
Created attachment 281849 [details] [review] Add EvAnnotationTextMarkup class
Created attachment 281850 [details] [review] libdocument: adding text markup highlight
Created attachment 281851 [details] [review] pdf: reading poppler annot highlight
Created attachment 281852 [details] [review] shell: icon for text markup annotations
Created attachment 289831 [details] [review] Creates a trivial class for text markup annotations Starting over on this problem. After implementing all text highlight annotation we decided the quadrilaterals are not needed. In any case, they are definitely not needed for reading the highlights. I am attaching three patches which are sufficient for evince to recognize highlighting in documents and add those to the list of annotations in the sidebar.
Created attachment 289832 [details] [review] libdocument: adding annotation highlight Second patch.
Created attachment 289833 [details] [review] Recognizes poppler highlight annotations and lists in the sidebar. Third patch.
Review of attachment 289831 [details] [review]: Thanks for the patch, and sorry for the delay reviewing this. I've just pushed it. ::: libdocument/ev-annotation.c @@ +1250,3 @@ +ev_annotation_text_markup_init (EvAnnotationTextMarkup *annot) +{ + EV_ANNOTATION (annot)->type = EV_ANNOTATION_TYPE_UNKNOWN; This shouldn't be needed, since it's already done by the parent init method
Review of attachment 289832 [details] [review]: Pushed as well
Review of attachment 289833 [details] [review]: ::: backend/pdf/ev-poppler.cc @@ +2875,3 @@ + case POPPLER_ANNOT_HIGHLIGHT: { + ev_annot = ev_annotation_text_markup_highlight_new (page); + } You don't need the {} here. I've pushed this too as a separate patch. ::: shell/ev-sidebar-annotations.c @@ +427,3 @@ } + if (EV_IS_ANNOTATION_TEXT (annot) || EV_IS_ANNOTATION_TEXT_MARKUP (annot)) { This is not correct, we should use a different icon for markup annots. All annots are actually temporary solutions until we have a better icon, so for now I think we can use select-all. I've pushed a patch to use select-all.
After thinking again about this, I've changed text markup annots to use a common annotation type. Since we are using the same class in the end, I think it's better to use a type attribute for the different text markup annots. This way it's imposible to create a text markup annotation with an unknown annotation type, for example, and will also allow us to change the markup type without having to delete the annotation and create a new one.
Hi Carlos, thank you for pushing the patches! You make a good point regarding the types of annotations, but as I was rebasing the branch supporting adding annotations on top of these changes I realised something. When the user clicks on a button in the sidebar to create a text-markup annotation, we need to now in advance the type of annotation he/she wants (not only if it is text markup, but also highlight, strike out, etc). At this point, there is no annotation object created yet, and the type is passed as a parameter to ev-view so that the correct annotation is created. If we just have a generic text-markup type, how will we know which object to create? Is the problem clear? Happy new year :) Best, Giselle
(In reply to comment #71) > Hi Carlos, > > thank you for pushing the patches! > You make a good point regarding the types of annotations, but as I was rebasing > the branch supporting adding annotations on top of these changes I realised > something. When the user clicks on a button in the sidebar to create a > text-markup annotation, we need to now in advance the type of annotation he/she > wants (not only if it is text markup, but also highlight, strike out, etc). At > this point, there is no annotation object created yet, and the type is passed > as a parameter to ev-view so that the correct annotation is created. If we just > have a generic text-markup type, how will we know which object to create? > Is the problem clear? I'm not sure I understand. When creating a new text markup annotation, the user should always select first the type of text markup, or we should use one as default (highlight). I think it's better to force the user to choose one, so that instead of having a button, we would use a menu or a button menu with a default value. > Happy new year :) Happy new year! > Best, > Giselle
(In reply to Carlos Garcia Campos from comment #72) > > I'm not sure I understand. When creating a new text markup annotation, the > user should always select first the type of text markup, or we should use > one as default (highlight). I think it's better to force the user to choose > one, so that instead of having a button, we would use a menu or a button > menu with a default value. It's exactly this type of annotation that I am talking about. Take the following procedure: 1. user selects an annotation to add on the menu (let's say it's highlight) 2. user clicks on the screen 3. ev_view_create_annotation needs to be called. This method will be responsible for instantiating the correct annotation. It receives the annotation type as a parameter. If the only annotation type available is MARKUP, it does not know which object to create (or view will not know what to draw on the screen). I came upon this problem while trying to rebase wip/highlight. Is it ok with you if, when reworking the wip/highlight code, I add the different types of markup annotations again? Best, Giselle
(In reply to giselle from comment #73) > (In reply to Carlos Garcia Campos from comment #72) > > > > I'm not sure I understand. When creating a new text markup annotation, the > > user should always select first the type of text markup, or we should use > > one as default (highlight). I think it's better to force the user to choose > > one, so that instead of having a button, we would use a menu or a button > > menu with a default value. > > It's exactly this type of annotation that I am talking about. Take the > following procedure: > > 1. user selects an annotation to add on the menu (let's say it's highlight) > 2. user clicks on the screen > 3. ev_view_create_annotation needs to be called. > > This method will be responsible for instantiating the correct annotation. It > receives the annotation type as a parameter. If the only annotation type > available is MARKUP, it does not know which object to create (or view will > not know what to draw on the screen). I came upon this problem while trying > to rebase wip/highlight. > I understand the problem now, sorry, I had forgotten that the window passes the annotation type to the view that eventually creates the annotation. We will have the same problem, if we want to be able to add a Text annotation with a different icon initially, for example. I don't remember why it's done this way, I guess it's because the annotation has to be created with a rectangle, so we wait until we know that. Maybe we could a new parameter, for annotation subtypes, using a guint, since all subtypes will always be enum values, passing 0 for annots without a subtype, that will be ignored anyway. This way we could pass the EvAnnotationTextIcon or EvAnnotationTextMarkupType. I don't really like the solution that much, but I can't think of something better. > Is it ok with you if, when reworking the wip/highlight code, I add the > different types of markup annotations again? I prefer to keep the EvAnnotationTextMarkupType, because changing the type of an annotation doesn't make sense, but changing a subtype in this case does. I prefer to find a solution in the ev_view_begin_add_annotation side. > Best, > Giselle
*** Bug 747868 has been marked as a duplicate of this bug. ***
I've been working with Giselle's branch and I've just pushed initial support for highlight annotations. There are still some things to do, performance can be improved, and I'm sure there will be some bugs. Please, use new bug reports for those things, I'm closing this finally. Thank you.
*** Bug 762218 has been marked as a duplicate of this bug. ***