Reproducible with master (>3.12)

Created attachment 290196 [details] [review]
patch file for git master

Hello,
Here is the solution that I propose:
CTRL+MOUSE wheel zoom has been changed into view to be pointer-centered.
An another alternative may be to show two more menu entries for define zoom CTRL+MOUSE  mode:
-window-centered wheel zoom
-mouse pointer-centered wheel zoom

Review of attachment 290196 [details] [review]:

Thanks for the patch, I agree we should scroll to the mouse position when zooming, the thing is more noticeable when using the zoom gesture. However, I don't think we need a new scrolling mode, since scroll to center is actually a special case of scroll to mouse point, when the mouse point is at the center. So, we could keep a single mode (scroll to center) that calculates the center point based on the mouse pointer position when known, falling back to the center point when unknown, and using gtk_gesture_get_bounding_box_center() for the case of gestures. I think that would be consistent with what eog does, for example, and we wouldn't need to change or add new API.

::: libview/ev-view-private.h
@@ +239,3 @@
+
+	/* Current mouse position */
+	GdkPoint mouse_position;

I don't think we need to keep this, and it might be racy, we can ask the device manager for the current mouse position.

::: libview/ev-view.c
@@ +109,3 @@
 #define EV_STYLE_CLASS_INVERTED      "inverted"
 
+

New line here?

@@ +648,3 @@
 			break;
+		case SCROLL_TO_MOUSEPOSITION: {
+			gint mouse_pos=(orientation == GTK_ORIENTATION_HORIZONTAL)?view->mouse_position.x:view->mouse_position.y;

Please, add some spaces here, it should be something like:

gint mouse_pos = (orientation == GTK_ORIENTATION_HORIZONTAL) ? view->mouse_position.x : view->mouse_position.y;

::: libview/ev-view.h
@@ +32,3 @@
 #include "ev-jobs.h"
 
+

New line here too?

::: shell/ev-window.c
@@ +4575,2 @@
 	ev_document_model_set_sizing_mode (ev_window->priv->model, EV_SIZING_FREE);
+	ev_view_zoom_out_center (EV_VIEW (ev_window->priv->view));

Note that the evince previewer also uses this api, so it needs to be updated. However, I would try any solution that doesn't require changing the api.

More importantly, this patch does not implement the desired behavior.

• The current behavior is that the point that used to be at the center of the window remains at the center of the window.

• The behavior with this patch is that the point that used to be under the mouse pointer jumps to the center of the window.

• The desired behavior is that the point that used to be under the mouse pointer _remains under the mouse pointer_.

Hello,
Thank you for your remarks.
I have worked again on my patch taking in account your remarks.
The zoom CTRL+SCROLL  center isn't anymore at the page center but stays under the mouse pointer.
ev_view_zoom_center function and SCROLL_TO_MOUSE_POSITION enum member have been deleted to keep API protocol unchanged.
The zoom center position is taken in account in ev_view_scroll_event for the case of  CTRL+SCROLL zoom, it is saved into view->zoom_center_x and view->zoom_center_y (beforehand Gdkpoint mouse_position) variables for the following reason:

Since  ev_view_scroll_event function is called before ev_view_set_adjustment_values (scroll update) and mouse pointer may have moved between these two functions, zoom center position will be slightly wrong. I think that zoom center position in ev_view_set_adjustment_values must match real scroll event position (in ev_view_scroll_event).

If CTRL+SCROLL zoom isn't used (other uses of SCROLL_TO_CENTER mode: ev_view_zoom_in,  ev_view_zoom_out), zoom center position is the one of page center computed in  ev_view_set_adjustment_values.

Concerning tablet gestures management (GtkGesture), please check what is done in  zoom_gesture_scale_changed_cb function, i haven't test it because i haven't got tablet emulator, i would like informations on test for tablets.

Created attachment 291725 [details] [review]
2nd version of patch to evince 3.14.1

please do not considered 2nd version of patch because of test lines not removed, only version 3 should be reviewed.

Created attachment 291728 [details] [review]
3rd version of bug731669 patch

Frederic you can jsut mark the patch as obsolete :) 
I did it for you. Thanks for the patch

Review of attachment 291728 [details] [review]:

This works much better, I have just a few suggestions about the code.

::: evince_src/libview/ev-view.c
@@ +630,3 @@
 	page_size = gtk_adjustment_get_page_size (adjustment);
+	zoom_center = (orientation == GTK_ORIENTATION_HORIZONTAL) ?
+		view->zoom_center_x : view->zoom_center_y;

Instead of doing this here you could initialize zoom_center in the previous if where we are initializing everything that depends on orientation.
What we are actually changing is the default zoom center, that is currently the page center. So instead of checking everywhere if zoom_center is > 0 and do a different thing, we could keep the same code by checking zoom_center here and using the page center as fallback. 

if (zoom_center < 0)
        zoom_center = page_size * 0.5;

And then use zoom_center everywhere instead of page_size * 0.5

@@ +640,3 @@
     	        case SCROLL_TO_PAGE_POSITION:
 			break;
+		case SCROLL_TO_CENTER: {

The braces are no needed here since you are not defining any variable inside the case block.

@@ +641,3 @@
 			break;
+		case SCROLL_TO_CENTER: {
+			if (zoom_center > 0)				

Shouldn't this be >= 0? I guess we should initialize zoom_center_x/y to -1 so that when using the keyboard we use the page center.

@@ +642,3 @@
+		case SCROLL_TO_CENTER: {
+			if (zoom_center > 0)				
+				factor = (value + zoom_center) / upper;

This should work in both cases if zoom_center is initialized as I suggested.

@@ +674,3 @@
+		       if (zoom_center > 0) {
+			       new_value = CLAMP (upper * factor - zoom_center + 0.5,
+						  0, upper - page_size);

Ditto.

@@ +676,3 @@
+						  0, upper - page_size);
+			       (orientation == GTK_ORIENTATION_HORIZONTAL) ?
+				       (view->zoom_center_x=-1.0):(view->zoom_center_y=-1.0);

This looks a bit weird to me, use normal if instead, and leave  space before and after the =

@@ +683,3 @@
+			}
+			gtk_adjustment_set_value (adjustment, (int)new_value);		
+		}

Braces aren't needed here either.

@@ +7193,3 @@
+		view->zoom_center_x = center_x;
+		view->zoom_center_y = center_y;
+	}

You don't need to use local variables here, you can use &view->zoom_center_x, &view->zoom_center_y directly. They will not be modified if gtk_gesture_get_bounding_box_center returns FALSE.

::: evince_src/libview/ev-view-private.h
@@ +239,3 @@
+	/* Current zoom center */
+	gdouble zoom_center_x;
+	gdouble zoom_center_y;

I think these should be initialize to -1 in ev_view_init

Comment on attachment 291728 [details] [review]
3rd version of bug731669 patch

Addressed my own comments and pushed to git master. Thanks!