genious7 / FanFictionReader

An android app that improves browsing Fan Fiction stories on a mobile device
GNU General Public License v3.0
56 stars 24 forks source link

scroll story screen up/down by keyboard / gestures #39

Closed orionlee closed 5 years ago

orionlee commented 5 years ago

closes #37

Allow users to scroll story screen page up / down with

  1. keyboard: PageUp or / and PageDown or \
  2. gesture: swipe left / right at screen bottom

Note: Use of /, \ keys for page up / down is to support smaller keyboards where PageUp/Down keys require additional Fn. It might need to be user-configurable.

pzmarzly commented 5 years ago

Looks good. Before the next release, though, it would be nice to have configurable thresholds for this feature (currently mOneInchInPx is vertical threshold, I don't see any horizontal - it would need to be compared against abs(vX)). Also, for user-friendliness there should be a checkbox to disable this new behavior (it could work by simply setting vertical threshold to a negative number or horizontal one to infinity).

I'm not saying you need to be the person to implement this.

orionlee commented 5 years ago

Options to enable / disable gesture: I agree it should be an option for general release. I am waiting to see if this is likely to be adopted before doing more work.

I am unclear about the comment for comparing mOneInchInPx against abs(vX). mOneInchInPx is used to define screen bottom (the bottom 1-inch of the screen space) where swipes are accepted. It is orthogonal to fling speed abs(vX).

A proper swipe detection should have done some additional checks, such as abs(event1.getY() - event2.getY() < SOME_THRESHOLD. However, given empirically (at least on my device) event1 is often null, I skip those checks.

pzmarzly commented 5 years ago

I did not mean to suggest comparing mOneInchInPx against abs(vX), but rather to have 2 comparisons:

var thresholdX = // ..., TODO: add as a setting
var thresholdY = mOneInchInPx; // current value, TODO: add as a setting
var deltaY = // ...
var deltaX = abs(vX);
if (deltaY > thresholdY
 && deltaX > thresholdX) { /* trigger */ }

Someone may be trying to swipe up/down but accidentally move sideways by few pixels.

orionlee commented 5 years ago
  1. I see for case swiping up/down with some moving sideways, a guard is needed in the form of: abs(event2.getX() - event1.getX()) <= threasholdMaxX , rather than abs(vX), as vX is the velocity, which does not indicate how much sideways movement has been made.

  2. For the code as-is, given it is supports swiping at the bottom of the screen, the edge case that will fail is swiping down that ends at the bottom of the screen.

  3. Empirically on my device, event1 is often null (i.e., somehow the initial touch / MotionEvent is not captured), so for now the guard is not implemented. Doing it properly probably will require more lower level code to get an approximation of event1 when it is null.

pzmarzly commented 5 years ago

I am not that familiar with Android development. But it seems to me as if the event dispatching code (AOSP) did not receive ACTION_DOWN, but only ACTION_UP and ACTION_MOVE. In such scenario, we probably could call onTouchEvent(MotionEvent ev) manually like so:

boolean overrideNeeded = false;
boolean needToInsertFakeActionDownOnNextActionMove = true;
boolean actionDownSpotted = false;

boolean onTouchEvent(MotionEvent ev) {
    if (overrideNeeded) {
        if (ev.type == ACTION_MOVE && needToInsertFakeActionDownOnNextActionMove) {
            needToInsertFakeActionDownOnNextActionMove = false;
            MotionEvent ev2 = /* prepare ACTION_DOWN event with the same data as in `ev` */;
            return super.onTouchEvent(ev2);
        }
        if (ev.type == ACTION_UP) {
            needToInsertFakeActionDownOnNextActionMove = true;
        }
    } else {
        if (ev.type == ACTION_DOWN) {
            actionDownSpotted = true;
        }
        if (ev.type == ACTION_UP) {
            if (!actionDownSpotted) {
                overrideNeeded = true;
            }
            actionDownSpotted = false;
        }
    }
    return super.onTouchEvent(ev);
}

Then there is a chance that some vendor modified platform_frameworks_base and managed to pass Google certification despite shipping a broken build of Android. In such case we would need to either ship this scrolling functionality disabled by default, or disable it in case of null event.

orionlee commented 5 years ago

@pzmarzly Yes, as you say, it'll appear the codes need to use the first ACTION_MOVE event as a fallback in order to implement the additional guard..

orionlee commented 5 years ago

@genious7 @pzmarzly the bottom gesture codes ended up being somewhat complicated to handle two issues that seem to be specific to StoryDisplayActivity's layout

  1. A gesture's initial ACTION_DOWN MotionEvent is often missing.
  2. really horizontal swipes are often not detected (i.e., the gesture is not passed to OnTouchListener at all)

I suggest the above two issues are specific to StoryDisplayActivity's layout, because I just tried creating a simple standalone Activity with a big TextView with the same gesture code. The two issues do not appear at all.

Maybe it has to do with the fact that for StoryDisplayActivity's, the OnTouchListener is applied to a ListView (of the story text), rather than a simple TextView.