-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8328953 : JEditorPane.read throws ChangedCharSetException #17567
Conversation
Hi @rjolly, welcome to this OpenJDK project and thanks for contributing! We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user rjolly" as summary for the issue. If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing |
/signed |
Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated! |
@rjolly This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
Up |
@rjolly Thank you for your contribution. Could you provide more details about the issue? The PR needs to be linked to a JBS issue. I'll create one for you after I get a more detailed description of the problem from you. A test case is also welcome. And it's needed a regression test for the change anyway, if it's not hard to create. I believe a test case can be written for this issue. |
js>JEditorPane=javax.swing.JEditorPane |
@rjolly This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 847 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TejeshR13, @aivanov-jdk) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
Hi @aivanov-jdk , thanks for your response. Is there a place where I can put my non-regression test? Thanks! |
@rjolly Sorry for my delayed reply. I have submitted JDK-8328953: JEditorPane.read throws ChangedCharSetException for you. The subject may be amended to describe issue better. I attached the test case. I can reproduce the bug with Java 11 and above. I think it is a regression from JDK-8146319 which added try-with-resources. |
Your changeset looks good to me, it resolves the problem, the HTML stream is re-read with the correct charset and displays correctly. The test that I attached to JBS can be converted to a jtreg regression test and added to your PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified the fix with the shared test (regression test from JDK-8146319), its working fine.
@rjolly Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
@rjolly You should edit the subject of the PR to |
You should not force-push to a branch that's already on the review. If you want to update to latest master, use |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified the fix with the shared test, its working fine.
@@ -0,0 +1,75 @@ | |||
/* | |||
* Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Append Copywrite year.
* questions. | ||
*/ | ||
|
||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We normally place jtreg tags after imports.
private static final String HTML_CYRILLIC = | ||
"<html lang=\"ru\">\n" + | ||
"<head>\n" + | ||
" <meta http-equiv=\"Content-Type\" content=\"text/html; charset=windows-1251\">\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line exceeds column length which we usually restrict to 80...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fits into 100-column limit, so not strictly necessary… Could be broken though to fit into 80-column limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the copyright year to 1997, 2024
. That is you change 2023 to 2024.
@@ -0,0 +1,75 @@ | |||
/* | |||
* Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved. | |
* Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. |
It's a new file.
public static void main(String[] args) { | ||
SwingUtilities.invokeLater(EditorPaneCharset::new); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
:
Element root = doc.getDefaultRootElement(); | |
Element body = root.getElement(1); | |
for (int i = 0; i < body.getElementCount(); i++) { | |
Element p = body.getElement(i); |
Should you have any questions, don't hesitate to ask.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, place the test into JEditorPane
directly, that is remove 8328953
subfolder. New tests use flat structure unless supporting files are required.
/* | ||
* @test | ||
* @key headful | ||
* @bug 8328953 | ||
* @summary JEditorPane.read throws ChangedCharSetException | ||
* @run main EditorPaneCharset | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
try(Reader r = (charset != null) ? new InputStreamReader(in, charset) : | ||
new InputStreamReader(in)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I too agree on this point. The fix description has to elaborated, we are not able to make out in diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pulled the change into my local workspace to test it. IDE hides white-space differences and the fix becomes clear. I hid white-space differences in GitHub diff viewer: only two lines are changed. :)
A sample on how to merge master in another PR: #9825 (comment) |
" <meta http-equiv=\"Content-Type\" content=\"text/html; charset= | ||
windows-1251\">\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a valid Java syntax? The file does not compile.
I would prefer wrapping at attribute boundary rather than a value of an attribute.
No tabs are allowed in OpenJDK source code, replace with four spaces.
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.
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.
Element root = document.getDefaultRootElement(); | ||
Element body = root.getElement(1); | ||
Element p = body.getElement(0); | ||
String pText = document.getText(p.getStartOffset(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unhandled exception here (BadLocationException
), for getText()
. Also extra braces at end.
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.
"<p>" + CYRILLIC_TEXT + "</p>\n" + | ||
"</body></html>\n"; | ||
|
||
public static void main() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static void main() throws IOException { | |
public static void main(String[] args) | |
throws IOException, BadLocationException { |
The main
function should be like this. In my comment, 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.
} catch (IOException e) { | ||
throw new RuntimeException(e); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} catch (BadLocationException e) { | ||
throw new RuntimeException(e); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, remove this try-catch block and allow BadLocationException
to escape.
private EditorPaneCharset() { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor is not needed. It used it in my test, but now it's empty.
private static final String HTML_CYRILLIC = | ||
"<html lang=\"ru\">\n" + | ||
"<head>\n" + | ||
" <meta http-equiv=\"Content-Type\" content=\"text/html; charset=windows-1251\">\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
" <meta http-equiv=\"Content-Type\" content=\"text/html; charset=windows-1251\">\n" + | |
" <meta http-equiv=\"Content-Type\" " + | |
" content=\"text/html; charset=windows-1251\">\n" + |
|
||
/* | ||
* @test | ||
* @key headless |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @key headless |
There's no headless key; only headful tests need to be marked specifically.
* @test | ||
* @key headless | ||
* @bug 8328953 | ||
* @summary JEditorPane.read throws ChangedCharSetException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @summary JEditorPane.read throws ChangedCharSetException | |
* @summary Verifies JEditorPane.read doesn't throw ChangedCharSetException | |
but handles it and reads HTML in the specified encoding |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
"<html lang=\"ru\">\n" + | ||
"<head>\n" + | ||
" <meta http-equiv=\"Content-Type\" " + | ||
" content=\"text/html; charset=windows-1251\">\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
" content=\"text/html; charset=windows-1251\">\n" + | |
" content=\"text/html; charset=windows-1251\">\n" + |
I suggest moving it by one space to align to the start of the http-equiv
attribute.
String pText = document.getText(p.getStartOffset(), | ||
p.getEndOffset() - p.getStartOffset()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String pText = document.getText(p.getStartOffset(), | |
p.getEndOffset() - p.getStartOffset()); | |
String pText = document.getText(p.getStartOffset(), | |
p.getEndOffset() - p.getStartOffset()); |
Let's align the second argument to the opening parenthesis.
Document document = editorPane.getDocument(); | ||
// Shouldn't throw ChangedCharSetException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document document = editorPane.getDocument(); | |
// Shouldn't throw ChangedCharSetException | |
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the comment below, it looks fine.
Testing results are green. I'll run the tests once again after you update the code.
String pText = document.getText(p.getStartOffset(), | ||
p.getEndOffset() - p.getStartOffset()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String pText = document.getText(p.getStartOffset(), | |
p.getEndOffset() - p.getStartOffset()); | |
String pText = document.getText(p.getStartOffset(), | |
p.getEndOffset() - p.getStartOffset() - 1); |
The test fails for me without -1
because pText
contains \n
. It didn't fail when I tested it earlier.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are green now.
Thank you for your contribution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
@rjolly You've got two approvals now. You can issue the |
/integrate |
/sponsor |
Going to push as commit 245514d.
Your commit was automatically rebased without conflicts. |
@aivanov-jdk @rjolly Pushed as commit 245514d. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
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.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17567/head:pull/17567
$ git checkout pull/17567
Update a local copy of the PR:
$ git checkout pull/17567
$ git pull https://git.openjdk.org/jdk.git pull/17567/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 17567
View PR using the GUI difftool:
$ git pr show -t 17567
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17567.diff
Webrev
Link to Webrev Comment