On Wed, 16 Apr 2025 12:07:45 GMT, Jan Lahoda <[email protected]> wrote:
>> The `java.io.Console` has several backends: a simple on in `java.base`, a
>> more convenient one in `jdk.internal.le` (with line-reading based on JLine)
>> and one for JShell.
>>
>> The backend based on JLine is proving to be a somewhat problematic - JLine
>> is very powerful, possibly too powerful and complex for the simple task of
>> editing a line with no completion, no history, no variables, no commands,
>> etc. As a consequence, there are inevitable sharp edges in this backend.
>>
>> The idea in this PR is to replace the use of JLine in the `jdk.internal.le`
>> backend with a simple escape code interpreter, that only handles a handful
>> of keys/codes (left/right arrow, home, end, delete, backspace, enter), and
>> ignores the rest. The goal is to have something simple with less surprising
>> behavior.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reflecting review feedback: Adding makefile comment as suggested
src/java.base/share/classes/jdk/internal/io/BaseJdkConsoleImpl.java line 175:
> 173: protected final Charset charset;
> 174: protected final Object readLock;
> 175: protected final Object writeLock;
Recommend mentioning that when both locks must be acquired, the `writeLock`
must be acquired first. Unfortunately we cannot just use a
`ReentrantReadWriteLock`.
src/java.base/share/classes/jdk/internal/io/BaseJdkConsoleImpl.java line 181:
> 179: protected final Formatter formatter;
> 180:
> 181: protected abstract char[] readline(boolean password) throws
> IOException;
I recommend a more distinct method name, like `nextLine` or `implReadLine`;
using `readline` to distinguish from `readLine` is weird.
src/java.base/share/classes/jdk/internal/io/BaseJdkConsoleImpl.java line 184:
> 182:
> 183: @SuppressWarnings("this-escape")
> 184: public BaseJdkConsoleImpl(Charset charset) {
Suggestion:
protected BaseJdkConsoleImpl(Charset charset) {
src/jdk.internal.le/share/classes/jdk/internal/console/JdkConsoleImpl.java line
38:
> 36: * JdkConsole implementation based on the platform's TTY, with basic
> keyboard navigation.
> 37: */
> 38: public final class JdkConsoleImpl extends BaseJdkConsoleImpl {
We should choose a different simple name for this class, especially when we are
importing classes from `jdk.internal.io` package.
src/jdk.internal.le/share/classes/jdk/internal/console/JdkConsoleImpl.java line
57:
> 55: }
> 56:
> 57: protected char[] readline(boolean password) throws IOException {
`@Override`
src/jdk.internal.le/share/classes/jdk/internal/console/LastErrorException.java
line 28:
> 26:
> 27: @SuppressWarnings("serial")
> 28: public class LastErrorException extends RuntimeException{
Suggestion:
public class LastErrorException extends RuntimeException {
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24242#discussion_r2055177151
PR Review Comment: https://git.openjdk.org/jdk/pull/24242#discussion_r2055179038
PR Review Comment: https://git.openjdk.org/jdk/pull/24242#discussion_r2055177261
PR Review Comment: https://git.openjdk.org/jdk/pull/24242#discussion_r2055183690
PR Review Comment: https://git.openjdk.org/jdk/pull/24242#discussion_r2055181215
PR Review Comment: https://git.openjdk.org/jdk/pull/24242#discussion_r2055181573
