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

Fix binds #11

Open
wants to merge 1 commit into
base: master
from
Open

Fix binds #11

wants to merge 1 commit into from

Conversation

@Gireen
Copy link
Contributor

Gireen commented Feb 2, 2020

@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Feb 7, 2020

I wonder if the bug, at first, is the need for the / prefix.

The / is only required to type commands in the in-game client console (not required in the client/server tty console, not required in the .cfg files, etc.), it's not required with delay, etc.

This is only required in the in-game client console because the console is abused as a chat interface (to not have to type the say command).

Also, the support for this is very inconsistent: if you type a command with / in the tty console, it works, if you use history to redo the same command from the in-game console, it does not work because the / is missing…

@slipher

This comment has been minimized.

Copy link
Contributor

slipher commented Feb 7, 2020

The modcase command has this bizarre behavior:

			if ( *v == '/' || *v == '\\' )
			{
				Cmd::BufferCommandTextAfter(va("%s\n", v + 1), true);
			}
			else
			{
				Cmd::BufferCommandTextAfter(va("vstr %s\n", v), true);
			}

I can't imagine why you would want the default behavior to be vstr rather than treating it as a command.

@slipher
slipher approved these changes Feb 7, 2020
@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Feb 7, 2020

What do you call, “a command”? a command does not contain the / which is just a hack to be able to chat in console. Only the in-game console requires the /.

You can make this simple test, run unvanquished, load a map, then, in your terminal console (not the in-game one), type say hello, the game will print hello, then in game, open console, select previous command from history using the “up” key, and press enter, the game will print say hello.

You can also do try that:

delay 1000 say hello

The next second, it will print “hello”.

it's also possible to do:

/delay 1000 /say hello

But that's just to be compatible with mistakes people may do by being misleaded by the console chat hack.

@slipher

This comment has been minimized.

Copy link
Contributor

slipher commented Feb 7, 2020

This question has no relation to the questionable design of modcase, but if you don't like the current behavior of default commands, you can try setting cl_consoleCommand and com_consoleCommand.

@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Feb 7, 2020

			if ( *v == '/' || *v == '\\' )
			{
				Cmd::BufferCommandTextAfter(va("%s\n", v + 1), true);
			}
			else
			{
				Cmd::BufferCommandTextAfter(va("vstr %s\n", v), true);
			}

I can't imagine why you would want the default behavior to be vstr rather than treating it as a command.

That's not what I'm saying, there is no reason to do any vstr there. I'm asking for this instead:

			Cmd::BufferCommandTextAfter(va("%s\n", v), true);

Or this:

			if ( *v == '/' || *v == '\\' )
			{
				Cmd::BufferCommandTextAfter(va("%s\n", v + 1), true);
			}
			else
			{
				Cmd::BufferCommandTextAfter(va("%s\n", v), true);
			}

People who want to do a vstr would explicitely write vstr in their command, like this:

teambind humans     hw:e   "modcase alt \"vstr something\" vstr anotherthing"

So, we must have to make sure this works:

teambind humans     hw:e   "modcase alt \"deconstruct marked\" deconstruct"

I'm OK if this is also supported, for the sole purpose of being permissive with mistakes:

teambind humans     hw:e   "modcase alt \"/deconstruct marked\" /deconstruct"
@slipher

This comment has been minimized.

Copy link
Contributor

slipher commented Feb 7, 2020

Yes I completely agree that the vstr behavior is stupid.

@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Feb 7, 2020

So, I'm OK with this PR with the useless / as it allows to ship a fixed default keybind without requiring to ship a new engine.

But we must make sure the engine does not require that /. And we may revert the useless / later once a new engine not requiring it is shipped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.