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

JLine2 interactive option does not handle input interactivly correct #767

Open
querqueq opened this issue Jul 17, 2019 · 9 comments
Open
Labels
theme: integration An issue or change related to integration with other frameworks, shells or operating systems theme: shell An issue or change related to interactive (JLine) applications

Comments

@querqueq
Copy link
Contributor

querqueq commented Jul 17, 2019

When using an interactive option in combination with jline2, prompting for the option value does not work correctly. It asks only once for the first character and allows no further input. On a Windows machine (cmd) it asks twice, once for the first character and another time for all following characters. Only the characters input the second time are printed.
Also trying to tab complete the value of an interactive option prompts for input in the same manner.
After this bug occurs, all characters are printed twice but only on Linux (bash).

Here's a recording on how it looks: https://asciinema.org/a/ILmUZhTRxld4lVujR1Engn8Ba

The following code shows this behaviour:

@Command(name = "", subcommands = {PasswordCommand.class})
public class PicocliInteractiveTest implements Callable<Integer> {

	@Command(name = "pw")
	static class PasswordCommand implements Callable<Integer> {

		@Option(names = {"-p"}, interactive = true, arity = "0..1", required = true)
		private char[] password = new char[0];

		public Integer call() throws Exception {
			System.out.println("\nPW: " + new String(password));
			return 0;
		}
	}

	public static void main(String[] args) throws Exception {
		ConsoleReader reader = new ConsoleReader();
		reader.setPrompt("demo>");

		PicocliInteractiveTest parent = new PicocliInteractiveTest();
		CommandLine cli = new CommandLine(parent);
		reader.addCompleter(new PicocliJLineCompleter(cli.getCommandSpec()));

		String line;
		while ((line = reader.readLine()) != null) {
			ArgumentList list =
					new WhitespaceArgumentDelimiter().delimit(line, line.length());
			cli.execute(list.getArguments());
		}
	}

	public Integer call() throws Exception {
		return 0;
	}
}

Version: 4.0.0-beta-2

@remkop
Copy link
Owner

remkop commented Jul 18, 2019

Thank you for the asciinema recording and the code to reproduce the problem.

Looking at the code for jline2 ConsoleReader, it hooks in a custom signal handler to deal with keyboard interrupts. I guess that this does not interact well with calls to Console.readPassword, which may under the hood do something similar...

What this means is basically that interactive = true options cannot be used with JLine2.

One idea for a solution is to use the new custom parameter consumer mechanism instead of interactive=true. In your custom consumer, you may be able to use a JLine2 API to read a password from the JLine2 ConsoleReader with echo off...

@remkop remkop added the theme: shell An issue or change related to interactive (JLine) applications label Jul 18, 2019
@querqueq
Copy link
Contributor Author

Thank you! A custom password handler with IParameterConsumer works:

@Command(name = "", subcommands = {PasswordCommand.class})
public class PicocliInteractiveTest implements Callable<Integer> {

	private static final String PROMPT = "demo>";
	//Reader could be passed to IParameterConsumer with custom IFactory
	private static ConsoleReader reader;

	@Command(name = "pw")
	static class PasswordCommand implements Callable<Integer> {

		//arity is ignored by PasswordParameterConsumer
		@Option(names = {"-p"}, required = true, parameterConsumer = PasswordParameterConsumer.class)
		private String requiredPassword;

		@Option(names = {"-o"}, parameterConsumer = PasswordParameterConsumer.class)
		private String optionalPassword;

		@Parameters(index = "0", arity = "1")
		private String parameter;

		public Integer call() throws Exception {
			System.out.format("Required Password: %s%n"
					+ "Optional Password: %s%n"
					+ "Parameter: %s%n"
					, requiredPassword, optionalPassword, parameter);
			return 0;
		}
	}

