Created attachment 275266 [details] [review]
appDisplay: Remove redundant checks for pageNumber

We already do these at the beginning of the function.

Created attachment 275267 [details] [review]
appDisplay: Fix pages getting "stuck" under touch

We often call goToPage like:

  this.goToPage(this._currentPage - 1);

(or + 1). During panning, these are based on the velocity values
of the gesture action. If we're already on the first or last page
and the panning ends, it's possible to get goToPage that's either
-1 or greater than the last page.

During normal usage, this isn't a problem, since the Y values will
be correct, always. But when panning, the Y values stick to the
finger, and thus if we return early, we won't snap to the exact page,
making it seem like things get "stuck".

Fix this the simple way by clamping to the correctly-ranged values
of pageNumber

Created attachment 275268 [details] [review]
appDisplay: Make page panning smoother on touch

Currently, our logic for page panning isn't great. If the user starts a
pan upwards and hesitates over a new page, we'll go to the *next* page
on release, since the difference is greater, but the velocity wound down
to 0.

Instead of trying to treat it like page down or scrolls, simply do the
math to find the page where the user scrolled to.

This is unfortunately broken for fast swipes, since the user doesn't get
far enough into the new page to make a difference. I'm getting the
impression we'll need a gesture recognizer for this, though, however
crude. Simple hacks I tried, like a velocity multiplier, didn't work
properly.

Review of attachment 275266 [details] [review]:

ups right

Review of attachment 275267 [details] [review]:

oh right, I overkilled this.

Review of attachment 275268 [details] [review]:

::: js/ui/appDisplay.js
@@ +535,3 @@
+
+        let velocity = -action.get_velocity(0)[2];
+        let endPanValue = this._adjustment.value + velocity;

How about taking into account acceleration too?
In this manner fast swipes would work too.

::: js/ui/iconGrid.js
@@ +649,3 @@
+        return this._availableHeightPerPageForItems();
+    },
+

Florian,

Do you remember why we did _availableHeightPerPageForItems private? I remember I had that public.

fwiw this is a previous work I did for taking acceleration into account (in this case for touchapd devices)
Basically I do a median of few delta values, and calculate acceleration of that.
https://bug707973.bugzilla-attachments.gnome.org/attachment.cgi?id=254782

(In reply to comment #6)
> Do you remember why we did _availableHeightPerPageForItems private? I remember
> I had that public.

I don't, but my guess would be that nothing outside of PaginatedIconGrid needed it.

(In reply to comment #8)
> (In reply to comment #6)
> > Do you remember why we did _availableHeightPerPageForItems private? I remember
> > I had that public.
> 
> I don't, but my guess would be that nothing outside of PaginatedIconGrid needed
> it.

Then maybe this should be changed to getPageHeight for simplicity.

Although it is not true at all, since the page height is the height of the rows with at least one item. So for example last page probably won't have a entire filled page. That's why it is named availableHeightPerPageForItems and not getPageHeight in the first place.

Not very strong about that, fine to push as is for me.

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