RFR: 8328953 : JEditorPane.read throws ChangedCharSetException

[Alexey Ivanov]

On Thu, 25 Jan 2024 11:11:47 GMT, rjolly <duke at openjdk.org> 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.

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/javax/swing/JEditorPane.java line 1:

> 1: /*

Please update the copyright year to `1997, 2024`. That is you change 2023 to 2024.

src/java.desktop/share/classes/javax/swing/JEditorPane.java line 621:

> 619:         String charset = (String) getClientProperty("charset");
> 620:         try(Reader r = (charset != null) ? new InputStreamReader(in, charset) :
> 621:                 new InputStreamReader(in)) {

The changeset looks confusing in the diff. It becomes clearer if you disable showing whitespace differences.

You could've elaborated on the fix in the description.

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.

test/jdk/javax/swing/JEditorPane/8328953/EditorPaneCharset.java line 1:

> 1: /*

Please, place the test into `JEditorPane` directly, that is remove `8328953` subfolder. New tests use flat structure unless supporting files are required.

test/jdk/javax/swing/JEditorPane/8328953/EditorPaneCharset.java line 2:

> 1: /*
> 2:  * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.

Suggestion:

 * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.

It's a new file.

test/jdk/javax/swing/JEditorPane/8328953/EditorPaneCharset.java line 30:

> 28:  * @summary JEditorPane.read throws ChangedCharSetException
> 29:  * @run main EditorPaneCharset
> 30:  */

It hasn't been agreed to, yet I prefer the jtreg placed right before the test class declaration rather than before the imports.

Either way is fine.

test/jdk/javax/swing/JEditorPane/8328953/EditorPaneCharset.java line 53:

> 51:     public static void main(String[] args) {
> 52:         SwingUtilities.invokeLater(EditorPaneCharset::new);
> 53:     }

It is not a good jtreg regression test.

I believe the test can be headless, you don't need to display the component. It could even create `JEditorPane` in its `main` method. The expectation is that `editorPane.read` does not throw `ChangedCharSetException`. I propose adding a comment about the fact.

Then, I'd verify that the text in the editor pane is the expected text. So you will take the document root and descend the three of elements to `<p>`. The text inside the paragraph must match the text in the HTML sample, it should be *“Привет, мир!”* (it means *“Hello World”*, I'm not creative). You could move the text into a constant to have the text to compare to and concatenate the constant into the HTML snippet.

Something like that:

main() throws IOException {
    // Shouldn't throw ChangedCharSetException
    editorPane.read(…);

    Element root = document.getDefaultRootElement();
    Element p = /* */;
    String pText = document.getText(p.getStartOffset(),
                                    p.getEndOffset() - p.getStartOffset()));
    if (!CYRILLIC_TEXT.equals(pText)) {
        throw new RuntimeException("Text doesn't match");
    }
}


where `CYRILLIC_TEXT` is the constant with the text in the HTML snippet.

You can use this test as an example on how to get to the `<p>` element in `HTMLDocument`:  
https://github.com/openjdk/jdk/blob/cc1800fa4de3c1369efd46f5ca967ea82763f5d5/test/jdk/javax/swing/text/html/HTMLDocument/HTMLUnderlineStrike.java#L65-L68

Should you have any questions, don't hesitate to ask.

-------------

PR Review: https://git.openjdk.org/jdk/pull/17567#pullrequestreview-1960082452
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1539045914
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1539087982
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1539065172
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1539047328
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1539066742
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1539063737