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

autocomplete & suggestions for nested subcommands not working in Picocli Shell JLine3? #969

Closed
niklas97 opened this issue Mar 2, 2020 · 12 comments · Fixed by #987
Closed
Labels
theme: auto-completion An issue or change related to auto-completion theme: shell An issue or change related to interactive (JLine) applications
Milestone

Comments

@niklas97
Copy link

niklas97 commented Mar 2, 2020

For Picocli Shell JLine3, Is there any support for suggestions & autocomplete of nested subcommands? I tried to accomplish just that by adding a subcommand to one of the subcommands (the "cmd" named command in this case) of the example code at https://github.com/remkop/picocli/tree/master/picocli-shell-jline3 but it did not work. I.e., I can autocomplete the first subcommand but no subcommand of that subcommand.

Based on the description of PicocliCommands it should work.

Thanks in advance!

/ Niklas

@niklas97 niklas97 changed the title autocomplete & suggestions for nested subcommands autocomplete & suggestions for nested subcommands not working? Mar 2, 2020
@niklas97 niklas97 changed the title autocomplete & suggestions for nested subcommands not working? autocomplete & suggestions for nested subcommands not working for Picocli Shell JLine3? Mar 2, 2020
@niklas97 niklas97 changed the title autocomplete & suggestions for nested subcommands not working for Picocli Shell JLine3? autocomplete & suggestions for nested subcommands not working in Picocli Shell JLine3? Mar 2, 2020
@niklas97
Copy link
Author

niklas97 commented Mar 2, 2020

It works with PicocliJLineCompleter but then I do not know how to make it work with builtins as well

@remkop
Copy link
Owner

remkop commented Mar 3, 2020

@niklas97 Thank you for raising this!

I think I found the cause: Looking at the code for PicocliCommands it seems to only add completions for the top-level command and its immediate subcommands. Instead, it should recursively go down the full hierarchy.

@remkop remkop added this to the 4.3 milestone Mar 3, 2020
@remkop remkop added theme: auto-completion An issue or change related to auto-completion type: bug 🐛 theme: shell An issue or change related to interactive (JLine) applications status: in-progress 👷‍♂️ labels Mar 3, 2020
@niklas97
Copy link
Author

niklas97 commented Mar 3, 2020

@remkop Great!

@remkop
Copy link
Owner

remkop commented Mar 3, 2020

@mattirn What do you think is the best approach for supporting nested sub-subcommands? Do you think it can be done with the existing org.jline.builtins.Completers.SystemCompleter class?

@mattirn
Copy link
Contributor

mattirn commented Mar 6, 2020

I think we should wrote completer that support nested sub-subcommands i.e. somewhere in compileCompleters()
I think something similar has been already done by @mel4tonin4

@mattirn
Copy link
Contributor

mattirn commented Mar 20, 2020

We can probably resolve this by using the same approach as here JLine3 #518

   public PicocliCommands(Supplier<Path> workDir, CommandLine cmd) {
       this.workDir = workDir;
       this.cmd = cmd;
       commands = new ArrayList<>(cmd.getCommandSpec().subcommands().keySet());
       aliasCommand = extractAliases(cmd);
       for (CommandLine sub : cmd.getSubcommands().values()) {
           this.register(sub.getCommandName(), New PicocliCommands(workdir, sub));
       }
    }

    public void register(String command, CommandRegistry subcommandRegistry) {
        subcommands.put(command, subcommandRegistry);
    }

    public Completers.SystemCompleter compileCompleters() {
        Completers.SystemCompleter out = new Completers.SystemCompleter();
        for (String command : commandNames()) {
            if (subcommands.containsKey(command)) {
                for(Map.Entry<String, Completer> entry : subcommands.get(command).compileCompleters().getCompleters().entrySet()) {
                    List<Completer> cmps = ((ArgumentCompleter)cc).getCompleters();
                    cmps.add(0, new StringsCompleter(command));      // add completer for main command
                    Completer last = cmps.get(cmps.size() - 1);
                    if (last instanceof OptionCompleter) {           // adjust option completer
                        ((OptionCompleter)last).setStartPos(cmps.size() - 1);
                        cmps.set(cmps.size() - 1, last);
                    }
                    out.add(command, new ArgumentCompleter(cmps));   // add a new completer to the map   
                }
            } else {
                out.add(command, commandExecute.get(command).compileCompleter().apply(command));
            }
        }
        return out;
    }

