GNOME Bugzilla – Bug 729249
Feature request: add "preserve zoom mode" option
Last modified: 2016-05-23 16:20:02 UTC
As was discussed in Bug 728410 Evince changes a zoom mode automatically if a corresponding annotation is present in a PDF document. Such a behavior makes experience of using Evince destructive. I propose to add an option, e.g. "Preserve Zoom Level", which makes Evince to change a zoom level only by user, thus in particular ignore PDF annotations like `/FitH`. What do you think about such an idea?
I for one would love to see this.
I have implemented this as a switch in the main menu. All details at https://github.com/eduard-beutel/evince-preserve-zoom.
Eduard, thanks for your work on this. I just pulled a copy of your GitHub repository, built and tried this. A few comments: - The Evince developers will probably require you to base your work on the version of Evince at git master (currently at version 3.13.90) before this will be accepted. - The Evince developers will probably want you to attach a patch file here, which is standard practice in the GNOME world, rather than only posting the URL of a GitHub repository. - You haven't documented this new feature in Evince's help pages. Probably the page "Moving around a document" would be the right place for this to be explained. - With your current implementation, I need to select Preserve Zoom again every time I run Evince, which is a pain. I pretty much always want this option to be on. You could either (a) remember it between Evince runs for each document separately (just like Evince does for the Inverted Colors option), or (b) just make this a global setting that's shared between all documents, and also preserved between Evince runs. Either way would be fine with me.
Adam, thank you for all the helpful input. I'll get right on it. If you select Preserve Zoom in any document and then Save Current Settings as Default, Preserve Zoom will remain activated for all documents. I will post a 3.13.90 patch here.
Aha - I hadn't noticed the Save Current Settings as Default option. Great to know about this - thanks.
Created attachment 284316 [details] [review] New Preserve Zoom Feature (Bug 729249) Subject: [PATCH] New Preserve Zoom Feature (Bug 729249) - implemented preserve zoom switch that binds to a preserve_zoom attribute in EvView. When this is activated links are blocked from changing the zoom level. See goto_dest() in ev-view.c. - feature is implemented as optional, that means false by default - integrated with settings / save current settings as default - wrote help Patch for 3.13.90.
I forgot to mention, you can test using: http://spivey.oriel.ox.ac.uk/mike/zrm/zrm.pdf http://arxiv.org/pdf/hep-ph/9609377v1
Thanks, Eduard! I hope that one of the Evince developers will be able to review this before long. I'd be happy to see this land.
My pleasure. I'm also waiting on a review. @evince-maint: If there's anything I can do to help speed up integration, please let me know.
Review of attachment 284316 [details] [review]: Thanks for the patch and sorry for the delay reviewing it. ::: data/org.gnome.Evince.gschema.xml.in @@ +75,3 @@ <default>132</default> </key> + <key type="b" name="preserve-zoom"> I don't think this should be in the default schema, this should be a global setting like override-restrictions or auto-reload. The name preserve-zoom is probably a bit confusing, you should add a description explaining what this is for, and probably considering a better name . . . ::: libview/ev-view-private.h @@ +178,3 @@ GtkWidget *loading_window; guint loading_timeout; + gboolean preserve_zoom; I would probably use sometihng like allow_links_change_zoom or something like that ::: libview/ev-view.c @@ +1838,3 @@ +{ + ev_document_model_set_page (view->model, page); +} It looks unnecessary to add this wrapper. @@ +1874,3 @@ type = ev_link_dest_get_dest_type (dest); + if( view->preserve_zoom && if( view -> if (view @@ +1877,3 @@ + view->sizing_mode == EV_SIZING_FIT_PAGE && + view->continuous == FALSE) { + goto_page(view, page); leave a space between function name and ( @@ +1878,3 @@ + view->continuous == FALSE) { + goto_page(view, page); + } else if(view->preserve_zoom) { if(view -> if (view @@ +1879,3 @@ + goto_page(view, page); + } else if(view->preserve_zoom) { + goto_y_dest(view,dest); leave a space between function name and ( @@ +1904,3 @@ + break; + default: + g_assert_not_reached (); This is not correctly indented. I think I prefer to add an if to every goto_*_dest method to do everything instead of changing the zoom. @@ +8916,3 @@ + +void +ev_view_set_preserve_zoom (EvView *view, gboolean preserve_zoom) ev_view_set_allow_links_change_zoom() @@ +8922,3 @@ + +gboolean +ev_view_get_preserve_zoom (EvView *view) Same for the getter ::: libview/ev-view.h @@ +58,3 @@ +void ev_view_set_preserve_zoom (EvView *view, + gboolean preserve_zoom); +gboolean ev_view_get_preserve_zoom (EvView *view); Please line up the arguments with the previous ones. ::: shell/ev-window.c @@ +1114,3 @@ + + /* View */ + if (!ev_metadata_has_key (metadata, "preserve-zoom")) { I don't think this is something you might want to do differently depending on the document, I see it more like a global setting, so I wouldn't add it to the metadata at all. @@ +1411,3 @@ + /* View */ + ev_view_set_preserve_zoom(view, g_settings_get_boolean (settings, "preserve-zoom")); + ev_window_preserve_zoom_changed_cb(ev_window); You should connect to the changed signal, instead of calling the changed callback manually. @@ +5855,3 @@ { "zoom-in", ev_window_cmd_view_zoom_in }, { "zoom-out", ev_window_cmd_view_zoom_out }, + { "preserve-zoom", NULL, NULL, "false", ev_window_cmd_preserve_zoom }, We don't expose global settings to the UI, so I would leave it just as a gsetting for now, I'm sure most of the users will not know what preserve zoom menu item is for.
Created attachment 284908 [details] [review] Allow links change zoom feature. @anyone-interested: This feature is now implemented as a gsetting, and it's not accessible from the user interface anymore. @Carlos Hi Carlos, thank you for the detailed review. I've made all the changes you requested, with one exception: You said: "I think I prefer to add an if to every goto_*_dest method to do everything instead of changing the zoom." I interpreted this as removing the if around the switch(ev-view.c:1870) and goto_y_dest() and pushing it down into the goto_*_dest functions. This would create a lot of duplicate code with the same functionality as currently in goto_y_dest(). Let me know what you think. The patch is for 3.13.90, it includes the previous patch.
Comment on attachment 284908 [details] [review] Allow links change zoom feature. This is a bit confusing, I started to review the patch and I thought you had submitted the same patch again. It's a lot easier to review if you squash your patches into a single patch before submitting an updated patch. I've just removed some trailing whitespaces, added g_return macros to new public methods and pushed the patch to git master. Thanks!
(In reply to comment #11) > Created an attachment (id=284908) [details] [review] > Allow links change zoom feature. > > @anyone-interested: > > This feature is now implemented as a gsetting, and it's not accessible from the > user interface anymore. > > @Carlos > > Hi Carlos, thank you for the detailed review. I've made all the changes you > requested, with one exception: > > You said: "I think I prefer to add an if to every goto_*_dest method to do > everything instead of changing the zoom." > > I interpreted this as removing the if around the switch(ev-view.c:1870) and > goto_y_dest() and pushing it down into the goto_*_dest functions. This would > create a lot of duplicate code with the same functionality as currently in > goto_y_dest(). I still think the coude would be easier to read this way, so I've also pushed a follow up patch to do that. > Let me know what you think. > The patch is for 3.13.90, it includes the previous patch. Thanks!
@ Adam Thank you Adam for all your help and for making this feature possible. @ Carlos Thank you for the quick integration. I would like to underline Adam's point of having this feature in the user interface, or making the gsetting true by default. Links changing zoom level is an extremely annoying "feature", even if it is in the standard. @ Everyone You can try this feature by using gsettings to set the allow-links-change-zoom key from the org.gnome.Evince schema to false.
Thank you, Eduard Beutel!