On Wed, 27 Mar 2024 10:43: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.
Other than a couple of nits, it looks good to me.
I'll submit a test job to run all the tests with this fix and this test.
test/jdk/javax/swing/JEditorPane/EditorPaneCharset.java line 48:
> 46: "<head>\n" +
> 47: " <meta http-equiv=\"Content-Type\" " +
> 48: " content=\"text/html; charset=windows-1251\">\n" +
Suggestion:
" content="text/html; charset=windows-1251">\n" +
I suggest moving it by one space to align to the start of the `http-equiv`
attribute.
test/jdk/javax/swing/JEditorPane/EditorPaneCharset.java line 57:
> 55: editorPane.setContentType("text/html");
> 56: Document document = editorPane.getDocument();
> 57: // Shouldn't throw ChangedCharSetException
Suggestion:
Document document = editorPane.getDocument();
// Shouldn't throw ChangedCharSetException
I'd add a blank line here too. It starts a new block which is the point of the
test.
test/jdk/javax/swing/JEditorPane/EditorPaneCharset.java line 68:
> 66: Element p = body.getElement(0);
> 67: String pText = document.getText(p.getStartOffset(),
> 68: p.getEndOffset() -
> p.getStartOffset());
Suggestion:
String pText = document.getText(p.getStartOffset(),
p.getEndOffset() - p.getStartOffset());
Let's align the second argument to the opening parenthesis.
-------------
PR Review: https://git.openjdk.org/jdk/pull/17567#pullrequestreview-1962948785
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540870025
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540872420
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540872209
