Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: scroll optimize #334

Merged
merged 3 commits into from
Mar 12, 2024
Merged

fix: scroll optimize #334

merged 3 commits into from
Mar 12, 2024

Conversation

ivanfei-1
Copy link
Member

@ivanfei-1 ivanfei-1 commented Mar 8, 2024

The super_sliver_list package seems to fix this long existing problem mentioned in pr #162 and issue #201. Since the branch is long gone, I am opening a new one here.

I have tested the performance on the material-2 branch, where the improvement is significant.

Before merging this branch, you should test on your own device with a different version of flutter.

@ivanfei-1 ivanfei-1 added bug Something isn't working enhancement New feature or request high priority It should be worked on NOW labels Mar 8, 2024
@ivanfei-1 ivanfei-1 requested a review from w568w March 8, 2024 17:59
@ivanfei-1 ivanfei-1 removed the high priority It should be worked on NOW label Mar 8, 2024
@w568w
Copy link
Member

w568w commented Mar 9, 2024

I have not got a suitable post to test with. #60678 is deep enough but most floors are simply plain texts. We may need a post with many floors which mostly have complex Markdown content to reproduce parsing performance degradation.

Some problems noticed:

  • On Android, in a very deep post (e.g. #60678), if "jump to the bottom" is tapped and finding any floor A which refers to an earlier floor B in the same post, trying jumping to floor B by tapping "Locate" in A's reference will always scroll the list to the top, rather than scroll to B.
    I guess it may have something to do with lazy loading feature of SuperListView, which will not load the previous floors when scrolling to bottom (i.e. their heights are all zero), and thus we mistake the (First floor -> B floor)'s height sum as zero, too.
    Edit: Not the case. The top height seems to be calculated correctly:

    I/flutter (23367): itemTop: 1685.0
    I/flutter (23367): itemTop: 1211.0
    
  • Tapping "jump to the bottom" twice will make the list only show the first 10 items.

@w568w
Copy link
Member

w568w commented Mar 9, 2024

*sigh*

I think we need to overhaul the entire PagedListView (and StateKey), because there are some implementations that don't follow the Flutter state specification at all, and there are a lot of hacks. They're very likely to cause mysterious bugs.

@w568w
Copy link
Member

w568w commented Mar 12, 2024

Update: the 2nd problem seems to be introduced in 2d260ce, where the _hole.floors!.prefetch is reset to only first 10 items during each build.

Anyway, I will rewrite this part. It feels really awkward now.

…m that the list only shows the first 10 items after clicking "jump to bottom" twice
@ivanfei-1
Copy link
Member Author

ivanfei-1 commented Mar 12, 2024

LGTM. Basic features have been tested, and no bug is found so far. I think we can merge this branch before thorough testing.

@ivanfei-1 ivanfei-1 merged commit 716ceb1 into main Mar 12, 2024
@w568w w568w linked an issue Mar 12, 2024 that may be closed by this pull request
@ivanfei-1 ivanfei-1 added the good first issue Good for newcomers label Mar 13, 2024
@w568w w568w deleted the scroll-optimize branch March 13, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] 旦夕在浏览某些帖子时崩溃
2 participants