GNOME Bugzilla – Bug 649044
Cannot delete annotations
Last modified: 2014-08-15 11:26:50 UTC
There seems to be no way to delete existing annotations, be it by right-clicking the annotation icon or the item in the sidebar's list.
Deleting annots is not supported by poppler yet.
(In reply to comment #1) > Deleting annots is not supported by poppler yet. Is there a bug for this on fd.o bugzilla? I can't find it!
No there isn't.
(In reply to comment #3) > No there isn't. Well, that's pretty mind-boggling. Anyway, filed https://bugs.freedesktop.org/show_bug.cgi?id=40473
The poppler problem seems to be fixed (see here: https://bugs.freedesktop.org/show_bug.cgi?id=40473).
This is the same as bug #676152 where José Aliste states to have a patch ready.
*** Bug 676152 has been marked as a duplicate of this bug. ***
Created attachment 220200 [details] [review] Patch 1/4
Created attachment 220201 [details] [review] patch 2
Created attachment 220202 [details] [review] patch 3
Created attachment 220203 [details] [review] patch 4 Carlos, here are 4 patches that implement the remove annotation. Could you please review them when you have the time?
Review of attachment 220203 [details] [review]: ::: backend/pdf/ev-poppler.cc @@ +2899,3 @@ + PopplerAnnot *poppler_annot; + + PopplerPage *poppler_page; I forgot to erase this. Of course, I will erase this before committing.
Review of attachment 220200 [details] [review]: I think we don't need this, see my comments in the other patches.
Review of attachment 220201 [details] [review]: ::: libdocument/ev-document-annotations.c @@ +90,3 @@ + EvDocumentAnnotationsInterface *iface = EV_DOCUMENT_ANNOTATIONS_GET_IFACE (document_annots); + + return (iface->remove_annotation != NULL); You don't need parentheses here ::: libdocument/ev-document-annotations.h @@ +60,3 @@ + /* Text Markup Annotations */ + EV_ANNOTATIONS_SAVE_TEXT_MARKUP_TYPE = 1 << 9, This change looks unrelated, no?
Review of attachment 220202 [details] [review]: ::: shell/ev-window.c @@ +6797,3 @@ + + ev_document_doc_mutex_unlock (); + ev_view_reload_page (EV_VIEW (window->priv->view), page, NULL); The view adds annotations, so I think it makes sense if the view also removed the annotations. So, instead of making reload_page public we could add ev_view_remove_annot() and move this code to the view.
Review of attachment 220203 [details] [review]: ::: backend/pdf/ev-poppler.cc @@ +2906,3 @@ + + poppler_page_remove_annot (poppler_page, poppler_annot); + g_object_unref (poppler_annot); This is not enough, annots are also saved here in the backend, so you should remove the removed annot from the GHashMap.
(In reply to comment #16) > Review of attachment 220202 [details] [review]: > > ::: shell/ev-window.c > @@ +6797,3 @@ > + > + ev_document_doc_mutex_unlock (); > + ev_view_reload_page (EV_VIEW (window->priv->view), page, NULL); > > The view adds annotations, so I think it makes sense if the view also removed > the annotations. So, instead of making reload_page public we could add > ev_view_remove_annot() and move this code to the view. I am reworking on this. Even if it makes sense to have a ev_view_remove_annot(), the problem is that maybe you have two views associated with the same Document....So I presume that for that case we still need to make ev_view_reload_page public.
Seeing as Poppler has implemented this, can we have this enabled in 3.8 in some form? You can always iterate on the UI in later releases, but this is a really important feature for me.
I'm the developer for the UberStudent Linux distro for students in higher education and researchers. The issue of Evince not being able to delete annotations has brought me here to make this comment. I'm taking my time to do this. Annotating PDFs, and the ability to delete them, is nothing more than a basic essential function. The ability to delete annotations is ESSENTIAL if you want people in academia to use Linux. Without it, the issue snowballs into whole streams of developers and potential adopters simply dropping Linux as an alternative, and even opensource project developers who literally advocate usage of closed source alternatives (e.g., http://www.docear.org/support/user-manual/#PDF_X-Change_Viewer_Windows_and_Linux_users). An example of an academician's thoughts on why annotations AND the ability to delete them is so important is at http://verahill.blogspot.com/2013/02/338-annotating-pdfs-in-linux-revisited.html.
Stephen, your time would be better spent writing a patch for the problem or helping (in this case) get the existing patches corrected and merged.
Gokçan is working on this for his SoC that started today.
> this feature is ESSENTIAL if you want people in academia to use Linux If you are *the* developer for a distro whose users demand such feature so strongly, I seriously hope you're aware of the existence of a linux version of Adobe Acrobat Reader...
Please, calm down. This is not a forum.
I apologize for caring?
(In reply to comment #25) > I apologize for caring? Hi Stephen, no need to apologize, IMVHO. Personally, I appreciate distributions that want to improve PDF experience to their users and particularly if they want to use Evince for that purpose. A written media like Bugzilla, which receive different kind of reports from people with different backgrounds, sometimes leaves the door opened to misunderstandings because of different reasons (language barrier, wording, lack of inflections, etc) that usually are not an issue in face to face conversations. That said, this feature is important for many of us. However, evince is developed by volunteers in their spare time, and there are other issues that are as important as this one: fix bugs that make evince crash (some of them require several hours to debug), improve rendering support, improve forms supports, improve accessibility support, improve performance when documents can't load fast enough to be responsive, and so on.
I think Steven has all the reason. You shouldn't give the feature of add annotations if you won't to give the option of delete it. Unless you should warn to the user. In addition, this bug has, at least, 2 years old (I know 'add annotations' is possible since 11.04); I think there are few regrets to justify this feature has not been implemented yet. I want trust in free software, let me believe.
Created attachment 275729 [details] [review] libdocument: Add remove_annotation to DocumentAnnotations Patch clean up
Created attachment 275730 [details] [review] libview: add ev_view_remove_annotation Implement ev_view_remove_annotation to avoid making ev_view_reload public.
Created attachment 275731 [details] [review] shell: Add RemoveAnnot action to the view popup Patch clean up to make it fit with ev_view_remove_annotation
Created attachment 275733 [details] [review] libview: add ev_view_remove_annotation This patch closes the annotation window and unset the focus (likely on the annotation being removed)
Created attachment 275734 [details] [review] libdocument: add ev_mapping_list_set_remove Add ev_mapping_list_set_remove, necessary to remove the annotation from an mapping_list.
Created attachment 275735 [details] [review] pdf: Implement remove_annotation virtual func Addressed Carlos comment wrt remove the annotation from the mapping in ev-poppler. There are a couple of issues (maybe related to this set of patches or something I am missing): 1. New annotations cannot be selected under a specific circumstance: Remove every annotation in a page, add a new one. The annotation is added, but the cursor does not change. Like if ev_view didn't know there is one. This does not happen if there is at least one annotation left. 2. Exposes a bug weakness in Evince to name new annotations. For example: . Add 3 annotations in page 2: annot-2-0, annot-2-2, annot-2-3 . Add Remove the first or second annotation . Add a new one, the annotation is going to be named annot-2-3, which already exists and it is present. 3. The sidebar is not refreshed. So, the annotations removed are still there. Suggestions are welcome for the (1).
Created attachment 278935 [details] [review] Refreshing annotations' sidebar upon deletion I belive this fixes issue #3 from last comment. This is implemented on top of master plus 4 out the 5 previous patches posted by Germán. The only patch that was left out was the changes on the shell to add deletion to the pop-up menu (I had already implemented some changes on this menu before adding the delete option and simplifying it).
Created attachment 279421 [details] [review] Resets the annotation mapping of a page when it is empty This is regarding observation 1 of Germán. I think that the reason why the view was not recognizing the newly added annotation after deleting all other from the page is because the hashmap where the annotations are being kept still had a list (with 0 elements), and the page cache was never refreshed. After fixing this, another issue surfaced: - A document has an annotation A on page X with text T. - Delete A. - Add annotation B with text T'. - Save a copy of the document overwriting it. - The document is reloaded but now B has text T. - Close evince and open it again with the same document. - Annotation B has text T'. This might be related to the numbering bug Germán mentioned.
Created attachment 279423 [details] [review] Resets the annotation mapping when there are no more annotations Small fix on the previous patch.
Created attachment 279425 [details] [review] Making the annotation names unique by using timestamps. This fixes the bug I mentioned on comment 35 and observation 2 of comment 33.
Dear Evince developers, I understand Evince is developed in your spare time and I am very grateful for your efforts, believe me. But you will agree that it is simply ridiculous that this bug hasn't been fixed for more than three years now. We are not talking about making Evince use the GPU to render a document or having a nice "text reflow" feature as in other readers, we just want to be able to delete annotations. Let me be clear: I am not "whining" about this bug. I'm just wondering why it is taking so long. After all it is the default reader on GNOME (and, therefore, of many major distributions) and this bug affects tons of people (not all of whom are able or willing to file a bug report or append a comment to this thread). Is it really so difficult to fix it (it's a genuine question)? Thank you for your time.
(In reply to comment #38) > Dear Evince developers, > > I understand Evince is developed in your spare time and I am very grateful for > your efforts, believe me. But you will agree that it is simply ridiculous that > this bug hasn't been fixed for more than three years now. We are not talking > about making Evince use the GPU to render a document or having a nice "text > reflow" feature as in other readers, we just want to be able to delete > annotations. If people were working full time 3 years... it might sound ridiculous, as you state it. Sometimes it is just hours in a month. > Let me be clear: I am not "whining" about this bug. I'm just wondering why it > is taking so long. After all it is the default reader on GNOME (and, therefore, > of many major distributions) and this bug affects tons of people (not all of > whom are able or willing to file a bug report or append a comment to this > thread). Is it really so difficult to fix it (it's a genuine question)? Please, propose a solution for the problems in the patches.
Review of attachment 279425 [details] [review]: Cool, I have just pushed it with a few modifications, see below. Thanks! ::: backend/pdf/ev-poppler.cc @@ +3045,3 @@ if (!ev_annotation_get_name (ev_annot)) { + guint64 timestamp = g_get_real_time(); + gchar *name = g_strdup_printf ("annot-%lu", timestamp); We don't need to cache the timestamp in a local variable. You should use G_GUINT64_FORMAT instead of %lu. @@ +3226,3 @@ } else { + timestamp = g_get_real_time(); + name = g_strdup_printf ("annot-%lu", timestamp); So this is duplicated 3 times, we can move this into a helper function.
Review of attachment 275729 [details] [review]: Ok, pushed
Review of attachment 275731 [details] [review]: Pushed a new patch for the new GMenus instead of this one.
Review of attachment 275733 [details] [review]: Fixed the issues mentioned below and pushed too. ::: libview/ev-view.c @@ +3226,3 @@ + window = g_object_get_data (G_OBJECT (annot), "popup"); + if (window) + gtk_widget_hide (window); This is not correct. Annotation windows are children of the view. We should destroy the window and remove the children. @@ +3237,3 @@ + ev_page_cache_mark_dirty (view->page_cache, view->current_page); + + ev_view_reload_page (view, page, NULL); We should only redraw the annot area, I've added a FIXME there for now.
Review of attachment 275734 [details] [review]: Fixed the problems and pushed as well. ::: libdocument/ev-mapping-list.c @@ +174,3 @@ +/** + * ev_mapping_list_set_remove: ev_mapping_list_remove @@ +184,3 @@ +EvMappingList * +ev_mapping_list_remove (EvMappingList *mapping_list, + gconstpointer data) This is very confusing, because the function expects to receive a EvMapping, but you are using a gconstpointer data like if it expected a mapping data instead. We should explicitly use EvMapping * here. @@ +186,3 @@ + gconstpointer data) +{ + mapping_list->list = g_list_remove (mapping_list->list, data); The mapping is created with a GDestroyNotify to free the mapping data. You are leaking here both the mapping and its data.
Review of attachment 275735 [details] [review]: Same here, fixed the issues and pushed. ::: backend/pdf/ev-poppler.cc @@ +3187,3 @@ + + poppler_page_remove_annot (poppler_page, poppler_annot); + g_object_unref (poppler_annot); I don't think this is correct. It's working because you were leaking the EvAnnotation. When the EvAnnotation is created, the poppler annot is attached to the EvAnnot as data with g_object_unref as GDestroyNotify, so the poppler annot should be unrefed by the EvAnnotation when it's destroyed, not manually here by us. We should remove the poppler annot before removing the mapping from the list, because ev_ampping_list_remove will delete the EvAnnotation that will unref the poppler annot.
Review of attachment 279423 [details] [review]: Pushed the first part of the patch, and a new patch to add a flags parameter to ev_page_cache_mark_dirty(). ::: backend/pdf/ev-poppler.cc @@ +3118,3 @@ + list = ev_mapping_list_get_list (mapping_list); + + if (g_list_length (list) == 0) You can use ev_mapping_list_length() directly here. ::: libview/ev-page-cache.c @@ +220,3 @@ if (cache->flags & EV_PAGE_DATA_INCLUDE_ANNOTS) { + if (data->annot_mapping && g_list_length (ev_mapping_list_get_list (data->annot_mapping)) == 0) + data->annot_mapping = NULL; This is not exactly correct. This function returns the flags to be used depending on the actual contents. This should be reset always when the cache is marked as dirty, since we are going to get new mapping, and the old one is leaked. We need to add a way to specify what's dirty to reset it before caching it again.
Review of attachment 278935 [details] [review]: Fixed the issues and pushed. Thanks. ::: libview/ev-view-private.h @@ +241,3 @@ void (*annot_added) (EvView *view, EvAnnotation *annot); + void (*annot_removed) (EvView *view); You should also add the EvAnnotation parameter. ::: libview/ev-view.c @@ +3239,3 @@ ev_view_reload_page (view, page, NULL); + + g_signal_emit (view, signals[SIGNAL_ANNOT_REMOVED], 0, annot); annot might be destroyed at this point by ev_document_annotations_remove_annotation(), since we don't know if the caller has the last reference or not, we shuld ref the annot before removing it and unref it again after the signal emission. @@ +6646,3 @@ + g_cclosure_marshal_VOID__OBJECT, + G_TYPE_NONE, 0, + G_TYPE_NONE); This is not correct, you are using VOID__OBJECT, but saying there are 0 arguments, and in g_signal_emit you are passing the annot as argument. ::: shell/ev-window.c @@ +5795,3 @@ static void +view_annot_removed (EvView *view, + EvWindow *window) This should receive the EvAnnotation too
Ok, I've reviewed all the patches and addressed my own comments because we are very close to the freeze. All patches are now in master, please test it and report any problem, I might messed it up while merging patches. Thanks!.