On Wed, 27 Mar 2024 08:48:55 GMT, rjolly <[email protected]> wrote:
>> ChangedCharSetException is used to amend the charset during read according
>> to html directives. Currently it causes immediate exit of the method which
>> in turn causes failure to load html documents with charset directives (even
>> if the latter must not change after all). This PR restores the catch
>> operation as it was before the use of try with resources.
>
> rjolly has updated the pull request incrementally with one additional commit
> since the last revision:
>
> 8328953 : JEditorPane.read throws ChangedCharSetException
>
> The fix is to add a nested `try`-block inside `try-with-resource`; all the
> exceptions are handled in the nested `try`; the outer `try`-with-resouces
> only closes the input stream.
Changes requested by aivanov (Reviewer).
test/jdk/javax/swing/JEditorPane/EditorPaneCharset.java line 35:
> 33: /*
> 34: * @test
> 35: * @key headless
Suggestion:
There's no *headless* key; only *headful* tests need to be marked specifically.
test/jdk/javax/swing/JEditorPane/EditorPaneCharset.java line 37:
> 35: * @key headless
> 36: * @bug 8328953
> 37: * @summary JEditorPane.read throws ChangedCharSetException
Suggestion:
* @summary Verifies JEditorPane.read doesn't throw ChangedCharSetException
but handles it and reads HTML in the specified encoding
test/jdk/javax/swing/JEditorPane/EditorPaneCharset.java line 47:
> 45: "<html lang=\"ru\">\n" +
> 46: "<head>\n" +
> 47: " <meta http-equiv=\"Content-Type\" content=\"text/html;
> charset=windows-1251\">\n" +
Suggestion:
" <meta http-equiv="Content-Type" " +
" content="text/html; charset=windows-1251">\n" +
test/jdk/javax/swing/JEditorPane/EditorPaneCharset.java line 52:
> 50: "</body></html>\n";
> 51:
> 52: public static void main() throws IOException {
Suggestion:
public static void main(String[] args)
throws IOException, BadLocationException {
The `main` function should be like this. In [my
comment](https://github.com/openjdk/jdk/pull/17567#discussion_r1539063737), I
referred to it as `main()` because typing all these in a comment is tedious.
Yes, Java now allows `main` without arguments but this feature is not supported
in previous versions of Java, and tests should not rely on this feature.
Alternatively, you may use `throws Exception` to allow all kinds of exceptions.
Using `throws Exception` is more common. I don't have a strong preference in
this case.
test/jdk/javax/swing/JEditorPane/EditorPaneCharset.java line 65:
> 63: } catch (IOException e) {
> 64: throw new RuntimeException(e);
> 65: }
Now that you added `throws IOException` to `main` declaration, you can allow it
to escape to fail the test. Please do. Remove the try-catch block.
test/jdk/javax/swing/JEditorPane/EditorPaneCharset.java line 78:
> 76: } catch (BadLocationException e) {
> 77: throw new RuntimeException(e);
> 78: }
Similarly, remove this try-catch block and allow `BadLocationException` to
escape.
test/jdk/javax/swing/JEditorPane/EditorPaneCharset.java line 82:
> 80:
> 81: private EditorPaneCharset() {
> 82: }
The constructor is not needed. It used it in my test, but now it's empty.
-------------
PR Review: https://git.openjdk.org/jdk/pull/17567#pullrequestreview-1962874040
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540832221
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540834671
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540828514
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540814672
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540822881
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540824315
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540825793
