Skip to content

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
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