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 to current Chapter #1101

Merged
merged 8 commits into from
Jun 14, 2024

Conversation

CD-Z
Copy link
Collaborator

@CD-Z CD-Z commented Jun 11, 2024

Cleaned ChapterDrawer and fixed the Scroll to current Chapter behaviour.

@Palloxin
Copy link
Contributor

Palloxin commented Jun 11, 2024

Does it fix this too?
https://discord.com/channels/835746409357246465/1178895455237701652/1243675525449252935 (discord link to a message in LNReader server)

@CD-Z
Copy link
Collaborator Author

CD-Z commented Jun 11, 2024

Yes, this issue resulted from memoizing the index of the current chapter. The problem was that it returned 0 at first, because the chapter list hasn't been loaded yet. That is why it always points to the first chapter.

@nyagami
Copy link
Member

nyagami commented Jun 12, 2024

And I think using chapter id to detect the list item position is not correct for all novels.
There some novel has this order: id: 455, id: 456, id: 445,
you should use chapter.position, position per novel page or the index inside chapter drawer list

@CD-Z
Copy link
Collaborator Author

CD-Z commented Jun 12, 2024

@nyagami Why? I use the findIndex function to return the index on the chapter drawer list if the id of the currently opened chapter matches the id of the chapter in the list. This has to work, else the navigation to the chapter would also be broken, since this also depends on the id of the chapter in the list.

@nyagami
Copy link
Member

nyagami commented Jun 12, 2024

I mean this.

if (
  listAscending
    ? viewableItems[0].item.id < chapter.id
    : viewableItems[0].item.id > chapter.id
)
  • chapter.position is more reliable.
  • The indexes extracted from the list is the most reliable.
    Because Flashlist ViewToken has index, we only need the index in the list of current chapter
interface ViewToken {
  index: number;
  isViewable: boolean;
  item: string;
  key: string;
  timestamp: number;
}

And I dont think navigation would be broken no matter what you are using.

@CD-Z
Copy link
Collaborator Author

CD-Z commented Jun 12, 2024

  • chapter.position is more reliable

true, but I didn't know this existed since the type was missing.

@nyagami
Copy link
Member

nyagami commented Jun 12, 2024

Ops, I forgot the custom page novels. you can try the test Hako plugin.
so I think extracting the current chapter index from the list is the best way =))

@CD-Z
Copy link
Collaborator Author

CD-Z commented Jun 12, 2024

I tested it now for a bit with a few novels and haven't encountered any problems.

Comment on lines 78 to 80
func: () => {
listRef.current?.scrollToIndex({ index: 0, animated: true });
},
Copy link
Member

@nyagami nyagami Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using index (undefinded if scroll to end) instead of a callback is better.

const scroll = useCallback((index?: number) => {
 //scroll here
  }, [listRef.current]);

Also, moving the ListFooter to this file and call scroll callback directly in button oppress could be cleaner.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now refactored the file and I think I got what you meant. At least the file is now definitly cleaner.

@nyagami nyagami merged commit ea33690 into LNReader:master Jun 14, 2024
1 check passed
@CD-Z CD-Z deleted the feature/fix_scroll_to_current_chapter branch June 14, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants