This sounds more suited as a separate command like sm_banmenu.

Sometimes I call sm_ban just to check for the order of the required parameters, and would rather not have to close an extra menu when I do that.

[sm_ban playerName timeInMinutes "Reason"]? Didn't think it was that hard to remember.

If it's that annoying of a change, then we can close this PR.

This PR still displays the usage message, though it also shows the menu. I figured it wouldn't really be that annoying since how often really is it needed to use /ban with no args just to see usage parameters? There is also an ongoing discussion in the sourcemod GH regarding this suggestion - an idea is to offer a cvar to toggle this behavior change.

Just looking at it now again, feel like this would break any RCON scripts/plugins that call to this command, as it does not check for player validity correctly.

How would it break rcon scripts? When rcon or sm_rcon is ran, clientId returned is 0. Ive had zero issues since implementing this on my own servers.

It is an admin cmd, so if there is an issue with an invalid client, then the problem lies elsewhere.

I've seen occasions, where plugins bootstrap off of player commands, and zero issues for yourself doesn't mean for others.

I double checked through the flow of the ban menu and it does the same exact thing that /ban playerName time "Reason" would do (see CreateBan). If there is an issue with an invalid client, then the problem is not with the menu, but elsewhere in the plugin.

The problem is triggering the menu accidentally due to a player bootstrap plugin. This is not a typical situation, but I've seen it in the past while resolving people's problems. Nevertheless, I believe this should be reserved for admin menu only, requiring the generic flag rather than just bundling it just for the sake of convenience.

In order to make sure this conversation has at least been productive in any sense, could you produce an example of where my new feature may cause some sort of problem? Doing so could perhaps allow me to learn and improve this code, otherwise I have failed understand the issue due to vagueness.

Lets say a plugin runs ServerCommand sm_ban. That would result in client == 0, which would not display the menu. If a plugin runs FakeClientCommand on sm_ban and that client does not have access, that would result in an error message displayed that the user does not have access, which again would not display the menu.

Also "just for the sake of convenience."

Isnt that the entire point of improving plugins? Convenience? Making it easier to do things?

Plugins bootstrap FakeClientCommand to detect compatibility rather than the includes.

Isnt that the entire point of improving plugins? Convenience? Making it easier to do things?

No. It was just yesterday someone asked help on admin menu flags; there's no need to give admins the ban menu if it was designed to work with the generic flag's admin menu and we ought to respect the existing flag system.

I dont understand what you mean by "Plugins bootstrap FakeClientCommand to detect compatibility rather than the includes." You keep saying bootstrap. This is gibberish.

Also afaik, fakeclientcommand can't override the admin flags for a registered admin command.

The admin menu also checks to see if a player has access to sm_ban before adding it to the client's admin menu list, which necessarily isn't ADMNFLAG_GENERIC.

Like I've said, if somehow a plugin is able to open the menu via sm_ban, then something else is wrong with either SM or something internal to sb++.

So Im beginning to suspect that you dont understand how plugins work. At this point I don't even care. My two goals were to contribute and to learn. Neither of those have happened to far.

Admin menu requires the generic flag, and like I said this isn't a common case, so you don't need to get all trigger happy about it.

So Im beginning to suspect that you dont understand how plugins work. At this point I don't even care.

That's your opinion, and if you want to believe that, then do it, if you don't want people's take on this, then don't bother.

By default, admin menu uses adminflag generic, yes, but because that is what sm_ban is set to by default. If it is overriden, then it is a set to whatever flag it is overridden to. This further points to your lack of understanding on how plugins work. You have provided zero working evidence as to why my feature would break anything, and instead are going off your "feelings" and "beliefs".

You have provided zero working evidence as to why my feature would break anything, and instead are going off your "feelings" and "beliefs".

I do not have any working evidence because I'm NOT the minority subjective group in question. If you want I can spin up an example tomorrow.

Taking the minority factor away, the only thing I have against this is the idea to keep the menu as a submenu in admin menu only; as geo mentioned it's not clear to its command name

Edit: By default, Admin Menu itself only requires ADMNFLAG_GENERIC, but in order to populate the menu with sm_ban, it checks client flag for ADMFLAG_BAN, which is what is set when sm_ban is created.

RegAdminCmd("sm_ban", CommandBan, ADMFLAG_BAN, "sm_ban <#userid|name> <minutes|0> [reason]", "sourcebans");

Because of this, it means that a client is REQUIRED to have either ADMNFLAG_BAN or whatever flag(s) the command is overridden by.

"Taking the minority factor away, the only thing I have against this is the idea to keep the menu as a submenu in admin menu only; as geo mentioned it's not clear to its command name"

For this reason, I have left in the ReplyToCommand line when args == 0. It will still display the usage parameters, whether or not client == 0.

Locked as too heated, will continue in DM with JoinedSenses.