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

Change localizedCaseInsensitiveCompare to localizedStandardCompare #451

Merged
merged 2 commits into from
Jun 10, 2024

Conversation

domkm
Copy link
Contributor

@domkm domkm commented Jun 8, 2024

Both Finder and Files sort by name using localizedStandardCompare. By using localizedCaseInsensitiveCompare, we introduce potentially counterintuitive behavior. For example, a user may import multiple files which appear to be sorted correctly in Finder/Files and then discover that we play them in a different order.

Like localizedCaseInsensitiveCompare, localizedStandardCompare is also case insensitive, but also accounts for numeric non-lexicographic sorting. For example (1...25).map { String($0) } will not change order when sorted with localizedStandardCompare, but with localizedCaseInsensitiveCompare the order will change to ["1", "10", "11", "12", ...].

I also noticed localizedCaseInsensitiveContains being used. Based on my reading of the code, I think normal contains may be appropriate, but, if not, perhaps localizedStandardContains should be used instead.

private func hasStyles(_ html: String) -> Bool {
html.localizedCaseInsensitiveContains("<link ")
|| html.localizedCaseInsensitiveContains(" style=")
|| html.localizedCaseInsensitiveContains("</style>")
}

…` to make sorting the same as Finder and Files
Copy link
Member

@mickael-menu mickael-menu left a comment

Choose a reason for hiding this comment

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

That's a sensible change, thank you!

I also noticed localizedCaseInsensitiveContains being used. Based on my reading of the code, I think normal contains may be appropriate, but, if not, perhaps localizedStandardContains should be used instead.

In the snippet you shared, we do want to be case insensitive to also match for example <LINK STYLE=""> which is valid HTML.

@mickael-menu mickael-menu merged commit 989440b into readium:develop Jun 10, 2024
5 of 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.

None yet

2 participants