GNOME Bugzilla – Bug 509029
Keybindings for jumping back/forward in history
Last modified: 2014-04-15 13:01:35 UTC
[ From http://bugs.debian.org/460259 by Daniel Blaschke ] In a Debian bug report it was suggested that keybindings be added to jump back and forward in history (for example after following a hyperlink to the bibliography of a document). Xpdf apparently does this by using the b and v keys. I'm not sure how this will fit in with in with the redesign of the "Back" button (bug 424833), but I guess it's something to consider.
Please stick to the known browser shortcuts (http://www.codinghorror.com/blog/2006/02/standard-browser-keyboard-shortcuts.html). Most of them are already implemented in Evince. So jumping back/forward in history should be Alt+left and Alt+right, respectively.
It would be nice if you added binding for mouse buttons back and forth.
Created attachment 257807 [details] [review] Shortcuts for going back/forward in history.
New to bugzilla. My comments about the bug disappeared. I have added <alt>p and <alt>n as shortcuts to go back and forward in history. I am a beginner just learning to contribute to evince. I am looking for a guide to my patch.
Review of attachment 257807 [details] [review]: I would prefer if you used the Alt+left and Alt+right keys for jumping back/forward in history, as suggested by Jakob Voss in comment #1. I didn't have the time to test it yet though. ::: shell/ev-window.c @@ +6088,3 @@ + G_CALLBACK(ev_window_cmd_go_previous_history)}, + { "GoForwardHistory",NULL, N_(""),"<alt>N", + N_("Go to forward page in history"), "Go to forward page in history" sounds terrible. Maybe "Go to next page in history" or "Go forward one page in history"?
(In reply to comment #5) Alt+left and Alt+right are already in use. They are used to scroll horizontally when the document is in zoomed mode. Hence I used alt+p and alt+n. > Review of attachment 257807 [details] [review]: > > I would prefer if you used the Alt+left and Alt+right keys for jumping > back/forward in history, as suggested by Jakob Voss in comment #1. > I didn't have the time to test it yet though. > > ::: shell/ev-window.c > @@ +6088,3 @@ > + G_CALLBACK(ev_window_cmd_go_previous_history)}, > + { "GoForwardHistory",NULL, N_(""),"<alt>N", > + N_("Go to forward page in history"), > > "Go to forward page in history" sounds terrible. Maybe "Go to next page in > history" or "Go forward one page in history"?
Created attachment 257828 [details] [review] Changed tooltip to "Go to next page in history"
Review of attachment 257828 [details] [review]: ::: shell/ev-window.c @@ +4629,3 @@ + ev_history_go_back(history); +} + Fix the style. Space between the function name and the parenthesis. @@ +4638,3 @@ + ev_history_go_forward(history); +} + Same here. @@ +6089,3 @@ + { "GoForwardHistory",NULL, N_(""),"<alt>N", + N_("Go to next page in history"), + G_CALLBACK(ev_window_cmd_go_forward_history)}, We use _(...) instead of N_(...). Also, fix the style as mentioned previously.
(In reply to comment #6) > (In reply to comment #5) > Alt+left and Alt+right are already in use. They are used to scroll horizontally > when the document is in zoomed mode. Hence I used alt+p and alt+n. This can be changed. Although, I would wait for any of the maintainers to say anything (easy to change, though). Behdad makes a good case in: https://bugzilla.gnome.org/show_bug.cgi?id=655469 "Both Adobe Reader and Google Chrome use Left/Right Arrow keys to switch to prev/next page when there is no need for horizontal scroll. In Evince they work only if view is set to "Best Fit", but not when it's set to a percentage value or "Fit Page Width" (which is the most popular view AFAIK). I think it would very helpful to have a consistent behaviour for arrows keys, thus making Evince use Left/Right arrow keys for page navigation as long as there is need for horizontal scrolling."
Review of attachment 257828 [details] [review]: Regarding to the commit message: Try to makes them fit to column 72. It makes easier to read them later in a console. Run 'git log' in your evince repository to get an idea. You will also notice that some commits messages breaks this "rule", but still it is good practice to try to follow it.
Created attachment 258097 [details] [review] Shortcuts to go back and forward in history Made the changes suggested above regarding coding style, and size of commit message.
(In reply to comment #8) > Review of attachment 257828 [details] [review]: > > ::: shell/ev-window.c > @@ +4629,3 @@ > + ev_history_go_back(history); > +} > + > > Fix the style. Space between the function name and the parenthesis. > > @@ +4638,3 @@ > + ev_history_go_forward(history); > +} > + > > Same here. > > @@ +6089,3 @@ > + { "GoForwardHistory",NULL, N_(""),"<alt>N", > + N_("Go to next page in history"), > + G_CALLBACK(ev_window_cmd_go_forward_history)}, > > We use _(...) instead of N_(...). Also, fix the style as mentioned previously. Changed it to "" instead of N_(""). I did this because "" was being used instead of _("") at other places in code.
Review of attachment 258097 [details] [review]: Thanks for the patch, it looks good in general, but there are a few minor details. You should also disable the actions when going back/forward is not possible, see ev_window_update_actions_sensitivity(), you can use ev_history_can_go_back/forward. ::: shell/ev-window.c @@ +4624,3 @@ +ev_window_cmd_go_previous_history (GtkAction *action, EvWindow *ev_window) +{ + g_return_if_fail (EV_IS_WINDOW (ev_window)); This is not correctly indented, we use 8 spaces @@ +4627,3 @@ + + EvHistory *history = ev_window->priv->history; + ev_history_go_back (history); We don't need the variable, you can simply use ev_history_go_back (ev_window->priv->history); @@ +4636,3 @@ + + EvHistory *history = ev_window->priv->history; + ev_history_go_forward (history); Ditto. @@ +6089,3 @@ + { "GoForwardHistory", NULL, "", "<alt>N", + N_("Go to next page in history"), + G_CALLBACK (ev_window_cmd_go_forward_history) }, Indentation is wrong here too, please check the other entries. ::: shell/evince-ui.xml @@ +91,3 @@ + <accelerator name="PreviousHistoryAccel" action="GoPreviousHistory"/> + <accelerator name="ForwardHistoryAccel" action="GoForwardHistory"/> The action names are a bit inconsistent, use either previous/next or back/forward. I would use back/forward, since it's what we already use in the EvHistory API.
Created attachment 258438 [details] [review] Shortcuts for going back/forward in history I have made the changes suggested with respect to indentation. Also changed function names and accelerator names appropriately. Regarding disabling the actions when going back/forward is not possible, I am stuck with an issue. I wrote the following code in the function ev_window_update_actions_sensitivity function: if (ev_history_can_go_back (ev_window->priv->history)){ ev_window_set_action_sensitive (ev_window, "GoBackHistory", TRUE); } else { ev_window_set_action_sensitive (ev_window, "GoBackHistory", FALSE); } The else case is being reached after going one page back in history, and there are still pages in the history to go back. And hence, <alt>P doesn't let me go back any more. Along with this, the button to go back in history also becomes deactivated. Can someone help me with this?
(In reply to comment #14) .. > The else case is being reached after going one page back in history, and there > are still pages in the history to go back. And hence, <alt>P doesn't let me go > back any more. Along with this, the button to go back in history also becomes > deactivated. > Can someone help me with this? It seems to be because of this https://bug509029.bugzilla-attachments.gnome.org/attachment.cgi?id=258438 . Also, the validation is done inside ev_history_go_back / forward as well, so even if you don't add the code to change sensitivity, history won't change if it can't go forward/backward.
(In reply to comment #14) > The else case is being reached after going one page back in history, and there > are still pages in the history to go back. And hence, <alt>P doesn't let me go > back any more. Along with this, the button to go back in history also becomes > deactivated. > Can someone help me with this? (In reply to comment #15) > It seems to be because of this > https://bug509029.bugzilla-attachments.gnome.org/attachment.cgi?id=258438 . Sorry, added the wrong link.! https://bugzilla.gnome.org/show_bug.cgi?id=724549
Created attachment 272476 [details] [review] Add shortcuts for going back/forward in history I have added the code to disable the actions when going back/forward is not possible, in Spandana's patch (Comment #14). I connected it to history changed signal to prevent actions from getting disabled after one click on back/forward button as mentioned in previous comments.
Review of attachment 272476 [details] [review]: This looks good to me.
Review of attachment 272476 [details] [review]: I've removed the new translatable strings and pushed the patch to master. Thanks. ::: shell/ev-window.c @@ +6112,3 @@ + G_CALLBACK (ev_window_cmd_go_back_history) }, + { "GoForwardHistory", NULL, "", "<alt>N", + N_("Go to next page in history"), Since these actions are only for shortcuts, and thee messages you are using are not consistent with the history widget, I think we can simply not introduce new translatable string that are not exposed in the UI at all.