Re: RFR: 8301991: Convert l10n properties resource bundles to UTF-8 native [v2]
On Tue, 20 Aug 2024 09:07:54 GMT, Pavel Rappo wrote: >> Justin Lu has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Replace InputStreamReader with BufferedReader > > src/jdk.jartool/share/classes/sun/tools/jar/resources/jar_pt_BR.properties > line 95: > >> 93: >> 94: main.usage.summary=Uso: jar [OPTION...] [ [--release VERSION] [-C dir] >> files] ... >> 95: main.usage.summary.try=Tente `jar --ajuda' para obter mais informações. > > I was looking for something unrelated in properties files, and found this. It > is surprising to see an option name being localised; it must be a bug. Good catch, Pavel. It is indeed a bug. This type of overtranslation l10n bug happens all the time, and hard to catch. - PR Review Comment: https://git.openjdk.org/jdk/pull/15694#discussion_r1723520963
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base` [v2]
On Fri, 19 Apr 2024 19:21:13 GMT, Jonathan Gibbons wrote: >> Please review a set of updates to clean up use of `/**` comments in the >> vicinity of declarations. >> >> There are various categories of update: >> >> * "Box comments" beginning with `/**` >> * Misplaced doc comments before package or import statements >> * Misplaced doc comments after the annotations for a declaration >> * Dangling `/**` comments relating to a group of declarations, separate from >> the doc comments for each of those declarations >> * Use of `/**` for the legal header at or near the top of the file >> >> In one case, two doc comments before a declaration were merged, which fixes >> a bug/omission in the documented serialized form. > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > Update > src/java.base/share/classes/sun/net/www/protocol/file/FileURLConnection.java > > Fix grammatical typo > > Co-authored-by: Alexey Ivanov OK, fair enough. Approving for the `icu` part - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18846#pullrequestreview-2012372872
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base` [v2]
On Fri, 19 Apr 2024 19:21:13 GMT, Jonathan Gibbons wrote: >> Please review a set of updates to clean up use of `/**` comments in the >> vicinity of declarations. >> >> There are various categories of update: >> >> * "Box comments" beginning with `/**` >> * Misplaced doc comments before package or import statements >> * Misplaced doc comments after the annotations for a declaration >> * Dangling `/**` comments relating to a group of declarations, separate from >> the doc comments for each of those declarations >> * Use of `/**` for the legal header at or near the top of the file >> >> In one case, two doc comments before a declaration were merged, which fixes >> a bug/omission in the documented serialized form. > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > Update > src/java.base/share/classes/sun/net/www/protocol/file/FileURLConnection.java > > Fix grammatical typo > > Co-authored-by: Alexey Ivanov Unless it is absolutely necessary, I would not fix comments in `jdk.internal.icu` sources, as they are in the upstream code, and would like to minimize the merging effort. - PR Comment: https://git.openjdk.org/jdk/pull/18846#issuecomment-2067190364
Re: RFR: 8325950: Make sure all files in the JDK pass jcheck [v2]
On Fri, 16 Feb 2024 12:43:25 GMT, Magnus Ihse Bursie wrote: >> Since jcheck only checks file in a commit, there is a possibility of us >> getting files in the repository that would not be accepted by jcheck. This >> can happen when extending the set of files checked by jcheck, or if jcheck >> changes how it checks files (perhaps due to bug fixes). >> >> I have now run jcheck over the entire code base, and fixed a handful of >> issues. With these changes, jcheck accept the entire code base. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Replace spaces with \t in test properties file LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17871#pullrequestreview-1885586969
Re: RFR: 8325950: Make sure all files in the JDK pass jcheck
On Thu, 15 Feb 2024 17:28:52 GMT, Andy Goryachev wrote: >> Please do not replace those tabs with spaces as they are intentional for >> testing the runtime to safely ignore them. I suggest replacing them with >> Unicode escapes (`\u000b`) > > `\u000b` is VT (vertical tab) > `\u0009` or `\t` perhaps? Right. `\t` is better to avoid such a mistake. - PR Review Comment: https://git.openjdk.org/jdk/pull/17871#discussion_r1491403426
Re: RFR: 8325950: Make sure all files in the JDK pass jcheck
On Thu, 15 Feb 2024 15:48:38 GMT, Magnus Ihse Bursie wrote: >> This looks weird indeed. Luckily it's just test code. Did you run the test >> after this change? > > All the java/util/Currency tests pass. I also searched the code for "ZZ" but > could not find any references in the test. Please do not replace those tabs with spaces as they are intentional for testing the runtime to safely ignore them. I suggest replacing them with Unicode escapes (`\u000b`) - PR Review Comment: https://git.openjdk.org/jdk/pull/17871#discussion_r1491352359
Re: RFR: 8325558: Add jcheck whitespace checking for properties files
On Fri, 9 Feb 2024 13:35:55 GMT, Magnus Ihse Bursie wrote: > This is an attempt to finally implement the idea brought forward in > JDK-8295729: Properties files is essentially source code. It should have the > same whitespace checks as all other source code, so we don't get spurious > trailing whitespace changes or leading tabs instead of spaces. > > With Skara jcheck, it is possible to increase the coverage of the whitespace > checks. > > However, this turned out to be problematic, since trailing whitespace is > significant in properties files. That issue has mostly been sorted out in a > series of PRs, and this patch will finish the job with the few remaining > files, and actually enable the check in jcheck. > @naotoj Thanks! Would you care to also submit a review? My bad. I thought I approved this PR. - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17789#pullrequestreview-1875811619
Re: RFR: 8325558: Add jcheck whitespace checking for properties files
On Fri, 9 Feb 2024 13:35:55 GMT, Magnus Ihse Bursie wrote: > This is an attempt to finally implement the idea brought forward in > JDK-8295729: Properties files is essentially source code. It should have the > same whitespace checks as all other source code, so we don't get spurious > trailing whitespace changes or leading tabs instead of spaces. > > With Skara jcheck, it is possible to increase the coverage of the whitespace > checks. > > However, this turned out to be problematic, since trailing whitespace is > significant in properties files. That issue has mostly been sorted out in a > series of PRs, and this patch will finish the job with the few remaining > files, and actually enable the check in jcheck. Skimmed through the changes and all look good to me. Good to have `jcheck` detect those unneeded trailing spaces. - PR Review: https://git.openjdk.org/jdk/pull/17789#pullrequestreview-1872951198
Re: RFR: JDK-8325189: Enable this-escape javac warning in java.base
On Fri, 2 Feb 2024 23:36:41 GMT, Joe Darcy wrote: > After the "this-escape" lint warning was added to javac (JDK-8015831), the > base module was not updated to be able to compile with this warning enabled. > This PR makes the necessary changes to allow the base module to build with > the warning enabled. Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17692#pullrequestreview-186335
Re: RFR: 8301991: Convert l10n properties resource bundles to UTF-8 native [v2]
On Wed, 13 Sep 2023 17:38:28 GMT, Justin Lu wrote: >> JDK .properties files still use ISO-8859-1 encoding with escape sequences. >> It would improve readability to see the native characters instead of escape >> sequences (especially for the L10n process). The majority of files changed >> are localized resource files. >> >> This change converts the Unicode escape sequences in the JDK .properties >> files (both in src and test) to UTF-8 native characters. Additionally, the >> build logic is adjusted to read the .properties files in UTF-8 while >> generating the ListResourceBundle files. >> >> The only escape sequence not converted was `\u0020` as this is used to >> denote intentional trailing white space. (E.g. `key=This is the >> value:\u0020`) >> >> The conversion was done using native2ascii with options `-reverse -encoding >> UTF-8`. >> >> If this PR is integrated, the IDE default encoding for .properties files >> need to be updated to UTF-8. (IntelliJ IDEA locks .properties files as >> ISO-8859-1 unless manually changed). > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Replace InputStreamReader with BufferedReader Looks good to me, although I did not look at each l10n file, but sampled some. Thanks for tackling this conversion. - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15694#pullrequestreview-1625154951
Re: RFR: 8311943: Cleanup usages of toLowerCase() and toUpperCase() in java.base [v3]
On Tue, 15 Aug 2023 10:48:29 GMT, Glavo wrote: >> Clean up misuses of `toLowerCase()`/`toUpperCase()` in java.base. > > Glavo has updated the pull request with a new target base due to a merge or a > rebase. The pull request now contains four commits: > > - Merge remote-tracking branch 'origin/master' into case-conversion-java-base > - Revert StreamTokenizer.java > - Update DateTimeFormatterBuilder > - Avoid locale-sensitive case conversions in java.base LGTM. Thanks for the changes. - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14763#pullrequestreview-1581093609
Re: RFR: 8311122: Fix typos in java.base
On Thu, 29 Jun 2023 21:17:49 GMT, Pavel Rappo wrote: > Please review this IDE-assisted typo hunt, which I plan to backport to jdk21. LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14718#pullrequestreview-1506183994
Re: RFR: 8309632: JDK 21 RDP1 L10n resource files update [v2]
On Fri, 16 Jun 2023 04:39:42 GMT, Justin Lu wrote: >> Please review this PR which updates the JDK's localized resources since the >> previous L10n translation drop (1/26). >> >> To help with reviewing the changes, @jonathan-gibbons created a tool which >> displays the localized changes next to the original file's changes in UTF-8 >> native. Those files can be viewed >> [here](https://cr.openjdk.org/~jlu/output/) >> >> For example, >> >> > src="https://github.com/openjdk/jdk21/assets/67398801/e68a48a8-916e-4860-afb0-526924c6a3d3";> >> >> >> Please note that the HTML files only apply to .properties, and not .java >> resources. > > Justin Lu has updated the pull request incrementally with five additional > commits since the last revision: > > - Review: correct previous commit, remove 标 as well > - Review: Remove 本机 from 'javac.opt.Xlint.desc.output-file-clash' for zh_CN > - Review: Use old 'err.crash' zh_CN value > - Review: use Naoto's recommended ja values > - Revert 'main.help.opt.any.file' value for _de Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk21/pull/11#pullrequestreview-1484380340
Re: RFR: 8309632: JDK 21 RDP1 L10n resource files update [v2]
On Fri, 16 Jun 2023 15:06:30 GMT, Jonathan Gibbons wrote: >> Left some comments on the translations mainly in Japanese. It is now very >> easy to look at the l10n changes in the generated HTML. One small comment to >> the tool is that it would be nice if the order in HTML (alphabetically >> sorted currently) aligns with the order in the actual resource file. It is a >> great improvement anyways. > >> Left some comments on the translations mainly in Japanese. It is now very >> easy to look at the l10n changes in the generated HTML. One small comment to >> the tool is that it would be nice if the order in HTML (alphabetically >> sorted currently) aligns with the order in the actual resource file. It is a >> great improvement anyways. > > 1. It is not easy to recover the order in the actual resource file: the > contents are read into a `Properties` object which uses a `Hashtable` to > store the data. > 2. There is no guarantee that the order is the same in all the resource > files, so we would have to choose one, presumably the default one, to > determine the presentation order. > 3. The iteration order would be undefined when resources are removed from one > or more resource files Makes sense. Thanks for the explanation @jonathan-gibbons - PR Comment: https://git.openjdk.org/jdk/pull/14430#issuecomment-1594999086
Re: RFR: 8309632: JDK 21 RDP1 L10n resource files update [v4]
On Tue, 13 Jun 2023 22:09:40 GMT, Justin Lu wrote: >> Please review this PR which updates the JDK's localized resources since the >> previous L10n translation drop (1/26). >> >> To help with reviewing the changes, @jonathan-gibbons created a tool which >> displays the localized changes next to the original file's changes in UTF-8 >> native. Those files can be viewed >> [here](https://cr.openjdk.org/~jlu/output/) >> >> For example, >> >> > src="https://github.com/openjdk/jdk21/assets/67398801/e68a48a8-916e-4860-afb0-526924c6a3d3";> >> >> >> Please note that the HTML files only apply to .properties, and not .java >> resources. > > Justin Lu has updated the pull request incrementally with three additional > commits since the last revision: > > - Review: correct previous commit, remove 标 as well > - Review: Remove 本机 from 'javac.opt.Xlint.desc.output-file-clash' for zh_CN > - Review: Use old 'err.crash' zh_CN value Looks good, Justin - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14430#pullrequestreview-1478236336
Re: RFR: 8309632: JDK 21 RDP1 L10n resource files update [v2]
On Mon, 12 Jun 2023 22:32:14 GMT, Justin Lu wrote:
>> Please review this PR which updates the JDK's localized resources since the
>> previous L10n translation drop (1/26).
>>
>> To help with reviewing the changes, @jonathan-gibbons created a tool which
>> displays the localized changes next to the original file's changes in UTF-8
>> native. Those files can be viewed
>> [here](https://cr.openjdk.org/~jlu/output/)
>>
>> For example,
>>
>> > src="https://github.com/openjdk/jdk21/assets/67398801/e68a48a8-916e-4860-afb0-526924c6a3d3";>
>>
>>
>> Please note that the HTML files only apply to .properties, and not .java
>> resources.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Revert 'main.help.opt.any.file' value for _de
Left some suggestions. Should be looked at by translators later.
src/java.desktop/macosx/classes/com/apple/laf/resources/aqua_ja.properties line
63:
> 61:
> FileChooser.newFolderExistsError.textAndMnemonic=\u305D\u306E\u540D\u524D\u306F\u3059\u3067\u306B\u4F7F\u7528\u3055\u308C\u3066\u3044\u307E\u3059
> 62: FileChooser.chooseButton.textAndMnemonic=\u9078\u629E
> 63:
> FileChooser.chooseButtonToolTip.textAndMnemonic=\u9078\u629E\u3057\u305F\u30D5\u30A1\u30A4\u30EB\u3092\u8FFD\u52A0\u3057\u307E\u3059
Suggestion:
FileChooser.chooseButtonToolTip.textAndMnemonic=\u9078\u629e\u3055\u308c\u3066\u3044\u308b\u30d5\u30a1\u30a4\u30eb\u3092\u9078\u629e\u3057\u307e\u3059
src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler_ja.properties
line 502:
> 500:
> compiler.warn.forward.ref=\u521D\u671F\u5316\u3055\u308C\u308B\u524D\u306E\u5909\u6570''{0}''\u3092\u53C2\u7167\u3057\u3088\u3046\u3068\u3057\u307E\u3057\u305F
> 501:
> 502:
> compiler.warn.possible.this.escape=\u30B5\u30D6\u30AF\u30E9\u30B9\u524D\u306B\u6307\u5B9A\u53EF\u80FD\u306A''this''\u30A8\u30B9\u30B1\u30FC\u30D7\u306F\u5B8C\u5168\u306B\u521D\u671F\u5316\u3055\u308C\u307E\u3059
Suggestion:
compiler.warn.possible.this.escape=\u30b5\u30d6\u30af\u30e9\u30b9\u304c\u521d\u671f\u5316\u3055\u308c\u308b\u524d\u306e''this''\u30a8\u30b9\u30b1\u30fc\u30d7\u306e\u53ef\u80fd\u6027\u304c\u3042\u308a\u307e\u3059
src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler_ja.properties
line 504:
> 502:
> compiler.warn.possible.this.escape=\u30B5\u30D6\u30AF\u30E9\u30B9\u524D\u306B\u6307\u5B9A\u53EF\u80FD\u306A''this''\u30A8\u30B9\u30B1\u30FC\u30D7\u306F\u5B8C\u5168\u306B\u521D\u671F\u5316\u3055\u308C\u307E\u3059
> 503:
> 504:
> compiler.warn.possible.this.escape.location=\u4E8B\u524D\u306B\u6307\u5B9A\u53EF\u80FD\u306A''this''\u30A8\u30B9\u30B1\u30FC\u30D7\u306F\u547C\u51FA\u3057\u3092\u4ECB\u3057\u3066\u3053\u3053\u306B\u51FA\u73FE\u3057\u307E\u3059
Suggestion:
compiler.warn.possible.this.escape.location=\u4e8b\u524d\u306e''this''\u30a8\u30b9\u30b1\u30fc\u30d7\u306f\u3053\u306e\u547c\u3073\u51fa\u3057\u3067\u51fa\u73fe\u3057\u307e\u3059
src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler_ja.properties
line 956:
> 954:
> compiler.err.unclosed.text.block=\u9589\u3058\u3089\u308C\u3066\u3044\u306A\u3044\u30C6\u30AD\u30B9\u30C8\u30FB\u30D6\u30ED\u30C3\u30AF
> 955:
> 956:
> compiler.err.string.template.is.not.well.formed=\u6587\u5B57\u5217\u30C6\u30F3\u30D7\u30EC\u30FC\u30C8\u304C\u6574\u5F62\u5F0F\u3067\u306F\u3042\u308A\u307E\u305B\u3093
Suggestion:
compiler.err.string.template.is.not.well.formed=\u6587\u5b57\u5217\u30c6\u30f3\u30d7\u30ec\u30fc\u30c8\u304c\u9069\u683c\u3067\u306f\u3042\u308a\u307e\u305b\u3093
src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler_ja.properties
line 958:
> 956:
> compiler.err.string.template.is.not.well.formed=\u6587\u5B57\u5217\u30C6\u30F3\u30D7\u30EC\u30FC\u30C8\u304C\u6574\u5F62\u5F0F\u3067\u306F\u3042\u308A\u307E\u305B\u3093
> 957:
> 958:
> compiler.err.text.block.template.is.not.well.formed=\u30C6\u30AD\u30B9\u30C8\u30FB\u30D6\u30ED\u30C3\u30AF\u30FB\u30C6\u30F3\u30D7\u30EC\u30FC\u30C8\u304C\u6574\u5F62\u5F0F\u3067\u306F\u3042\u308A\u307E\u305B\u3093
Suggestion:
compiler.err.text.block.template.is.not.well.formed=\u30c6\u30ad\u30b9\u30c8\u30fb\u30d6\u30ed\u30c3\u30af\u30fb\u30c6\u30f3\u30d7\u30ec\u30fc\u30c8\u304c\u9069\u683c\u3067\u306f\u3042\u308a\u307e\u305b\u3093
-
Changes requested by naoto (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14430#pullrequestreview-1477963618
PR Review Comment: https://git.openjdk.org/jdk/pull/14430#discussion_r1228635514
PR Review Comment: https://git.openjdk.org/jdk/pull/14430#discussion_r1228648714
PR Review Comment: https://git.openjdk.org/jdk/pull/14430#discussion_r1228651272
PR Review Comment: https://git.openjdk.org/jdk/pull/14430#discussion_r1228653057
PR Review Comment: https://git.openjdk.org/jdk/pull/14430#discussion_r1228654384
Re: RFR: 8309632: JDK 21 RDP1 L10n resource files update [v2]
On Mon, 12 Jun 2023 22:32:14 GMT, Justin Lu wrote:
>> Please review this PR which updates the JDK's localized resources since the
>> previous L10n translation drop (1/26).
>>
>> To help with reviewing the changes, @jonathan-gibbons created a tool which
>> displays the localized changes next to the original file's changes in UTF-8
>> native. Those files can be viewed
>> [here](https://cr.openjdk.org/~jlu/output/)
>>
>> For example,
>>
>> > src="https://github.com/openjdk/jdk21/assets/67398801/e68a48a8-916e-4860-afb0-526924c6a3d3";>
>>
>>
>> Please note that the HTML files only apply to .properties, and not .java
>> resources.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Revert 'main.help.opt.any.file' value for _de
Left some comments on the translations mainly in Japanese. It is now very easy
to look at the l10n changes in the generated HTML. One small comment to the
tool is that it would be nice if the order in HTML (alphabetically sorted
currently) aligns with the order in the actual resource file. It is a great
improvement anyways.
src/java.desktop/macosx/classes/com/apple/laf/resources/aqua_ja.properties line
63:
> 61:
> FileChooser.newFolderExistsError.textAndMnemonic=\u305D\u306E\u540D\u524D\u306F\u3059\u3067\u306B\u4F7F\u7528\u3055\u308C\u3066\u3044\u307E\u3059
> 62: FileChooser.chooseButton.textAndMnemonic=\u9078\u629E
> 63:
> FileChooser.chooseButtonToolTip.textAndMnemonic=\u9078\u629E\u3057\u305F\u30D5\u30A1\u30A4\u30EB\u3092\u8FFD\u52A0\u3057\u307E\u3059
The translation seems wrong. It means to *add* the selected file, which is not
the same meaning as in English file
src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler_de.properties
line 140:
> 138:
> 139: # 0: type, 1: name
> 140: compiler.err.annotation.unrecognized.attribute.name=Annotation @{0}
> weist das unbekannte Attribut "{1}" auf
Is it intentional to replace single quotes with double quotes? It is done in
several locations in this file, but other languages keep using single quotes as
in the source English resource file.
src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler_ja.properties
line 505:
> 503:
> 504:
> compiler.warn.possible.this.escape.location=\u4E8B\u524D\u306B\u6307\u5B9A\u53EF\u80FD\u306A''this''\u30A8\u30B9\u30B1\u30FC\u30D7\u306F\u547C\u51FA\u3057\u3092\u4ECB\u3057\u3066\u3053\u3053\u306B\u51FA\u73FE\u3057\u307E\u3059
> 505:
These translations are very cryptic. Cannot understand it at first hand.
src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler_ja.properties
line 959:
> 957:
> 958:
> compiler.err.text.block.template.is.not.well.formed=\u30C6\u30AD\u30B9\u30C8\u30FB\u30D6\u30ED\u30C3\u30AF\u30FB\u30C6\u30F3\u30D7\u30EC\u30FC\u30C8\u304C\u6574\u5F62\u5F0F\u3067\u306F\u3042\u308A\u307E\u305B\u3093
> 959:
The translation of "well-formed" is incorrect. It is not 整数式
-
PR Review: https://git.openjdk.org/jdk/pull/14430#pullrequestreview-1477795175
PR Review Comment: https://git.openjdk.org/jdk/pull/14430#discussion_r1228530280
PR Review Comment: https://git.openjdk.org/jdk/pull/14430#discussion_r1228521409
PR Review Comment: https://git.openjdk.org/jdk/pull/14430#discussion_r1228538758
PR Review Comment: https://git.openjdk.org/jdk/pull/14430#discussion_r1228529769
Re: RFR: 7065228: To interpret case-insensitive string locale independently
On Tue, 16 May 2023 10:38:52 GMT, Darragh Clarke wrote: > Updated instances of `toLowerCase` and `toUpperCase` in several net and io > files to specify `Locale.ROOT` to ensure that case conversion issues don't > occur, > > I didn't add any new tests but ran tier 1-3 with no issues LGTM. Nice clean-up! - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14006#pullrequestreview-1428917297
Re: RFR: 8301991: Convert l10n properties resource bundles to UTF-8 native [v6]
On Thu, 11 May 2023 20:21:57 GMT, Justin Lu wrote: >> This PR converts Unicode sequences to UTF-8 native in .properties file. >> (Excluding the Unicode space and tab sequence). The conversion was done >> using native2ascii. >> >> In addition, the build logic is adjusted to support reading in the >> .properties files as UTF-8 during the conversion from .properties file to >> .java ListResourceBundle file. > > Justin Lu has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 16 commits: > > - Convert the merged master changes to UTF-8 > - Merge master and fix conflicts > - Close streams when finished loading into props > - Adjust CF test to read in with UTF-8 to fix failing test > - Reconvert CS.properties to UTF-8 > - Revert all changes to CurrencySymbols.properties > - Bug6204853 should not be converted > - Copyright year for CompileProperties > - Redo translation for CS.properties > - Spot convert CurrencySymbols.properties > - ... and 6 more: https://git.openjdk.org/jdk/compare/4386d42d...f15b373a I think this is fine, as those properties files are JDK's own. I believe the benefit of moving to UTF-8 outweighs the issue you wrote, which can be remedied by changing the encoding in the IDEs. - PR Comment: https://git.openjdk.org/jdk/pull/12726#issuecomment-1544722480
Re: RFR: 8304911: Use OperatingSystem enum in some modules
On Tue, 4 Apr 2023 19:22:48 GMT, Roger Riggs wrote: > With the addition of `jdk.internal.util.OperatingSystem` references to the > system property `os.name` can be replaced. > This PR exports jdk.internal.util to: > - java.prefs, > - java.security.jgss, > - java.smartcardio, > - jdk.charsets, > - jdk.net, > - jdk.zipfs LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13335#pullrequestreview-1371756979
Re: RFR: 8301991: Convert l10n properties resource bundles to UTF-8 native [v5]
On Fri, 17 Mar 2023 22:27:48 GMT, Justin Lu wrote: >> This PR converts Unicode sequences to UTF-8 native in .properties file. >> (Excluding the Unicode space and tab sequence). The conversion was done >> using native2ascii. >> >> In addition, the build logic is adjusted to support reading in the >> .properties files as UTF-8 during the conversion from .properties file to >> .java ListResourceBundle file. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Close streams when finished loading into props Hmm, I just wonder why they are sticking to ISO-8859-1 as the default. I know j.u.Properties defaults to 8859-1, but PropertyResourceBundle, which is their primary use defaults to UTF-8 since JDK9 (https://openjdk.org/jeps/226) - PR Comment: https://git.openjdk.org/jdk/pull/12726#issuecomment-1492682703
Re: RFR: JDK-8305206: Add @spec tags in java.base/java.* (part 1) [v2]
On Thu, 30 Mar 2023 20:45:08 GMT, Jonathan Gibbons wrote: >> Please review a change to add `@spec` tags (and remove some equivalent >> `@see` tags) to the main "core-libs" packages in `java.base` module. >> >> This is similar to, and a subset of, PR #11073. That PR was withdrawn, and >> based on the ensuing discussion and suggestion, is now being handled with a >> series of PRs for various separate parts of the system. Follow-up PRs will >> be provided for the rest of `java.base`, for `java.desktop`, and for XML >> APIs. The "LangTools" modules have already been updated. The "External >> Specifications" page has been temporarily [disabled][] until this work is >> complete. >> >> While the primary content of the change was automated, I've manually >> adjusted the formatting, to break long lines. >> >> It is clear there is significant inconsistency in the ordering of block tags >> in doc comment. We might want to (separately) consider normalizing the >> order of the tags, perhaps according to the order defined for the tags in >> the generated output, as given [here][] >> >> [here]: >> https://github.com/openjdk/jdk/blob/83cf28f99639d80e62c4031c4c9752460de5f36c/make/Docs.gmk#L68 >> [disabled]: >> https://github.com/openjdk/jdk/blob/83cf28f99639d80e62c4031c4c9752460de5f36c/make/Docs.gmk#L115 > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > address review feedback Changes in i18n-related classes look good to me. - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13248#pullrequestreview-1366032864
Re: RFR: 8301991: Convert l10n properties resource bundles to UTF-8 native [v4]
On Fri, 17 Mar 2023 20:31:27 GMT, Andy Goryachev wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Adjust CF test to read in with UTF-8 to fix failing test
>
> make/jdk/src/classes/build/tools/compileproperties/CompileProperties.java
> line 226:
>
>> 224: Properties p = new Properties();
>> 225: try {
>> 226: FileInputStream input = new FileInputStream(propertiesPath);
>
> Should this stream be closed in a finally { } block?
or better be `try-with-resources`?
-
PR: https://git.openjdk.org/jdk/pull/12726
Re: RFR: 8301991: Convert l10n properties resource bundles to UTF-8 native
On Thu, 23 Feb 2023 09:04:23 GMT, Justin Lu wrote: > This PR converts Unicode sequences to UTF-8 native in .properties file. > (Excluding the Unicode space and tab sequence). The conversion was done using > native2ascii. > > In addition, the build logic is adjusted to support reading in the > .properties files as UTF-8 during the conversion from .properties file to > .java ListResourceBundle file. test/jdk/java/text/Format/NumberFormat/CurrencySymbols.properties line 156: > 154: zh=\u00A4 > 155: zh_CN=\uFFE5 > 156: zh_HK=HK$ Why are they not encoded into UTF-8 native? - PR: https://git.openjdk.org/jdk/pull/12726
Re: RFR: JDK-8300133: Use generalized see and link tags in core libs [v2]
On Fri, 13 Jan 2023 22:50:53 GMT, Joe Darcy wrote: >> With generalized see and link tags that can refer to anchors (JDK-8200337), >> the see and link tags in core libraries should be updated to use this >> feature when possible. This PR covers such updates for java.base. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Fix typo found in code review. Changes in `ResoruceBundle` look good. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.org/jdk/pull/12000
Re: RFR: JDK-8300133: Use generalized see and link tags in core libs
On Fri, 13 Jan 2023 21:30:06 GMT, Joe Darcy wrote:
> With generalized see and link tags that can refer to anchors (JDK-8200337),
> the see and link tags in core libraries should be updated to use this feature
> when possible. This PR covers such updates for java.base.
src/java.base/share/classes/java/util/ResourceBundle.java line 270:
> 268: * are packaged in a JAR file. Resource bundle only JAR file can be
> readily
> 269: * deployed as an {@linkplain
> java.lang.module.ModuleFinder##automatic-modules
> 270: * automatic module1}. For example, if the JAR file contains the
Typo: extra `1`
-
PR: https://git.openjdk.org/jdk/pull/12000
Re: [jdk20] RFR: 8298133: JDK 20 RDP1 L10n resource files update - msgdrop 10 [v5]
On Fri, 16 Dec 2022 03:38:54 GMT, Damon Nguyen wrote: >> Open l10n drop >> All tests passed > > Damon Nguyen has updated the pull request incrementally with one additional > commit since the last revision: > > Revert old translation. Fix lang codes There is another translation drop scheduled in January, so our plan is to figure out why the source extraction in this drop mistakenly used old files till then, then address it in the January l10n drop. - PR: https://git.openjdk.org/jdk20/pull/35
Re: [jdk20] RFR: 8298133: JDK 20 RDP1 L10n resource files update - msgdrop 10 [v4]
On Thu, 15 Dec 2022 23:01:35 GMT, Damon Nguyen wrote:
>> Open l10n drop
>> All tests passed
>
> Damon Nguyen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Revert double quote as well
Marked as reviewed by naoto (Reviewer).
src/jdk.jdeps/share/classes/com/sun/tools/jdeprscan/resources/jdeprscan_de.properties
line 26:
> 24: #
> 25:
> 26: main.usage=Verwendung: jdeprscan [Optionen] '{dir|jar|class}'
> ...\n\nOptionen:\n--class-path PATH\n--for-removal\n
> --full-version\n -? -h --help\n -l--list\n--release {0}\n -v
> --verbose\n--version
Good catch!
-
PR: https://git.openjdk.org/jdk20/pull/35
Re: [jdk20] RFR: 8298133: JDK 20 RDP1 L10n resource files update - msgdrop 10 [v3]
On Thu, 15 Dec 2022 20:43:42 GMT, Damon Nguyen wrote: >> Open l10n drop >> All tests passed > > Damon Nguyen has updated the pull request incrementally with one additional > commit since the last revision: > > Fix https and changed URL back Thanks for the update, Damon. Looks good for this drop. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.org/jdk20/pull/35
Re: [jdk20] RFR: 8298133: JDK 20 RDP1 L10n resource files update - msgdrop 10 [v2]
On Thu, 15 Dec 2022 02:45:48 GMT, Damon Nguyen wrote:
>> Open l10n drop
>> All tests passed
>
> Damon Nguyen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Removed trailing whitespace
Changes requested by naoto (Reviewer).
src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac_de.properties
line 217:
> 215: javac.msg.usage.nonstandard.footer=Diese zus\u00E4tzlichen Optionen
> k\u00F6nnen jederzeit ohne vorherige Ank\u00FCndigung ge\u00E4ndert werden.
> 216:
> 217: javac.msg.bug=Im Compiler ({0}) ist eine Ausnahme aufgetreten. Erstellen
> Sie auf der Java-Seite zum Melden von Bugs (http://bugreport.java.com) einen
> Bugbericht, nachdem Sie die Bugdatenbank (http://bugs.java.com) auf Duplikate
> gepr\u00FCft haben. Geben Sie in Ihrem Bericht Ihr Programm, die folgende
> Diagnose und die Parameter an, die Sie dem Java-Compiler \u00FCbergeben
> haben. Vielen Dank.
How come `https` turned into `http`? I see these happening all over the places
other than this location as well.
src/jdk.jdeps/share/classes/com/sun/tools/jdeps/resources/jdeps_de.properties
line 134:
> 132: public.api.replacement.column.header=Vorgeschlagene Ersetzung
> 133: artifact.not.found=nicht gefunden
> 134:
> jdeps.wiki.url=https://wiki.openjdk.java.net/display/JDK8/Java+Dependency+Analysis+Tool
This seems to change backward. `openjdk.java.net` has changed to `openjdk.org`
Same issues are observed in other locales.
-
PR: https://git.openjdk.org/jdk20/pull/35
Re: RFR: 8295803: Console should be usable in jshell and other environments [v7]
On Tue, 6 Dec 2022 18:10:38 GMT, Naoto Sato wrote: >> src/java.base/share/classes/java/io/Console.java line 99: >> >>> 97: */ >>> 98: >>> 99: public class Console implements Flushable >> >> Should we perhaps `seal` this class and only `permit` `ProxyingConsole` to >> `extend` it? > > Right. Will address it after this PR. Filed: https://bugs.openjdk.org/browse/JDK-8298416 - PR: https://git.openjdk.org/jdk/pull/11421
Integrated: 8295803: Console should be usable in jshell and other environments
On Tue, 29 Nov 2022 19:38:02 GMT, Naoto Sato wrote: > This is to allow Console to be used even when it is not attached to the > platform provided terminal, such as the case when the standard input is > redirected. `System.console()` now returns a Console implementation based on > `jdk.internal.le` terminal by default, or jshell implementation if available. > A corresponding CSR has been drafted. This pull request has now been integrated. Changeset: 8a9911ef Author:Naoto Sato URL: https://git.openjdk.org/jdk/commit/8a9911ef1762ae837e427ec9d91b1399ba33b6e4 Stats: 684 lines in 17 files changed: 661 ins; 11 del; 12 mod 8295803: Console should be usable in jshell and other environments Reviewed-by: jlaskey, alanb - PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v12]
On Wed, 7 Dec 2022 18:57:19 GMT, Alan Bateman wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Addressing review comments
>
> src/java.base/share/classes/java/io/ProxyingConsole.java line 167:
>
>> 165: @Override
>> 166: public void close() throws IOException {
>> 167: r.close();
>
> Console specifies that the invoking close on the Reader and Writer does not
> close the underlying stream. So I think this close (and
> WrappingWriter::close) need to be a no-op too.
Right. Made them as no-op.
-
PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v13]
> This is to allow Console to be used even when it is not attached to the > platform provided terminal, such as the case when the standard input is > redirected. `System.console()` now returns a Console implementation based on > `jdk.internal.le` terminal by default, or jshell implementation if available. > A corresponding CSR has been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Refined comments, no-op in WrappingReader/Writer::close() - Changes: - all: https://git.openjdk.org/jdk/pull/11421/files - new: https://git.openjdk.org/jdk/pull/11421/files/187f94fb..ef1a74f2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=12 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=11-12 Stats: 6 lines in 2 files changed: 2 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/11421.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11421/head:pull/11421 PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v11]
On Wed, 7 Dec 2022 11:54:11 GMT, Alan Bateman wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Making the wrapper classes static
>
> src/java.base/share/classes/java/io/Console.java line 625:
>
>> 623: };
>> 624: return AccessController.doPrivileged(pa);
>> 625: } catch (Throwable ignore) {
>
> I don't think we should be catching and ignoring throwable here. The only
> case that would be okay to ignore here is SCE due to SecurityException as the
> jline provider doesn't work with a SM set.
OK, reverted the catch clause.
> src/java.base/share/classes/java/io/ProxyingConsole.java line 175:
>
>> 173:
>> 174: public WrappingWriter(PrintWriter pw, Object lock) {
>> 175: super(pw);
>
> PrintWriter doesn't provide a way to provide to lock object so this means the
> overridden methods will synchronize on "lock", the non-overridden methods
> will synchronize on "pw". So we will need to look at this.
Created a package private constructor just for `ProxyingConsole` to synchronize
on the specified lock object.
-
PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v12]
> This is to allow Console to be used even when it is not attached to the > platform provided terminal, such as the case when the standard input is > redirected. `System.console()` now returns a Console implementation based on > `jdk.internal.le` terminal by default, or jshell implementation if available. > A corresponding CSR has been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Addressing review comments - Changes: - all: https://git.openjdk.org/jdk/pull/11421/files - new: https://git.openjdk.org/jdk/pull/11421/files/40de5d0a..187f94fb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=11 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=10-11 Stats: 10 lines in 3 files changed: 8 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11421.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11421/head:pull/11421 PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v11]
> This is to allow Console to be used even when it is not attached to the > platform provided terminal, such as the case when the standard input is > redirected. `System.console()` now returns a Console implementation based on > `jdk.internal.le` terminal by default, or jshell implementation if available. > A corresponding CSR has been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Making the wrapper classes static - Changes: - all: https://git.openjdk.org/jdk/pull/11421/files - new: https://git.openjdk.org/jdk/pull/11421/files/a54aed5f..40de5d0a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=10 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=09-10 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11421.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11421/head:pull/11421 PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v10]
> This is to allow Console to be used even when it is not attached to the > platform provided terminal, such as the case when the standard input is > redirected. `System.console()` now returns a Console implementation based on > `jdk.internal.le` terminal by default, or jshell implementation if available. > A corresponding CSR has been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Synchronize reader/writer by wrapping them - Changes: - all: https://git.openjdk.org/jdk/pull/11421/files - new: https://git.openjdk.org/jdk/pull/11421/files/6cd9843e..a54aed5f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=08-09 Stats: 56 lines in 1 file changed: 54 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11421.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11421/head:pull/11421 PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v9]
On Tue, 6 Dec 2022 19:17:44 GMT, Naoto Sato wrote: >> This is to allow Console to be used even when it is not attached to the >> platform provided terminal, such as the case when the standard input is >> redirected. `System.console()` now returns a Console implementation based on >> `jdk.internal.le` terminal by default, or jshell implementation if >> available. A corresponding CSR has been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Further addressing review comments. Provided separate independent locks in `ProxyingConsole` for now. Later it can be re-org'ed as Alan suggested for cleaner implementation/interface separation. - PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v9]
> This is to allow Console to be used even when it is not attached to the > platform provided terminal, such as the case when the standard input is > redirected. `System.console()` now returns a Console implementation based on > `jdk.internal.le` terminal by default, or jshell implementation if available. > A corresponding CSR has been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Further addressing review comments. - Changes: - all: https://git.openjdk.org/jdk/pull/11421/files - new: https://git.openjdk.org/jdk/pull/11421/files/204690cf..6cd9843e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=07-08 Stats: 27 lines in 2 files changed: 20 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/11421.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11421/head:pull/11421 PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v7]
On Tue, 6 Dec 2022 07:34:45 GMT, Alan Bateman wrote: >> src/java.base/share/classes/java/io/Console.java line 616: >> >>> 614: >>> JdkConsoleProvider.DEFAULT_PROVIDER_MODULE_NAME); >>> 615: return >>> ServiceLoader.load(JdkConsoleProvider.class).stream() >>> 616: .map(ServiceLoader.Provider::get) >> >> Furthermore, I think in its current form it means that this will >> load/instantiate any `JdkConsoleProvider` implementations that are >> accessible to the thread context classloader but may not have been from the >> module configured through `jdk.console` system property. That could >> potentially mean, in the best case, unnecessary classloading of additional >> classes and in the worst case, could result in `ServiceLoader.Provider::get` >> throwing a `ServiceConfigurationError` error for any of such unused provider >> implementations, thus forcing us to use `java.io.Console` instance. > >> Furthermore, I think in its current form it means that this will >> load/instantiate any `JdkConsoleProvider` implementations that are >> accessible to the thread context classloader but may not have been from the >> module configured through `jdk.console` system property. That could >> potentially mean, in the best case, unnecessary classloading of additional >> classes and in the worst case, could result in `ServiceLoader.Provider::get` >> throwing a `ServiceConfigurationError` error for any of such unused provider >> implementations, thus forcing us to use `java.io.Console` instance. > > You are right that the ServiceLoader.load should specify the system class > loader or the boot layer. However, there isn't an accessibility issue as a > class loader just load classes so it's more about visibility and whether the > TCCL will ultimately delegate to the application class loader. `module-info.java` in the java.base only allows `jdk.internal.le` and `jdk.jshell` modules to access the `jdk.internal.io.JdkConsoleProvider` interface. So unless the user intentionally exports it, no other implementations are effectively instantiated. - PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v7]
On Tue, 6 Dec 2022 06:19:37 GMT, Jaikiran Pai wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed the copyright year
>
> src/java.base/share/classes/java/io/Console.java line 99:
>
>> 97: */
>> 98:
>> 99: public class Console implements Flushable
>
> Should we perhaps `seal` this class and only `permit` `ProxyingConsole` to
> `extend` it?
Right. Will address it after this PR.
> src/java.base/share/classes/java/io/Console.java line 615:
>
>> 613: var consModName = System.getProperty("jdk.console",
>> 614:
>> JdkConsoleProvider.DEFAULT_PROVIDER_MODULE_NAME);
>> 615: return
>> ServiceLoader.load(JdkConsoleProvider.class).stream()
>
> Are we intentionally using thread context classloader (which can be different
> depending on which caller ends up first accessing/initializing the
> `java.io.Console` class) to load these services?
>
> I initially thought that `java.io.Console` might be used/initialized early in
> the bootstrap process of Java so the classloader could perhaps be
> deterministic, but running a trivial Java application with `-verbose:class`
> shows that `java.io.Console` doesn't get instantiated during the launch, so
> that leaves this code to "first access wins" situation and maybe an
> "incorrect" context classloader which doesn't have access the configured
> `jdk.console` module may end up causing this code to default to
> `java.io.Console`?
>
>
> public class Hello {
> public static void main(final String[] args) {
> }
> }
>
>
> java -verbose:class Hello.java
>
>
> Instead, should we perhaps use the ModuleLayer to find this configured module
> and then use its classloader to load the `JdkConsoleProvider` service
> provider? Something like:
>
>
> final Optional mod = ModuleLayer.boot().findModule(consModName);
> // ... if not present default to java.io.Console else use the module's
> classloader to try and load the JdkConsoleProvider
> return ServiceLoader.load(JdkConsoleProvider.class,
> mod.get().getClassLoader()).stream()..
Changed it to use the boot layer `ServiceLoader.load(ModuleLayer.boot(),
JdkConsoleProvider.class)`
> src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
> line 113:
>
>> 111: public JdkConsoleImpl() {
>> 112: try {
>> 113: terminal = TerminalBuilder.builder().build();
>
> The `java.io.Console` in its static initialization code has some logic to
> determine the `Charset` to use. Should that same `Charset` (or logic) be used
> here to build the terminal? Something like
> `TerminalBuilder.builder().encoding(fooBarCharset).build();`.
Initially, I thought of having charset as an argument to the constructor but
realized jline would delve into the platform and figure out the same encoding,
so I omitted it. However now I realized that the user could specify the
standard out encoding via the `stdout/err.encoding` system property. So I
revived the charset argument, as in your suggestion.
-
PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v8]
> This is to allow Console to be used even when it is not attached to the > platform provided terminal, such as the case when the standard input is > redirected. `System.console()` now returns a Console implementation based on > `jdk.internal.le` terminal by default, or jshell implementation if available. > A corresponding CSR has been drafted. Naoto Sato 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. The pull request contains 22 additional commits since the last revision: - Addressing review comments - Merge branch 'master' into JDK-8295803-PluginConsole - Fixed the copyright year - oleole -> ole - Addressing review comments. - Changed the expected behavior when the SecurityManager is enabled - Added more tests, loading with doPrivileged() - Adds a test - Removed JavaIOAccess.charset() which is no longer needed - Minor fixup - ... and 12 more: https://git.openjdk.org/jdk/compare/ce6f50f2...204690cf - Changes: - all: https://git.openjdk.org/jdk/pull/11421/files - new: https://git.openjdk.org/jdk/pull/11421/files/8b6859ed..204690cf Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=06-07 Stats: 47908 lines in 1086 files changed: 19444 ins; 21552 del; 6912 mod Patch: https://git.openjdk.org/jdk/pull/11421.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11421/head:pull/11421 PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v4]
On Sun, 4 Dec 2022 17:09:15 GMT, Alan Bateman wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Changed the expected behavior when the SecurityManager is enabled > > src/java.base/share/classes/java/io/Console.java line 627: > >> 625: }); >> 626: } >> 627: private static Console cons; > > The initialization of cons is replaced in this PR so maybe we can make it > final at the same time. Changed it to final - PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v7]
> This is to allow Console to be used even when it is not attached to the > platform provided terminal, such as the case when the standard input is > redirected. `System.console()` now returns a Console implementation based on > `jdk.internal.le` terminal by default, or jshell implementation if available. > A corresponding CSR has been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Fixed the copyright year - Changes: - all: https://git.openjdk.org/jdk/pull/11421/files - new: https://git.openjdk.org/jdk/pull/11421/files/31656331..8b6859ed Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11421.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11421/head:pull/11421 PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v6]
> This is to allow Console to be used even when it is not attached to the > platform provided terminal, such as the case when the standard input is > redirected. `System.console()` now returns a Console implementation based on > `jdk.internal.le` terminal by default, or jshell implementation if available. > A corresponding CSR has been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: oleole -> ole - Changes: - all: https://git.openjdk.org/jdk/pull/11421/files - new: https://git.openjdk.org/jdk/pull/11421/files/00a0e3fc..31656331 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=04-05 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11421.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11421/head:pull/11421 PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v5]
> This is to allow Console to be used even when it is not attached to the > platform provided terminal, such as the case when the standard input is > redirected. `System.console()` now returns a Console implementation based on > `jdk.internal.le` terminal by default, or jshell implementation if available. > A corresponding CSR has been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Addressing review comments. - Changes: - all: https://git.openjdk.org/jdk/pull/11421/files - new: https://git.openjdk.org/jdk/pull/11421/files/7c0740aa..00a0e3fc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=03-04 Stats: 57 lines in 5 files changed: 26 ins; 22 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/11421.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11421/head:pull/11421 PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v4]
> This is to allow Console to be used even when it is not attached to the > platform provided terminal, such as the case when the standard input is > redirected. `System.console()` now returns a Console implementation based on > `jdk.internal.le` terminal by default, or jshell implementation if available. > A corresponding CSR has been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Changed the expected behavior when the SecurityManager is enabled - Changes: - all: https://git.openjdk.org/jdk/pull/11421/files - new: https://git.openjdk.org/jdk/pull/11421/files/4c38b01b..7c0740aa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=02-03 Stats: 28 lines in 3 files changed: 5 ins; 7 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/11421.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11421/head:pull/11421 PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v2]
On Fri, 2 Dec 2022 15:21:40 GMT, Weijun Wang wrote: >> Naoto has confirmed that the password prompt from keytool does not echo, >> good! >> >> The intention is that Console be usable in jshell so I think the issue is >> that readPassword is echo'ing when used in jshell. Maybe someone >> experimenting with the Console API might run into this but we can separate >> out that issue. > > Still not sure what the expected behavior is, but for keytool, because of the > updated check, `sun.security.util.Password` now uses `System.in.read` instead > of `Console.readPassword`, therefore the password is echoing. I tried > removing the check and force `Console.readPassword` to be called. There is no > echo but the return key also does not work. I have to Ctrl-C to break out. I thought that in `jshell`, `System.console()` returns null, so it is always not using `Console.readPassword`. Anyway, I think the scenario is not practical (changing password using jshell), and in the future, jshell can provide its own Console implementation (that's a part of this enhancement's motivation) that would nicely handle this situation. - PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v3]
> This is to allow Console to be used even when it is not attached to the > platform provided terminal, such as the case when the standard input is > redirected. `System.console()` now returns a Console implementation based on > `jdk.internal.le` terminal by default, or jshell implementation if available. > A corresponding CSR has been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Added more tests, loading with doPrivileged() - Changes: - all: https://git.openjdk.org/jdk/pull/11421/files - new: https://git.openjdk.org/jdk/pull/11421/files/3d2b01f6..4c38b01b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=01-02 Stats: 137 lines in 5 files changed: 121 ins; 2 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/11421.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11421/head:pull/11421 PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8296546: Add @spec tags to API [v4]
On Thu, 1 Dec 2022 19:37:28 GMT, Jonathan Gibbons wrote: >> src/java.base/share/classes/java/lang/Character.java line 172: >> >>> 170: * occur. For example, in a future release, synchronization may fail. >>> 171: * >>> 172: * @spec https://www.unicode.org/reports/tr27 Unicode 3.1.0 >> >> This should probably be removed, as the original link (explaining `U+n` >> notation) is broken. > > @naotoj The edits are driven by a script, using info about existing links in > the same doc comment. If you don't think this reference is appropriate, it > would be better to either remove the existing link (and I'll regenerate this > patch) or else this patch goes through and you fix up both the existing link > and the `@spec` tag afterwards. Either way is fine with me. I will fix it up if you choose the latter. - PR: https://git.openjdk.org/jdk/pull/11073
Re: RFR: 8295803: Console should be usable in jshell and other environments [v2]
On Thu, 1 Dec 2022 17:53:58 GMT, Weijun Wang wrote:
>> I can still see the password on screen. Here `in` is a
>> `jdk.jshell.execution.Util` so the updated check on line 58 above failed.
>>
>> $ jshell -C--add-exports -Cjava.base/sun.security.tools.keytool=ALL-UNNAMED
>> -R--add-exports -Rjava.base/sun.security.tools.keytool=ALL-UNNAMED
>> | Welcome to JShell -- Version 20-internal
>> | For an introduction type: /help intro
>>
>> jshell> sun.security.tools.keytool.Main.main("-genkeypair -keyalg EC -alias
>> a -dname CN=A -keystore ks".split(" "))
>> Enter keystore password: changeit
>> Re-enter new password: changeit
>> Generating 384 bit EC (secp384r1) key pair and self-signed certificate
>> (SHA384withECDSA) with a validity of 90 days
>> for: CN=A
>
> What's the expected behavior?
I confirmed that the standalone `keytool` did not echo the input, which should
be OK for this IMO.
-
PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments [v2]
> This is to allow Console to be used even when it is not attached to the > platform provided terminal, such as the case when the standard input is > redirected. `System.console()` now returns a Console implementation based on > `jdk.internal.le` terminal by default, or jshell implementation if available. > A corresponding CSR has been drafted. Naoto Sato has updated the pull request incrementally with two additional commits since the last revision: - Adds a test - Removed JavaIOAccess.charset() which is no longer needed - Changes: - all: https://git.openjdk.org/jdk/pull/11421/files - new: https://git.openjdk.org/jdk/pull/11421/files/96a46843..3d2b01f6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=00-01 Stats: 70 lines in 4 files changed: 64 ins; 6 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/11421.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11421/head:pull/11421 PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8295803: Console should be usable in jshell and other environments
On Wed, 30 Nov 2022 17:07:35 GMT, Jim Laskey wrote:
>> This is to allow Console to be used even when it is not attached to the
>> platform provided terminal, such as the case when the standard input is
>> redirected. `System.console()` now returns a Console implementation based on
>> `jdk.internal.le` terminal by default, or jshell implementation if
>> available. A corresponding CSR has been drafted.
>
> src/java.base/share/classes/java/io/Console.java line 617:
>
>> 615:
>> 616: public Charset charset() {
>> 617: return cons.charset();
>
> What happens when cons is null?
Turned out that `JavaIOAccess.charset()` is not used anywhere. Will remove this
method. Will also add a test.
-
PR: https://git.openjdk.org/jdk/pull/11421
RFR: 8295803: Console should be usable in jshell and other environments
This is to allow Console to be used even when it is not attached to the platform provided terminal, such as the case when the standard input is redirected. `System.console()` now returns a Console implementation based on `jdk.internal.le` terminal by default, or jshell implementation if available. A corresponding CSR has been drafted. - Commit messages: - Minor fixup - GetPropertyAction.privilegedGetProperty - Changed to use SharedSecrets - Password.readPassword() fix - Always returns Console, even without TTY attached - Comments refresh, clean-up - Removed charset from factory, added new jdk.console property - Revived the provider i/f - Replacing SPI with proxy delegate - javadoc - ... and 3 more: https://git.openjdk.org/jdk/compare/ae5b1f76...96a46843 Changes: https://git.openjdk.org/jdk/pull/11421/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11421&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8295803 Stats: 387 lines in 10 files changed: 373 ins; 0 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/11421.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11421/head:pull/11421 PR: https://git.openjdk.org/jdk/pull/11421
Re: RFR: 8296546: Add @spec tags to API
On Thu, 10 Nov 2022 01:10:13 GMT, Jonathan Gibbons wrote: > Please review a "somewhat automated" change to insert `@spec` tags into doc > comments, as appropriate, to leverage the recent new javadoc feature to > generate a new page listing the references to all external specifications > listed in the `@spec` tags. > > "Somewhat automated" means that I wrote and used a temporary utility to scan > doc comments looking for HTML links to selected sites, such as `ietf.org`, > `unicode.org`, `w3.org`. These links may be in the main description of a doc > comment, or in `@see` tags. For each link, the URL is examined, and > "normalized", and inserted into the doc comment with a new `@spec` tag, > giving the link and tile for the spec. > > "Normalized" means... > * Use `https:` where possible (includes pretty much all cases) > * Use a single consistent host name for all URLs coming from the same spec > site (i.e. don't use different aliases for the same site) > * Point to the root page of a multi-page spec > * Use a consistent form of the spec, preferring HTML over plain text where > both are available (this mostly applies to IETF specs) > > In addition, a "standard" title is determined for all specs, determined > either from the content of the (main) spec page or from site index pages. > > The net effect is (or should be) that **all** the changes are to just **add** > new `@spec` tags, based on the links found in each doc comment. There should > be no other changes to the doc comments, or to the implementation of any > classes and interfaces. > > That being said, the utility I wrote does have additional abilities, to > update the links that it finds (e.g. changing to use `https:` etc,) but those > features are _not_ being used here, but could be used in followup PRs if > component teams so desired. I did notice while working on this overall > feature that many of our links do point to "outdated" pages, some with > eye-catching notices declaring that the spec has been superseded. Determining > how, when and where to update such links is beyond the scope of this PR. > > Going forward, it is to be hoped that component teams will maintain the > underlying links, and the URLs in `@spec` tags, such that if references to > external specifications are updated, this will include updating the `@spec` > tags. > > To see the effect of all these new `@spec` tags, see > http://cr.openjdk.java.net/~jjg/8296546/api.00/ > > In particular, see the new [External > Specifications](http://cr.openjdk.java.net/~jjg/8296546/api.00/external-specs.html) > page, which you can also find via the new link near the top of the > [Index](http://cr.openjdk.java.net/~jjg/8296546/api.00/index-files/index-1.html) > pages. src/java.base/share/classes/java/lang/Character.java line 172: > 170: * occur. For example, in a future release, synchronization may fail. > 171: * > 172: * @spec https://www.unicode.org/reports/tr27 Unicode 3.1.0 This should probably be removed, as the original link (explaining `U+n` notation) is broken. - PR: https://git.openjdk.org/jdk/pull/11073
Re: RFR: 8295729: Add jcheck whitespace checking for properties files [v3]
On Tue, 25 Oct 2022 13:43:56 GMT, Weijun Wang wrote: > I wonder if this means even if we get everything right now the tool might add > trailing spaces again later. Good catch, Max. Yes, that should be dealt with in the translation process. > I suggest we focus on the English files this time and file a bug to the > translation tool. Agree. - PR: https://git.openjdk.org/jdk/pull/10792
Re: RFR: 8295729: Add jcheck whitespace checking for properties files [v3]
On Mon, 24 Oct 2022 20:08:02 GMT, Andy Goryachev wrote: > Good idea! I wonder if this should be done as a unit test. go through all the > bundles and check leading/trailing whitespace. Right. Definitely not a job for `jcheck`, but it should be considered somewhere in the l10n process. - PR: https://git.openjdk.org/jdk/pull/10792
Re: RFR: 8295729: Add jcheck whitespace checking for properties files [v3]
On Mon, 24 Oct 2022 19:21:07 GMT, Magnus Ihse Bursie wrote: >> Properties files is essentially source code. It should have the same >> whitespace checks as all other source code, so we don't get spurious >> trailing whitespace changes. >> >> With the new Skara jcheck, it is possible to increase the coverage of the >> whitespace checks (in the old mercurial version, this was more or less >> impossible). >> >> The only manual change is to `.jcheck/conf`. All other changes were made by >> running `find . -type f -iname "*.properties" | xargs gsed -i -e 's/[ >> \t]*$//'`. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Revert "Remove check for .properties from jcheck" > >This reverts commit c91fdaa19dc06351598bd1c0614e1af3bfa08ae2. > - Change trailing space and tab in values to unicode encoding The problem here is that all those (unnecessary) trailing spaces are appended by the external translators, who are not aware those spaces should not be at the end. I think what we can do is check the original English properties values that the engineers provided, and if there is no trailing spaces there, we can safely remove trailing spaces in localized bundles. - PR: https://git.openjdk.org/jdk/pull/10792
Re: RFR: 8295729: Remove trailing whitespace from non-value lines in properties files [v2]
On Fri, 21 Oct 2022 08:17:46 GMT, Magnus Ihse Bursie wrote: >> Properties files is essentially source code. It should have the same >> whitespace checks as all other source code, so we don't get spurious >> trailing whitespace changes. >> >> With the new Skara jcheck, it is possible to increase the coverage of the >> whitespace checks (in the old mercurial version, this was more or less >> impossible). >> >> The only manual change is to `.jcheck/conf`. All other changes were made by >> running `find . -type f -iname "*.properties" | xargs gsed -i -e 's/[ >> \t]*$//'`. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Remove check for .properties from jcheck > - Restore trailing whitespace for property values One possible solution to this is to replace those dangling white spaces with explicit Unicode escapes, i.e. "\u0009" and "\u0020". This way jcheck can safely be enabled to detect future unwanted trailing spaces, and possibly existing ones can be visually recognizable by the engineers to correct them. - PR: https://git.openjdk.org/jdk/pull/10792
Re: RFR: 8295729: Add jcheck whitespace checking for properties files
On Thu, 20 Oct 2022 11:58:58 GMT, Magnus Ihse Bursie wrote: > Properties files is essentially source code. It should have the same > whitespace checks as all other source code, so we don't get spurious trailing > whitespace changes. > > With the new Skara jcheck, it is possible to increase the coverage of the > whitespace checks (in the old mercurial version, this was more or less > impossible). > > The only manual change is to `.jcheck/conf`. All other changes were made by > running `find . -type f -iname "*.properties" | xargs gsed -i -e 's/[ > \t]*$//'`. (retracting approval) - Changes requested by naoto (Reviewer). PR: https://git.openjdk.org/jdk/pull/10792
Re: RFR: 8295729: Add jcheck whitespace checking for properties files
On Thu, 20 Oct 2022 11:58:58 GMT, Magnus Ihse Bursie wrote: > Properties files is essentially source code. It should have the same > whitespace checks as all other source code, so we don't get spurious trailing > whitespace changes. > > With the new Skara jcheck, it is possible to increase the coverage of the > whitespace checks (in the old mercurial version, this was more or less > impossible). > > The only manual change is to `.jcheck/conf`. All other changes were made by > running `find . -type f -iname "*.properties" | xargs gsed -i -e 's/[ > \t]*$//'`. Retracting my approval too. Thanks for the catch, Andy! That sounds reasonable > Magnus - PR: https://git.openjdk.org/jdk/pull/10792
Re: RFR: 8295729: Add jcheck whitespace checking for properties files
On Thu, 20 Oct 2022 11:58:58 GMT, Magnus Ihse Bursie wrote: > Properties files is essentially source code. It should have the same > whitespace checks as all other source code, so we don't get spurious trailing > whitespace changes. > > With the new Skara jcheck, it is possible to increase the coverage of the > whitespace checks (in the old mercurial version, this was more or less > impossible). > > The only manual change is to `.jcheck/conf`. All other changes were made by > running `find . -type f -iname "*.properties" | xargs gsed -i -e 's/[ > \t]*$//'`. LGTM. Haven't looked at all the l10n files. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.org/jdk/pull/10792
Re: RFR: JDK-8294994: Update Jarsigner and Keytool i18n tests to validate i18n compliance [v4]
On Wed, 12 Oct 2022 18:31:16 GMT, Bill Huang wrote: >> The jarsigner and keytool are localized into English, German, Japanese and >> Simplified Chinese. This task is to modify the existing i18n tests to >> validate i18n compliance in these tools. >> >> In addition, this task also contains changes for manual test enhancement and >> simplification which originated from >> [JDK-8292663](https://bugs.openjdk.org/browse/JDK-8292663). > > Bill Huang has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed typo. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.org/jdk/pull/10635
Re: RFR: JDK-8294994: Update Jarsigner and Keytool i18n tests to validate i18n compliance [v2]
On Mon, 10 Oct 2022 23:23:00 GMT, Bill Huang wrote:
>> The jarsigner and keytool are localized into English, German, Japanese and
>> Simplified Chinese. This task is to modify the existing i18n tests to
>> validate i18n compliance in these tools.
>>
>> In addition, this task also contains changes for manual test enhancement and
>> simplification which originated from
>> [JDK-8292663](https://bugs.openjdk.org/browse/JDK-8292663).
>
> Bill Huang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Implemented review comments.
Looks much better now. A minor comment on the library.
test/lib/jdk/test/lib/TestUI.java line 30:
> 28: import java.awt.event.WindowEvent;
> 29:
> 30: public class TestUI {
I would expect some class description for this library. Also, could the library
name be more descriptive than "TestUI"?
-
PR: https://git.openjdk.org/jdk/pull/10635
Re: RFR: JDK-8294994: Update Jarsigner and Keytool i18n tests to validate i18n compliance [v2]
On Tue, 11 Oct 2022 15:17:13 GMT, Weijun Wang wrote: > I have a question, why must this test be manual? Can't we compare the > localized texts? Thought about that, but it could be a nuisance if we compared word-to-word translations, considering the situation if an engineer made some changes in the English resource bundle, but l10n may not come at the same time which is guaranteed to fail. - PR: https://git.openjdk.org/jdk/pull/10635
Re: RFR: JDK-8294994: Update Jarsigner and Keytool i18n tests to validate i18n compliance
On Mon, 10 Oct 2022 18:18:55 GMT, Bill Huang wrote:
> The jarsigner and keytool are localized into English, German, Japanese and
> Simplified Chinese. This task is to modify the existing i18n tests to
> validate i18n compliance in these tools.
>
> In addition, this task also contains changes for manual test enhancement and
> simplification which originated from
> [JDK-8292663](https://bugs.openjdk.org/browse/JDK-8292663.
Looks good overall. Some minor suggestions.
test/jdk/sun/security/tools/keytool/i18n.java line 62:
> 60: * @library /test/lib
> 61: * @run main/manual/othervm i18n zh CN
> 62: */
Do you need to triplicate these `@test` tags? Would 3-lines of `@run` suffice?
Also setting the locale by `-Duser.language/country` and `getProperty` them in
the main would be preferable to passing them as the test case arguments.
test/jdk/sun/security/tools/keytool/i18n.java line 250:
> 248: private Thread currentThread = null;
> 249:
> 250: public static class DialogBuilder {
This seems to be a boilerplate for displaying the panel. Could this be
separated from the test and converted into some library?
test/jdk/sun/security/tools/keytool/i18n.java line 334:
> 332: } else if (args.length == 2) {
> 333: Locale.setDefault(Locale.of(args[0], args[1]));
> 334: }
Can be eliminated with the suggestion above.
test/jdk/sun/security/tools/keytool/i18n.java line 335:
> 333: Locale.setDefault(Locale.of(args[0], args[1]));
> 334: }
> 335: final String LANG = Locale.getDefault().getDisplayLanguage();
Instead of `getDisplayLanguage()`, it should issue `getDisplayName()`, as for
`zh-CN` case, it simply displays `Chinese` in the current impl. It's ambiguous
whether it is simplified or traditional.
Also, `LANG` should be lowercase, as it is not a constant.
-
PR: https://git.openjdk.org/jdk/pull/10635
Re: [jdk19] RFR: 8290889: JDK 19 RDP2 L10n resource files update - msgdrop 10 [v4]
On Fri, 29 Jul 2022 18:13:25 GMT, Alisen Chung wrote: >> open l10n msg drop >> All tests passed. > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > added comments in CurrencyNames root in base, moved US CurrencyNames back > to base, readded original Chinese translation LGTM - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.org/jdk19/pull/154
Re: [jdk19] RFR: 8290889: JDK 19 RDP2 L10n resource files update - msgdrop 10 [v3]
On Tue, 26 Jul 2022 20:31:44 GMT, Alisen Chung wrote: >> open l10n msg drop >> All tests passed. > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > removed localized files from base - src/jdk.localedata/share/classes/sun/util/resources/ext/CurrencyNames_en_US.properties: this should not be moved from `java.base` module - I still need some comments explaining `XXX=XXX` in the root bundle of `CurrencyNames`. Something like These currency symbol entries are for the root bundle only to avoid throwing a MissingResourceException. Should not be copied into each localized resource bundle. src/jdk.localedata/share/classes/sun/util/resources/ext/CurrencyNames_zh_CN.properties line 63: > 61: # written authorization of the copyright holder. > 62: > 63: CNY=\uffe5 Do not remove the original translation. - Changes requested by naoto (Reviewer). PR: https://git.openjdk.org/jdk19/pull/154
Re: [jdk19] RFR: 8290889: JDK 19 RDP2 L10n resource files update - msgdrop 10
On Sat, 23 Jul 2022 03:03:25 GMT, Alisen Chung wrote: > open l10n msg drop > All tests passed. Changes requested by naoto (Reviewer). src/java.base/share/classes/sun/util/resources/CurrencyNames_de.properties line 63: > 61: # written authorization of the copyright holder. > 62: > 63: ADP=ADP These currency symbols (`XXX=XXX`) are all in the base `CurrencyNames.properties`, so not needed in each localized resources (They are in the base in order to avoid throwing exceptions). Also, the localized files should not reside in `java.base`, but to be merged with the ones in `jdk.localedata` module These comments hold for all localized resources (de, ja, zh-CN). For furure reference, it may be good to add some comment in the base `CurrencyNames.properties` about this. src/java.base/share/data/currency/CurrencyData_de.properties line 1: > 1: # `CurrencyData.properties` is not a localizable resource bundle, but a properties file containing currency data. No need to add these localized resources. - PR: https://git.openjdk.org/jdk19/pull/154
Re: RFR: 8289768: Clean up unused code [v2]
On Wed, 6 Jul 2022 05:32:29 GMT, Daniel Jeliński wrote: >> This patch removes many unused variables and one unused label reported by >> the compilers when relevant warnings are enabled. >> >> The unused code was found by compiling after removing `unused` from the list >> of disabled warnings for >> [gcc](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L190) >> and >> [clang](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L203), >> and enabling >> [C4189](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4189?view=msvc-170) >> MSVC warning. >> >> I only removed variables that were uninitialized or initialized without side >> effects. I verified that the removed variables were not used in any >> `#ifdef`'d code. I checked that the changed code still compiles on Windows, >> Linux and Mac, both in release and debug versions. > > Daniel Jeliński 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. The pull request contains four additional > commits since the last revision: > > - Merge remote-tracking branch 'origin' into unused-variables > - Remove unused variables (windows) > - Remove unused variables (macos) > - Remove unused variables (linux) Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.org/jdk/pull/9383
