Created attachment 212041 [details] [review]
Patch to enable GDK_SMOOTH_SCROLL events

If we really want to add these as public functions, they need to have docs comments, including Since: annotation, and an entry in the docs' sections.txt file.

Would it be better to have them as public functions or static functions?

Unless you foresee a need to call these directly from outside ev-view, they should be private. (We can always make them public later!)

Created attachment 212160 [details] [review]
Amended patch to enable GDK_SMOOTH_SCROLL events

Alright, here's an amended patch that has the two functions as static functions instead.

Review of attachment 212160 [details] [review]:

::: libview/ev-view.c
@@ +3304,3 @@
+			gdouble factor = pow (delta < 0 ? ZOOM_IN_FACTOR : ZOOM_OUT_FACTOR, fabs (delta));
+
+			if (ev_view_can_zoom (view, factor)) {

Why Can't  you just use ev_view_can_zoom_in &&  ev_view_can_zoom_out ?

@@ +3361,3 @@
+			        decrement = view->total_delta_x < 0 ? -1.0 : 1.0;
+			        for (; fabs (view->total_delta_x) >= 1.0; view->total_delta_x -= decrement)
+				        if (decrement < 0)

you should add braces at least to the for statements... these for statements without the init part also look weird (but no opinion on this)

@@ +5478,2 @@
 }
 

I don't see the need for this method. (see above). Otherwise, I would just do 
return ev_view_can_zoom_in && ev_view_can_zoom_out

@@ +5495,1 @@
 }

Why are you changing the behaviour of this func? unless I am missing something, you should not change the behaviour of this func.

@@ +5501,2 @@
 }
 

ditto

(In reply to comment #6)
> Review of attachment 212160 [details] [review]:
> 
> ::: libview/ev-view.c
> @@ +3304,3 @@
> +            gdouble factor = pow (delta < 0 ? ZOOM_IN_FACTOR :
> ZOOM_OUT_FACTOR, fabs (delta));
> +
> +            if (ev_view_can_zoom (view, factor)) {
> 
> Why Can't  you just use ev_view_can_zoom_in &&  ev_view_can_zoom_out ?

Because scrolling now happens in sub-tick increments (I'm not sure what to call it, but by "tick", I mean Button[4-7] events or GDK_SCROLL_{UP,DOWN,LEFT,RIGHT} events). `delta' is a fractional value indicating how much of a scroll has happened. For example, if under the old mechanism it takes you 1cm of movement on a touchpad per scroll tick, you move by 1.5cm on the touchpad, delta will be equal to 1.5.

In other words, this particular hunk makes it possible to zoom in/out using ctrl+scroll smoothly on hardware that supports it.

ev_view_can_zoom_{in,out} calculate the possibility of zooming with a constant factor (1.2, and 1/1.2), resulting in discrete zooming even on GDK_SCROLL_SMOOTH supported hardware.

Likewise for ev_view_zoom{in,out}.
 
> @@ +3361,3 @@
> +                    decrement = view->total_delta_x < 0 ? -1.0 : 1.0;
> +                    for (; fabs (view->total_delta_x) >= 1.0;
> view->total_delta_x -= decrement)
> +                        if (decrement < 0)
> 
> you should add braces at least to the for statements... these for statements
> without the init part also look weird (but no opinion on this)
> 
> @@ +5478,2 @@
>  }
> 
> 
> I don't see the need for this method. (see above). Otherwise, I would just do 
> return ev_view_can_zoom_in && ev_view_can_zoom_out

I think you're misunderstanding the purpose of this function. Perhaps it should be renamed to ev_view_can_zoom_for_factor()? This function does not check that both zooming in and out are possible, but tests that zooming for a specifically given factor is possible (note the extra gdouble factor argument).

> 
> @@ +5495,1 @@
>  }
> 
> Why are you changing the behaviour of this func? unless I am missing something,
> you should not change the behaviour of this func.

I didn't. The behaviour is exactly the same as before. The code that's in ev_view_[can_]zoom was duplicated and more or less identical in both ev_view_[can_]_zoom_{in,out} before (with the exception of the ZOOM_{IN,OUT}_FACTOR).



> 
> @@ +5501,2 @@
>  }
> 
> 
> ditto

