Rework command alias handling, step 1 #6685
Conversation
This decouples Command from CommandMap, and moves the burden of tracking registered aliases to CommandMap. This allows lots of simplification, and also solves a few weird usage message issues. - Active (registered) aliases are now tracked via CommandMapEntry - Commands overriding other commands' aliases now update the original command's registered alias list properly - Command alias lists now include prefixed aliases - Prefixed aliases are now included in command data provided to the client - Server-side /pocketmine:help is now visible on the client - Permission testing can now provide context that's more relevant to the command invocation - e.g. if a user doesn't have /time set permission, it'll now show a more specific message where previously it would just show that the permission for /time was denied - User-specified label is now used for permission messages instead of command name - this is more consistent with user expectations - /help can now see prefixed aliases - Removed magic alias registration behaviour for VanillaCommand that I don't think anyone expected - Aliases are now provided to CommandMap via register() parameters, instead of being retrieved from the provided Command - Command->get/setAliases(), get/setLabel() and getName() are removed - Command->getName() pushed down to PluginCommand, as it's only useful for CommandExecutors as a command ID and shouldn't be used anywhere else
![pmmp-admin-bot[bot]](https://avatars.githubusercontent.com/in/1059952?s=60&v=4){kind=small}
dktapps
added
Category: API
|
|
||
| public function getUsage() : Translatable|string{ | ||
| //TODO: usage messages ought to use user-specified alias, not command preferred | ||
| //command-preferred is confusing if the user used a different alias |
There was a problem hiding this comment.
maybe adding an alias input parameter to the getUsage() method.
also update the default command usage in the language file from:
commands.tp.usage=/tp [target player] OR /tp [target player] [ ]
to:
commands.tp.usage=/{%0} [target player] OR /{%0} [target player] [ ]
then, translate {%0} to the used alias?
There was a problem hiding this comment.
Yes, it's just a lot of work
|
I don't like the look of it. Sending fully prefixed/namespaced commands to clients feels spammy. It should be cleaner by default.
|
|
Yeah, I'm not sure about the fully-qualified names either. I think the way that those are handled probably needs to be rethought. Longer term I think I want to have those move towards being internalised command IDs which are used by aliases to locate a command (see #3371), rather than being able to be directly invoked. But that's too big of a change for one PR. |
To be honest this was more of a side effect of changing the tracking of the registered aliases. Previously the command registration wasn't aware of its own prefixed aliases, which lead to all kinds of interesting surprises for people trying to block commands by name rather than using permissions, since the aliases weren't visible in Filtering out the prefixed aliases is a bit problematic since we currently don't restrict aliases or fallback prefixes from containing |
This PR started out as an effort to decouple Command and CommandMap, but it's turned into a bit more than that.
A summary of changes:
Commandno longer tracks its own registered aliases (now the job ofCommandMap), breaking circular dependencyCommandMap->register()/helpnow shows prefixed aliases such aspocketmine:helpCommand->getName()removed & pushed down toPluginCommand, as it's only useful forCommandExecutorusersRelevant issues
CommandandCommandMap(aliases, label) #6508