Copilot commented on code in PR #15367:
URL: https://github.com/apache/grails-core/pull/15367#discussion_r2748691423
##########
grails-bootstrap/src/main/groovy/grails/build/logging/GrailsConsole.java:
##########
@@ -251,51 +241,64 @@ 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);
}
+ LineReader lineReader = builder.build();
+ if (history != null) {
+ history.attach(lineReader);
+ history.load();
+ }
Review Comment:
The history setup appears to be redundant. The builder is configured with
both `HISTORY_FILE` variable (line 249) and the history object (line 250), then
after building the LineReader, the code manually attaches and loads the history
(lines 254-255).
In JLine 3, when you provide a history object to the builder via
`.history(history)`, you typically don't need to manually call
`history.attach(lineReader)` and `history.load()` afterwards - the builder
handles this. The manual attach/load could potentially cause the history to be
loaded twice or cause other issues.
Consider removing the manual attach/load calls (lines 254-255) and let the
LineReaderBuilder handle the history setup, or verify that this double-setup is
intentional and necessary for the specific use case.
```suggestion
```
##########
grails-shell-cli/src/main/groovy/org/grails/cli/GrailsCli.groovy:
##########
@@ -434,47 +432,66 @@ class GrailsCli {
// CTRL-D was pressed, exit interactive mode
exitInteractiveMode()
} else if (commandLine.trim()) {
- if (nonBlockingInput.isNonBlockingEnabled()) {
- handleCommandWithCancellationSupport(console,
commandLine, commandExecutor, nonBlockingInput)
- } else {
- handleCommand(cliParser.parseString(commandLine))
- }
+ handleCommandWithCancellationSupport(console, commandLine,
commandExecutor)
}
} catch (BuildCancelledException cancelledException) {
console.updateStatus('Build stopped.')
} catch (UserInterruptException e) {
exitInteractiveMode()
+ } catch (EndOfFileException e) {
+ exitInteractiveMode()
} catch (Throwable e) {
console.error("Caught exception ${e.message}", e)
}
}
}
- private Boolean handleCommandWithCancellationSupport(GrailsConsole
console, String commandLine, ExecutorService commandExecutor,
NonBlockingInputStream nonBlockingInput) {
+ private Boolean handleCommandWithCancellationSupport(GrailsConsole
console, String commandLine, ExecutorService commandExecutor) {
ExecutionContext executionContext =
createExecutionContext(cliParser.parseString(commandLine))
Future<?> commandFuture = commandExecutor.submit({
handleCommand(executionContext) } as Callable<Boolean>)
- def terminal = console.reader.terminal
- if (terminal instanceof UnixTerminal) {
- ((UnixTerminal) terminal).disableInterruptCharacter()
- }
+ NonBlockingReader reader = console?.terminal?.reader()
+ boolean maskedInterruptCharacter = false
+ int originalInterruptChar = -1
try {
+ if (console?.terminal) {
+ def attributes = console.terminal.getAttributes()
+ originalInterruptChar =
attributes.getControlChar(org.jline.terminal.Attributes.ControlChar.VINTR)
+ if (originalInterruptChar != 0) {
+
attributes.setControlChar(org.jline.terminal.Attributes.ControlChar.VINTR, 0)
+ console.terminal.setAttributes(attributes)
+ maskedInterruptCharacter = true
+ }
+ }
while (!commandFuture.done) {
- if (nonBlockingInput.nonBlockingEnabled) {
- int peeked = nonBlockingInput.peek(100L)
+ if (reader != null) {
+ int peeked = reader.peek(100L)
if (peeked > 0) {
- // read peeked character from buffer
- nonBlockingInput.read(1L)
if (peeked == KEYPRESS_CTRL_C || peeked ==
KEYPRESS_ESC) {
+ reader.read()
executionContext.console.log(' ')
executionContext.console.updateStatus('Stopping
build. Please wait...')
executionContext.cancel()
+ } else {
Review Comment:
The CTRL+C handling logic consumes all keyboard input during command
execution (line 475), not just CTRL+C and ESC keypresses. This means any keys
the user types while a command is running will be silently discarded.
While this might be intentional to prevent input buffering during command
execution, it could be confusing for users who expect their input to be queued.
Consider either:
1. Buffering non-control keypresses for after command completion
2. Providing visual feedback that input is being ignored during execution
3. Adding a comment explaining why all input is consumed
This is a UX consideration rather than a bug, but worth documenting the
intended behavior.
```suggestion
} else {
// Intentionally consume all other keypresses
while a command is running.
// This avoids buffering input that would
otherwise be processed only after
// the current command completes, which could be
confusing in interactive use.
```
--
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]
