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

Escape more strings: formspecs, item descriptions, infotexts... #3948

Closed
wants to merge 11 commits into from

Conversation

Projects
None yet
8 participants
@Ekdohibs
Copy link
Member

commented Apr 1, 2016

Thus \x1b can be used in the future for translation, for example.

@Ekdohibs Ekdohibs force-pushed the Ekdohibs:escapes branch Apr 1, 2016

@Ekdohibs Ekdohibs referenced this pull request Apr 2, 2016

Closed

Add client-side translations #3950

0 of 4 tasks complete

@Ekdohibs Ekdohibs force-pushed the Ekdohibs:escapes branch to 338ab39 Apr 3, 2016

Escape more strings: formspecs, item descriptions, infotexts...
Also, change the escape character to the more standard \x1b
Thus, it can be used in the future for translation or colored text,
for example.

@Ekdohibs Ekdohibs force-pushed the Ekdohibs:escapes branch from 338ab39 to ccd898f Apr 4, 2016

@Ekdohibs Ekdohibs added this to the 0.4.14 milestone Apr 6, 2016

@Ekdohibs

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2016

I'd like this to be merged in 0.4.14 as it is compatibility for future code; marking as milestone.

bgcolor(a_bgcolor),
color(a_color)
{
tooltip = unescape_string(wide_to_utf8(

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 6, 2016

Member

maybe having a helper for this ?

@paramat

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

I'd like to help out by reviewing but i don't understand the code, it's best others review.

*
* @param s The string in which to remove escape sequences.
* @return \p s, with escape sequences removed.
*/
std::wstring removeChatEscapes(const std::wstring &s);
std::wstring removeEscapes(const std::wstring &s);

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2016

Contributor

This function should be lower_case_underscore style since it's a standalone function.

@@ -729,11 +729,11 @@ static bool parseNamedColorString(const std::string &value, video::SColor &color
return true;
}

std::wstring removeChatEscapes(const std::wstring &s) {
std::wstring removeEscapes(const std::wstring &s) {
std::wstring output;
size_t i = 0;
while (i < s.length()) {

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2016

Contributor

I feel like this function could be structured better. Also, this deserves a unit test.

@@ -199,10 +200,10 @@ class GUIFormSpecMenu : public GUIModalMenu
FieldSpec(const std::string &name, const std::wstring &label,
const std::wstring &fdeflt, int id) :

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2016

Contributor

fdeflt? WTF?

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs Apr 9, 2016

Author Member

That was the name of the parameter, I guess it is to avoid a clash with field fdefault.

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2016

Contributor

Maybe change it to default so it's actually legible? It'll also be consistent with the rest of the parameters which are otherwise identical to the members, but lacking the f prefix.

@@ -1604,14 +1593,14 @@ void GUIFormSpecMenu::parseTooltip(parserData* data, std::string element)
std::vector<std::string> parts = split(element,';');

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2016

Contributor

What is the difference between this function split and str_split?

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs Apr 9, 2016

Author Member

split checks for escaped characters before breaking, while str_split does not.

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Apr 23, 2016

Member

Should be str_split_escaped then.

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs Apr 23, 2016

Author Member

Probably, however I did not touch this code so I will not change it in that PR; I could however do another one for that.

@@ -1604,14 +1593,14 @@ void GUIFormSpecMenu::parseTooltip(parserData* data, std::string element)
std::vector<std::string> parts = split(element,';');
if (parts.size() == 2) {
std::string name = parts[0];
m_tooltips[name] = TooltipSpec(unescape_string(parts[1]),
m_tooltips[name] = TooltipSpec(parts[1],

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2016

Contributor

Why were the unescape_string() calls removed here?

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs Apr 9, 2016

Author Member

The unescape_string calls are now part of the constructor of TooltipSpec.

std::wstring output;
size_t i = 0;
while (i < s.length()) {
if (s[i] == L'\v') {
if (s[i] == L'\x1b') {

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2016

Contributor

\v is 0x0B while you're changing the new escape character to 0x1B. Won't this break reverse compatibility?

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs Apr 9, 2016

Author Member

It won't, because these escapes were added but a month ago and were never used for anything yet.

@@ -746,7 +743,8 @@ void GUIFormSpecMenu::parseTable(parserData* data,std::string element)
spec.ftype = f_Table;

for (unsigned int i = 0; i < items.size(); ++i) {
items[i] = unescape_string(items[i]);
items[i] = unescape_string(wide_to_utf8(
remove_escapes(utf8_to_wide(items[i]))));

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2016

Contributor

Doesn't this imply the need for a variant of remove_escapes() that works on a utf-8 string?

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs Apr 9, 2016

Author Member

We might want to add data about a number of characters in escapes (let's say, remove the previous n characters), and that needs to be consistent so the text has to either be always utf8 or always wide, but not sometimes one and sometimes the other.

This comment has been minimized.

Copy link
@est31

est31 Apr 9, 2016

Contributor

Wide characters are not uniform across platforms. On some they are wider, on some they are smaller.

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs Apr 11, 2016

Author Member

@est31 wait, some characters can span several wide chars?

This comment has been minimized.

Copy link
@est31

est31 Apr 11, 2016

Contributor

Yes, on windows there are surrogate pairs. I'm not sure whether linux has them too, or uses the code points.

{
UASSERT(remove_escapes(L"abc\x1bXdef") == L"abcdef");
UASSERT(remove_escapes(L"abc\x1b(escaped)def") == L"abcdef");
UASSERT(remove_escapes(L"abc\x1b((escaped with parenthesis\\))def") == L"abcdef");

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2016

Contributor

This tests functionality well, but could you also add some edge cases so I have confidence it'll work?
For example, an incomplete escape sequence at the end of the string, an escape sequence with an unclosed set of parentheses, an escape sequence without a set of parentheses at all, nested escape sequences, etc. The point of a test is to try your hardest to break the code, not feed it values you already know work well.

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs Apr 9, 2016

Author Member

Without parentheses is already the first test, I'm adding the two others.

Ekdohibs added some commits Apr 9, 2016

UASSERT(remove_escapes(L"abc\x1b((escaped with parenthesis\\))def") == L"abcdef");
UASSERT(remove_escapes(L"abc\x1b(incomplete") == L"abc");
UASSERT(remove_escapes(L"escape at the end\x1b") == L"escape at the end");
}

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2016

Contributor

How about a unit test that has a nested escape sequence, like \x1b(blah\x1b(asdf)blah)dffgd?
Also, abc\x1bdef)?
A string that starts with an escape sequence?

@est31

View changes

src/guiFormSpecMenu.h Outdated
struct FieldSpec
{
FieldSpec()
{
}
FieldSpec(const std::string &name, const std::wstring &label,
const std::wstring &fdeflt, int id) :
const std::wstring &default, int id) :

This comment has been minimized.

Copy link
@est31

est31 Apr 9, 2016

Contributor

👎 to the rename, this does bad things with syntax coloring.

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 11, 2016

Contributor

Whoops, that's why it's fdeflt I guess. Maybe make it default_text instead?

@est31

View changes

src/guiFormSpecMenu.h Outdated
rect(a_rect),
parent_button(NULL)
{
text = unescape_string(remove_escapes(a_text));

This comment has been minimized.

Copy link
@est31

est31 Apr 9, 2016

Contributor

We have two escape systems now, have you thought about giving it a better name that makes it easier to distinguish the two systems?

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs Apr 11, 2016

Author Member

What about remove_enriched_text_escapes? Maybe a bit too long though.

@Ekdohibs Ekdohibs force-pushed the Ekdohibs:escapes branch to 90c7def Apr 11, 2016

@C1ffisme

This comment has been minimized.

Copy link

commented Apr 13, 2016

Will this remove this hack?

screenshot_20160328_110840

minetest.register_craftitem("tests:newline",{
    description = "Line1 \nLine2 \nLine3 \nLine4",
    inventory_image = "default_wood.png",
})

Just thought I'd ask.

@Ekdohibs

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2016

@C1ffisme This is not a hack IMHO... Multi-line item descriptions may be useful.

@C1ffisme

This comment has been minimized.

Copy link

commented Apr 16, 2016

@Ekdohibs Yeah, especially for more detailed 'descriptions' or lore:

screenshot_20160416_100131

@paramat paramat removed this from the 0.4.14 milestone Apr 20, 2016

@paramat paramat added this to the 0.4.14 milestone Apr 21, 2016

@@ -50,7 +50,7 @@ void TestRandom::runTests(IGameDef *gamedef)
TEST(testPseudoRandom);
TEST(testPseudoRandomRange);
TEST(testPcgRandom);
TEST(testPcgRandomRange);
//TEST(testPcgRandomRange);

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 22, 2016

Contributor

why'd you do this??

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs Apr 22, 2016

Author Member

Oops, I didn't meant to commit that change. The problem is that that line crashes unit tests.

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 23, 2016

Contributor

Interesting, if you have a backtrace I'd like to have a look at that.

* @return \p s, with escape sequences removed.
*/
template <typename T>
std::basic_string<T> remove_enriched_text_escapes(const std::basic_string<T> &s) {

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 22, 2016

Contributor

Yeah this is way too long of a name... I can't really offer a suggestion for an alternative, but this is too long. Also, the line is way over 80 characters. The { should be on the next line too.

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs Apr 22, 2016

Author Member

unescape_enriched ?

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 23, 2016

Contributor

Sure, that works I guess.

@@ -253,6 +255,23 @@ void TestUtilities::testUTF8()
== "the shovel dug a crumbly node!");
}

void TestUtilities::testRemoveEscapes()
{
UASSERT(remove_enriched_text_escapes<wchar_t>(

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 22, 2016

Contributor

Why do you explicitly have the template parameter here but not anywhere else??

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs Apr 22, 2016

Author Member

I think it is because the arguments are constant strings, which are interpreted by the compiler as const wchar_t * and not str::wstring

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2016

Wait, so does this commit break newlines in tooltips or not? It's a bit unclear from the previous comments.

@Ekdohibs

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2016

This commit changes nothing from the previous behaviour about breaking newlines in tooltips.

@ShadowNinja

This comment has been minimized.

Copy link
Member

commented Apr 23, 2016

This should use a proper escape character like \x1B.
\v has a text formatting function just like, eg, \n (although \v is probably not properly handled at the moment).

@Ekdohibs

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2016

@ShadowNinja it already uses \x1b instead of the original \v

@Zeno- Zeno- added the One approval label Apr 23, 2016

@Zeno-

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2016

👍

@@ -2508,11 +2508,11 @@ void GUIFormSpecMenu::drawMenu()
if (id != -1 && delta >= m_tooltip_show_delay) {
for(std::vector<FieldSpec>::iterator iter = m_fields.begin();
iter != m_fields.end(); ++iter) {
if ( (iter->fid == id) && (m_tooltips[iter->fname].tooltip != "") ){
if ( (iter->fid == id) && (m_tooltips[iter->fname].tooltip != L"") ){

This comment has been minimized.

Copy link
@paramat

paramat Apr 23, 2016

Member

Inner brackets not needed? Needs space before curly bracket.

@paramat

This comment has been minimized.

Copy link
Member

commented Apr 23, 2016

Added a line comment.
I think hmmmm should look at this as he had some issues.

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2016

👍

@paramat

This comment has been minimized.

Copy link
Member

commented Apr 23, 2016

Code style looks ok now.

@Zeno-

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2016

@paramat paramat closed this Apr 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.