remkop added a commit that referenced this issue Mar 21, 2020
remkop added a commit that referenced this issue Mar 21, 2020
@remkop
Copy link
Owner

remkop commented Mar 21, 2020

Ideally PicocliCommands should support subcommands up to any depth. This will likely require some way to recursively go down the command hierarchy.

@mattirn, I have taken a stab at modifying the compileCompleters method to prepare for this. Can you take a look?

The next step would be to recursively call compileCompleters for each subcommand, but I am not sure if the same SystemCompleter instance can be used, or whether this also needs to be nested or otherwise modified.

remkop added a commit that referenced this issue Mar 21, 2020
…tion support for nested sub-subcommands; simplify code
remkop added a commit that referenced this issue Mar 21, 2020
…tion support for nested sub-subcommands; simplify code
remkop added a commit that referenced this issue Mar 21, 2020
…tion support for nested sub-subcommands; simplify further
@mattirn
Copy link
Contributor

mattirn commented Mar 21, 2020

I have added a code snippet to my previous comment in order to understand better my proposal.

I'm afraid that your idea does not work with existing implementation. For a simple commands system completer should have completer like

new ArgumentCompleter(new StringsCompleter(cmd), optionCompleter)

for sub-commands

new ArgumentCompleter(new StringsCompleter(cmd), new StringsCompleter(sub-cmd), optionCompleter)

for subsub-commands

new ArgumentCompleter(new StringsCompleter(cmd), new StringsCompleter(sub-cmd), new StringsCompleter(subsub-cmd), optionCompleter)

etc

remkop added a commit that referenced this issue Mar 21, 2020
@remkop
Copy link
Owner

remkop commented Mar 21, 2020

@mattirn Thanks for taking a look!

Sub-commands (and nested sub-subcommands) themselves also have aliases and options, and I would like to do more than just completing the nested subcommand name.

The first argument of the OptionCompleter constructor is a nested completer. We currently always pass in a NullCompleter.INSTANCE, but I believe that this is where we should pass the completers for subcommands.

I thought support for nested subcommands could be accomplished by making this nested completer an AggregateCompleter containing a SystemCompleter for each subcommand.

I have refactored a bit further, but I am stuck here.

The problem is that SystemCompleter does not work until after compile is called on it, and there is no way to call compile on nested completers... Can this design be changed?

@remkop
Copy link
Owner

remkop commented Mar 21, 2020

Actually, that last problem is relatively easy to solve by having a custom SystemCompleter subclass that calls compile() on nested completers. What I'm bumping into next is that the PicocliCommands class creates a SystemCompleter that only completes subcommands, but not options for the top-level command. This makes it hard to use it recursively. I'm still looking, but is this an oversight or intentional?

https://github.com/remkop/picocli/blob/master/picocli-shell-jline3/src/main/java/picocli/shell/jline3/PicocliCommands.java#L131

@mattirn
Copy link
Contributor

mattirn commented Mar 22, 2020

Sub-commands aliases I think can be easily supported. It is enough to add them in sub-command completer. But sub-command options is more challenging problem, I'm not really thought about this yet.

The first argument of the OptionCompleter is reserved for command positional parameters. If command has more than one parameter you should set OptionCompleter first argument as a list of completers. In the list first position you have completer for the command first parameter, in the second position for the command second parameter and so on. If it is used for sub-command completion then the last sub-command options can be entered already after the first command.

In addition to the sub-command completer problem we have to fix also commandDescription() method. But I think that it will be rather straightforward.

mattirn added a commit to mattirn/picocli that referenced this issue Apr 10, 2020
remkop pushed a commit that referenced this issue Apr 10, 2020
remkop added a commit that referenced this issue Apr 10, 2020
@remkop
Copy link
Owner

remkop commented Apr 10, 2020

@niklas97 Please take a look at the latest picocli master: Thanks to @mattirn's PR the jline3 completion with picocli nested subcommands has improved a lot! (Nice work, @mattirn!)

@niklas97 If you want to verify, do this:

git clone https://github.com/remkop/picocli.git
cd picocli
gradlew publishToMavenLocal

That should publish picocli-shell-jline3-4.2.1-SNAPSHOT to your local .m2 Maven cache. You can then try this in a project that uses the info.picocli:picocli-shell-jline3-4.2.1-SNAPSHOT dependency. Also make sure to use the latest JLine 3.14.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: auto-completion An issue or change related to auto-completion theme: shell An issue or change related to interactive (JLine) applications
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants