GNOME Bugzilla – Bug 736370
Reload just the page when changing the annotation properties
Last modified: 2018-05-22 15:51:54 UTC
Currently when updating the properties of an annotation we invalidate the complete pixbuf cacache with a call to ev_view_reload(). This is super slow on large documents... I know in the past we rejected making ev_view_reload_page() public but for this, I don't see any other proper solution. So I am attaching two patches. One that makes ev_view_reload_page public and the other which uses this function to reload just the page of the annotation instead of everything.
Created attachment 285783 [details] [review] libview: make ev_view_reload_page public
Created attachment 285784 [details] [review] shell: Reload only the page of the annotation when changing its properties
(In reply to comment #0) > Currently when updating the properties of an annotation we invalidate the > complete pixbuf cacache with a call to ev_view_reload(). This is super slow on > large documents... I know in the past we rejected making ev_view_reload_page() > public but for this, I don't see any other proper solution. So I am attaching > two patches. One that makes ev_view_reload_page public and the other which uses > this function to reload just the page of the annotation instead of everything. ev_view_reload_page() has a region argument that it's easier to handle inside the view, since you need to convert coordinates and take scroll into account. How would you solve the FIXME: update annot region only from the window? So, since the annotations are created and removed by the view, we could also make the view update them, with something like ev_view_update_annotation() that receives the annotation and save mask and calls ev_document_annotations_save_annotation(). The view can check whether the annotation is visible or not, when not visible we invalidate the annots in the page cache, and when visible we update the page and schedule a redraw of the annotation area.
Maybe ev_view_save_annotation() is a more consistent name, but you get the point.
My impression is that the FIXME was assuming that the rendering is slow on the cairo part, while it's slow on the poppler core part. I see us having a problem with reloading only the region of the annotation when we really have partial rendering support...or tile rendering...So I'd revisit this at the moment. My impression is that it does not help a lot... That being said. I understand and agree with your point about ev_view_reload. It's not simple to make it public API. While I understand the solution of adding a ev_view_save_annotation(), it looks a bit akward to me to call a a libview function to save the annotation...maybe a better option would be to emit a changed signal or somthing on the annotation or the page cache or whatever and then let the ev-view update the view itself?
(In reply to comment #5) > My impression is that the FIXME was assuming that the rendering is slow on the > cairo part, while it's slow on the poppler core part. No, repainting only the damage area is a just a micro optimization. I was not suggesting to do that to fix this problem, but just as a nice addition. > I see us having a problem > with reloading only the region of the annotation when we really have partial > rendering support...or tile rendering...So I'd revisit this at the moment. My > impression is that it does not help a lot... > > That being said. I understand and agree with your point about ev_view_reload. > It's not simple to make it public API. While I understand the solution of > adding a ev_view_save_annotation(), it looks a bit akward to me to call a a > libview function to save the annotation...maybe a better option would be to > emit a changed signal or somthing on the annotation or the page cache or > whatever and then let the ev-view update the view itself? I don't see what's wrong with ev_view_save_annotation() considering that we already have being_add_annotation() and remove_annotation() in EvView.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME'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.gnome.org/GNOME/evince/issues/509.