Support fling for nested scroll on TimetableGrid. #1273

usuiat · 2023-09-23T14:45:36Z

Issue

close Nested Scroll on TimetableGrid should support fling operation. #776

Overview (Required)

Support fling operation for nested scroll on TimetableGrid.

After drag gesture on TimetableGrid ends, the header height and the tab height expand or shrink in conjunction with TimetableGrid flinging.

Movie (Optional)

Before	After
before.mp4	after.mp4

github-actions · 2023-09-23T14:48:17Z

Hi @usuiat! Codes seem to be unformatted. To resolve this issue, please run ./gradlew detekt --auto-correct and fix the results of ./gradlew lintDebug.. Thank you for your contribution.

github-actions · 2023-09-23T15:09:39Z

Test Results

216 tests 216 ✔️ 9m 2s ⏱️
  11 suites     0 💤
  11 files     0 ❌

Results for commit 3f69c52.

♻️ This comment has been updated with latest results.

swimmy-reo · 2023-09-25T11:32:06Z

...sessions/src/main/java/io/github/droidkaigi/confsched2023/sessions/section/TimetableSheet.kt

@@ -128,17 +127,11 @@ fun TimetableSheet(
                }

                is GridTimetable -> {
-                    val nestedScrollDispatcher = remember { NestedScrollDispatcher() }


Thanks for the great PR!
I would like to comment on a point of concern.
Is there any reason why you originally defined the NestedScrollDispatcher and applied Modifier nestedScroll here?

At first I was misunderstanding that I needed to pass a nestedScrollDispatcher and a timetableSheetContentScrollState.nestedScrollConnection to one Modifier.nestedScroll() argument.
But in this case, the TimetableGrid's nestedScrollDispatcher is independent of the TimetableSheet. Therefore I moved the dispatcher into TimetableGrid class.

Thank you very much. I am convinced.

swimmy-reo

I was a little curious about the behavior when I moved it in my hand, so let me share it with you.
（We are still in the process of figuring out which part of the problem can be resolved, so we will only share once.）

First, is it an expected behavior that the scroll position returns to the original position after a slight fling at the initial position?

Second, when I scroll in the direction that the border wraps around, it scrolls in the opposite direction. Is this unintended behavior?

(This may just be on my end, so please let me know if this does not reproduce 🙏)

1st	2nd
default.mp4	default.mp4

swimmy-reo · 2023-09-25T14:11:55Z

...sessions/src/main/java/io/github/droidkaigi/confsched2023/sessions/section/TimetableSheet.kt

@@ -128,17 +127,11 @@ fun TimetableSheet(
                }

                is GridTimetable -> {
-                    val nestedScrollDispatcher = remember { NestedScrollDispatcher() }


Thank you very much. I am convinced.

usuiat · 2023-09-26T11:25:43Z

@swimmy-reo

First, is it an expected behavior that the scroll position returns to the original position after a slight fling at the initial position?
Second, when I scroll in the direction that the border wraps around, it scrolls in the opposite direction. Is this unintended behavior?

Thank you for pointing out.
These are not expected behaviors but I can reproduce them.

I referred to the Modifier.scrollable() code when implementing this fling operation.
So I will check the code carefully again.

- Use a position relative to the root component to calculate velocity. - Reset VelocityTracker when drag gesture starts. - Cancel fling animation when another drag gesture starts. - Don't do scroll process when the position does not change.

usuiat · 2023-09-26T15:38:46Z

@swimmy-reo
I have fixed some not good code.
Could you check again? Thank you.

swimmy-reo · 2023-09-27T16:59:17Z

.../sessions/src/main/java/io/github/droidkaigi/confsched2023/sessions/section/TimetableGrid.kt

            .pointerInput(Unit) {
                detectDragGestures(
+                    onDragStart = {
+                        scrollState.resetTracking()


It's so good! 👍

swimmy-reo · 2023-09-27T20:10:03Z

.../sessions/src/main/java/io/github/droidkaigi/confsched2023/sessions/section/TimetableGrid.kt

@@ -672,6 +713,9 @@ private class TimetableScreen(
        position: Offset,
        nestedScrollDispatcher: NestedScrollDispatcher,
    ) {
+        // If the position does not change, VelocityTracker malfunctions. Therefore return here.
+        if (dragAmount == Offset.Zero) return


Wow! I did not know that! 😮

swimmy-reo · 2023-09-27T20:31:38Z

🏅 🏅 🏅 🏅 I was amazed at how quickly the correction was handled! Amazing! 🏅 🏅 🏅 🏅

Let me make one suggestion.
I was concerned about the fact that scrolling in the grid is no longer working. I think it would be better to be able to scroll as there is a certain number of contents.

I think it would be good to see if the tabs are collapsed when switching the cancelFling flag that you defined.

current	ideal
Screen_recording_20230928_045221.mp4	Screen_recording_20230928_045022.mp4

// In TimetableGrid.kt

// add isTabExpandable property (from TimetableTabState)
data class TimetableGridUiState(val timetable: Timetable, val isTabExpandable: Boolean? = null )

@Composable
fun TimetableGrid(
    timetable: Timetable,
    isTabExpandable: Boolean?, // new
    onTimetableItemClick: (TimetableItem) -> Unit,
    modifier: Modifier = Modifier,
    contentPadding: PaddingValues = PaddingValues(),
) {
      ...
 }


@OptIn(ExperimentalFoundationApi::class)
@Composable
fun TimetableGrid(
    timetable: Timetable,
    isTabExpandable: Boolean?, // new
    timetableState: TimetableState,
    modifier: Modifier = Modifier,
    contentPadding: PaddingValues = PaddingValues(),
    content: @Composable (TimetableItem, Int) -> Unit,
) {
      ...
}

@Stable
class ScreenScrollState(
    initialScrollX: Float = 0f,
    initialScrollY: Float = 0f,
) {
    ...
    suspend fun scroll(
        scrollX: Float,
        scrollY: Float,
        timeMillis: Long,
        position: Offset,
        isTabExpandable: Boolean? = null, // new
    ) {
        if (isTabExpandable == false) cancelFling = true // fixed
       ...
      }
   }

To use CompositionLocal, I'm thinking of passing the data in a bucket relay format since the data is of limited use.

As before, let me know if you can't reproduce this!
Also, please let me know if I said something wrong!

（Digression）
It's very difficult to verbalize what the ideal behavior of nested scrolling would be, so it's tough to know without trying to modify it. 😅

usuiat · 2023-09-30T11:47:03Z

@swimmy-reo
I have tried but cannot reproduce it.
Is fling not working at all? On my end, it is generally working.

Yes, sometimes the fling does not work (scrolling stops as soon as I release my finger), but I think this is the same as before. (This is not a good, though 🤔)

usuiat · 2023-09-30T12:12:00Z

@swimmy-reo
If you are running the app in debug variant, could you try release (devRelease) variant?
In my environment, the scrolling tends to stop with debug variant. However, in the release variant the scrolling runs relatively smoothly.
If you are the same, this may be a performance issue.

swimmy-reo · 2023-09-30T17:55:02Z

I see!
I see that the fling is working properly in usui's hand...
I'll check it out.

By the way, I tried it with a release build.

swimmy-reo · 2023-09-30T18:14:35Z

I'm having someone else check it out to see if it works.
If it works correctly, I would like to approve it as is!

To be honest, my device specs are low lol.
If there are more devices to reproduce, I would like to consider the performance aspect.

swimmy-reo · 2023-10-03T01:06:22Z

I will reply this evening!

swimmy-reo · 2023-10-03T13:07:48Z

I have had others look at this and the degree of reproduction seems to vary from device to device.

Below is a release build with Pixel Fold.
When scrolling from bottom to top, the fling does not work, and when scrolling from top to bottom, the fling works.

pixel fold
demo.mp4

I asked others to debug in detail the point in pixel fold where the fling does not work when scrolling from bottom to top, and found that the fling does not work when scrolling in the direction where there is no grid, but it works when scrolling in the direction where there is a grid.

no fling	fling
no_fring.mp4	yes_fring.mp4

Here we see that the reproducibility varies from device to device.
Since addressing this device difference is outside the scope of this PR, we would like to isolate it as a separate issue and approve it here!

Here is the issue.
I'll write more details later.

swimmy-reo

Thank you for your hard work! It was very helpful! 😄

usuiat · 2023-10-04T03:57:37Z

I appreciate your sincere review and people who helped👏
I learned a lot working on this Issue.

RyuNen344

❤️

RyuNen344 · 2023-10-04T04:01:17Z

@usuiat
Could you merge this PR if you can ?

usuiat · 2023-10-04T13:17:49Z

@RyuNen344
I don't think I have the authority to merge.

usuiat added 2 commits September 23, 2023 23:23

Create NestedScrollDispatcher object in TimeTableGrid.

30580bb

Implement fling for nested scroll of TimetableGrid.

b786a70

usuiat requested a review from a team as a code owner September 23, 2023 14:45

usuiat requested a review from RyuNen344 September 23, 2023 14:45

Format codes.

98e0c88

github-actions bot temporarily deployed to deploygate-distribution September 23, 2023 15:15 Inactive

swimmy-reo self-requested a review September 25, 2023 08:37

swimmy-reo reviewed Sep 25, 2023

View reviewed changes

Fix unexpected fling behaviors.

3f69c52

- Use a position relative to the root component to calculate velocity. - Reset VelocityTracker when drag gesture starts. - Cancel fling animation when another drag gesture starts. - Don't do scroll process when the position does not change.

github-actions bot temporarily deployed to deploygate-distribution September 26, 2023 15:53 Inactive

swimmy-reo reviewed Sep 27, 2023

View reviewed changes

swimmy-reo approved these changes Oct 3, 2023

View reviewed changes

swimmy-reo mentioned this pull request Oct 3, 2023

Timetable Grid does not fling depending on Android device #1276

Open

RyuNen344 approved these changes Oct 4, 2023

View reviewed changes

swimmy-reo merged commit f255ed2 into DroidKaigi:main Oct 4, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support fling for nested scroll on TimetableGrid. #1273

Support fling for nested scroll on TimetableGrid. #1273

usuiat commented Sep 23, 2023

github-actions bot commented Sep 23, 2023

github-actions bot commented Sep 23, 2023 •

edited

Loading

swimmy-reo Sep 25, 2023 •

edited

Loading

usuiat Sep 25, 2023

swimmy-reo Sep 25, 2023 •

edited

Loading

swimmy-reo left a comment •

edited

Loading

swimmy-reo Sep 25, 2023 •

edited

Loading

usuiat commented Sep 26, 2023

usuiat commented Sep 26, 2023

swimmy-reo Sep 27, 2023

swimmy-reo Sep 27, 2023

swimmy-reo commented Sep 27, 2023 •

edited

Loading

usuiat commented Sep 30, 2023

usuiat commented Sep 30, 2023

swimmy-reo commented Sep 30, 2023 •

edited

Loading

swimmy-reo commented Sep 30, 2023

swimmy-reo commented Oct 3, 2023

swimmy-reo commented Oct 3, 2023 •

edited

Loading

swimmy-reo left a comment

usuiat commented Oct 4, 2023

RyuNen344 left a comment

RyuNen344 commented Oct 4, 2023

usuiat commented Oct 4, 2023

Support fling for nested scroll on TimetableGrid. #1273

Support fling for nested scroll on TimetableGrid. #1273

Conversation

usuiat commented Sep 23, 2023

Issue

Overview (Required)

Movie (Optional)

github-actions bot commented Sep 23, 2023

github-actions bot commented Sep 23, 2023 • edited Loading

Test Results

swimmy-reo Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

usuiat Sep 25, 2023

Choose a reason for hiding this comment

swimmy-reo Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

swimmy-reo left a comment • edited Loading

Choose a reason for hiding this comment

swimmy-reo Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

usuiat commented Sep 26, 2023

usuiat commented Sep 26, 2023

swimmy-reo Sep 27, 2023

Choose a reason for hiding this comment

swimmy-reo Sep 27, 2023

Choose a reason for hiding this comment

swimmy-reo commented Sep 27, 2023 • edited Loading

usuiat commented Sep 30, 2023

usuiat commented Sep 30, 2023

swimmy-reo commented Sep 30, 2023 • edited Loading

swimmy-reo commented Sep 30, 2023

swimmy-reo commented Oct 3, 2023

swimmy-reo commented Oct 3, 2023 • edited Loading

swimmy-reo left a comment

Choose a reason for hiding this comment

usuiat commented Oct 4, 2023

RyuNen344 left a comment

Choose a reason for hiding this comment

RyuNen344 commented Oct 4, 2023

usuiat commented Oct 4, 2023

github-actions bot commented Sep 23, 2023 •

edited

Loading

swimmy-reo Sep 25, 2023 •

edited

Loading

swimmy-reo Sep 25, 2023 •

edited

Loading

swimmy-reo left a comment •

edited

Loading

swimmy-reo Sep 25, 2023 •

edited

Loading

swimmy-reo commented Sep 27, 2023 •

edited

Loading

swimmy-reo commented Sep 30, 2023 •

edited

Loading

swimmy-reo commented Oct 3, 2023 •

edited

Loading