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

Add server side ncurses terminal #3292

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
@est31
Copy link
Contributor

commented Oct 24, 2015

Some things still TODO (see commit msg), but most of them are done.
The server side terminal features:

  • Admin can chat with players, execute commands, and see their output
  • Nick completion on tab press
  • Already existent abstractions for making a remote terminal, not bound to the server
  • Only partial buffer updates to spare draw time and resources
  • Ability to set log level from inside console
  • Runs in its own thread, so it isn't limited by the server's lag; it "feels" like being on a client
  • Restores original loglevel setup of stdout/stderr log output streams when it exits
  • Exits before fatal errors are printed. This shows the error directly in terminal. debug.txt remains completely unimpaired, and logs everything.

As it uses the same console like the well known f10 console, it can scroll back within its own buffer, and has a command history available with the arrow keys.

still WIP in many parts, but got tired of its life as separate branch.

@Ryan-Nolan

This comment has been minimized.

Copy link

commented Oct 24, 2015

I cant wait for this to be finished, :D

@rubenwardy

View changes

builtin/init.lua Outdated
@@ -7,6 +7,7 @@

-- Initialize some very basic things
function core.debug(...) core.log(table.concat({...}, "\t")) end
print = core.debug -- Override native print and use our logging system

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Oct 24, 2015

Member

Isn't this already done?
print is logged to debug.txt afaik.

This comment has been minimized.

Copy link
@est31

est31 Oct 25, 2015

Author Contributor

Nope, print isn't logged to debug.txt right now. Perhaps it was before the logging PR, but not now. Right now it seems that print directly outputs to stdout, which leads the console to "freak out"; e.g. you won't see your own input anymore. Therefore this is needed.

@LeMagnesium

View changes

