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

#1654: Handle entity properties containing double quotation marks #1688

Merged
merged 7 commits into from Jan 24, 2017

Conversation

kduske
Copy link
Collaborator

@kduske kduske commented Jan 23, 2017

Closes #1654.

Double quotation marks are marked as issues. Two quick fixes are available: Delete property and Replace double with single quotation marks. Furthermore, they are escaped with backslashes when writing a map, and are unescaped when reading again.

@kduske kduske requested a review from ericwa January 23, 2017 21:38
@kduske kduske self-assigned this Jan 23, 2017
@ericwa
Copy link
Collaborator

ericwa commented Jan 24, 2017

Pasting this:

// entity 0
{
"classname" "monster_knight"
"origin" "-48 -16 40"
"angle" "-0"
"message" "test\\\\"
}

shows up in the UI as message test\ - one of the slashes gets lost.

Otherwise everything looks good.

@kduske
Copy link
Collaborator Author

kduske commented Jan 24, 2017

That's because it interprets two consecutive backslashes as one escaped backslash. So strictly speaking, backslashes will have to be escaped, and they are when TB writes the map file.

@kduske
Copy link
Collaborator Author

kduske commented Jan 24, 2017

See also the additional test case.

@ericwa
Copy link
Collaborator

ericwa commented Jan 24, 2017

Is the escape character esc implicitly included in the chars argument of StringUtils::escape/unescape?

This implies it is:

ASSERT_EQ(String("asdf\\"), StringUtils::unescape("asdf\\\\", ""));

but then this fails:

ASSERT_EQ(String("asdf\\\\"), StringUtils::unescape("asdf\\\\\\\\", ""));

Btw, raw string literals are working in ELParserTest.parseStringLiteralWithDoubleQuotationMarks:
ASSERT_EL_EQ(R"(asdf" "asdf)", R"("asdf\" \"asdf")");
Maybe we can use them in StringUtilsTest as well.

const String data("{"
"\"classname\" \"worldspawn\""
"\"path\" \"c:\\\\a\\\\b\\\\c\\\\\""
"}");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this work?

const String data(R"({)"
                  R"("classname" "worldspawn")"
...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had used raw string literals, only to have VC++ being unable to parse them. So they are banned, sorry.

@kduske
Copy link
Collaborator Author

kduske commented Jan 24, 2017

Is the escape character esc implicitly included in the chars argument of StringUtils::escape/unescape?

It is.

@kduske
Copy link
Collaborator Author

kduske commented Jan 24, 2017

ASSERT_EQ(String("asdf\\\\"), StringUtils::unescape("asdf\\\\\\\\", ""));

Should not have failed. It's fixed now. Thanks for the thorough review.

Copy link
Collaborator

@ericwa ericwa left a comment

Choose a reason for hiding this comment

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

Looks good now!

@kduske
Copy link
Collaborator Author

kduske commented Jan 24, 2017

I'm sure something else will break on windows or something. Goddamn backslashes.

@kduske kduske merged commit 229fd34 into release/v2.0.0 Jan 24, 2017
@kduske kduske deleted the feature/1654 branch November 6, 2017 20:24
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.

None yet

2 participants