saket / press

Cross-platform markdown editor written in Kotlin Multiplatform (work in progress)
1.87k stars 113 forks source link

Scrolling troubles #62

Closed JakeWharton closed 3 years ago

JakeWharton commented 4 years ago

I'm having a lot of trouble scrolling the example files. The inbox (RIP) recycler view action takes precedence somewhere around 50% of the time.

https://photos.app.goo.gl/v5DzRWvBJthfWoKi9

(I swear in the video scrolling was working about twice as much as it otherwise was.)

saket commented 4 years ago

Damn, issues that I can't reproduce on my phone are my least favorite ones. Are you able to consistently reproduce this (across app restarts)?

JakeWharton commented 4 years ago

Yep. Android 11, Pixel 5. Every time, unfortunately.

saket commented 4 years ago

Can you please try out this APK? It prints a toast with debug info every time a scroll begins. A screenshot will hopefully give me some hints about the problem.

JakeWharton commented 4 years ago

Sure, random person on the internet, I'll install your APK!

https://photos.app.goo.gl/AXtTfKVdPoR2Exut8

JakeWharton commented 4 years ago

Show touches was off, but it was always done in the center-ish/middle-ish of the screen.

saket commented 4 years ago

Very interesting. InboxRecyclerView is incorrectly calculating the direction of gestures. I see canScrollVertically(1) on my phone vs -1 on yours when scrolling to the bottom of a note.

https://github.com/saket/press/blob/27526c7b6ce0073cfbdfc66f1e530bc96193992b/androidApp/src/main/java/press/widgets/InterceptPullToCollapseOnView.kt#L15

I'm simply comparing MotionEvent.rawY values (source). Would you happen to know if we aren't supposed to use rawY?

fun onTouch(event: MotionEvent, consumeDowns: Boolean): Boolean {
    when (event.actionMasked) {
      ACTION_DOWN -> {
        downY = event.rawY
      }

      ACTION_MOVE -> {
        val totalSwipeDistanceY = event.rawY - downY
        val upwardSwipe = totalSwipeDistanceY < 0F
        ...

Sure, random person on the internet, I'll install your APK!

The joys of debugging UI issues 😔. The other alternatives are to buy a Pixel 5 for testing or ask you to help me debug the touch listener.

saket commented 4 years ago

I wonder if I should be using MotionEvent#y instead of raw coordinates. Will try out.

SimonMarquis commented 3 years ago

Hi @saket, I'm experiencing the same issue here on my Pixel 4 XL. I think it's related to how you handle the deltaUpwardSwipe and it's even more likely to appear when you have a higher touch refresh rate (that might explain why you don't reproduce it).

On my device, swiping slowly will reproduce the issue, while swiping fast won't.

Here is what happen in PullToCollapseListener when I swipe upward fast:

MotionEvent { action=ACTION_DOWN, x[0]=1006.0, y[0]=2001.0 }
MotionEvent { action=ACTION_MOVE, x[0]=1041.0, y[0]=1476.0 }

The first ACTION_MOVE has a negative deltaY therefore the gesture will be intercepted by the nested view.

But here is what happen when I swipe upward slow:

MotionEvent { action=ACTION_DOWN, x[0]=508.0, y[0]=2088.0 }
MotionEvent { action=ACTION_MOVE, x[0]=508.0, y[0]=2088.0 }
MotionEvent { action=ACTION_MOVE, x[0]=508.0, y[0]=2079.5679 }

The first ACTION_MOVE has a deltaY equals to 0, and therefore the gesture will start the pull callback.

I think you might have to use the touchSlop on the vertical axis as well,

saket commented 3 years ago

Interesting. I don't think it's related to the refresh rate because I've been using a 120hz display to test Press. I ended up deciding to add support for nested scrolling to InboxRecyclerView so that I don't have to deal with coordinates on my own. Will have it ready soon. Thanks anyway @SimonMarquis!

saket commented 3 years ago

Should be fixed by a21985b08d959a7bc08bb645295fdfd3bbdf20f1. Nested scrolling FTW!

saket commented 3 years ago

Released in v1.8: https://github.com/saket/press/releases/tag/v1.8. @JakeWharton @SimonMarquis I didn't have a Pixel 4/5 to test, but I'm hoping my fix will work. Please let me know if it still doesn't?

SimonMarquis commented 3 years ago

This version works flawlessly on my pixel 4 XL! 🙌