On Fri, 16 May 2025 18:11:39 GMT, Naoto Sato <na...@openjdk.org> wrote:
> `java.io.Console` uses the charset specified by the `stdout.encoding` system
> property for both input and output. While this is generally sufficient, since
> Console is intended for interactive terminal use, some platforms allow
> different encodings to be configured for input and output. In such cases,
> using a single encoding may lead to incorrect behavior when reading from the
> terminal. To address this, the newly introduced system property,
> `stdin.encoding`, should be used specifically for input where appropriate.
src/java.base/share/classes/java/io/Console.java line 560:
> 558: static final Charset STDIN_CHARSET =
> 559: Charset.forName(System.getProperty("stdin.encoding"),
> UTF_8.INSTANCE);
> 560: static final Charset STDOUT_CHARSET =
I guess these can be marked `private`?
Suggestion:
private static final Charset STDIN_CHARSET =
Charset.forName(System.getProperty("stdin.encoding"), UTF_8.INSTANCE);
private static final Charset STDOUT_CHARSET =
src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
line 160:
> 158:
> 159: try {
> 160: Terminal terminal =
> TerminalBuilder.builder().encoding(outCharset)
Shouldn't ideally `JdkConsole::charset` and `Terminal::encoding` be adapted for
stdin/stdout variants?
test/jdk/java/io/Console/CharsetTest.java line 1:
> 1: /*
Copyright year update is missing.
test/jdk/java/io/Console/StdinEncodingTest.java line 46:
> 44: * @run junit StdinEncodingTest
> 45: */
> 46: public class StdinEncodingTest {
AFAICT, there is no similar test (e.g., one using a mock `CharsetProvider`) for
`stdout.encoding`. Will it be addressed by another ticket? Shall we consider
adding a similar `StdoutEncodingTest` too? (Not necessarily in this PR.)
test/jdk/java/io/Console/StdinEncodingTest.java line 49:
> 47:
> 48: @Test
> 49: @EnabledOnOs({OS.LINUX, OS.MAC})
Shall we replace `@EnabledOnOs` with the `@requires (os.family == "linux" |
os.family == "mac")` JTreg directive instead per
[JDK-8211673](https://bugs.openjdk.org/browse/JDK-8211673)?
test/jdk/java/io/Console/csp/provider/MockCharsetProvider.java line 36:
> 34:
> 35: // A test charset provider that decodes every input byte into its
> uppercase
> 36: public class MockCharsetProvider extends CharsetProvider {
*Nit:* Maybe a more self-explanatory name, e.g., `UpperCasingCharsetProvider`?
test/jdk/java/io/Console/csp/provider/MockCharsetProvider.java line 83:
> 81: while (in.remaining() > 0) {
> 82: char c = (char)in.get();
> 83: if (c != '\n') {
`Character.toUpperCase('\n') == '\n'`, not? If so, do we still need this
`if`-branching?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25271#discussion_r2097641573
PR Review Comment: https://git.openjdk.org/jdk/pull/25271#discussion_r2097630717
PR Review Comment: https://git.openjdk.org/jdk/pull/25271#discussion_r2097578092
PR Review Comment: https://git.openjdk.org/jdk/pull/25271#discussion_r2097718253
PR Review Comment: https://git.openjdk.org/jdk/pull/25271#discussion_r2097671831
PR Review Comment: https://git.openjdk.org/jdk/pull/25271#discussion_r2097711653
PR Review Comment: https://git.openjdk.org/jdk/pull/25271#discussion_r2097704883
