GNOME Bugzilla – Bug 785627
Proposed patches to history
Last modified: 2018-05-22 17:17:03 UTC
There have been many complaints in the past with the behavior of history in Evince. I would like to propose some fixes. Currently, EvHistory listens for the “page-changed” signal and adds a page to the history when the new page differs from the old page by >1. This leads to all sorts of inconsistencies and frustrating behavior: *One cannot go back to the old page since it is currently not added to the history. This makes reading technical papers quite frustrating since one often would like to jump to an equation or bibliographical reference and return to where one left off. There have been some proposals to add both the old page and the new page to the history, but so far no patches appear to have been merged. (Bug 710681) *Such a fix would not address the case when the target of the link is on the next page. For instance, if one navigates through an article in the following order 1->2->3->4->5 and then jumps to an equation which happens to be on page 6, then pressing the back button would yank one back to page 1. *Any easy fix of course would be to have EvHistory keep track of every page change (reference). However some Evince developers have previously objected to such behavior; thus only page increments of 2 or more are recorded. (Bug 696891). Consequently, ordinary scrolling in dual page mode causes many pages to be added to the history since the page number always jumps by at least 2. There is no reason dual page mode should behave different from single page mode with respect to the history. Also, currently the <alt>P and <alt>N shortcuts to jump backward and forward in history don’t work. (Bug 781323)
Created attachment 356638 [details] [review] Rearchitect history management part 1
Created attachment 356639 [details] [review] Rearchitect history management part 2
The patch I am proposing changes decouples EvHistory from page changes and moves most of the burden of updating the history to EvWindow (which holds an instance of EvHistory). The patch series I am proposing *disconnects EvHistory from the “page-changed” signal. *makes ev_history_add_link_for_page a public method *changes the callback for “handle-link” in EvWindow to explicitly add the previous page to the history in addition to the activated link. *Creates a “bookmark-selection-changed” signal for EvSidebarBookmarks, which is emitted when the user activates a bookmark *Adds history logic to EvWindow’s “goto-bookmark” action. The guiding principle is that although we might not want to record ordinary scrolling in the history, we always want to record page changes that result from selecting a link. These modifications are more invasive than those proposed by Anuj in Bug 710681 but seem more natural; in particular they avoid awkward gymnastics with modifying the title of the current link in the history. In conjunction with a proposed GTK patch to fix some keyboard accelerators (Bug 785286) for going forward and backward in history, this patch seems to resolve some longstanding complaints about the behavior of the history in Evince. I have posted some test builds of Evince with these patches, as well as a patched GTK, in an Ubuntu PPA. Please let me know if you would like other ways to test as well.
PPA link: https://launchpad.net/~casey-jao/+archive/ubuntu/testing Please let me know if you would like builds for other distributions as well. Note that since EvHistory no longer listens for page changes, Evince would no longer log in the history history when one jumps to the first or last page via Ctrl+Home or Ctrl+End. (This constitutes a regression from before (Bug 696891); however, the previous behavior also displayed an annoying inconsistency since scrolling to the first or last page in continuous mode via Home or End adds intermediate pages to the history. (Bug 758598). This issue should be addressed in a later patch.
My apologies, the reference for the GTK bug should be Bug 785236.
Review of attachment 356638 [details] [review]: As a first step for review, can you try to split this patch... For instance, the add new bookmark-selection-changed makes sense and it's independent of the rest, so it would be better to split this part (at least). Maybe other parts can also be split. thank you for working on this.
Created attachment 356643 [details] [review] History management patch: core logic
Created attachment 356644 [details] [review] History management patches: bookmarks
Done.
Please do let me know if you would like the patches split further. Also, in the long term these are just interim fixes -- eventually I would like the history to record not just one's previous page before jumping to a link but also the position on the page -- in other words, instead of adding a dest_page link one would like to add a dest_xyz link. But it may be better to work on those patches under a separate bug later so that we don't change too many moving parts at once.
Review of attachment 356643 [details] [review]: Thanks for working on this. It's a bit late in the release cycle for a change in behavior like this, but since it's something many people have complained about I'm open to push this right before the freeze. It would be great if other people who complained and tried to fix ti could test this patches and provide feedback.
Review of attachment 356644 [details] [review]: ::: shell/ev-sidebar-bookmarks.c @@ +42,3 @@ +enum { + SELECTION_CHANGED, I'm not sure selection changed is the best name. Maybe just activated? This is emitted when a bookmark is activated no? @@ +108,3 @@ selection = gtk_tree_view_get_selection (GTK_TREE_VIEW (priv->tree_view)); page = ev_sidebar_bookmarks_get_selected_page (sidebar_bookmarks, selection); + gint old_page = ev_document_model_get_page (priv->model); Please, move the variable declaration to the declarations block. ::: shell/ev-sidebar-bookmarks.h @@ +51,2 @@ void (*add_bookmark) (EvSidebarBookmarks *sidebar_bookmarks); + void (*selection_changed) (EvSidebarBookmarks *sidebar_bookmarks, This is not correctly indented.
Created attachment 357030 [details] [review] History management path: bookmarks Incorporates suggestions from Comment 12.
Created attachment 357735 [details] [review] Fixes an error in the "core logic" patch in ev-view-private.h The argument "old_page" in the default "handle_link" handler (which is never called) should be a gint, not a pointer.
Created attachment 357920 [details] [review] Add pages to history when jumping to the first or last page This final patch restores the previous behavior of adding pages to history when the user jumps to the first or last page in the document. Since we no longer rely on the "page-changed" signal to manage the history, the behavior is consistent whether one is in continuous mode. This addresses Bug 696891 and Bug 758598.
These patches were written against version 3.24.0 (since that is what I was using at the time) and do not apply directly to the 3.25.91 or master branches. One needs to first apply them to the 3.24.0 branch and then merge them into master. Is it necessary to post the modified patches here?
Yes, please rebase them.
Created attachment 359263 [details] [review] History patches: core logic Rebased to 3.25.91
Created attachment 359264 [details] [review] History patches: bookmark functionality Rebased to 3.25.91
Created attachment 359265 [details] [review] History patches: record in history when jumping to first or last page Rebased to 3.25.91
Created attachment 361672 [details] [review] History patches: core logic Rebased to 3.26.0
Created attachment 361673 [details] [review] History patches: bookmark functionality Rebased to 3.26.0
Created attachment 361674 [details] [review] History patches: add links when jumping to first or last page Rebased to 3.26.0 I have been using these patches for a while. Any chance of getting these reviewed for the next release cycle?
To make testing easier, I have prepared a github repository at https://github.com/cjao/evince-testing with the patches together with a manifest for building with flatpak-builder (essentially copied from the gnome-nightly repository).
Created attachment 370631 [details] [review] History patches: core logic Rebase to 3.27.92
Created attachment 370632 [details] [review] History patches: bookmarks Rebase to 3.27.92
Created attachment 370633 [details] [review] History patches: add links when jumping to first or last page Rebase to 3.27.92
I applied your patches against master. I like how following a link works (i.e. jumping to a reference and going back). Ideally it would be great to leave the view exactly the same as before jumping (e.g. if I am viewing the bottom-half of the page before jumping, go back there), but that is not important for the moment. There is a use case that puzzle me: I have a document, which I skim to the Table of Contents. A links there takes me to the List of Tables, and a link there takes to an actual table. Thus, the history has: * Page 1. First page * Page 5. Table of contents (page where I first followed a link). * Page 9. List of Tables (page where I arrived from the previous step), and where I follow a link to a table) * Page 125. Page with the table I followed. From now on, I skim or read the document page by page. I get to page 141, and I decide to check something. * Back takes me to Page 9 (because the history assumes that if I am reading from 125, I want to go back to history -1) * Forward takes me to page 125. But, I would like to go where I was, that is, page 141. I would think that if I am in a page that is far from the last item item in the history, then it should be added. Does this make sense for you?
Review of attachment 370631 [details] [review]: ::: libview/ev-view-private.h @@ +267,3 @@ GtkOrientation orientation); void (*handle_link) (EvView *view, + gint old_page, Nitpick: semantically it seems that previous_page is clearer than old_page. ::: libview/ev-view.c @@ +7472,3 @@ + NULL, + G_TYPE_NONE, 2, + G_TYPE_INT, G_TYPE_OBJECT); The last line is not indenten as the previous lines: ^I^I^I+space instead of ^I^I+spaces. Although the indentation might not be consistent across the whole code, at least it should be consistent in the neighbourhood. ::: shell/ev-history.h @@ +60,3 @@ EvLink *link); +void ev_history_add_link_for_page (EvHistory *history, + gint page); I think it should be better just 'ev_history_add_page'. ::: shell/ev-window.c @@ +913,3 @@ } } + ev_history_add_link_for_page (window->priv->history, old_page); Why should this call be here? It seems the whole method is about handling a link. But, the same method is being used for dual purpose. I wonder if it would be better to add a signal to handle the page. What do other people think?
Review of attachment 370632 [details] [review]: ::: shell/ev-sidebar-bookmarks.c @@ +231,3 @@ if (page >= 0) { + gint old_page = ev_document_model_get_page (priv->model); + g_signal_emit (sidebar_bookmarks, signals[ACTIVATED], 0, old_page, page); Indentation is inconsistent. I prefer previous_page instead of old_page, as it seems semantically clearer. @@ +550,3 @@ + NULL, + G_TYPE_NONE, 2, + G_TYPE_INT, G_TYPE_INT); Indentation is inconsistent. ::: shell/ev-sidebar-bookmarks.h @@ +52,3 @@ + void (*activated) (EvSidebarBookmarks *sidebar_bookmarks, + gint old_page, + gint page); In ev-history, they are called old_page and new_page. Considering they have the same purpose, the name could be consistent.
Review of attachment 370633 [details] [review]: ::: shell/ev-window.c @@ +77,3 @@ #include "ev-keyring.h" #include "ev-view.h" +#include "ev-view-private.h" We should not be calling a private part of the library. Unless I am missing something.
(In reply to Germán Poo-Caamaño from comment #28) > > I would think that if I am in a page that is far from the last item item in > the history, then it should be added. > > Does this make sense for you? We could splice such a page into the history upon going back or forward. But I would need to test such a patch for a while to see how well it would work.
(In reply to Germán Poo-Caamaño from comment #29) > Review of attachment 370631 [details] [review] [review]: > > ::: shell/ev-window.c > @@ +913,3 @@ > } > } > + ev_history_add_link_for_page (window->priv->history, old_page); > > Why should this call be here? It seems the whole method is about handling a > link. But, the same method is being used for dual purpose. > > I wonder if it would be better to add a signal to handle the page. What do > other people think? Thanks for the comments. The purpose of that call in view_handle_link_cb is to save the current page in the history before jumping to another page.
(In reply to Germán Poo-Caamaño from comment #29) > Review of attachment 370631 [details] [review] [review]: > > ::: libview/ev-view-private.h > @@ +267,3 @@ > GtkOrientation orientation); > void (*handle_link) (EvView *view, > + gint old_page, > > Nitpick: semantically it seems that previous_page is clearer than old_page. Perhaps. But "old_page" is already used in several other contexts (like "page_changed_cb"). Should those be changed as well?
Created attachment 370946 [details] [review] History patches: core logic Revised patch.
Created attachment 370947 [details] [review] History patches: bookmarks revised patch
Created attachment 370948 [details] [review] History patches: add links when jumping to first or last page revised patch
-- 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/810.