Skip to content

Fix: Populate NavPoints even if playOrder not present#55

Merged
danigm merged 1 commit intodanigm:masterfrom
stumpapp:master
Aug 4, 2025
Merged

Fix: Populate NavPoints even if playOrder not present#55
danigm merged 1 commit intodanigm:masterfrom
stumpapp:master

Conversation

@aaronleopold
Copy link
Copy Markdown
Contributor

Hey there! 👋

Sorry for the random PR! This proposed change relates to a bug reported for Stump where a user reported an empty toc for some of their books. I believe I narrowed it down to epub-rs only including NavPoints which have a defined playOrder.

I'm happy to discuss and iterate on this change if it isn't within your preferred scope for the library. I also want to note that it could be possible to add id to the NavPoint as a fallback for sorting if desired, but didn't implement any of that since it would kinda be inconsistent for the Ord implementation in the scenario where one NavPoint has a play_order but another has id.

Copy link
Copy Markdown
Owner

@danigm danigm left a comment

Choose a reason for hiding this comment

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

Looks good to me

@danigm
Copy link
Copy Markdown
Owner

danigm commented Aug 4, 2025

I think that this implementation is okay enough, as far as I can tell, if there's no playorder, the order will be the order of appearance. I'm not sure if this is the right way of sorting in this case or if it's better to use a different approach, but in any case with this implementation is better than not parsing at all, so let's merge and fix the sorting in a future PR if we need an improvement.

@danigm danigm merged commit 8ffbbf1 into danigm:master Aug 4, 2025
6 checks passed
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.

2 participants