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

Colored chat and other strings #4077

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
8 participants
@Ekdohibs
Copy link
Member

commented May 2, 2016

This pull request adds colored chat, item descriptions, tooltips, etc.
Colors are ignored on non-freetype builds.
It is still WIP - still needs to be done: squash the commits, anything else?
screenshot_20160502_231004

@lisacvuk

This comment has been minimized.

Copy link
Contributor

commented May 3, 2016

I've got a question, more questions actually:

  1. Is it possible to change to background color of item descriptions?
  2. Can I color the text to multiple colors, like "Red part/Blue part"?
@Ekdohibs

This comment has been minimized.

Copy link
Member Author

commented May 3, 2016

  1. Not yet, although planned
  2. You can do that: for example, the "1/13" in the screenshot is made of a single label.
@lisacvuk

This comment has been minimized.

Copy link
Contributor

commented May 3, 2016

Ok, thanks!

@lisacvuk

This comment has been minimized.

Copy link
Contributor

commented May 3, 2016

Hm, I'm getting a segfault on startup. I'm quite bad at the debugging. Here's output.

lisacvuk@mordor:~/mt_cc/minetest/bin$ ./minetest
2016-05-03 08:14:58: WARNING[Main]: system-wide share not found at "/usr/local/share/minetest"
2016-05-03 08:14:58: WARNING[Main]: system-wide share found at "/home/lisacvuk/mt_cc/minetest/bin/../share/minetest"
Segmentation fault (core dumped)
lisacvuk@mordor:~/mt_cc/minetest/bin$ gdb minetest
GNU gdb (Ubuntu 7.7.1-0ubuntu5~14.04.2) 7.7.1
Copyright (C) 2014 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from minetest...done.
(gdb) start
Temporary breakpoint 1 at 0x4b3a80
Starting program: /home/lisacvuk/mt_cc/minetest/bin/minetest 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Temporary breakpoint 1, 0x00000000004b3a80 in main ()
(gdb) run
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: /home/lisacvuk/mt_cc/minetest/bin/minetest 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
2016-05-03 08:15:26: WARNING[Main]: system-wide share not found at "/usr/local/share/minetest"
2016-05-03 08:15:26: WARNING[Main]: system-wide share found at "/home/lisacvuk/mt_cc/minetest/bin/../share/minetest"
[New Thread 0x7fffea54d700 (LWP 4795)]
[Thread 0x7fffea54d700 (LWP 4795) exited]
[New Thread 0x7fffea54d700 (LWP 4796)]
[New Thread 0x7fffe5d4b700 (LWP 4797)]
[New Thread 0x7fffe554a700 (LWP 4798)]
[New Thread 0x7fffe4d49700 (LWP 4799)]
[New Thread 0x7fffd7fff700 (LWP 4800)]
[New Thread 0x7fffd77fe700 (LWP 4801)]

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff5790130 in __dynamic_cast () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
(gdb) quit
A debugging session is active.

        Inferior 1 [process 4793] will be killed.

Quit anyway? (y or n) y
lisacvuk@mordor:~/mt_cc/minetest/bin$ 
@Ekdohibs

This comment has been minimized.

Copy link
Member Author

commented May 3, 2016

@lisacvuk hm, strange, I don't get that... could you provide a backtrace please? (with "bt" or "bt full" in gdb).

@Ekdohibs Ekdohibs force-pushed the Ekdohibs:colored_chat branch 4 times, most recently May 3, 2016

@Ekdohibs Ekdohibs removed the WIP label May 4, 2016

@Ekdohibs Ekdohibs force-pushed the Ekdohibs:colored_chat branch May 4, 2016

@Zeno-

View changes

