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

Update to sm_ban, sm_kick, & sm_map where args == 0 to display menu #838

Merged
merged 8 commits into from Sep 3, 2018
Merged

Update to sm_ban, sm_kick, & sm_map where args == 0 to display menu #838

merged 8 commits into from Sep 3, 2018

Conversation

@JoinedSenses
Copy link
Contributor

@JoinedSenses JoinedSenses commented Jun 27, 2018

This change makes it so /kick, /ban, and /map open the already created methods for displaying their menus when there are no args.

The reason for the feature is to take advantage of menus that already exist and to make the commands easier to use.

The client == 0 check prevents them from opening if it was ran via rcon, sm_rcon, or server command. Client auth is also checked because its a registered admin command.

Usage params will display if client == 0 and args < min

For example, a moderator wants to change a map, instead of running through the admin menu, they can instead type just /map to display available maps and choose one.

If a mod wants to quickly ban or kick someone without having to either run through the admin menu or type it out, they could then type the corresponding commands with no args to open the menus.

if (client != 0 && args == 0)
{
DisplayBanTargetMenu(client);
}
ReplyToCommand(client, "[SM] Usage: sm_ban <#userid|name> <minutes|0> [reason]");
Copy link
Member

@Headline Headline Jun 28, 2018

Choose a reason for hiding this comment

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

Is it intended to error w/ usage when an argument-less sm_ban would now be considered valid?

ditto for the other commands, as well

Copy link
Contributor Author

@JoinedSenses JoinedSenses Jun 28, 2018

Choose a reason for hiding this comment

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

I left in the replytocommand as a reminder of the usage just in case it's needed and also as a fallback where client ==0.

Copy link
Member

@Headline Headline Jun 28, 2018

Choose a reason for hiding this comment

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

I don't think I'm alone in thinking that we probably shouldn't print usage errors if it's gonna be valid. Other than that this lgtm

Copy link
Contributor Author

@JoinedSenses JoinedSenses Jun 28, 2018

Choose a reason for hiding this comment

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

Maybe/maybe not. Either way, there are going to be people who either like it or not. I also suggested this idea also to sbpp, but it was an "annoying change", though I think the opposite. This feature is implemented on my servers and the moderators/admins like it, especially the menu display for /map, considering it's already a feature for /nominate.

Copy link
Contributor Author

@JoinedSenses JoinedSenses Jun 28, 2018

Choose a reason for hiding this comment

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

If sm_map and sm_kick were to at least be consistent with sm_nominate, then I suppose the usage parameter return could be removed *unless client = 0

*edit

Copy link
Member

@Headline Headline Sep 3, 2018

Choose a reason for hiding this comment

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

I think that's best. It'd be annoying if users typed !ban Headline 0 "lemon" and got hit with usage errors every time.

@KyleSanderson
Copy link
Member

@KyleSanderson KyleSanderson commented Jun 28, 2018

At the very least if we're even going to consider merging something similar to this (it should be a convar) the cmdsrc needs to be evaluated.

@Kenzzer
Copy link
Contributor

@Kenzzer Kenzzer commented Jun 28, 2018

And what if I'm a newbie to SM and wanna know the params of sm_ban how do I check that since you will literally slam a menu I don't want in my face. I suggest making plural version of these commands (sm_bans, sm_maps, sm_kicks ect...) doing what you suggested.

@JoinedSenses
Copy link
Contributor Author

@JoinedSenses JoinedSenses commented Jun 28, 2018

Haha "slam a menu"? That's a bit dramatic. The method I used to add the menus will still post the usage parameters to chat/console, and will also display the menu. I'm not sure what cmdsrc is, and by cvar, do you mean adding an option to enable/disable the menus when args == 0?

@JoinedSenses
Copy link
Contributor Author

@JoinedSenses JoinedSenses commented Jun 28, 2018

Also, see above conversation with Headline regarding consistency with sm_nominate

@JoinedSenses
Copy link
Contributor Author

@JoinedSenses JoinedSenses commented Jun 30, 2018

On top of everything else, I have now also renamed the translation file for basecommands to be consistent with every other translation file (plugin.basecommands -> basecommands.phrases)

@JoinedSenses JoinedSenses changed the title Update to sm_ban, sm_kick, & sm_map where args == 0 to display menu Update to /ban, /kick, /map where args == 0 to display menu (and translation file) Jun 30, 2018
@JoinedSenses JoinedSenses changed the title Update to /ban, /kick, /map where args == 0 to display menu (and translation file) Update to sm_ban, sm_kick, & sm_map where args == 0 to display menu Jun 30, 2018
@asherkin
Copy link
Member

@asherkin asherkin commented Aug 11, 2018

I'd be moderately OK (but still not a fan) of doing this for chat commands, but console usage of these should definitely not be opening a menu.

Is there a driving force behind this? PRs with no description are rather annoying (at the very least, every feature addition needs a use case.)

@JoinedSenses
Copy link
Contributor Author

@JoinedSenses JoinedSenses commented Aug 11, 2018

This change makes it so /kick, /ban, and /map open the already created methods for displaying their menus when there are no args.

The reason for the feature is to take advantage of menus that already exist and to make the commands easier to use.

The client == 0 check prevents them from opening if it was ran via rcon, sm_rcon, or server command. Client auth is also checked because its a registered admin command.

Usage params will still display for each method when given incorrect args.

For example, a moderator wants to change a map, instead of running through the admin menu, they can instead type just /map to display available maps and choose one.

If a mod wants to quickly ban or kick someone without having to either run through the admin menu or type it out, they could then type the corresponding commands with no args to open the menus.

Copy link
Member

@Headline Headline left a comment

Just the comment from before, otherwise I'm okay with taking this.

if (client != 0 && args == 0)
{
DisplayBanTargetMenu(client);
}
ReplyToCommand(client, "[SM] Usage: sm_ban <#userid|name> <minutes|0> [reason]");
Copy link
Member

@Headline Headline Sep 3, 2018

Choose a reason for hiding this comment

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

I think that's best. It'd be annoying if users typed !ban Headline 0 "lemon" and got hit with usage errors every time.

@JoinedSenses
Copy link
Contributor Author

@JoinedSenses JoinedSenses commented Sep 3, 2018

The latest change requested from Headline will now only display usage params if client == 0 && args == 0, or if args is greater than 0 and less than 2 for sm_ban

Copy link
Member

@asherkin asherkin left a comment

I feel strongly enough that this should be checking the ReplySource rather than the client index to r-, it at least needs to be discussed. I also think the menu should only show on 0 args.

@JoinedSenses
Copy link
Contributor Author

@JoinedSenses JoinedSenses commented Sep 3, 2018

With these changes, the menu does only show on 0 args. No idea what you mean by the reply source? What sources should be checked for and how? It currently runs a check against client == 0, which would occur on rcon, sm_rcon, and servercommand.

My previous comment was regarding usage params reply, not the menu.

Edit: I see - console vs chat. Ill look into replysource. Not a bad idea.

Tested and worked as expected.
@JoinedSenses
Copy link
Contributor Author

@JoinedSenses JoinedSenses commented Sep 3, 2018

New commit, which now checks both reply source and client. Tested - usage params always display when issued via console and menu would not display.

Menu will only display if there are 0 arguments, client != 0, and the command was sent in chat.

@asherkin asherkin merged commit a5b2249 into alliedmodders:master Sep 3, 2018
2 checks passed
@asherkin
Copy link
Member

@asherkin asherkin commented Sep 3, 2018

Thanks! Nice work.

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

Successfully merging this pull request may close these issues.

None yet

5 participants