DroidKaigi / conference-app-2023

The Official Conference App for DroidKaigi 2023
Apache License 2.0
645 stars 206 forks source link

Support fling for nested scroll on TimetableGrid. #1273

Closed usuiat closed 1 year ago

usuiat commented 1 year ago

Issue

Overview (Required)

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
github-actions[bot] commented 1 year ago

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[bot] commented 1 year ago

Test Results

216 tests   216 :heavy_check_mark:  9m 2s :stopwatch:   11 suites      0 :zzz:   11 files        0 :x:

Results for commit 3f69c525.

:recycle: This comment has been updated with latest results.

usuiat commented 1 year ago

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

usuiat commented 1 year ago

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

swimmy-reo commented 1 year ago

🏅 🏅 🏅 🏅 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
// 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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

I will reply this evening!

swimmy-reo commented 1 year ago

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

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

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.

usuiat commented 1 year ago

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

RyuNen344 commented 1 year ago

@usuiat Could you merge this PR if you can ?

usuiat commented 1 year ago

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