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

[Danielle Foré]

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

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

[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.
>

[Victor Martinez (victored)]

This looks better Robert, I appreciate you took the time to make the corrections!

I personally hate arguing over coding style because there's much more intellectual value in how the code works and its logic than how it actually looks, and I'm a big fan of elementary's coding style (I use it consistently) so I can understand why Corentin and Daniel want to enforce it everywhere :)

But there's one thing most people in the software industry will agree with, and it's that MIXING coding styles produce messy and unprofessional-looking code.

We want to improve this project and its code quality (which is great already), not decrease it.

For that reason, we should mark any branch not following Shotwell's coding style as "Needs Fixing" for the very same reason we'd prevent a branch using Shotwell's coding style from being merged into Granite: code base consistency, we want the whole file to look as if it was written by the same person.

I hope this is seen as a constructive suggestion rather than as the thoughts of someone trying to alienate other developers. That's definitely not my intention.

review: Approve

[Corentin Noël (tintou)]

This kind of question should not prevent a branch of being merged.

review: Approve

=== modified file 'src/Page.vala'
--- src/Page.vala	2014-05-24 23:00:56 +0000
+++ src/Page.vala	2014-05-29 06:36:37 +0000
@@ -1048,14 +1048,12 @@
                 double dx, dy;
                 bool vertical = false;
                 bool horizontal = false;
-                if (event.get_scroll_deltas (out dx, out dy)) {
+                if (event.get_scroll_deltas(out dx, out dy)) {
                     if (dx != 0) {
-                        horizontal = dx > 0 ? on_mousewheel_right (event)
-                                            : on_mousewheel_left (event);
+                        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);
+                        vertical = dy > 0 ? on_mousewheel_down(event) : on_mousewheel_up(event);
                     }
                     return horizontal || vertical;
                 }