Core: Text Processor Echo

[MatthewMarinets]

What is this fixing or adding?

This is a small change to core / MultiServer.py, to the text command processor. I'm noticing while working on the starcraft 2 client that it's very difficult to tell where output in response to one command ends and the next starts. I've started putting little markers to help me see things while testing, and decided before adding it to a second or third command I should probably try to turn this into a global feature instead.

I've made this feature opt-in by setting the echo_commands property to true, with the possibility of adding prefixes and suffixes to the output (defaults to "$ " prefix and no suffix).

Note: I go by "Phanerus" on the discord if you need to contact me there. I've mostly only worked on starcraft 2 development.

How was this tested?

I've added a unit test for this that monkey-patches CommandProcessor.output to collect output messages and analyze it's behaving correctly.

Locally, I tried setting StarcraftClientProcessor.echo_commands to true, and playing around with some output. Commands were echoed as expected.

If this makes graphical changes, please attach screenshots.

[benny-dreamly]

looks good to me

[agilbert1412]

You should use the syntax self.AssertNotIn() and such, instead of directly using the assert keyword

[MatthewMarinets]

Updated

[black-sliver]

This should be in a separate class/TestCase, not in TestResolvePlayerName, to avoid confusion.

[MatthewMarinets]

Didn't notice the name on that, good catch. Updated.

[black-sliver]

Can I ask what you plan on using this for? I feel like typical solutions would have the parent process handle echoing the stdin.

---

Oh nvmd that question, I guess, it's to allow the same code to be reused across all CommandProcessors. I think I'd personally still do that in CommonClient instead, unless there is a reason this should be in CommandProcessor instead?

[MatthewMarinets]

Can I ask what you plan on using this for? I feel like typical solutions would have the parent process handle echoing the stdin.

Oh nvmd that question, I guess, it's to allow the same code to be reused across all CommandProcessors. I think I'd personally still do that in CommonClient instead, unless there is a reason this should be in CommandProcessor instead?

I started my search for adding this in CommonClient; per my understanding, all the clients inherit from ClientCommandProcessor in that file, but there's no obvious place to add this code there. Instead, it inherits from CommandProcessor, which lives in MultiServer.py. That makes this the most obvious place to add this. I'm not too terribly familiar with metaclasses so there may be some way to do some modification further down the inheritance tree; I generally do my best to avoid inheritance as much as possible in my own code, so I don't really want to add anything to make the inheritance complexity problem worse in future.

As for a wrapping process handling this, it doesn't. This code gets used by our GUI client, which doesn't echo commands. Running the GUI process from the command-line also makes an interactive console there, which also doesn't echo. Unless I'm missing something, the Archipelago code is less a command processor and more a complete terminal, and it's not fulfilling all the responsibilities I'd expect of a terminal.

My original intention with this is as I say in the description -- I was finding myself adding something like this per-command in Starcraft 2 code, but once you add something like this 2~3 times you start asking if there's a good way to factor it out. Unless I want to basically branch all the client / command processor code from both CommonClient.py and MultiServer.py for starcraft use, I have to modify one of those files. I'm not really familiar with core dev, so I kept my changes as concise and unobtrusive as possible.

[Berserker66]

Haven't tested this yet, but does it and do we want it to echo _cmd_admin too?

[MatthewMarinets]

Haven't tested this yet, but does it and do we want it to echo _cmd_admin too?

I'm not familiar with that command, but I didn't do any special treatment of any commands. Reading the description, it looks like it has a password field so it probably shouldn't be echoed. It's been over a month so I don't remember all the details of my initial testing, but I think this echo also finds its way to logs, and that's not somewhere passwords should live.

How should I approach this? I see admin has a password field and it looks like some options can contain passwords. I could make a decorator similar to @raw_text, maybe something like @no_echo which will prevent a command from being logged. This blanket approach may be irritating for /option as only some things have passwords. I could do special treatment of the !admin command, and otherwise just search for the substring "password" and print some default prompt like "command containing a password".

[black-sliver]

I feel like typical solutions would have the parent process handle echoing the stdin.

This code gets used by our GUI client, which doesn't echo commands

Well, that's kind of the point. I feel like the client should echo them, not the CommandProcessor. If you use --nogui (or use the stdin in parallel), you already get the echo while typing in the console. If berserker is fine with this, I won't block it though.

[MatthewMarinets]

I feel like typical solutions would have the parent process handle echoing the stdin.

This code gets used by our GUI client, which doesn't echo commands

Well, that's kind of the point. I feel like the client should echo them, not the CommandProcessor. If you use --nogui (or use the stdin in parallel), you already get the echo while typing in the console. If berserker is fine with this, I won't block it though.

Ah I see, in that case I'd be fine with declining this PR and taking another stab at this on the console-side. I could look into getting that "hitting up brings up the last command" behaviour while I'm there. I assume that's accomplished by adding some handler on a widget within GameManager in kvui.py, to post some json message to the context?