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

added (console-)command usage #389

Merged
merged 4 commits into from Jan 3, 2016

Conversation

Projects
None yet
5 participants
@H-M-H
Member

H-M-H commented Dec 23, 2015

would fix #387
However, I am not exactly satisfied with this as servers can not send the usage to clients properly. At the moment servers just concat params and usage and sent it to client, which causes that old clients can not display the whole text as their buffer is too small.

@def-

This comment has been minimized.

Member

def- commented Dec 23, 2015

Now you have duplicate information, ?s and [file]. Alternatively the parser could be changed to parse [file] but that might be too much work.

@heinrich5991

This comment has been minimized.

Contributor

heinrich5991 commented Dec 23, 2015

This kinda duplicates work of #388.

@H-M-H

This comment has been minimized.

Member

H-M-H commented Dec 23, 2015

Well it was already done when #388 appeared.

@def- like this we would loose typeinformation.

@Henningstone

This comment has been minimized.

Contributor

Henningstone commented Dec 23, 2015

Also, #388 obviously won't work.

@Laxa

This comment has been minimized.

Contributor

Laxa commented Dec 23, 2015

I guess the best option would be to rework all the "Help" argument inside the Console()->Register.
Sadly, we can't always have a solution that will fit all cases. Client needs to be updated once in a while.

@Laxa

This comment has been minimized.

Contributor

Laxa commented Dec 23, 2015

Another idea : modify the parser to have a comment char like ?s#[File]

commanddescription can now be placed directly in
the commandparamsstring: "s[file]"

@H-M-H H-M-H force-pushed the H-M-H:command_usage branch to 8e24ef8 Dec 28, 2015

@H-M-H

This comment has been minimized.

Member

H-M-H commented Dec 28, 2015

@def- @Laxa I liked you idea the new format is now: parametertype + description in brackets.
Example: "i[some number] s[a string] ?i[optional number]"

Edit: the only problem left is that old clients do not have enough space in their buffer to display the whole parameterstring in remote-console but I guess this problem is neglectable.

@H-M-H

This comment has been minimized.

Member

H-M-H commented Jan 3, 2016

Okay, no complaints so far, my locals tests were fine so I will merge this.

H-M-H added a commit that referenced this pull request Jan 3, 2016

Merge pull request #389 from H-M-H/command_usage
added (console-)command usage fixes #387

@H-M-H H-M-H merged commit e41b4f4 into ddnet:master Jan 3, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@heinrich5991

This comment has been minimized.

Contributor

heinrich5991 commented Jan 3, 2016

If you're asking for complaints, this is what I thought when reading the code:

  1. Why stuff the extra information into the format string for the parameters instead of making a second parameter that contains a space (or whatever)-separated list of parameter names?
  2. This is as you said not really backward-compatible, but it wouldn't be hard with 1).
@H-M-H

This comment has been minimized.

Member

H-M-H commented Jan 3, 2016

As you see I did it with a usage string first but as deen pointed out this is kinda redundant and adding this requires changes in networkcode. The problem is now that the server can send further information here:

CMsgPacker Msg(NETMSG_RCON_CMD_ADD);
Msg.AddString(pCommandInfo->m_pName, IConsole::TEMPCMD_NAME_LENGTH);
Msg.AddString(pCommandInfo->m_pHelp, IConsole::TEMPCMD_HELP_LENGTH);
Msg.AddString(pCommandInfo->m_pParams, IConsole::TEMPCMD_PARAMS_LENGTH);
SendMsgEx(&Msg, MSGFLAG_VITAL, ClientID, true);
but the new clients would not be able to process it as they do not know if the server already sends them:
else if((pPacket->m_Flags&NET_CHUNKFLAG_VITAL) != 0 && Msg == NETMSG_RCON_CMD_ADD)
{
const char *pName = Unpacker.GetString(CUnpacker::SANITIZE_CC);
const char *pHelp = Unpacker.GetString(CUnpacker::SANITIZE_CC);
const char *pParams = Unpacker.GetString(CUnpacker::SANITIZE_CC);
if(Unpacker.Error() == 0)
m_pConsole->RegisterTemp(pName, pParams, CFGFLAG_SERVER, pHelp);
}

edit: thanks for complaints btw :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment