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

COMMON: added support for reading ini files with different 8bit encod… #1812

Merged
merged 1 commit into from Aug 20, 2019

Conversation

@whiterandrek
Copy link
Member

commented Aug 18, 2019

This pull request adds support for reading ini files with encodings like CP-1250, CP-1251, CP-1252, etc.
Currently, those ini files can't be read because isAlnum function returns zero for chars which are not standard ASCII characterts.

@@ -36,6 +36,7 @@ bool INIFile::isValidName(const String &name) {
}

INIFile::INIFile() {
_8bitMode = false;

This comment has been minimized.

Copy link
@bluegr

bluegr Aug 19, 2019

Member

The name is a bit confusing. It could be renamed to something like _allowNonEnglishCharacters

@@ -125,7 +126,7 @@ bool INIFile::loadFromStream(SeekableReadStream &stream) {
section.comment = comment;
comment.clear();

assert(isValidName(section.name));
assert(_8bitMode || isValidName(section.name));

This comment has been minimized.

Copy link
@bluegr

bluegr Aug 19, 2019

Member

It would be better to extend the functionality of isValidName(), instead of short-circuiting its usage all over the place

@whiterandrek whiterandrek force-pushed the whiterandrek:ini branch from ba894e8 to 62a1a11 Aug 19, 2019

@whiterandrek whiterandrek requested a review from bluegr Aug 19, 2019

@whiterandrek

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

@bluegr
Because isValidName is static method.

@bluegr

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

Hm, I don’t remember why it’s static. Will need to check the code later on from a PC

@bluegr

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

I've adapted the usage of isValidName(), and it's no longer static, either. You will need to redo your changes, unfortunately

@whiterandrek whiterandrek force-pushed the whiterandrek:ini branch from 62a1a11 to 485e7eb Aug 20, 2019

@bluegr

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Great, thanks! Merging

@bluegr bluegr merged commit c205468 into scummvm:master Aug 20, 2019

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.