	public static void main(String[] args) throws Exception {
		reader = new ConsoleReader();

		reader.setPrompt(PROMPT);

		PicocliInteractiveTest parent = new PicocliInteractiveTest();
		CommandLine cli = new CommandLine(parent);
		reader.addCompleter(new PicocliJLineCompleter(cli.getCommandSpec()));

		String line;
		while ((line = reader.readLine()) != null) {
			ArgumentList list = new WhitespaceArgumentDelimiter().delimit(line, line.length());
			cli.execute(list.getArguments());
			//Reset prompt because it gets overwritten by reader.readLine(..)
			reader.setPrompt(PROMPT);
		}
	}

	public Integer call() throws Exception {
		return 0;
	}

	public static class PasswordParameterConsumer implements IParameterConsumer {

		public void consumeParameters(Stack<String> args, ArgSpec argSpec, CommandSpec commandSpec) {
			try {
				//Reads password as string not as char array
				//character mask '\0' == echo off
				argSpec.setValue(reader.readLine(String.format("Enter %s: ", argSpec.paramLabel()), '\0'));
			} catch (IOException e) {
				throw new CommandLine.ParameterException(commandSpec.commandLine(), "Error while reading password", e, argSpec, "");
			}
		}
	}
}

@remkop
Copy link
Owner

remkop commented Jul 18, 2019

Great! Glad we were able to solve that.

Would you be interested in helping to improve the documentation so that other users won’t run into this?

I’m thinking along the line of a brief note in the user manual section on interactive options, and an example custom consumer in the readme for the picocli-shell-jline2/3 module(s).

What do you think?

@remkop
Copy link
Owner

remkop commented Jul 18, 2019

By the way, picocli 4.0 GA has been released so you may want to upgrade. :-)

@querqueq
Copy link
Contributor Author

Sure, I'll create a PR with a note and an example for both modules.

@remkop
Copy link
Owner

remkop commented Jul 29, 2019

Thank you for the PR in #775!
I went ahead and merged it.

However, looking at your example above, I actually like the inner class a bit better, because it is smaller and does not require a custom factory.

The main purpose of the example in the shell-jline2 README is to illustrate how JLine 2 and picocli can be used together in general, and specifically how to use PicocliJLineCompleter, and I worry that too much focus on the interactive password may distract from the main point...

Perhaps it makes sense to have a separate section in the README for interactive passwords? So the first example would be like it was originally, with the subcommands and everything, and we could add a separate section, "Interactive Passwords" that has a short paragraph that explains the problem (like what you added to the user manual), plus a separate example that shows the IParameterConsumer solution. (This separate example code may not need many subcommands.)

I'm not proposing to change the test classes under test/java/picocli/shell/jline2/example, only the README.

What do you think?

@remkop
Copy link
Owner

remkop commented Jul 30, 2019

I'm a bit surprised that you did not encounter problems with jline3. Note that jline3 by default gives a dumb terminal. To get "advanced capabilities" you need to have either Jansi or JNA library in your classpath along with the jline-terminal-jansi or jline-terminal-jna jar.

Is that what you tested?

@querqueq
Copy link
Contributor Author

querqueq commented Aug 1, 2019

You right about the example being to bloated. I'll put it in its own section in the README but I think the test classes should also reflect what's in the README.

I'm a bit surprised that you did not encounter problems with jline3. Note that jline3 by default gives a dumb terminal.

I tried it with the dumb terminal in eclipse and in linux bash without any problems. I will also see if it works in Windows' cmd.

@remkop
Copy link
Owner

remkop commented Aug 1, 2019

I tried it with the dumb terminal

I wasn't sure if the dumb terminal would intercept interrupts and have this problem. I took a brief look at the code for jline3 and I'm still not sure but I suspect that even the dumb terminal intercepts interrupts. Still, if you could check on Windows that would be great.

I'll put it [the example] in its own section in the README

Thank you very much!

the test classes should also reflect what's in the README.

Makes sense.

@remkop remkop added the theme: integration An issue or change related to integration with other frameworks, shells or operating systems label Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: integration An issue or change related to integration with other frameworks, shells or operating systems theme: shell An issue or change related to interactive (JLine) applications
Projects
None yet
Development

No branches or pull requests

2 participants