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

Jump to bottom

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

Closed
wants to merge 11 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ccd898f
Escape more strings: formspecs, item descriptions, infotexts...
Ekdohibs Apr 4, 2016
53e6623
Change tooltip to a wstring to avoid useless wstring <-> utf8 convers…
Ekdohibs Apr 9, 2016
15ed01b
Change name to remove_escapes and add unittests.
Ekdohibs Apr 9, 2016
c5385a5
Add more unittests.
Ekdohibs Apr 9, 2016
7538011
Add comment that unescaping was moved.
Ekdohibs Apr 9, 2016
a3f7cb1
Even more escapes
Ekdohibs Apr 9, 2016
90c7def
fdeflt -> default_text
Ekdohibs Apr 11, 2016
b3183df
remove_escapes -> remove_enriched_text_escapes, and make it work with…
Ekdohibs Apr 20, 2016
7ce2bcd
Re-add unintentionally removed testPcgRandomRange
Ekdohibs Apr 22, 2016
84b1bc7
remove_enriched_text_escapes -> unescape_enriched
Ekdohibs Apr 22, 2016
c0ff1c2
Style fix
Ekdohibs Apr 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/chat.cpp
View file
Open in desktop
Expand Up @@ -679,8 +679,8 @@ ChatBackend::~ChatBackend()

void ChatBackend::addMessage(std::wstring name, std::wstring text)
{
name = removeChatEscapes(name);
text = removeChatEscapes(text);
name = remove_escapes(name);
text = remove_escapes(text);

// Note: A message may consist of multiple lines, for example the MOTD.
WStrfnd fnd(text);
Expand Down
6 changes: 3 additions & 3 deletions src/game.cpp
View file
Open in desktop
Expand Up @@ -3728,7 +3728,7 @@ void Game::handlePointingAtNode(GameRunData *runData,
NodeMetadata *meta = map.getNodeMetadata(nodepos);

if (meta) {
infotext = utf8_to_wide(meta->getString("infotext"));
infotext = remove_escapes(utf8_to_wide(meta->getString("infotext")));
} else {
MapNode n = map.getNodeNoEx(nodepos);

Expand Down Expand Up @@ -3804,13 +3804,13 @@ void Game::handlePointingAtObject(GameRunData *runData,
const v3f &player_position,
bool show_debug)
{
infotext = utf8_to_wide(runData->selected_object->infoText());
infotext = remove_escapes(utf8_to_wide(runData->selected_object->infoText()));

if (show_debug) {
if (infotext != L"") {
infotext += L"\n";
}
infotext += utf8_to_wide(runData->selected_object->debugInfoText());
infotext += remove_escapes(utf8_to_wide(runData->selected_object->debugInfoText()));
}

if (input->getLeftState()) {
Expand Down
53 changes: 22 additions & 31 deletions src/guiFormSpecMenu.cpp
View file
Open in desktop
Expand Up @@ -609,8 +609,6 @@ void GUIFormSpecMenu::parseButton(parserData* data,std::string element,
if(!data->explicit_size)
warningstream<<"invalid use of button without a size[] element"<<std::endl;

label = unescape_string(label);

std::wstring wlabel = utf8_to_wide(label);

FieldSpec spec(
Expand Down Expand Up @@ -733,7 +731,6 @@ void GUIFormSpecMenu::parseTable(parserData* data,std::string element)
geom.X = stof(v_geom[0]) * (float)spacing.X;
geom.Y = stof(v_geom[1]) * (float)spacing.Y;


core::rect<s32> rect = core::rect<s32>(pos.X, pos.Y, pos.X+geom.X, pos.Y+geom.Y);

FieldSpec spec(
Expand All @@ -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]))));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

//now really show table
Expand Down Expand Up @@ -818,7 +816,8 @@ void GUIFormSpecMenu::parseTextList(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]))));
}

//now really show list
Expand Down Expand Up @@ -889,7 +888,7 @@ void GUIFormSpecMenu::parseDropDown(parserData* data,std::string element)
}

for (unsigned int i=0; i < items.size(); i++) {
e->addItem(utf8_to_wide(items[i]).c_str());
e->addItem(unescape_string(remove_escapes(utf8_to_wide(items[i]))).c_str());
}

if (str_initial_selection != "")
Expand Down Expand Up @@ -930,8 +929,6 @@ void GUIFormSpecMenu::parsePwdField(parserData* data,std::string element)

core::rect<s32> rect = core::rect<s32>(pos.X, pos.Y, pos.X+geom.X, pos.Y+geom.Y);

label = unescape_string(label);

