Skip to content
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

8294158: HTML formatting for PassFailJFrame instructions #15660

Closed
wants to merge 5 commits into from

Conversation

aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Sep 11, 2023

This enhancement provides HTML formatting for instructions in the manual test framework, PassFailJFrame.

Some tests could benefit from rich-formatted instructions, especially when the instructions are long.

See a sample usage in #15661.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8294158: HTML formatting for PassFailJFrame instructions (Enhancement - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15660/head:pull/15660
$ git checkout pull/15660

Update a local copy of the PR:
$ git checkout pull/15660
$ git pull https://git.openjdk.org/jdk.git pull/15660/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15660

View PR using the GUI difftool:
$ git pr show -t 15660

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15660.diff

Webrev

Link to Webrev Comment

Instructions for FileChooserSymLinkTest are rendered in HTML.
Each text component (JTextArea or JEditorPane) are configured
in its own dedicated method. This avoids unnecessary type casts.
@bridgekeeper
Copy link

bridgekeeper bot commented Sep 11, 2023

👋 Welcome back aivanov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Sep 11, 2023

@aivanov-jdk The following label will be automatically applied to this pull request:

  • client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Sep 11, 2023
@aivanov-jdk aivanov-jdk marked this pull request as ready for review September 11, 2023 13:18
@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 11, 2023
It reverts the accidental change where I unwrapped call to createUI
@mlbridge
Copy link

mlbridge bot commented Sep 11, 2023

Webrevs

instructionsText.setEditable(false);
instructionsText.setLineWrap(true);

JTextComponent text = instructions.startsWith("<html>")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably trim() and toLowerCase() could be useful here.

Do we consider adding adding one more builder method to force enable html rendering?
(e.g. when we just want to highlight only few words/sentences with <b> without adding <html> tag)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably trim() and toLowerCase() could be useful here.

Could be… On the other hand, in post XHTML era, the elements are usually in lower-case. Thus we should stick with it and use lowercase HTML elements anyway.

I think startsWith("<html>") covers the scenario well.

Although neither <html> nor <body> are required, I prefer to always add them. These two elements don't add much overhead, do they?

Do we consider adding adding one more builder method to force enable html rendering? (e.g. when we just want to highlight only few words/sentences with <b> without adding <html> tag)

I don't think it's needed. If you want the instructions rendered as HTML, you'll have to format them as HTML.

Forcing HTML rendering on plain text which highlights a couple of words with <b> will render the text unreadable: line ends are ignored by HTML, so you'll have to use <br> element to force line breaks. (A similar thing often happens in javadoc where a blank line is added as if to start a new paragraph, but without the explicit <p> element the text continues on the same line / paragraph.)

It is possible to wrap such instructions into <html><body><pre> but what stops you from doing it explicitly if it's what you want? (Note, however, the <pre> element disables line wrapping.)

I'd rather keep it simple.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be… On the other hand, in post XHTML era, the elements are usually in lower-case. Thus we should stick with it and use lowercase HTML elements anyway.

https://www.w3schools.com/html/html_elements.asp

HTML is Not Case Sensitive

HTML tags are not case sensitive: <P> means the same as <p>.

The HTML standard does not require lowercase tags, but W3C recommends lowercase in HTML, and demands lowercase for stricter document types like XHTML.

At W3Schools we always use lowercase tag names.