src/terminal_chat_console.h Outdated
const std::string &payload_text)
{
std::ostringstream os(std::ios_base::binary);
os << time << ": [" << thread_name << "]" << payload_text;

This comment has been minimized.

Copy link
@LeMagnesium

LeMagnesium Oct 25, 2015

Contributor

I think there should be a space character after the closing bracket otherwise you end up with the message appended to [ << thread_name << ] like this : https://lut.im/HeBDQBMbhW/OVPvnrqbwLYQz3xm.png

This comment has been minimized.

Copy link
@est31

est31 Oct 25, 2015

Author Contributor

good catch, thanks.

@est31 est31 force-pushed the est31:ncurses-console branch Oct 25, 2015

@cheapie

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2015

Being able to see the output of commands would be helpful.

@est31

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2015

You do see the output of both methods a mod can use to send the caller its output:

core.register_chatcommand("test_return", {
    params = "", description = "",
    func = function(name, param)
        minetest.chat_send_player(name, "hi from chat")
        return true, "Hi from return value"
    end,
})

Will print when invoked:

Issued command: /test_return
hi from chat
Hi from return value

@est31 est31 force-pushed the est31:ncurses-console branch 2 times, most recently Oct 26, 2015

@cheapie

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2015

/status returned nothing when I tried it.

/time 12000 did not return a "Time set to 12000" message, only a log entry.

@est31 est31 force-pushed the est31:ncurses-console branch 3 times, most recently Oct 26, 2015

@est31 est31 removed the WIP label Oct 26, 2015

@est31 est31 force-pushed the est31:ncurses-console branch 4 times, most recently Oct 26, 2015

@BlockMen

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2015

@est31 yes, builds under windows. But it seems server sends own chat msg back, so all own chat msg are now printed twice ingame. (no matter if using terminal or chat window)

@est31 est31 force-pushed the est31:ncurses-console branch 2 times, most recently Oct 26, 2015

@est31

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2015

fixed

@BlockMen

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2015

👍

@ShadowNinja

View changes

src/terminal_chat_console.cpp Outdated
break;
case 'L':
m_log_level--;
m_log_level = MYMAX(m_log_level, 1); // LL_NONE isn't accessible

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Oct 27, 2015

Member

This should use (int)LL_ERROR or similar instead of hard coding it's current integer equivalent.

@ShadowNinja

View changes

src/terminal_chat_console.cpp Outdated
printw(" ");
printw(g_version_hash);
printw(" ");
printw(itos(porting::getTimeS()).c_str());

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Oct 27, 2015

Member

Why the unix timestamp? This is unreadable. Use getTimestamp() or just don't show a timestamp.

@est31 est31 force-pushed the est31:ncurses-console branch 2 times, most recently Oct 28, 2015

@kwolekr

View changes

src/server.cpp Outdated
Listen to the admin chat, if available
*/
if (m_admin_chat) {
MutexAutoLock lock(m_env_mutex);

This comment has been minimized.

Copy link
@kwolekr

kwolekr Oct 29, 2015

Contributor

What?? Why is the envlock here of all places?

This comment has been minimized.

Copy link
@est31

est31 Oct 29, 2015

Author Contributor

Well, we don't know here whether envlock needs to be set, so we assume we need it for handleAdminChat. Now I could lock the environment inside handleAdminChat as instead, but then I would have to modify how the environment is locked for the regular chat, where the environment is locked in Server::ProcessData. Removing it from there isn't preferrable. Now I could move the locking of the mutex into the while loop, or even into the if case. It wouldn't make much sense however, as 99% of the events received from the admin chat interface are chat messages (the other type can be nick informs, but they only happen at startup). And locking unlocking then locking again isn't surely something good performace wise. One optimisation I could do is to move the lock into an if case that checks whether there are elements in the queue at all.

This comment has been minimized.

Copy link
@kwolekr

kwolekr Nov 1, 2015

Contributor

Ah okay nevermind, I see what's going on.

@nerzhul

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

Instead of locking environment, use a queue like i do in my own branch, transmit the message to it and read it from server thread . Envlock could be removed and you only need a mutex on the queue.

@est31

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2015

That's precisely what's happening @nerzhul . The console runs in the console thread, it sends the chat over to the server thread via a queue. The server thread then locks the environment to process the chat.

@nerzhul

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

@est31 okay, then env_lock should be limited to a more little area

Listen to the admin chat, if available
*/
if (m_admin_chat) {
if (!m_admin_chat->command_queue.empty()) {

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Oct 30, 2015

Member

These ifs can be combined.

This comment has been minimized.

Copy link
@est31

est31 Oct 31, 2015

Author Contributor

Not anymore.

@ShadowNinja

View changes

src/server.cpp Outdated
}


if (line != L"") {

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Oct 30, 2015

Member

Isn't this guaranteed to be true? (also, !line.empty() would read better)

This comment has been minimized.

Copy link
@est31

est31 Oct 30, 2015

Author Contributor

You are right, I'll remove it.

@kahrl

View changes

src/server.cpp Outdated
}
}
}
m_admin_chat->outgoing_queue.push_back(

This comment has been minimized.

Copy link
@kahrl

kahrl Oct 30, 2015

Contributor

I am getting a segfault here (with m_admin_chat == NULL). I think this should be inside the if block.

@est31 est31 force-pushed the est31:ncurses-console branch to 19ecec7 Oct 31, 2015

// esc prompt
move(m_rows - 1, 0);
clrtoeol();
printw("[ESC] Toogle ESC mode |"

This comment has been minimized.

Copy link
@kahrl

kahrl Nov 2, 2015

Contributor

typo

This comment has been minimized.

Copy link
@est31

est31 Nov 2, 2015

Author Contributor

Wrong char repeated lol

@kahrl

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2015

  • Backspace doesn't work for me. Fixed by adding "case 127" after "case KEY_BACKSPACE".
  • As discussed in IRC, apparently on some systems the tinfo library needs to be linked as well, else there will be undefined references during linking. Also it would be nice if non-ncurses curses libraries were supported.

I wrote a patch that makes cmake first check for ncursesw (and tinfo); if that fails, it uses FindCurses.cmake (shipped with CMake, also checks for tinfo) to find any other curses implementation.

@est31 est31 force-pushed the est31:ncurses-console branch 2 times, most recently Nov 3, 2015

@est31

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2015

Okay, rebased and squashed. @kahrl has +1ed this PR from IRC, but he wants @BlockMen to try a windows build first.

@est31 est31 force-pushed the est31:ncurses-console branch Nov 3, 2015

@est31

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2015

There are some bugs right now which would be pretty heavy regressions if this got in master, so I guess I'll have to fix them before it gets merged. Note for myself, fix these:

  • If you start a standalone server, it doesn't find the world from the passed name. Broken if --terminal is used or not.
  • If you create a server from the main menu's server tab, and the account is new, you can't log in.

@est31 est31 force-pushed the est31:ncurses-console branch 4 times, most recently Nov 3, 2015

@kahrl

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2015

@est31 Yeah, I didn't review the auth part much. Did someone else review that?

(That said, I can't reproduce the first of these two bugs.)

@est31

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2015

@kahrl in place build? I had a non-in-place build.

@kahrl

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2015

@est31, yeah, in place.

Add server side ncurses terminal
This adds a chat console the server owner can use for administration
or to talk with players.
It runs in its own thread, which makes the user interface immune to
the server's lag, behaving just like a client, except timeout.
As it uses the same console code as the f10 console, things like nick
completion or a scroll buffer basically come for free.
The terminal itself is written in a general way so that adding a
client version later on is just about implementing an interface.

Fatal errors are printed after the console exists and the ncurses
terminal buffer gets cleaned up with endwin(), so that the error still
remains visible.

The server owner can chose their username their entered text will
have in chat and where players can send PMs to.
This username then gets all privs and clients get prevented from
creating accounts under that name. If an account already exists,
it still can be used for a log in, so that the server owner can appear
with a consistent name to their players.

This change includes a contribution by @kahrl who has improved ncurses
library detection.

@est31 est31 force-pushed the est31:ncurses-console branch to d8a7061 Nov 6, 2015

@est31

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2015

Okay, it seems that the first bug was wrong usage by me, and not a bug at all. It's the same way in master as well.
The second bug has been solved by not giving the admin any privs until they have protected their account with a password. This especially means no messing with the builtin auth handler anymore. The other alternative would have been to tell auth handler code about whether we have an ncurses console running, or the server is started from the server tab.

@est31

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2015

@est31 est31 closed this Nov 6, 2015

@neoascetic

This comment has been minimized.

Copy link
Contributor

commented Nov 25, 2015

I am sorry, but what reasons to include this in the core? Could this be a side project, couldn't it? We may have simple lua mod that reads commands from some file or socket and executes them; on the other hand, regular server's stdout may be used to see results of the executed actions. I. e. there is a simpler implementation with the same functionality.

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Nov 25, 2015

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.