std::wstring wlabel = utf8_to_wide(label);

FieldSpec spec(
Expand Down Expand Up @@ -995,8 +992,6 @@ void GUIFormSpecMenu::parseSimpleField(parserData* data,
if(m_form_src)
default_val = m_form_src->resolveText(default_val);

default_val = unescape_string(default_val);
label = unescape_string(label);

std::wstring wlabel = utf8_to_wide(label);

Expand Down Expand Up @@ -1094,9 +1089,6 @@ void GUIFormSpecMenu::parseTextArea(parserData* data,
default_val = m_form_src->resolveText(default_val);


default_val = unescape_string(default_val);
label = unescape_string(label);

std::wstring wlabel = utf8_to_wide(label);

FieldSpec spec(
Expand Down Expand Up @@ -1197,7 +1189,6 @@ void GUIFormSpecMenu::parseLabel(parserData* data,std::string element)
if(!data->explicit_size)
warningstream<<"invalid use of label without a size[] element"<<std::endl;

text = unescape_string(text);
std::vector<std::string> lines = split(text, '\n');

for (unsigned int i = 0; i != lines.size(); i++) {
Expand Down Expand Up @@ -1243,7 +1234,7 @@ void GUIFormSpecMenu::parseVertLabel(parserData* data,std::string element)
((parts.size() > 2) && (m_formspec_version > FORMSPEC_API_VERSION)))
{
std::vector<std::string> v_pos = split(parts[0],',');
std::wstring text = utf8_to_wide(unescape_string(parts[1]));
std::wstring text = unescape_string(remove_escapes(utf8_to_wide(parts[1])));

MY_CHECKPOS("vertlabel",1);

Expand Down Expand Up @@ -1330,7 +1321,6 @@ void GUIFormSpecMenu::parseImageButton(parserData* data,std::string element,

image_name = unescape_string(image_name);
pressed_image_name = unescape_string(pressed_image_name);
label = unescape_string(label);

std::wstring wlabel = utf8_to_wide(label);

Expand Down Expand Up @@ -1430,7 +1420,7 @@ void GUIFormSpecMenu::parseTabHeader(parserData* data,std::string element)
e->setNotClipped(true);

for (unsigned int i = 0; i < buttons.size(); i++) {
e->addTab(utf8_to_wide(buttons[i]).c_str(), -1);
e->addTab(unescape_string(remove_escapes(utf8_to_wide(buttons[i]))).c_str(), -1);
}

if ((tab_index >= 0) &&
Expand Down Expand Up @@ -1489,7 +1479,6 @@ void GUIFormSpecMenu::parseItemImageButton(parserData* data,std::string element)
m_default_tooltip_bgcolor,
m_default_tooltip_color);

label = unescape_string(label);
FieldSpec spec(
name,
utf8_to_wide(label),
Expand Down Expand Up @@ -1604,14 +1593,14 @@ void GUIFormSpecMenu::parseTooltip(parserData* data, std::string element)
std::vector<std::string> parts = split(element,';');
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between this function split and str_split?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Should be str_split_escaped then.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

if (parts.size() == 2) {
std::string name = parts[0];
m_tooltips[name] = TooltipSpec(unescape_string(parts[1]),
m_tooltips[name] = TooltipSpec(parts[1],
Copy link
Contributor

Choose a reason for hiding this comment

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

Why were the unescape_string() calls removed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

m_default_tooltip_bgcolor, m_default_tooltip_color);
return;
} else if (parts.size() == 4) {
std::string name = parts[0];
video::SColor tmp_color1, tmp_color2;
if ( parseColorString(parts[2], tmp_color1, false) && parseColorString(parts[3], tmp_color2, false) ) {
m_tooltips[name] = TooltipSpec(unescape_string(parts[1]),
m_tooltips[name] = TooltipSpec(parts[1],
tmp_color1, tmp_color2);
return;
}
Expand Down Expand Up @@ -2242,16 +2231,18 @@ void GUIFormSpecMenu::drawList(const ListDrawSpec &s, int phase,
}

// Draw tooltip
std::string tooltip_text = "";
if (hovering && !m_selected_item)
tooltip_text = item.getDefinition(m_gamedef->idef()).description;
if (tooltip_text != "") {
std::vector<std::string> tt_rows = str_split(tooltip_text, '\n');
std::wstring tooltip_text = L"";
if (hovering && !m_selected_item) {
tooltip_text = utf8_to_wide(item.getDefinition(m_gamedef->idef()).description);
tooltip_text = remove_escapes(tooltip_text);
}
if (tooltip_text != L"") {
std::vector<std::wstring> tt_rows = str_split(tooltip_text, L'\n');
m_tooltip_element->setBackgroundColor(m_default_tooltip_bgcolor);
m_tooltip_element->setOverrideColor(m_default_tooltip_color);
m_tooltip_element->setVisible(true);
this->bringToFront(m_tooltip_element);
m_tooltip_element->setText(utf8_to_wide(tooltip_text).c_str());
m_tooltip_element->setText(tooltip_text.c_str());
s32 tooltip_width = m_tooltip_element->getTextWidth() + m_btn_height;
s32 tooltip_height = m_tooltip_element->getTextHeight() * tt_rows.size() + 5;
v2u32 screenSize = driver->getScreenSize();
Expand Down Expand Up @@ -2504,7 +2495,7 @@ void GUIFormSpecMenu::drawMenu()
u32 delta = 0;
if (id == -1) {
m_old_tooltip_id = id;
m_old_tooltip = "";
m_old_tooltip = L"";
} else {
if (id == m_old_tooltip_id) {
delta = porting::getDeltaMs(m_hovered_time, getTimeMs());
Expand All @@ -2517,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"") ){
Copy link
Contributor

Choose a reason for hiding this comment

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

Inner brackets not needed? Needs space before curly bracket.

if (m_old_tooltip != m_tooltips[iter->fname].tooltip) {
m_old_tooltip = m_tooltips[iter->fname].tooltip;
m_tooltip_element->setText(utf8_to_wide(m_tooltips[iter->fname].tooltip).c_str());
std::vector<std::string> tt_rows = str_split(m_tooltips[iter->fname].tooltip, '\n');
m_tooltip_element->setText(m_tooltips[iter->fname].tooltip.c_str());
std::vector<std::wstring> tt_rows = str_split(m_tooltips[iter->fname].tooltip, L'\n');
s32 tooltip_width = m_tooltip_element->getTextWidth() + m_btn_height;
s32 tooltip_height = m_tooltip_element->getTextHeight() * tt_rows.size() + 5;
int tooltip_offset_x = m_btn_height;
Expand Down Expand Up @@ -2875,7 +2866,7 @@ bool GUIFormSpecMenu::preprocessEvent(const SEvent& event)
core::position2d<s32>(x, y));
if (event.MouseInput.Event == EMIE_LMOUSE_PRESSED_DOWN) {
m_old_tooltip_id = -1;
m_old_tooltip = "";
m_old_tooltip = L"";
}
if (!isChild(hovered,this)) {
if (DoubleClickDetection(event)) {
Expand Down
15 changes: 8 additions & 7 deletions src/guiFormSpecMenu.h
View file
Open in desktop
Expand Up @@ -29,6 +29,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
#include "modalMenu.h"
#include "guiTable.h"
#include "network/networkprotocol.h"
#include "util/string.h"

class IGameDef;
class InventoryManager;
Expand Down Expand Up @@ -199,10 +200,10 @@ class GUIFormSpecMenu : public GUIModalMenu
FieldSpec(const std::string &name, const std::wstring &label,
const std::wstring &fdeflt, int id) :
Copy link
Contributor

Choose a reason for hiding this comment

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

fdeflt? WTF?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

fname(name),
flabel(label),
fdefault(fdeflt),
fid(id)
{
flabel = unescape_string(remove_escapes(label));
fdefault = unescape_string(remove_escapes(fdeflt));
send = false;
ftype = f_Unknown;
is_exit = false;
Expand Down Expand Up @@ -235,12 +236,12 @@ class GUIFormSpecMenu : public GUIModalMenu
}
TooltipSpec(std::string a_tooltip, irr::video::SColor a_bgcolor,
irr::video::SColor a_color):
tooltip(a_tooltip),
bgcolor(a_bgcolor),
color(a_color)
{
tooltip = unescape_string(remove_escapes(utf8_to_wide(a_tooltip)));
}
std::string tooltip;
std::wstring tooltip;
irr::video::SColor bgcolor;
irr::video::SColor color;
};
Expand All @@ -252,18 +253,18 @@ class GUIFormSpecMenu : public GUIModalMenu
}
StaticTextSpec(const std::wstring &a_text,
const core::rect<s32> &a_rect):
text(a_text),
rect(a_rect),
parent_button(NULL)
{
text = unescape_string(remove_escapes(a_text));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

What about remove_enriched_text_escapes? Maybe a bit too long though.

}
StaticTextSpec(const std::wstring &a_text,
const core::rect<s32> &a_rect,
gui::IGUIButton *a_parent_button):
text(a_text),
rect(a_rect),
parent_button(a_parent_button)
{
text = unescape_string(remove_escapes(a_text));
}
std::wstring text;
core::rect<s32> rect;
Expand Down Expand Up @@ -406,7 +407,7 @@ class GUIFormSpecMenu : public GUIModalMenu
u32 m_tooltip_show_delay;
s32 m_hovered_time;
s32 m_old_tooltip_id;
std::string m_old_tooltip;
std::wstring m_old_tooltip;

bool m_rmouse_auto_place;

Expand Down
7 changes: 4 additions & 3 deletions src/hud.cpp
View file
Open in desktop
Expand Up @@ -22,6 +22,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
#include "hud.h"
#include "settings.h"
#include "util/numeric.h"
#include "util/string.h"
#include "log.h"
#include "gamedef.h"
#include "itemdef.h"
Expand Down Expand Up @@ -313,7 +314,7 @@ void Hud::drawLuaElements(const v3s16 &camera_offset) {
(e->number >> 8) & 0xFF,
(e->number >> 0) & 0xFF);
core::rect<s32> size(0, 0, e->scale.X, text_height * e->scale.Y);
std::wstring text = utf8_to_wide(e->text);
std::wstring text = remove_escapes(utf8_to_wide(e->text));
core::dimension2d<u32> textsize = font->getDimension(text.c_str());
v2s32 offset((e->align.X - 1.0) * (textsize.Width / 2),
(e->align.Y - 1.0) * (textsize.Height / 2));
Expand Down Expand Up @@ -348,11 +349,11 @@ void Hud::drawLuaElements(const v3s16 &camera_offset) {
(e->number >> 8) & 0xFF,
(e->number >> 0) & 0xFF);
core::rect<s32> size(0, 0, 200, 2 * text_height);
std::wstring text = utf8_to_wide(e->name);
std::wstring text = remove_escapes(utf8_to_wide(e->name));
font->draw(text.c_str(), size + pos, color);
std::ostringstream os;
os << distance << e->text;
text = utf8_to_wide(os.str());
text = remove_escapes(utf8_to_wide(os.str()));
pos.Y += text_height;
font->draw(text.c_str(), size + pos, color);
break; }
Expand Down
9 changes: 9 additions & 0 deletions src/unittest/test_utilities.cpp
View file
Open in desktop
Expand Up @@ -45,6 +45,7 @@ class TestUtilities : public TestBase {
void testStringAllowed();
void testAsciiPrintableHelper();
void testUTF8();
void testRemoveEscapes();
void testWrapRows();
void testIsNumber();
void testIsPowerOfTwo();
Expand All @@ -71,6 +72,7 @@ void TestUtilities::runTests(IGameDef *gamedef)
TEST(testStringAllowed);
TEST(testAsciiPrintableHelper);
TEST(testUTF8);
TEST(testRemoveEscapes);
TEST(testWrapRows);
TEST(testIsNumber);
TEST(testIsPowerOfTwo);
Expand Down Expand Up @@ -253,6 +255,13 @@ void TestUtilities::testUTF8()
== "the shovel dug a crumbly node!");
}

void TestUtilities::testRemoveEscapes()
{
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");
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?


void TestUtilities::testWrapRows()
{
UASSERT(wrap_rows("12345678",4) == "1234\n5678");
Expand Down
4 changes: 2 additions & 2 deletions src/util/string.cpp
View file
Open in desktop
Expand Up @@ -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 remove_escapes(const std::wstring &s) {
std::wstring output;
size_t i = 0;
while (i < s.length()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

if (s[i] == L'\v') {
if (s[i] == L'\x1b') {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

++i;
if (i == s.length()) continue;
if (s[i] == L'(') {
Expand Down
6 changes: 3 additions & 3 deletions src/util/string.h
View file
Open in desktop
Expand Up @@ -387,12 +387,12 @@ inline void str_replace(std::string &str, const std::string &pattern,
}

/**
* Remove all chat escape sequences in \p s.
* Remove all escape sequences in \p s.
*
* @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 remove_escapes(const std::wstring &s);

/**
* Replace all occurrences of the character \p from in \p str with \p to.
Expand Down Expand Up @@ -476,7 +476,7 @@ inline std::string wrap_rows(const std::string &from,
* Removes backslashes from an escaped string (FormSpec strings)
*/
template <typename T>
inline std::basic_string<T> unescape_string(std::basic_string<T> &s)
inline std::basic_string<T> unescape_string(const std::basic_string<T> &s)
{
std::basic_string<T> res;

Expand Down