(In reply to comment #7)
> (In reply to comment #6)
> > Review of attachment 212160 [details] [review] [details]:
> > 
> > ::: libview/ev-view.c
> > @@ +3304,3 @@
> > +            gdouble factor = pow (delta < 0 ? ZOOM_IN_FACTOR :
> > ZOOM_OUT_FACTOR, fabs (delta));
> > +
> > +            if (ev_view_can_zoom (view, factor)) {
> > 
> > Why Can't  you just use ev_view_can_zoom_in &&  ev_view_can_zoom_out ?
> 
> Because scrolling now happens in sub-tick increments (I'm not sure what to call
> it, but by "tick", I mean Button[4-7] events or GDK_SCROLL_{UP,DOWN,LEFT,RIGHT}
> events). `delta' is a fractional value indicating how much of a scroll has
> happened. For example, if under the old mechanism it takes you 1cm of movement
> on a touchpad per scroll tick, you move by 1.5cm on the touchpad, delta will be
> equal to 1.5.
> 
> In other words, this particular hunk makes it possible to zoom in/out using
> ctrl+scroll smoothly on hardware that supports it.
> 
> ev_view_can_zoom_{in,out} calculate the possibility of zooming with a constant
> factor (1.2, and 1/1.2), resulting in discrete zooming even on
> GDK_SCROLL_SMOOTH supported hardware.
> 
> Likewise for ev_view_zoom{in,out}.
> 

Indeed, totally missed that :)


> > @@ +3361,3 @@
> > +                    decrement = view->total_delta_x < 0 ? -1.0 : 1.0;
> > +                    for (; fabs (view->total_delta_x) >= 1.0;
> > view->total_delta_x -= decrement)
> > +                        if (decrement < 0)
> > 
> > you should add braces at least to the for statements... these for statements
> > without the init part also look weird (but no opinion on this)
> > 
> > @@ +5478,2 @@
> >  }
> > 
> > 
> > I don't see the need for this method. (see above). Otherwise, I would just do 
> > return ev_view_can_zoom_in && ev_view_can_zoom_out
> 
> I think you're misunderstanding the purpose of this function. Perhaps it should
> be renamed to ev_view_can_zoom_for_factor()? This function does not check that
> both zooming in and out are possible, but tests that zooming for a specifically
> given factor is possible (note the extra gdouble factor argument).

Sure, I misunderstood. 


> 
> > 
> > @@ +5495,1 @@
> >  }
> > 
> > Why are you changing the behaviour of this func? unless I am missing something,
> > you should not change the behaviour of this func.
> 
> I didn't. The behaviour is exactly the same as before. The code that's in
> ev_view_[can_]zoom was duplicated and more or less identical in both
> ev_view_[can_]_zoom_{in,out} before (with the exception of the
> ZOOM_{IN,OUT}_FACTOR).


My point is still valid. In zoom_in and zoom_out funcs you only check for one condition, while in zoom func you check for both conditions (for max and min scale). I could imagine a situation where these two comparisons give different results. 

> 
> 
> 
> > 
> > @@ +5501,2 @@
> >  }
> > 
> > 
> > ditto

(In reply to comment #8)
> [...]
> > > @@ +5495,1 @@
> > >  }
> > > 
> > > Why are you changing the behaviour of this func? unless I am missing something,
> > > you should not change the behaviour of this func.
> > 
> > I didn't. The behaviour is exactly the same as before. The code that's in
> > ev_view_[can_]zoom was duplicated and more or less identical in both
> > ev_view_[can_]_zoom_{in,out} before (with the exception of the
> > ZOOM_{IN,OUT}_FACTOR).
> 
> 
> My point is still valid. In zoom_in and zoom_out funcs you only check for one
> condition, while in zoom func you check for both conditions (for max and min
> scale). I could imagine a situation where these two comparisons give different
> results. 

I see. I think I understand what you mean. Basically if the current zoom level is greater than the maximum zoom level, we still want to be able to zoom out, and vice versa?

Created attachment 212176 [details] [review]
Amended patch to enable GDK_SMOOTH_SCROLL events

Here's an amended patch that fixes the use-case mentioned in #8.

Created attachment 212657 [details] [review]
Patch to enable GDK_SMOOTH_SCROLL events

It looks like I forgot to commit my changes before running format-patch. Here's the real amended patch.

Is there any chance that this might make it into a 3.4.x release? Smooth scrolling makes reading documents much nicer when using a trackpad (and I imagine even more on a tablet)

Review of attachment 212657 [details] [review]:

The patch looks and works mostly ok to me, although the handling of best_fit && !continuous acts a bit weird, as diagonal scrolling may trigger page switches back and forth, even cancelling out each other. I would recommend tying together both delta_x/y, and having it tick when that combination reaches a threshold, something like:

view->total_delta_x += event->delta_x;
view->total_delta_y += event->delta_y;

if (pow (view->total_delta_x, 2) + pow (view->total_delta_y, 2) > 1)
  {
    /* handle moving to previous/next page based on overall direction */
    ...
    view->total-delta_x = view->total_delta_y = 0;
  }

I noticed that Nautilus appears to completely ignore horizontal smooth scrolling in its thumbnail mode, and vertical smooth scrolling in its compact mode. Perhaps to be consistent with Nautilus, Evince should ignore horizontal smooth scrolling (delta_x) in the case of best_fit && !continuous?

Created attachment 218823 [details] [review]
Updated patch to enable GDK_SMOOTH_SCROLL events

Alright, here's a patch that combines total_delta_x and total_delta_y into a single variable, total_delta.

Instead of doing a pythagoras calculation of the hypotenuse when scrolling diagonally, this has the effect of letting the two deltas add up.

So basically what we have is this:
 - scroll left or scroll up, go to previous page
 - scroll right or scroll down, go to next page

And if the user scrolls diagonally, it's more stable now that we don't keep flipping to and fro between two pages erratically:
 - if the user scrolls diagonally upwards towards the left, then we treat it as scrolling up, twice as fast.
 - if the user scrolls diagonally upwards towards the right, then we treat it as scrolling up, twice as slow.

The angle matters, of course, depending on how much physical distance you have to move to get the same scroll delta. I believe this mimics the previous (non-smooth-scrolling) behaviour for diagonal scrolls perfectly.

I have kept the loop in place, so that if the user scrolls really quickly and manages to get in total_delta > 2.0 in a single event, then we can flip pages faster as well, instead of rate-limiting it to 1 page per event.

Review of attachment 218823 [details] [review]:

I've split the patch and pushed the zoom methods refactoring. I've tried the patch and it doesn't seem to work in some cases for me. If I open a document in best fit non continuous mode, the first time I move the scroll wheel, it doesn't change the page because it seems total_delta is 0.

Created attachment 220592 [details]
Smooth scroll test case

I see it too. It looks like a Gtk/Gdk bug though. Using the attached test case, I got the following output:
------
GDK_SCROLL_SMOOTH 0, 0
GDK_SCROLL_DOWN
GDK_SCROLL_SMOOTH 0, 0
GDK_SCROLL_UP
------

It seems that the first scroll event using a normal mouse after focusing on the window yields a delta_x and delta_y of 0.

Created attachment 226288 [details] [review]
Refreshed patch for latest git sources

Here is the patch refreshed for the latest git sources.

*** Bug 567033 has been marked as a duplicate of this bug. ***

Created attachment 241682 [details] [review]
libview: Enable GDK_SMOOTH_SCROLL events for view

This enables the smooth/kinetic scrolling support found in GTK+ 3.3.18
with the document view, and handles them for the Ctrl+scroll,
Shift+scroll, and non-continuous best-fit mode scrolling cases.

Created attachment 241683 [details] [review]
libview: Allow zooming to the limits of the scale

If the current zoom level was within one zoom factor of the limit, it
was not possible to scroll towards the limit. This made smooth scrolling
near the limit awkward, as unless the scroll event had a large delta it
was impossible to reach the zoom limit. Non-smooth scrolling was also
affected, but it was just much more difficult to trigger.

Fix this by allowing zooming while the current zoom level is within one
zoom factor of the limit. Add a new ev_view_can_zoom() function to make
zooming by a factor (as with smooth scrolling) more convenient.

Comment on attachment 241682 [details] [review]
libview: Enable GDK_SMOOTH_SCROLL events for view

Thanks!, I've just pushed it to git master with some minor cosmetic changes.

(In reply to comment #21)
> Created an attachment (id=241683) [details] [review]
> libview: Allow zooming to the limits of the scale
> 
> If the current zoom level was within one zoom factor of the limit, it
> was not possible to scroll towards the limit. This made smooth scrolling
> near the limit awkward, as unless the scroll event had a large delta it
> was impossible to reach the zoom limit. Non-smooth scrolling was also
> affected, but it was just much more difficult to trigger.
> 
> Fix this by allowing zooming while the current zoom level is within one
> zoom factor of the limit. Add a new ev_view_can_zoom() function to make
> zooming by a factor (as with smooth scrolling) more convenient.

Thanks for the patch, but I'm not sure I understand it, we need to check the scale after applying the zoom to check if it's possible to zoom in/out, no?. In any case, this bug is about enabling smooth scroll events, could you please open a new bug report for this patch so that we can discuss it there?

What is missing to close this bug?

Nothing, the pending patch is unrelated to this bug. David, feel free to open a new bug report for the other patch.

I rebased David's patch and attached to a new bug (Bug 788480)