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: extract nested slices #1821

Merged
merged 5 commits into from
Nov 13, 2024
Merged

fix: extract nested slices #1821

merged 5 commits into from
Nov 13, 2024

Conversation

bachbui
Copy link
Contributor

@bachbui bachbui commented Nov 13, 2024

Fix for the util method extractSlices. When extracting slices, we need to adjust the ranges of marks that appear after each slice in the document. In order to do this, we keep track of the ranges that are to be deleted and then loop over them, adjusting the mark ranges as needed.

However, when we have nested slices, each slice may be split by another slice, causing a slice to have multiple ranges. In this case, the list of ranges to be deleted would be overlapping, and thus eventually cause the mark ranges to be adjusted incorrectly. This fix normalizes the list of ranges to be deleted before we start updating mark ranges by first sorting those ranges and then merging overlapping ranges into each other.

Fix for the util method `extractSlices`. When extracting slices,
we need to adjust the ranges of marks that appear after each slice
in the document. In order to do this, we keep track of the ranges
that are to be deleted and then loop over them, adjusting the mark
ranges as needed.

However, when we have nested slices, each slice may be split by
another slice, causing a slice to have multiple ranges. In this
case, the list of ranges to be deleted would be overlapping,
and thus eventually cause the mark ranges to be adjusted
incorrectly. This fix normalizes the list of ranges to be deleted
before we start updating mark ranges by first sorting those ranges
and then merging overlapping ranges into each other.
Copy link
Contributor

@colin-alexa colin-alexa left a comment

Choose a reason for hiding this comment

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

neat!

@@ -390,6 +400,24 @@ export function extractSlices(value: {
});
}

// Normalize the ranges again, as we may have extended ranges into each other
// First sort the ranges
rangesToDelete = rangesToDelete.sort(compareRanges).reduce((acc, range) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer a for loop here for performance and clarity reasons, but I don't care that much

Copy link
Collaborator

@tim-evans tim-evans left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@bachbui bachbui merged commit 4940868 into main Nov 13, 2024
3 checks passed
@bachbui bachbui deleted the overlapping-slices branch November 13, 2024 22:05
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