DroidKaigi / conference-app-2023

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

[iOS] Improve TimetableDayHeader performance #1075

Closed ry-itto closed 1 year ago

ry-itto commented 1 year ago

Idea Description

Reference images and links

Uploading Simulator Screen Recording - iPhone 14 Pro Max - 2023-09-04 at 03.13.09.mp4…

fumiyasac commented 1 year ago

@ry-itto

Perhaps the cause may be the code below.

TimetableListView(
    timetableTimeGroupItems: state.timeGroupTimetableItems,
    searchWord: "",
    onToggleBookmark: { id in
        viewModel.toggleBookmark(id)
    }
)

(After commenting out onToggleBookmark, the Header movement became smoother.)

ry-itto commented 1 year ago

@fumiyasac Thank you! But as far as I remember, it was before I added that process..

Here's a video of the upload failing example:

https://github.com/DroidKaigi/conference-app-2023/assets/30540303/41670a4a-8871-41da-adce-1cba80969f7d

Hmm.. onToggleBookmark is procedural process(not rendering process), so I think it isn't related to scroll performance..

fumiyasac commented 1 year ago

@ry-itto

Try commenting it out like below. It should move smoothly. (I'm sorry that I can't make time to investigate because I'm busy.)

TimetableListView(
    timetableTimeGroupItems: state.timeGroupTimetableItems,
    searchWord: "",
    onToggleBookmark: { id in
        //  👉 Comment out here, and horizontal scrolling becomes smooth, 
        // viewModel.toggleBookmark(id)
    }
)
sohichiro commented 1 year ago

@ry-itto If this section is commented out, the animation is broken, but the scrolling itself works smoothly.

ScrollViewWithVerticalOffset(
                        onOffsetChange: { offset in
                            //shouldCollapse = (offset < verticalOffsetThreshold) // <--- here is commented out
                        },

Alternatively, comment out another part like this and the scrolling will still work smoothly.

Section(
    header: TimetableDayHeader(
        selectedDay: viewModel.state.selectedDay,
        shouldCollapse: true,   // shouldCollapse is changed for true
        onSelect: {
            viewModel.selectDay(day: $0)
        }
    )
//.frame(height: shouldCollapse ? 53 : 82)
//.animation(.easeInOut(duration: 0.08), value: shouldCollapse)
)
fumiyasac commented 1 year ago

@sohichiro @ry-itto Thank you for their advices. If it is difficult to fix, I'll revert my commit. Sorry...

ry-itto commented 1 year ago

@fumiyasac @sohichiro Thanks! I'll look into this issue with your idea as well :pray:

ry-itto commented 1 year ago

@fumiyasac @sohichiro

This is caused by circular reference between TimetableViewModel instance and TimetabeListView 's closure ( toggleBookmark ). Currently, SwiftUI rendering is recursively called and it is caused by above. I'll fix that :pray:

ref: https://developer.apple.com/forums/thread/126890