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 special characters in file and folder names #904

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

barbeque-squared
Copy link
Member

fix #837

there are some other places where a TSearchRec is used, and we should probably also start getting rid of some of the RawByteString stuff because it's just asking for trouble, but short-term this should at least allow people on Windows to load all their songs again.
does not appear to have any effect on Linux, hence no ifdef guards.

@s09bQ5
Copy link
Collaborator

s09bQ5 commented Oct 6, 2024

Using TUnicodeSearchRec is harmless on Unix/Linux. The outcome is that the file name returned by FindFirst/FindNext is a UnicodeString instead of a RawByteString. This will cause a different implementation of Path to be used in TFileIterator.Next, i.e. the one that takes a WideString. The conversion from native character set to unicode is simply moved from Path to FindFirst/FindNext.

The only drawback is that using IPath.ToNative for the first argument of FindFirst becomes inefficient. IPath.ToWide would be better.

@barbeque-squared
Copy link
Member Author

I assume you mean the one(s) introduced by commit c7b7637 ? Basically whatever is in that Windows-only section should be ToWide ? (most already are (and always were) except this and some others...)

I have a feeling we can eventually get rid of all/most of this special string handling and just use string or (worst-case) UnicodeString for both platforms. And maybe get rid of ToNative entirely.

For this PR I agree it's worth trying to fix this one/few ToNative call(s) because song loading does call it a bajillion times, but if it breaks unrelated stuff I am inclined to just merge+release it anyway and fix it later (because a little bit slower loading is still better than it not loading some songs at all).

@barbeque-squared
Copy link
Member Author

Apparently that one can just be replaced and everything still works. I'll leave it open a day or two for s09bQ5 to comment on, otherwise it's going in because we need to release this fix soon.

@s09bQ5
Copy link
Collaborator

s09bQ5 commented Oct 7, 2024

Looks good

Copy link
Member

@basisbit basisbit left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the PR!

@basisbit basisbit merged commit 459a0f8 into master Oct 11, 2024
5 checks passed
@basisbit basisbit deleted the fix-special-characters branch October 11, 2024 19:19
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.

Song folders with non-ascii characters in their name are ignored without error on Windows
3 participants