So this is still a recommendation, not a requirement (we don't use XHTML).
But I agree that this can be helpful for a certain amount of consistency across tests(they can still use other tags in uppercase).

Although neither nor are required, I prefer to always add them. These two elements don't add much overhead, do they?

It is a matter of personal taste.
Normally, I prefer to indent a content of such tags by extra spaces, andThis can create additional formatting difficulties under the 80(120) character limit per line.
In those cases, I would prefer to omit those tags.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know HTML is not case sensitive. It's just HTML tags are always in lower case in recent years.

But I agree that this can be helpful for a certain amount of consistency across tests(they can still use other tags in uppercase).

Yep! In a way, this will enforce <html> in lower case, and I hope all other tags in the instructions.

Although neither nor are required, I prefer to always add them. These two elements don't add much overhead, do they?

It is a matter of personal taste.

Yes, it is… which could create inconsistencies.

Normally, I prefer to indent a content of such tags by extra spaces, and
This can create additional formatting difficulties under the 80(120) character limit per line.
In those cases, I would prefer to omit those tags.

I don't expect too many nesting levels in instructions. However, I agree it could be problematic…

In the sample in #15661, I keep <html><body> and the content on the same level.

Now that I looked at it again, I realised that I didn't follow the indentation consistently: first-level <li> aren't indented but third-level <li> are. I should correct this.

Because <pre> is used to format pieces of code, I decided not to indent the first-level <li> elements to avoid indentation of the code inside.

Copy link
Contributor

@prrace prrace Sep 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a problem with keeping it simple. If some one writing a test doesn't see the HTML interpreted, they'll quickly be able to figure out why.
One thing about these and the other enhancements I see is that there's an (increasing) need for a nice bit of documentation in PassFailJFrame itself about all its features.
Easiest to do this as you are adding them, so a doc comment block can be started with this one.
Probably as a class comment.

Suggested text
/**

  • PassFailJFrame documentation
  • Instructions for the user can be either plain text or HTML as supported by Swing.
  • Text beginning with <html> is presumed to be HTML.
    */

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing about these and the other enhancements I see is that there's an (increasing) need for a nice bit of documentation in PassFailJFrame itself about all its features.
Easiest to do this as you are adding them, so a doc comment block can be started with this one.
Probably as a class comment.

Yes, I absolutely agree, the class requires adding docs. It's what I wrote in PR #15665: the entire class requires a bit of TLC for documentation.

Yet I'd rather do it for PR #15665, it is easier to handle in one place rather than writing docs for one feature just to modify them right away for another feature.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prrace I've added description on how to use PassFailJFrame to #15665, it handles both HTML formatting for instructions (this PR) as well as creating test UI by passing a method reference, or rather an implementation of an interface.

@openjdk
Copy link

openjdk bot commented Sep 12, 2023

@aivanov-jdk 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:

8294158: HTML formatting for PassFailJFrame instructions

Reviewed-by: azvegint, prr

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 616 new commits pushed to the master branch:

  • 1165037: 8318569: Add getter methods for Locale and Patterns in ListFormat
  • 6f35274: 8318702: G1: Fix nonstandard indentation in g1HeapTransition.cpp
  • e272098: 8318160: javac does not reject private method reference with type-variable receiver
  • 54c613a: 8318693: Fix rendering for code blocks nested under list items in building.md
  • e67550c: 8318509: x86 count_positives intrinsic broken for -XX:AVX3Threshold=0
  • 8879c78: 8317689: [JVMCI] include error message when CreateJavaVM in libgraal fails
  • f9795d0: 8318222: RISC-V: C2 CmpU3
  • d1077d6: 8316046: x64 platforms unecessarily save xmm16-31 when UseAVX >= 3
  • 21d8a47: 8318701: Fix copyright year
  • d4b7612: 8318240: [AIX] Cleaners.java test failure
  • ... and 606 more: https://git.openjdk.org/jdk/compare/35bccacb6618e9ec686be895a9ef6ba8f3375ef0...master

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 12, 2023
@aivanov-jdk
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Oct 25, 2023

Going to push as commit 14090ef.
Since your change was applied there have been 625 commits pushed to the master branch:

  • d2d1592: 8318713: G1: Use more accurate age in predict_eden_copy_time_ms
  • c3cdfe2: 8318692: Add instructions for creating Ubuntu-based sysroot for cross compilation
  • ba7d08b: 8316961: Fallback implementations for 64-bit Atomic::{add,xchg} on 32-bit platforms
  • d7205e6: 8318102: macos10.14 check in CSystemColors can be removed.
  • 5ce718e: 8318100: Remove redundant check for Metal support
  • f1dfdc1: 8311877: [macos] Add CLI options to provide signing identity directly to codesign and productbuild
  • 9c819fd: 8318051: Duration.between uses exceptions for control flow
  • 1ddf826: 8316964: Security tools should not call System.exit
  • 1f2a80b: 8318306: java/util/Arrays/Sorting.java fails with "Array is not sorted at 8228-th position: 8251.0 and 8153.0"
  • 1165037: 8318569: Add getter methods for Locale and Patterns in ListFormat
  • ... and 615 more: https://git.openjdk.org/jdk/compare/35bccacb6618e9ec686be895a9ef6ba8f3375ef0...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 25, 2023
@openjdk openjdk bot closed this Oct 25, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 25, 2023
@openjdk
Copy link

openjdk bot commented Oct 25, 2023

@aivanov-jdk Pushed as commit 14090ef.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@aivanov-jdk aivanov-jdk deleted the 8294158-html-PassFail branch October 25, 2023 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client-libs-dev@openjdk.org integrated Pull request has been integrated
3 participants