Copilot commented on code in PR #15367:
URL: https://github.com/apache/grails-core/pull/15367#discussion_r2748506166
##########
grails-bootstrap/src/main/groovy/grails/build/logging/GrailsConsole.java:
##########
@@ -251,51 +241,50 @@ 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) throws
IOException {
+ LineReader lineReader = LineReaderBuilder.builder()
+ .terminal(terminal)
+ .option(LineReader.Option.DISABLE_EVENT_EXPANSION, true)
+ .build();
+ return lineReader;
}
/**
- * 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 {
+ reader = LineReaderBuilder.builder()
+ .terminal(terminal)
+ .completer(completer)
+ .option(LineReader.Option.DISABLE_EVENT_EXPANSION,
true)
+ .build();
+ if (history != null) {
+ reader.setVariable(LineReader.HISTORY_FILE, new
File(System.getProperty("user.home"), HISTORYFILE).toPath());
+ }
Review Comment:
The `addCompleter` method recreates the LineReader every time it's called,
but doesn't properly transfer history state. When the reader is recreated, the
history should be attached via `.history(history)` in the LineReaderBuilder,
and the history file path should also be set. Currently, only the HISTORY_FILE
variable is being set, which may not properly maintain history state across
reader recreations.
##########
grails-shell-cli/src/main/groovy/org/grails/cli/profile/commands/CreateAppCommand.groovy:
##########
@@ -119,37 +119,37 @@ class CreateAppCommand extends ArgumentCompletingCommand
implements ProfileRepos
}.collect() { String pn ->
"${pn.substring(valStr.size())} ".toString()
}
- candidates.addAll(candidateProfiles)
- return cursor
+ candidateProfiles.each { candidates.add(new
org.jline.reader.Candidate(it)) }
+ return
}
} else if (lastOption.key == FEATURES_FLAG) {
def val = lastOption.value
def profile =
profileRepository.getProfile(commandLine.hasOption(PROFILE_FLAG) ?
commandLine.optionValue(PROFILE_FLAG).toString() : getDefaultProfile())
def featureNames = profile.features.collect() { Feature f ->
f.name }
if (val == true) {
- candidates.addAll(featureNames)
- return cursor
+ featureNames.each { candidates.add(new
org.jline.reader.Candidate(it)) }
+ return
} else if (!profileNames.contains(val)) {
def valStr = val.toString()
if (valStr.endsWith(',')) {
def specified = valStr.split(',')
- candidates.addAll(featureNames.findAll { String f ->
+ featureNames.findAll { String f ->
!specified.contains(f)
- })
- return cursor
+ }.each { candidates.add(new
org.jline.reader.Candidate(it)) }
+ return
}
def candidatesFeatures = featureNames.findAll { String pn
->
pn.startsWith(valStr)
}.collect() { String pn ->
"${pn.substring(valStr.size())} ".toString()
}
- candidates.addAll(candidatesFeatures)
- return cursor
+ candidatesFeatures.each { candidates.add(new
org.jline.reader.Candidate(it)) }
+ return
Review Comment:
Same issue as with profile names: feature completion candidates should
contain the full feature name, not just the suffix. The candidates should be
created with the full feature name: `new org.jline.reader.Candidate(pn)`
instead of `new org.jline.reader.Candidate("${pn.substring(valStr.size())} ")`.
##########
grails-bootstrap/src/main/groovy/grails/build/logging/GrailsConsole.java:
##########
@@ -190,19 +187,12 @@ protected void initialize(InputStream systemIn,
PrintStream systemOut, PrintStre
redirectSystemOutAndErr(true);
- System.setProperty(ShutdownHooks.JLINE_SHUTDOWNHOOK, "false");
-
if (isInteractiveEnabled()) {
- reader = createConsoleReader(systemIn);
- reader.setBellEnabled(false);
- reader.setCompletionHandler(new CandidateListCompletionHandler());
- if (isActivateTerminal()) {
- terminal = createTerminal();
- }
-
+ terminal = createTerminal();
+ reader = createLineReader(terminal);
history = prepareHistory();
if (history != null) {
- reader.setHistory(history);
+ reader.setVariable(LineReader.HISTORY_FILE, new
File(System.getProperty("user.home"), HISTORYFILE).toPath());
Review Comment:
History is not being properly attached to the LineReader. In JLine 3,
history should be set via `.history(history)` in the LineReaderBuilder, not by
setting `HISTORY_FILE` variable. The current implementation creates a
`DefaultHistory` instance but never loads it from the history file or attaches
it to the reader. The history file path should be set using
`.variable(LineReader.HISTORY_FILE, path)` on the builder, and the history
instance should be attached using `.history(history)` on the builder. Without
properly loading and attaching the history, command history will not persist
across sessions.
```suggestion
Path historyPath = new File(System.getProperty("user.home"),
HISTORYFILE).toPath();
reader.setVariable(LineReader.HISTORY_FILE, historyPath);
if (history instanceof DefaultHistory) {
DefaultHistory defaultHistory = (DefaultHistory) history;
defaultHistory.attach(reader);
defaultHistory.load();
} else {
reader.setHistory(history);
}
```
##########
grails-bootstrap/src/main/groovy/grails/build/logging/GrailsConsole.java:
##########
@@ -34,23 +33,21 @@
import org.codehaus.groovy.runtime.StackTraceUtils;
import org.codehaus.groovy.runtime.typehandling.NumberMath;
-import jline.Terminal;
-import jline.TerminalFactory;
-import jline.UnixTerminal;
-import jline.console.ConsoleReader;
-import jline.console.completer.Completer;
-import jline.console.history.FileHistory;
-import jline.console.history.History;
-import jline.internal.Log;
-import jline.internal.ShutdownHooks;
-import jline.internal.TerminalLineSettings;
+import org.jline.reader.Completer;
+import org.jline.reader.History;
+import org.jline.reader.LineReader;
+import org.jline.reader.LineReaderBuilder;
+import org.jline.reader.impl.history.DefaultHistory;
+import org.jline.terminal.Terminal;
+import org.jline.terminal.TerminalBuilder;
+import org.jline.utils.AttributedString;
+import org.jline.utils.AttributedStyle;
Review Comment:
Unused imports: `AttributedString` and `AttributedStyle` are imported but
never used in the code. These should be removed.
```suggestion
```
##########
grails-bootstrap/src/main/groovy/org/grails/build/interactive/CandidateListCompletionHandler.java:
##########
@@ -18,76 +18,62 @@
*/
package org.grails.build.interactive;
-import java.io.IOException;
import java.util.List;
-import jline.console.ConsoleReader;
-import jline.console.CursorBuffer;
-import jline.console.completer.CompletionHandler;
+import org.jline.reader.Candidate;
+import org.jline.reader.Completer;
+import org.jline.reader.LineReader;
+import org.jline.reader.ParsedLine;
/**
- * Fixes issues with the default CandidateListCompletionHandler such as
clearing out the whole buffer when
- * a completion matches a list of candidates
+ * A Completer implementation that wraps candidate list completion behavior.
+ * In JLine 3, completion handling is integrated into the LineReader itself,
+ * so this class now acts as a utility completer that can be composed with
others.
*
* @author Graeme Rocher
* @since 2.0
*/
-public class CandidateListCompletionHandler implements CompletionHandler {
+public class CandidateListCompletionHandler implements Completer {
+ private final Completer delegate;
private boolean eagerNewlines = true;
- public void setAlwaysIncludeNewline(boolean eagerNewlines) {
- this.eagerNewlines = eagerNewlines;
+ public CandidateListCompletionHandler() {
+ this.delegate = null;
}
- public boolean complete(ConsoleReader reader,
@SuppressWarnings("rawtypes") List<CharSequence> candidates, int pos) throws
IOException {
- CursorBuffer buf = reader.getCursorBuffer();
-
- // if there is only one completion, then fill in the buffer
- if (candidates.size() == 1) {
- String value = candidates.get(0).toString();
-
- // fail if the only candidate is the same as the current buffer
- if (value.equals(buf.toString())) {
- return false;
- }
-
-
jline.console.completer.CandidateListCompletionHandler.setBuffer(reader, value,
pos);
-
- return true;
- }
-
- if (candidates.size() > 1) {
- String value = getUnambiguousCompletions(candidates);
+ public CandidateListCompletionHandler(Completer delegate) {
+ this.delegate = delegate;
+ }
-
jline.console.completer.CandidateListCompletionHandler.setBuffer(reader, value,
pos);
- }
+ public void setAlwaysIncludeNewline(boolean eagerNewlines) {
+ this.eagerNewlines = eagerNewlines;
+ }
Review Comment:
The `eagerNewlines` field is set via `setAlwaysIncludeNewline` method but is
never used in the implementation. In JLine 3, completion handling behavior
(including newlines) is controlled by the LineReader's configuration, not by
individual completers. This field and its setter method serve no purpose and
could be removed, or the class could be redesigned if custom completion
rendering is needed.
##########
grails-bootstrap/src/main/groovy/grails/build/logging/GrailsConsole.java:
##########
@@ -674,7 +661,7 @@ private void logSimpleError(String msg) {
}
public boolean isAnsiEnabled() {
- return Ansi.isEnabled() && (terminal != null &&
terminal.isAnsiSupported()) && ansiEnabled;
+ return Ansi.isEnabled() && (terminal != null && terminal.getType() !=
Terminal.TYPE_DUMB) && ansiEnabled;
Review Comment:
The constant `Terminal.TYPE_DUMB` does not exist in JLine 3. In JLine 3,
terminal types are identified differently. The correct way to check for a dumb
terminal is to use `terminal.getType().equals("dumb")` or check if the terminal
class is `DumbTerminal`. This will cause a compilation error.
```suggestion
return Ansi.isEnabled() && (terminal != null &&
!"dumb".equals(terminal.getType())) && ansiEnabled;
```
##########
grails-shell-cli/src/main/groovy/org/grails/cli/profile/commands/ArgumentCompletingCommand.groovy:
##########
@@ -54,15 +57,14 @@ abstract class ArgumentCompletingCommand implements
Command, Completer {
if (lastOption) {
def lastArg = lastOption.key
if (arg.name.startsWith(lastArg)) {
- candidates.add("${argName.substring(lastArg.length())}
".toString())
+ candidates.add(new
Candidate("${argName.substring(lastArg.length())} ".toString()))
Review Comment:
Similar completion issue: when completing a partial flag name, the code
creates a candidate with only the suffix (e.g.,
`"${argName.substring(lastArg.length())} "`). In JLine 3, candidates should
contain the complete flag including the leading dash, not just the remaining
characters. The candidate should be created with the full flag: `new
Candidate("-$argName ")` or `new Candidate(flag + " ")`.
```suggestion
candidates.add(new Candidate("$flag ".toString()))
```
--
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]
