Re: RFR: 8331535: Incorrect prompt for Console.readLine [v2]
> When JLine reads a line, there may be a prompt provided. However, JLine will > not interpret the prompt literally, it will handle `%` specially. As a > consequence, doing: > > System.console().readLine("%%s"); > > > will not print `%s`, as first `String.format` is used, which will convert > `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution > is to duplicate the `%`, so that JLine will print it. Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. - Changes: - all: https://git.openjdk.org/jdk/pull/19081/files - new: https://git.openjdk.org/jdk/pull/19081/files/58cbd42e..58cbd42e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19081&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19081&range=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19081.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19081/head:pull/19081 PR: https://git.openjdk.org/jdk/pull/19081
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v2]
On Fri, 3 May 2024 11:20:52 GMT, Pavel Rappo wrote: >> Jan Lahoda has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. > > src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java > line 101: > >> 99: try { >> 100: initJLineIfNeeded(); >> 101: return jline.readLine(fmt.formatted(args).replace("%", >> "%%")); > > I understand that [JLine interprets `%` in a > prompt](https://github.com/openjdk/jdk/blob/4ed38f5ad5f822ab948257ed39717ea919fd32ed/src/jdk.internal.le/share/classes/jdk/internal/org/jline/reader/impl/LineReaderImpl.java#L4050), > but are the interpretation rules documented on JLine GitHub page or > elsewhere? There's a description in the javadoc: https://www.javadoc.io/doc/org.jline/jline/latest/org/jline/reader/LineReader.html - PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1591196087
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v2]
On Fri, 3 May 2024 21:59:40 GMT, Pavel Rappo wrote: >>> Ideally, we should have separate tests that make sure that jdk.internal.le >>> is the default impl. >> >> We have a test that checks if `System.console()` returns the correct Console >> (or null) from the expected module >> (`test/jdk/java/io/Console/ModuleSelectionTest.java`) >> >>> We should also test this behaviour (i.e. % interpretation) on >>> jdk.internal.io Console impl. >> >> Yeah, that would be helpful. > >> We have a test that checks if `System.console()` returns the correct Console >> (or null) from the expected module >> (`test/jdk/java/io/Console/ModuleSelectionTest.java`) >> > > Good; then here we should indeed specify `-Djdk.console=jdk.internal.le`. > Initially, I suggested an alternative wording (i.e. "default"), but that's > inappropriate. After all, the test in question resides in > `test/jdk/jdk/internal/jline`. So it only makes sense that it tests that > concrete implementation. > >> Yeah, that would be helpful. >> > > I filed a bug: https://bugs.openjdk.org/browse/JDK-8331681 Thanks. I've added `-Djdk.console=jdk.internal.le`, and also added a test for the `java.base`'s console: `test/jdk/java/io/Console/ConsolePromptTest.java`. I can easily remove the latter if this is not a reasonable form. Or I can fix it as needed. Please let me know. Thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1591228737
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v2]
On Mon, 6 May 2024 16:05:58 GMT, Jan Lahoda wrote: >>> We have a test that checks if `System.console()` returns the correct >>> Console (or null) from the expected module >>> (`test/jdk/java/io/Console/ModuleSelectionTest.java`) >>> >> >> Good; then here we should indeed specify `-Djdk.console=jdk.internal.le`. >> Initially, I suggested an alternative wording (i.e. "default"), but that's >> inappropriate. After all, the test in question resides in >> `test/jdk/jdk/internal/jline`. So it only makes sense that it tests that >> concrete implementation. >> >>> Yeah, that would be helpful. >>> >> >> I filed a bug: https://bugs.openjdk.org/browse/JDK-8331681 > > Thanks. I've added `-Djdk.console=jdk.internal.le`, and also added a test for > the `java.base`'s console: `test/jdk/java/io/Console/ConsolePromptTest.java`. > I can easily remove the latter if this is not a reasonable form. Or I can fix > it as needed. Please let me know. > > Thanks. For the latter, since you are already at it, I just reassigned the bug to you 🙂 - PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1591302360
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v2]
On Mon, 6 May 2024 16:59:40 GMT, Naoto Sato wrote: > For the latter, since you are already at it, I just reassigned the bug to you > 🙂 @lahodaj, since a test for 8331681 is added to this PR, I'd suggest you do this: `/issue add 8331681`. - PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1591423527