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

Add open_file_skipping_BOM function to RC::File #556

Closed
wants to merge 1 commit into from
Closed

Conversation

narknon
Copy link
Collaborator

@narknon narknon commented Jun 5, 2024

No description provided.

UE4SS
UE4SS previously requested changes Jun 5, 2024
Copy link
Collaborator

@UE4SS UE4SS left a comment

Choose a reason for hiding this comment

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

Please run clang format.

deps/first/File/include/File/File.hpp Outdated Show resolved Hide resolved
deps/first/File/src/File.cpp Outdated Show resolved Hide resolved
deps/first/File/src/File.cpp Outdated Show resolved Hide resolved
deps/first/File/src/File.cpp Outdated Show resolved Hide resolved
deps/first/File/src/File.cpp Outdated Show resolved Hide resolved
@UE4SS
Copy link
Collaborator

UE4SS commented Jun 8, 2024

I'm not going to be able to properly review this because I don't know enough about the operations involved in what this PR is trying to accomplish.

@narknon
Copy link
Collaborator Author

narknon commented Jun 8, 2024

The build script was using utf8-sig, which adds the BOM signature to the start of text files. We need to be able to ignore that when parsing mods.txt

@UE4SS
Copy link
Collaborator

UE4SS commented Jun 8, 2024

The build script was using utf8-sig, which adds the BOM signature to the start of text files. We need to be able to ignore that when parsing mods.txt

Yes I know why this is needed, but what I meant was, I'm not qualified to say whether your code is correct and good.

@narknon
Copy link
Collaborator Author

narknon commented Jun 9, 2024

I'm open to any code that resolves this issue. I imagine there are likely better ways. Theoretically this method is temporarily needed for a release cycle

@Yangff
Copy link
Contributor

Yangff commented Jun 9, 2024

wfile.imbue should be set for utf8 case instead of utf16..? because handling utf8 with wstring can cause problem .. it will just pack two utf8 chars as if it's utf16...

but if I understand correctly, all UE4SS files should be utf8, so I really don't know why RC::File is using wstring.. (and that's why I'm trying to make all of them utf8 when porting it..)

@bitonality
Copy link
Contributor

Have we considered taking a 3rd party library dep for this use case?

I think it's awesome that we're aiming to stay compatible with the utf8 BOM files we've released, but we also have an opportunity to make our file parsing more robust. Since end users are able to modify config, maybe we should extend to support both utf8 and utf16?

@narknon
Copy link
Collaborator Author

narknon commented Jun 10, 2024

The third party dep for mods.txt is glz. This is merely a required functionality to maintain backwards compatibility, and this PR shouldn't expand in scope. If someone wants to make a bigger PR with other file parsing functionality they can

@narknon narknon requested a review from UE4SS June 10, 2024 19:10
@UE4SS UE4SS dismissed their stale review June 10, 2024 20:04

For some reason, its ignoring the fact that all my questions were resolved.

@igromanru
Copy link
Contributor

What is the state here? It's quite annoying that the first line is still ignored.
I wanted to test the changes, but open_file_skip_BOM is not called anywhere.

@UE4SS
Copy link
Collaborator

UE4SS commented Oct 6, 2024

What is the state here? It's quite annoying that the first line is still ignored. I wanted to test the changes, but open_file_skip_BOM is not called anywhere.

It's used in #540

@igromanru
Copy link
Contributor

The other PR is huge, can't we get this change tested and merged first?
Who knows when #540 will be done.

@UE4SS
Copy link
Collaborator

UE4SS commented Oct 6, 2024

What is the state here? It's quite annoying that the first line is still ignored. I wanted to test the changes, but open_file_skip_BOM is not called anywhere.

It's used in #540

The other PR is huge, can't we get this change tested and merged first? Who knows when #540 will be done.

Yes, and in fact we have to because #540 is dependent on this PR.

@narknon
Copy link
Collaborator Author

narknon commented Oct 6, 2024

This method won't be used. 540 needs to be rebased to use Yang's method, so this can be closed.

@narknon narknon closed this Oct 6, 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.

5 participants