Merge lp:~evfool/pantheon-photos/lp1274815 into lp:~pantheon-photos/pantheon-photos/trunk

[Robert Roth]

Handle the SMOOTH_SCROLL events to enable smooth scrolling.

Handle the SMOOTH_SCROLL events to enable smooth scrolling.

As the per-direction mousewheel events have been propagated to subclasses, and the smooth scroll (since 3.4) can have multiple directions (see [1]), check the delta values and propagate for all directions (at most two, one vertical and one horizontal scroll).

[1] https://developer.gnome.org/gdk3/stable/gdk3-Event-Structures.html#GdkScrollDirection

[Corentin Noël (tintou)]

Nice work, it's working as expected!

review: Approve

[Victor Martinez (victored)]

I'd really love to see a second iteration of this patch. This wasn't ready for merging.

1. Coding style is inconsistent with the rest of the file. Notice the space after 'get_scroll_deltas' and the unnecessary split of 'on_mousewheel_left' and 'on_mousewheel_up' to a second line. This surely looks cool in the diff, but not when you read the code as part of the whole file.

2. This patch could be smaller. Notice how the variables 'horizontal' and 'vertical' are not needed. They could have been returned right away from within their respective 'if' clauses.

PLEASE DON'T RUSH REVIEWS!

review: Needs Fixing

[lp:~evfool/pantheon-photos/lp1274815]

2521. By Robert Roth on 2014-05-29

Fixed code style issues

[Robert Roth (evfool)]

Thanks Victor for your review.
I agree with point 1, have fixed it, and pushed it, spaces after function names removed, unnecessary line split removed.

With point 2. though, I'm not sure I agree or I can follow you:
In my opinion we can't return from the if cases, as we have to make sure to execute both the horizontal and the vertical scrolling (this won't happen in case of a mouse, but happens in case of a laptop touchpad in cases where we have both horizontal and vertical scrollbar). If we would return from the first if clause, we would ignore the vertical movement.

[Corentin Noël (tintou)]

I disagree with you Victor, new code should now follow the elementary code
style.
Le 29 mai 2014 08:24, "Robert Roth" <email address hidden> a écrit :

> Thanks Victor for your review.
> I agree with point 1, have fixed it, and pushed it, spaces after function
> names removed, unnecessary line split removed.
>
> With point 2. though, I'm not sure I agree or I can follow you:
> In my opinion we can't return from the if cases, as we have to make sure
> to execute both the horizontal and the vertical scrolling (this won't
> happen in case of a mouse, but happens in case of a laptop touchpad in
> cases where we have both horizontal and vertical scrollbar). If we would
> return from the first if clause, we would ignore the vertical movement.
> --
> https://code.launchpad.net/~evfool/pantheon-photos/lp1274815/+merge/220611
> You are reviewing the proposed merge of
> lp:~evfool/pantheon-photos/lp1274815 into lp:pantheon-photos.
>

Unmerged revisions

=== modified file 'src/Page.vala'
--- src/Page.vala	2014-04-06 11:22:49 +0000
+++ src/Page.vala	2014-05-22 10:45:31 +0000
@@ -174,7 +174,7 @@
         event_source.add_events(Gdk.EventMask.BUTTON_PRESS_MASK | Gdk.EventMask.BUTTON_RELEASE_MASK
             | Gdk.EventMask.POINTER_MOTION_MASK | Gdk.EventMask.POINTER_MOTION_HINT_MASK
             | Gdk.EventMask.BUTTON_MOTION_MASK | Gdk.EventMask.LEAVE_NOTIFY_MASK
-            | Gdk.EventMask.SCROLL_MASK);
+            | Gdk.EventMask.SCROLL_MASK | Gdk.EventMask.SMOOTH_SCROLL_MASK);
         event_source.button_press_event.connect(on_button_pressed_internal);
         event_source.button_release_event.connect(on_button_released_internal);
         event_source.motion_notify_event.connect(on_motion_internal);
@@ -1039,7 +1039,23 @@
 
             case Gdk.ScrollDirection.RIGHT:
                 return on_mousewheel_right(event);
-           
+
+            case Gdk.ScrollDirection.SMOOTH:
+                double dx, dy;
+                bool vertical = false;
+                bool horizontal = false;
+                if (event.get_scroll_deltas (out dx, out dy)) {
+                    if (dx != 0) {
+                        horizontal = dx > 0 ? on_mousewheel_right (event)
+                                            : on_mousewheel_left (event);
+                    }
+                    if (dy != 0) {
+                        vertical = dy > 0 ? on_mousewheel_down (event)
+                                          : on_mousewheel_up (event);
+                    }
+                    return horizontal || vertical;
+                }
+                return false;
             default:
                 return false;
         }