Skip to content

Add open_file_skipping_BOM function to RC::File#556

Closed
narknon wants to merge 1 commit into
mainfrom
OpenSkipBOM
Closed

Add open_file_skipping_BOM function to RC::File#556
narknon wants to merge 1 commit into
mainfrom
OpenSkipBOM

Conversation

@narknon

@narknon narknon commented Jun 5, 2024

Copy link
Copy Markdown
Collaborator

No description provided.

UE4SS
UE4SS previously requested changes Jun 5, 2024

@UE4SS UE4SS left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please run clang format.

Comment thread deps/first/File/include/File/File.hpp Outdated
Comment thread deps/first/File/src/File.cpp Outdated
Comment thread deps/first/File/src/File.cpp Outdated
Comment thread deps/first/File/src/File.cpp Outdated
Comment thread deps/first/File/src/File.cpp Outdated
@UE4SS

UE4SS commented Jun 8, 2024

Copy link
Copy Markdown
Collaborator

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

narknon commented Jun 8, 2024

Copy link
Copy Markdown
Collaborator Author

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

UE4SS commented Jun 8, 2024

Copy link
Copy Markdown
Collaborator

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

narknon commented Jun 9, 2024

Copy link
Copy Markdown
Collaborator Author

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

Yangff commented Jun 9, 2024

Copy link
Copy Markdown
Contributor

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

narknon commented Jun 10, 2024

Copy link
Copy Markdown
Collaborator Author

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
Comment thread deps/first/File/src/File.cpp
@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
Copy Markdown
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

UE4SS commented Oct 6, 2024

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Contributor

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

@UE4SS

UE4SS commented Oct 6, 2024

Copy link
Copy Markdown
Collaborator

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

narknon commented Oct 6, 2024

Copy link
Copy Markdown
Collaborator Author

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