Copilot commented on code in PR #15367:
URL: https://github.com/apache/grails-core/pull/15367#discussion_r2748581949
##########
grails-forge/gradle.properties:
##########
@@ -35,7 +35,7 @@ groovyVersion=3.0.25
jacksonDatabindVersion=2.18.3
jakartaInjectVersion=1.0.5
# match the jansi version in grails-bom
-jansiVersion=1.18
+jansiVersion=2.4.1
Review Comment:
The version inconsistency between dependencies.gradle (jansi.version =
2.4.2) and grails-forge/gradle.properties (jansiVersion = 2.4.1) could lead to
dependency conflicts. The versions should be consistent across the entire
project.
```suggestion
jansiVersion=2.4.2
```
##########
grails-bootstrap/src/main/groovy/grails/build/logging/GrailsConsole.java:
##########
@@ -251,51 +234,54 @@ private boolean readPropOrTrue(String prop) {
return property == null ? true : Boolean.valueOf(property);
}
- protected ConsoleReader createConsoleReader(InputStream systemIn) throws
IOException {
- // need to swap out the output to avoid logging during init
- final PrintStream nullOutput = new PrintStream(new
ByteArrayOutputStream());
- final PrintStream originalOut = Log.getOutput();
- try {
- Log.setOutput(nullOutput);
- ConsoleReader consoleReader = new ConsoleReader(systemIn, out);
- consoleReader.setExpandEvents(false);
- return consoleReader;
- } finally {
- Log.setOutput(originalOut);
+ protected LineReader createLineReader(Terminal terminal, History history)
throws IOException {
+ LineReaderBuilder builder = LineReaderBuilder.builder()
+ .terminal(terminal)
+ .option(LineReader.Option.DISABLE_EVENT_EXPANSION, true);
+ if (history != null) {
+ builder.variable(LineReader.HISTORY_FILE, new
File(System.getProperty("user.home"), HISTORYFILE).toPath());
+ builder.history(history);
}
+ return builder.build();
}
/**
- * Creates the instance of Terminal used directly in GrailsConsole. Note
that there is also
- * another terminal instance created implicitly inside of ConsoleReader.
That instance
- * is controlled by the jline.terminal system property.
+ * Creates the instance of Terminal used directly in GrailsConsole.
*/
- protected Terminal createTerminal() {
- terminal = TerminalFactory.create();
- if (isWindows()) {
- terminal.setEchoEnabled(true);
- }
+ protected Terminal createTerminal() throws IOException {
+ Terminal terminal = TerminalBuilder.builder()
+ .system(true)
+ .build();
return terminal;
}
public void resetCompleters() {
- final ConsoleReader reader = getReader();
- if (reader != null) {
- Collection<Completer> completers = reader.getCompleters();
- for (Completer completer : completers) {
- reader.removeCompleter(completer);
- }
+ // In JLine 3, completers are set at LineReader creation time or via
setCompleter
+ // We'll handle this differently - completers are managed via the
LineReader
+ }
- // for some unknown reason / bug in JLine you have to iterate over
twice to clear the completers (WTF)
- completers = reader.getCompleters();
- for (Completer completer : completers) {
- reader.removeCompleter(completer);
+ public void addCompleter(Completer completer) {
+ // In JLine 3, we need to recreate the LineReader with the new
completer
+ // or use an AggregateCompleter. For now, this is a simplified
implementation.
+ if (terminal != null) {
+ try {
+ LineReaderBuilder builder = LineReaderBuilder.builder()
+ .terminal(terminal)
+ .completer(completer)
+ .option(LineReader.Option.DISABLE_EVENT_EXPANSION,
true);
+ if (history != null) {
+ builder.variable(LineReader.HISTORY_FILE, new
File(System.getProperty("user.home"), HISTORYFILE).toPath());
+ builder.history(history);
+ }
+ reader = builder.build();
+ } catch (Exception e) {
+ // ignore
}
}
}
Review Comment:
The addCompleter method recreates the entire LineReader each time it's
called, which means only the last completer added will be active. This is a
critical bug because multiple completers are added in GrailsCli (lines 252-253
and 409), but only the last one will work. In JLine 3, you should either:
1. Collect all completers first and then create a single AggregateCompleter
when building the LineReader, or
2. Use an AggregateCompleter that can have completers added to it, and set
it once on the LineReader.
The current implementation will break tab completion for all but the last
completer added.
##########
grails-bootstrap/src/main/groovy/grails/build/logging/GrailsConsole.java:
##########
@@ -875,10 +859,15 @@ private String readLine(String prompt, boolean secure) {
assertAllowInput(prompt);
userInputActive = true;
try {
- Character inputMask = secure ? SECURE_MASK_CHAR : defaultInputMask;
- return reader.readLine(prompt, inputMask);
- } catch (IOException e) {
- throw new RuntimeException("Error reading input: " +
e.getMessage());
+ if (secure) {
+ return reader.readLine(prompt, SECURE_MASK_CHAR);
+ } else {
+ return reader.readLine(prompt, defaultInputMask, (String)
null);
Review Comment:
The method signature for LineReader.readLine in JLine 3 may not match what's
being called here. In JLine 3, readLine typically has signatures like:
- `readLine(String prompt)`
- `readLine(String prompt, Character mask)`
- `readLine(String prompt, String rightPrompt, Character mask, String
buffer)`
Line 865 appears to pass three parameters where the third is a String null
cast, which doesn't match the standard JLine 3 API. This should likely be:
- `reader.readLine(prompt)` when defaultInputMask is null, or
- `reader.readLine(prompt, defaultInputMask)` when defaultInputMask is not
null
Please verify this matches the actual JLine 3 LineReader API.
```suggestion
} else if (defaultInputMask == null) {
return reader.readLine(prompt);
} else {
return reader.readLine(prompt, defaultInputMask);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