src/cguittfont/CGUITTFont.cpp Outdated
@@ -645,7 +658,7 @@ void CGUITTFont::draw(const core::stringw& text, const core::rect<s32>& position

CGUITTGlyphPage* page = n->getValue();

if (!use_transparency) color.color |= 0xff000000;
//if (!use_transparency) color.color |= 0xff000000;

This comment has been minimized.

Copy link
@Zeno-

Zeno- May 4, 2016

Contributor

I'd just remove this instead of commenting it out, since the logic is now in the loop below


virtual void draw(const EnrichedString& text, const core::rect<s32>& position,
video::SColor color, bool hcenter=false, bool vcenter=false,
const core::rect<s32>* clip=0);

This comment has been minimized.

Copy link
@Zeno-

Zeno- May 4, 2016

Contributor

I'd prefer clip = NULL

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs May 4, 2016

Author Member

I could change that, but I wanted to stay consistent with the code that already existed (and the one in Irrlicht). Thus, should I change it?

This comment has been minimized.

Copy link
@Zeno-

Zeno- May 4, 2016

Contributor

I guess not in that case

@Zeno-

View changes

src/client/guiChatConsole.cpp Outdated


#if USE_FREETYPE
// Draw colored text if FreeType is enabled

This comment has been minimized.

Copy link
@Zeno-

Zeno- May 4, 2016

Contributor

Coloured text relies on freetype?

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs May 4, 2016

Author Member

Yes, since CGUITTFont does rely on it.

@Ekdohibs Ekdohibs force-pushed the Ekdohibs:colored_chat branch May 4, 2016

@est31

This comment has been minimized.

Copy link
Contributor

commented May 5, 2016

I think @ShadowNinja would prefer if the files were named static_text.h and enriched_string.h.

@paramat paramat added the WIP label May 5, 2016

@Ekdohibs Ekdohibs force-pushed the Ekdohibs:colored_chat branch May 7, 2016

@est31

View changes

src/util/static_text.cpp Outdated
// draw the border

if (Border)
{

This comment has been minimized.

Copy link
@est31

est31 May 7, 2016

Contributor

The whole file violates style

@est31

View changes

builtin/game/misc.lua Outdated

local ESCAPE_CHAR = string.char(0x1b)
function core.get_color_escape_sequence(color)
return ESCAPE_CHAR .. "(color@" .. color .. ")"

This comment has been minimized.

Copy link
@est31

est31 May 7, 2016

Contributor

Wouldn't it be better to have a shorter color descriptor? Like "c@" instead of "color@" ?

@est31

View changes

builtin/game/misc.lua Outdated
end

function core.get_background_escape_sequence(color)
return ESCAPE_CHAR .. "(background@" .. color .. ")"

This comment has been minimized.

Copy link
@est31

est31 May 7, 2016

Contributor

same here

@lisacvuk

This comment has been minimized.

Copy link
Contributor

commented May 8, 2016

Can we have this in 0.4.14?

@Ekdohibs

This comment has been minimized.

Copy link
Member Author

commented May 8, 2016

@lisacvuk No, we are in freeze currently.

@est31

This comment has been minimized.

Copy link
Contributor

commented May 17, 2016

Okay about backwards compat, what about the following: you add a server side setting that sets the core.colorize to do nothing if enabled, and do what it does right now if disabled. Then server owners who want to keep backwards compat simply enable that setting.

@est31

View changes

minetest.conf.example Outdated
@@ -732,6 +732,11 @@
# type: string
# serverlist_url = servers.minetest.net

# Disable escape sequences, e.g. chat coloring.

This comment has been minimized.

Copy link
@est31

est31 May 21, 2016

Contributor

You should edit builtin/settingtypes.txt instead.

@kwolekr

View changes

src/util/enriched_string.cpp Outdated
SColor color(initial_color);
size_t i = 0;
while (i < s.length()) {
if (s[i] == L'\x1b') {

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

Since this if statement body is the majority of the function, wouldn't it be better to invert the condition so that the non-color-code case continues early? You could save a level of indentation that way.

@kwolekr

View changes

src/util/enriched_string.cpp Outdated
m_colors.push_back(source.m_colors[i]);
}

void EnrichedString::addCharNoColor(wchar_t c) {

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

This { needs to be on the next line.

@kwolekr

View changes

src/util/enriched_string.h Outdated
EnrichedString(const std::wstring &string, const std::vector<SColor> &colors);
void operator=(const wchar_t *str);
void addAtEnd(const std::wstring &s, const SColor color);
void addChar(const EnrichedString &source, size_t i);

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

What does i do?

@kwolekr

View changes

src/util/enriched_string.h Outdated
class EnrichedString {
public:
EnrichedString();
EnrichedString(const std::wstring &s, const SColor color = SColor(255, 255, 255, 255));

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

Why did you make all these SColors const? Did you intend to write this as const SColor &?

@kwolekr

View changes

src/util/enriched_string.h Outdated
#include <string>
#include <vector>
#include <SColor.h>
using namespace irr::video;

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

This is polluting the namespace of anything that includes enriched_text.h.

return core.get_color_escape_sequence(color) .. message .. core.get_color_escape_sequence("#ffffff")
end

end

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

This needs to have a newline here.

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs May 31, 2016

Author Member

Fixed.

@@ -1240,7 +1241,11 @@ static void updateChat(Client &client, f32 dtime, bool show_debug,

// Get new messages from error log buffer
while (!chat_log_error_buf.empty()) {
chat_backend.addMessage(L"", utf8_to_wide(chat_log_error_buf.get()));
std::wstring error_message = utf8_to_wide(chat_log_error_buf.get());
if (!g_settings->getBool("disable_escape_sequences")) {

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

I don't get it, are you writing your single-line-body if statements with brackets or without?

Please, pick one style and stick with it.

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs May 31, 2016

Author Member

They should all be with braces now.

@kwolekr

View changes

src/guiEngine.cpp Outdated
rect,false,true,0,-1);
addStaticText(m_device->getGUIEnvironment(), m_toplefttext,
rect, false, true, 0, -1);
//m_irr_toplefttext =

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

Shouldn't this be removed?

@kwolekr

View changes

src/guiFormSpecMenu.cpp Outdated
@@ -2255,15 +2256,15 @@ void GUIFormSpecMenu::drawList(const ListDrawSpec &s, int phase,
std::wstring tooltip_text = L"";
if (hovering && !m_selected_item) {
tooltip_text = utf8_to_wide(item.getDefinition(m_gamedef->idef()).description);
tooltip_text = unescape_enriched(tooltip_text);
//tooltip_text = unescape_enriched(tooltip_text);

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

Shouldn't this be removed?

@kwolekr

View changes

src/util/string.h Outdated
@@ -519,6 +519,39 @@ std::basic_string<T> unescape_enriched(const std::basic_string<T> &s)
return output;
}

template <typename T>
std::vector<std::basic_string<T> >
split(const std::basic_string<T> &s, T delim)

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

split( should be on the previous line.

@kwolekr

View changes

src/util/string.h Outdated

std::basic_string<T> current;
bool last_was_escape = false;
for (unsigned int i = 0; i < s.length(); i++) {

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

replace unsigned int with size_t

@kwolekr

View changes

src/util/string.h Outdated
template <typename T>
std::vector<std::basic_string<T> >
split(const std::basic_string<T> &s, T delim)
{

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

Doesn't this function shadow the split() in guiFormSpecMenu.cpp? Now there are three string split functions.

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs May 31, 2016

Author Member

Oops, I had forgotten to delete that one. It is exactly the same function, except that it accepts any kind of string.

}
}
//push last element
tokens.push_back(current);

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

I realize right now is not the time or place to do such an optimization, but this function is very copy-happy. Maybe constructing these strings in-place could cut down on some allocations and copying.

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs May 31, 2016

Author Member

Well, this function is the exact copy of the one that was in guiFormspecMenu.cpp. Maybe it can be optimized later.

@@ -519,6 +519,39 @@ std::basic_string<T> unescape_enriched(const std::basic_string<T> &s)
return output;
}

template <typename T>

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

Can you add a unit test for this function?

@kwolekr

View changes

src/util/enriched_string.h Outdated
}
inline bool operator!=(const EnrichedString &other) const
{
return (m_string != other.m_string || m_colors != other.m_colors);

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

Can't you just write this as !(this == other)?

@kwolekr

View changes

src/util/enriched_string.h Outdated
const wchar_t *c_str() const;
const std::vector<SColor> &getColors() const;
const std::wstring &getString() const;
inline bool operator==(const EnrichedString &other) const

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

Is there a reason for these operator overloads?

@kwolekr

View changes

src/util/enriched_string.h Outdated
#include <SColor.h>
using namespace irr::video;

class EnrichedString {

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

Unit tests?

@kwolekr

View changes

src/util/enriched_string.cpp Outdated
#include "enriched_string.h"
#include "util/string.h"
#include "log.h"
EnrichedString::EnrichedString()

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

Where's the newline?

@kwolekr

View changes

src/util/enriched_string.cpp Outdated
clear();
}

EnrichedString::EnrichedString(const std::wstring &string, const std::vector<SColor> &colors):

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

This is way over the column character limit.

@kwolekr

View changes

src/irrlicht_modifications/static_text.cpp Outdated
@@ -0,0 +1,678 @@
// Copyright (C) 2002-2012 Nikolaus Gebhardt
// Copyright (C) 2016 Nathanaël Courant

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 31, 2016

Contributor

Is there any way you could write a comment detailing what modifications you actually made to this file?

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented May 31, 2016

Why did you move guiChatConsole.cpp/h to client/ in this PR?

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented May 31, 2016

isn't irrlicht_modifications a bit too long and cumbersome? What's wrong with irrlicht_mods or similar?

Add colored text (not only colored chat).
Add documentation, move files to a proper place and avoid memory leaks.
Make it work with most kind of texts, and allow backgrounds too.

@Ekdohibs Ekdohibs force-pushed the Ekdohibs:colored_chat branch to 3c74143 May 31, 2016

@Ekdohibs

This comment has been minimized.

Copy link
Member Author

commented May 31, 2016

1d40385
14ef2b4

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.