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 --terminal-silent flag to minetest #14734

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jordan4ibanez
Copy link
Contributor

@jordan4ibanez jordan4ibanez commented Jun 7, 2024

Add compact, short information about your PR for easier understanding:

It allows you to hook in GUIs and also as a side effect you can type straight into the regular terminal.

  • Goal of the PR
    Some way to directly talk to minetest through the command line.
  • How does the PR work?
    It uses cin nonblocking with std::thread::sleep_for() (thanks Lars && ExeVirus) on another thread via the ILogOutput and Thread parent classes to get this pipe dream functioning.
  • Does it resolve any reported issue?
    I have no idea.
  • Does this relate to a goal in the roadmap?
    Still no idea.
  • If not a bug fix, why is this PR needed? What usecases does it solve?
    Because you should be able to talk to your local server through the terminal when it's being handled by other apps.

To do

  • Think of a better pr name.

This PR is a Ready for Review.

  • Cry because ncurses is so painful to work with.
  • Give up and try 50 other approaches.
  • Find a solution.

How to test

First, make sure your minetest.conf has

name = blah

Next, build with this pr, launch minetestserver or minetest --server with the flag --terminal-silent and then type straight into the terminal.

@jordan4ibanez jordan4ibanez force-pushed the terminal-silent branch 2 times, most recently from d369461 to 46dc675 Compare June 7, 2024 13:24
@rubenwardy rubenwardy added @ Startup / Config / Util Feature ✨ PRs that add or enhance a feature Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand labels Jun 7, 2024
@SwissalpS
Copy link

then type straight into the terminal.

any examples of what to type?

@jordan4ibanez
Copy link
Contributor Author

then type straight into the terminal.

any examples of what to type?

hi
/shutdown
an example of what to type

@jordan4ibanez
Copy link
Contributor Author

In case there's any other confusion as to what I mean by, you can directly type into the terminal (stdin), here is an image.
image

Do a minor module update

Document this

I gave it sleeping medicine

slow it down, use portable, be sassy

Add terminal-silent ability to minetest

Start the guttening

make it clean

wot

I can't believe this actually works

Time for more fun

checkpoint

Almost there

Getting better

horrors beyond human comprehension

WE HAVE LIFTOFF BOIS WOOOOO

I have absolutely no idea what I'm doing

gut it

top tier debug info

Found my typo

there

ur hell

literally duct tape this on

fix tabs

Now check for silent

Now only silent import

Silent

Silent thing

Integrate terminal-silent

I worded this badly

Remove this comment
@jordan4ibanez
Copy link
Contributor Author

Here is a video of a use case for it: https://youtu.be/X2O0gKai1CI

Here is the repo for the gui used in the video: https://github.com/jordan4ibanez/minetest_gui

@@ -374,6 +375,8 @@ static void set_allowed_options(OptionList *allowed_options)
_("Migrate from current mod storage backend to another (Only works when using minetestserver or with --server)"))));
allowed_options->insert(std::make_pair("terminal", ValueSpec(VALUETYPE_FLAG,
_("Feature an interactive terminal (Only works when using minetestserver or with --server)"))));
allowed_options->insert(std::make_pair("terminal-silent", ValueSpec(VALUETYPE_FLAG,
Copy link
Member

Choose a reason for hiding this comment

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

maybe something like stdin-control would be a better name

@@ -1089,7 +1092,7 @@ static bool run_dedicated_server(const GameParams &game_params, const Settings &
return false;
}

if (cmd_args.exists("terminal")) {
if (cmd_args.exists("terminal") || cmd_args.exists("terminal-silent")) {
#if USE_CURSES
Copy link
Member

Choose a reason for hiding this comment

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

there's no reason for the silent option to depend on curses.
would be counterproductive because you want to use it especially when you don't have curses

g_term_console_silent.stopAndWaitforThread();
} else {
g_term_console.stopAndWaitforThread();
}
Copy link
Member

Choose a reason for hiding this comment

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

this if-else dance could be avoided by defining an interface these two classes inherit

}

MutexedQueue<std::pair<LogLevel, std::string> > queue;
};
Copy link
Member

Choose a reason for hiding this comment

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

doesn't minetest log to stdout anyway? or is that inhibited with curses enabled?
I don't see the point of this.

// If you are running 100 commands per second, you might want to
// redesign the program interfacing with the server.
// This keeps the thread from maxing out the cpu core.
std::this_thread::sleep_for(std::chrono::milliseconds(10));
Copy link
Member

Choose a reason for hiding this comment

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

no need for nonblock (which afaik could also cause problems with logging) and the sleeping.
you can simply poll with a timeout of e.g. 1s (so it still exits correctly)


// Print if its a command (gets eaten by server otherwise)
if (msg[0] == L'/') {
m_chat_backend.addMessage(L"", (std::wstring)L"Issued command: " + msg);
Copy link
Member

Choose a reason for hiding this comment

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

m_chat_backend is totally unused except for this

@sfan5 sfan5 added Supported by core dev Not on the roadmap, yet some core dev decided to take care of this PR Action / change needed Code still needs changes (PR) / more information requested (Issues) and removed Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand labels Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Action / change needed Code still needs changes (PR) / more information requested (Issues) Feature ✨ PRs that add or enhance a feature @ Startup / Config / Util Supported by core dev Not on the roadmap, yet some core dev decided to take care of this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants