(TBD) Command Trigger rework
wysohn opened this issue ยท 25 comments
It's not providing enough functionality that the users want.
For example, some users want to fully control the tab-completion feature without the hint/candidate method,
and some users want to hide some commands created using TRG.
Further discussion on use-cases will be needed.
@wysohn
Sorry for late replying. I was really busy these three days 'bout graduation problem.
Anyway, I may agree that using Queue is better way to handling List by the reason that you said(No need to check the number of arguments)
But still, I'm not sure that if there's a situation that we have to use more than two arguments in multiple tab completion, how could we use it when it already consumed?
For instance, let's think about a command that arg0 defines the list of arg2, arg3, and arg4 's tab-completion. As you said, we can use peek() method to get arg0 without consuming it. However, this command is quite unique. Because arg1 's value also affacts the list of arg2, arg3, and arg4 's tab-completion. In other words, arg0 and also arg1 does affect to multiple arguments. In this situation, we must have both arg0 and arg1 value in arg2, arg3, arg4 block, which looks impossible with pure Queue model.
(toArray() might be a kind of work around? ๐ค)
@wysohn
Let me use a system that I maid as an example.
on CommandTrigger
IF args.length == 0
#CALL "AutoChestHelp" false
ELSE
operation = args[0]
IF operation.equalsIgnoreCase("check") && args.length > 3
areaType = args[1]
blockType = args[2]
IF areaType.equalsIgnoreCase("pos")
//someCodeInHere...
ELSEIF areaType.equalsIgnoreCase("radius")
rad = args[3]
//another code here
ENDIF
ENDIF
IF operation.equalsIgnoreCase("help") && args.length == 2
helpType = args[1]
IF helpType.equalsIgnoreCase("random"
#CALL "AutoChestHelp_Random"
ELSEIF helpType.equalsIgnoreCase("add")
#CALL "AutoChestHelp_Add"
ENDIF
ENDIF
ENDIF
As you can see in this CommandTrigger, there are random
and help
operation differed by args[0]
- /ac random <areaType> <blockType> [<radius>]
- /ac help <helpType>
As you can see, based on args[0]
, its tab-completion should be differentiated.
If args[0]
is random
, args[1]
ใฎใฟใใณใณใใชใทใงใณ should be <areaType>, which contains pos
, radius
args[2]
's tab-completion should be <blockType>, which contains Material.values().
finally, args[3]
is needed if args[1]
's value is radius
.
If args[0]
is help
, args[1]
's tab-completion should be <helpType>, which contains check
, and add
.
As this example have shown, there's so many cases that needed to be differentiated based on what has been typed on other args.
- prebuilt list
$playerlist aaa,bbb,ccc,... $otherlist ooo,ppp,...
Config:
...
tabcompleters:
- $playerlist
- aaa,bbb,ccc
- '' # empty
- $otherlist
- ooo,ppp
- Customized
For example, the command trigger is about to be executed (/mycommand apple banana cat do) where 'do' is the partially finished command.
Provided the variable tabs
to the script
IF trigger == "init"
tabs.register(LAMBDA args =>
arg = args.poll();
filterStartsWith(listOf("apple", "application", "abnormal"), arg)
ENDLAMBDA)
tabs.register(LAMBDA args =>
arg = args.poll();
filterStartsWith(listOf("banana", "boat", "beginner"), arg)
ENDLAMBDA)
tabs.register(LAMBDA args =>
arg = args.poll();
filterStartsWith(listOf("cat", "cut", "cause"), arg)
ENDLAMBDA)
tabs.register(LAMBDA args =>
arg = args.poll();
filterStartsWith(listOf("dog", "deed", "doom"), arg)
ENDLAMBDA)
ENDIF
IF trigger == "execute"
ENDIF
IF trigger == "init"
tabs.register(LAMBDA args =>
// we didn't consume the argument, so it's still in the queue
listOf()
ENDLAMBDA)
tabs.register(LAMBDA args =>
arg0 = args.poll(); // example of getting value of first argument in the second argument tab completer
arg1 = args.poll(); // this should be the partially completed command
IF arg0 == "abc"
filterStartsWith(listOf("booting", "ball", "baby"), arg1)
ELSE
filterStartsWith(listOf("banana", "boat", "beginner"), arg1)
ENDIF
ENDLAMBDA)
ENDIF
IF trigger == "execute"
ENDIF
each register() will add the custom tab completer, and when tab completion is requested by the user, all of the tab completers will be executed in insertion order, yet they will share the same args
.
the args
given in the LAMBDA function is the 'queue of commands', so each poll()
will consume one argument from it in First in First Out (FIFO) manner. Then, the LAMBDA function will return a List of Strings, which will be used as the tab completion for the user. (Note that filterStartsWith(List, String)
will be provided in CommonFunctions)
In our example (/mycommand apple banana cat do), the fourth tab completer will be executed, then for the user, dog
and doom
will be shown as possible tab completions choices.
The Customized Tab completion logic looks great for me, but with that logic, there would be one weak part which could be major problem for users. 'arg-arg Interaction'
Most user would want to change the tab-completion list based on what have been written on previous args. which means there should be a way to interact with each elements with tab-completer. And , it looked like arg only returns the one of args[].
so, what users exactly need is another variable that works like $cmdline.
The Customized Tab completion logic looks great for me, but with that logic, there would be one weak part which could be major problem for users. 'arg-arg Interaction' Most user would want to change the tab-completion list based on what have been written on previous args. which means there should be a way to interact with each elements with tab-completer. And , it looked like arg only returns the one of args[].
so, what users exactly need is another variable that works like $cmdline.
That's an interesting point
Would there be an example of how the previous argument is to be used?
I feel like the current Bukkit API's way of handling tab completion is kind of unnecessary since I can't really see when the user would want to infer the previous arguments for the tab completion of the current one.
I'm open to discussion about this feature
@wysohn Let me use a system that I maid as an example.
on CommandTrigger
IF args.length == 0 #CALL "AutoChestHelp" false ELSE operation = args[0] IF operation.equalsIgnoreCase("check") && args.length > 3 areaType = args[1] blockType = args[2] IF areaType.equalsIgnoreCase("pos") //someCodeInHere... ELSEIF areaType.equalsIgnoreCase("radius") rad = args[3] //another code here ENDIF ENDIF IF operation.equalsIgnoreCase("help") && args.length == 2 helpType = args[1] IF helpType.equalsIgnoreCase("random" #CALL "AutoChestHelp_Random" ELSEIF helpType.equalsIgnoreCase("add") #CALL "AutoChestHelp_Add" ENDIF ENDIF ENDIF
As you can see in this CommandTrigger, there are
random
andhelp
operation differed byargs[0]
- /ac random []
- /ac help
As you can see, based on
args[0]
, its tab-completion should be differentiated.If
args[0]
israndom
,args[1]
ใฎใฟใใณใณใใชใทใงใณ should be , which containspos
,radius
args[2]
's tab-completion should be , which contains Material.values(). finally,args[3]
is needed ifargs[1]
's value isradius
.If
args[0]
ishelp
,args[1]
's tab-completion should be , which containscheck
, andadd
.As this example have shown, there's so many cases that needed to be differentiated based on what has been typed on other args.
Sounds good. The proposal has changed. Let me know what you think
@wysohn
It looks great, yet I'm not really sure why args must be consumed from args which actually makes user hard to use pre-comsumed arguments.
As you can see in your second example code, if user want to use tab-completion in arg0
, it must be consumed in arg0
's LAMBDA-ENDLAMBDA block, which means if user want to use value of arg0
's value in another block, they must give up the tab-completion of arg0 value.
<args0> = astro, korea, eminem
<args1-1> =single, double, triple
<args1-2> = hana, dool, set
<args2> = <NUMBER>
Let's type this command
- /somecmd eminem single 6
but if args0
is eminem or astro, args1
should be single, double, triple
first, plugin will show tab-completion of command itself
- /s
- somecmd
- surgerycmd
- setspawn
then tab-complete to somecmd, and give space ' '
- /somecmd
- astro
- korea
- eminem
type e
as soon as typing e, the queue args
is now [e]
but going to use poll()
to consume it, now it's []
- /somecmd e
- eminem
type m
as soon as typing e, the queue args
is now [em]
but going to use poll()
to consume it, now it's []
- /somecmd e
- eminem
tab-complete eminem and add space
- /somecmd eminem
and there's a problem. because eminem already comsumed in first tab-completion, args
queue is now EMPTY [], so that there's no way to check whether args0 is eminem or not.
wouldn't there should be any way to get value like args.get(0), args.get(1)......
so that args
's value actually does not consumed but is able to get its value?
and it would be much easier if there is a method like last() to get current tab-completing part from the queue.
@wysohn It looks great, yet I'm not really sure why args must be consumed from args which actually makes user hard to use pre-comsumed arguments. As you can see in your second example code, if user want to use tab-completion in
arg0
, it must be consumed inarg0
's LAMBDA-ENDLAMBDA block, which means if user want to use value ofarg0
's value in another block, they must give up the tab-completion of arg0 value.= astro, korea, eminem
=single, double, triple
= hana, dool, set
=
Let's type this command
- /somecmd eminem single 6
but if
args0
is eminem or astro,args1
should be single, double, triplefirst, plugin will show tab-completion of command itself
- /s
- somecmd
- surgerycmd
- setspawn
then tab-complete to somecmd, and give space ' '
- /somecmd
- astro
- korea
- eminem
type e
as soon as typing e, the queue
args
is now [e] but going to usepoll()
to consume it, now it's []
- /somecmd e
- eminem
type m
as soon as typing e, the queue
args
is now [em] but going to usepoll()
to consume it, now it's []
- /somecmd e
- eminem
tab-complete eminem and add space
- /somecmd eminem
and there's a problem. because eminem already comsumed in first tab-completion,
args
queue is now EMPTY [], so that there's no way to check whether args0 is eminem or not.wouldn't there should be any way to get value like
args.get(0), args.get(1)......
so thatargs
's value actually does not consumed but is able to get its value? and it would be much easier if there is a method like last() to get current tab-completing part from the queue.
For the consumption part, you can check the top element by peek() method. This will give you the top element of the queue, but it will not consume it.
(Check out for Queue)
As for the eminem
example, it's not required for each block to consume the argument. Consumption is optional, and it can be handled in the next block, as I described in the second example.
as soon as typing e, the queue args is now [e]
but going to use poll() to consume it, now it's []/somecmd e
eminem
type mas soon as typing e, the queue args is now [em]
but going to use poll() to consume it, now it's []/somecmd e
eminem
tab-complete eminem and add space
/somecmd eminem
For this, notice that the first block doesn't have to consume but just return the listOf("astro", "korea", "eminem"), it will still show those three elements as the candidates
Then, in the second block, you can consume the queue twice to get both the first argument and second argument to figure out what to do. If the first poll() were "astro", then you would be able to return listOf("single, double, tripple"), and if it were "korean", then you would be able to return listOf("hana", "dool", "set")
The reason why I think the queue is a better model is that every command is naturally read from left to right, and we don't usually have to infer what's going to show up on the right. Using the List for argument is a mess because now we have to check a number of arguments, their index, and where each argument is relative to each other.
As you can see in your second example code, if user want to use tab-completion in arg0, it must be consumed in arg0 's LAMBDA-ENDLAMBDA block, which means if user want to use value of arg0's value in another block, they must give up the tab-completion of arg0 value.
I think this is the most serious problem we can think of. I still would naturally think that if an argument ever needs the information of the preceding argument, they are one body. In the eminem example, the second argument is dependent on the first argument, so they should be thought of as one body, not as a separate one. If, for example, the fourth argument ever needs to infer the first argument, the first, second, and the third argument will not be consumed, and they will be consumed altogether in the fourth block. But I think if that case ever arises, I would consider that command to be defective and can't really see any use-case of it.
@wysohn Sorry for late replying. I was really busy these three days 'bout graduation problem.
Anyway, I may agree that using Queue is better way to handling List by the reason that you said(No need to check the number of arguments)
But still, I'm not sure that if there's a situation that we have to use more than two arguments in multiple tab completion, how could we use it when it already consumed?
For instance, let's think about a command that arg0 defines the list of arg2, arg3, and arg4 's tab-completion. As you said, we can use peek() method to get arg0 without consuming it. However, this command is quite unique. Because arg1 's value also affacts the list of arg2, arg3, and arg4 's tab-completion. In other words, arg0 and also arg1 does affect to multiple arguments. In this situation, we must have both arg0 and arg1 value in arg2, arg3, arg4 block, which looks impossible with pure Queue model. (toArray() might be a kind of work around? thinking)
My school has started too so no worries xD
I understand your concern, but I can't really see any use-case of such a nested and complex command structure. We might be overthinking the problem. If I just can see a counterexample that shows the use of such a command structure in the real-world problem, then I must change it to just an array of arguments, but I can't think of any on top of my head at the moment.
The explanation you have shown is very theoretical, and I don't think that will ever arise in most of the use cases. As far as I can think of, one of the most complex plugins is WorldEdit, yet I don't think it has any command that cannot be structured with the queue. (And it's usually ideal to put all the variable arguments at the end of the command, so it will be easier to add more subcommands in the future.)
Why not use actual code for tab completion? Or maybe existing formats like commodore?
Why not use actual code for tab completion? Or maybe existing formats like commodore?
Hmm can you elaborate a bit more?
For example, like how Spigot does it (position-based) or how Minecraft does it (Brigadier library, then tab completion in the command trigger itself is better since it handles running the command as well). The commodore format is a format used by LuckPerms.
For example, like how Spigot does it (position-based) or how Minecraft does it (Brigadier library, then tab completion in the command trigger itself is better since it handles running the command as well). The commodore format is a format used by LuckPerms.
Spigot style requires tracking of the array length which I want to avoid. Also, the commands are always entered in left to right order, so queue sounds like a better solution to me (meaning that there will be no 'null' in-between each command, so idk what would be the benefit of using the array structure here).
Brigadier seems well structured, but we wouldn't want to rely on external libraries as much as possible. We could include the library included in the final jar file, but it would increase the file size.
I thought that Brigadier was included in Minecraft 1.13+ by default?
Sorry for the late reply. Was busy preparing for graduation
Was it? I had no clue xD
Well, then Brigadier might be a good alternative yet we would need to drop support for version 1.12.2 and before
But lets keep that as an option for future
New Command Trigger Tab-Completion Model Suggestion
Command Usage
NOT YET IMPLEMENTED
JSON
Tested and Fully functional on
- Bukkit-latest
- Bukkit-legacy
- Sponge
Example JSON:
{
"tabs": [
{
"index":0,
"candidates": "give,get,astro"
},
{
"index":1,
"candidates": "$playerlist",
"conditions": [
{
"index": 0,
"regex": "give|get"
}
]
},
{
"index":1,
"hint": "<amount>",
"conditions": [
{
"index": 0,
"regex": "astro"
}
]
},
{
"index":2,
"hint": "<amount>",
"conditions": [
{
"index": 0,
"regex": "give|get"
}
]
},
{
"index":2,
"hint":"<another>",
"candidates": "aaaa,bbbb,cccc",
"conditions": [
{
"index": 0,
"regex": "astro"
},
{
"index": 1,
"regex": "100"
}
]
}
],
"aliases": [],
"permissions": []
}
INDEX
As you can see, you can now specify INDEX of each tab-completer. Still, if "index" is missed, the order(index) of the object in list would be the "index" value like before. Also, you may add more than one tab-completer for one index, so that you can implement different hint/candidate based on "conditions".CONDITIONS
A new feature. You can now test specific String args[index] with regex.INDEX in CONDITION
The index of arguments that should be tested.REGEX in CONDITION
The regex string that should be used for test.So, in conclusion, the CONDTION acts like
matches(args[INDEX], REGEX)
And also, as you can see on example , you can add multiple CONDITION at one tab-completer, so that checking multiple arguments would be available (they treated as AND expression).
Ouch, some servers still use 1.12.2 for performance... could use the old system for fallback though. This seems quite urgent honestly, since the current system is flawed in many ways.
I assume this feature should be bumped to 3.4.x, not to 3.3.x
Hmm, I thought adding this feature on 3.3.0 would be best option because
-
current model(old one) is somehow malfunctioning for $playerlist which spamming console whenever you write any char on chat. So this change also can be treated as bugfix.
-
new model is almost done already and waiting for PR #497 being merged to development branch.
current model(old one) is somehow malfunctioning for $playerlist which spamming console whenever you write any char on chat. So this change also can be treated as bugfix.
Yeah from that point of view, it can be seen as a bug fix. However, as shown in the title of #501, it is a new tab-complete model for CommandTrigger. I'm thinking of this as a feature, not a bug fix. So I suggest that it is most appropriate to bump the minor version.
new model is almost done already and waiting for PR #497 being merged to development branch.
As just I said above, PR #497 is related to bug fixes.
Was that possible? Then I don't think it would be needed separately.
Edit: it does not work as of now.
current model(old one) is somehow malfunctioning for $playerlist which spamming console whenever you write any char on chat. So this change also can be treated as bugfix.
Yeah from that point of view, it can be seen as a bug fix. However, as shown in the title of #501, it is a new tab-complete model for CommandTrigger. I'm thinking of this as a feature, not a bug fix. So I suggest that it is most appropriate to bump the minor version.
new model is almost done already and waiting for PR #497 being merged to development branch.
As just I said above, PR #497 is related to bug fixes.
Kind of confusing for me. As you said, the PR #497 is bugfix, and PR #501 is new feature. Based on Semantic Versioning, PR #497 would be 3.3.0-Beta.9 (for us, 3.3.0.9-Beta)
and currently we don't have 3.3.0 release.
So in my opinion, this feature may be added as a part of 3.3.0 release's new feature, which means this feature also be included in 3.3.0.9-Beta prerelease.(Like we added LAMBDA feature or inventory title feature on 3.3.0.4-Beta)
In Semantic Versioning point, yes we may not add new feature on Beta because it should only be used as bugfix for new feature.๐
and currently we don't have 3.3.0 release.
Aye, we are still in beta. I forgot it ๐
I feel like the ability to disable tab-completion completely (like on older versions of TriggerReactor) is a good idea as well.
I feel like the ability to disable tab-completion completely (like on older versions of TriggerReactor) is a good idea as well.
I'll look into it. But, for me, you can represent it by simply removing the "tabs" element from json, so.. umm. Could you tell me why it is required?