JDK-8340210 : Add positionTestUI() to PassFailJFrame.Builder #21023

honkar-jdk · 2024-09-16T19:11:59Z

positionTestUI() option is added to PassFailJFrame (PFJ).

With this change multiple UI can be positioned using the PFJ new builder pattern by providing implementation for the Functional Interface PositionWindows.positionTestWindows(List<? extends Window> testWindows,InstructionUI instructionUI) in the test code.

Since the position implementation is done in test code it allows flexibility as the user can add custom positioning code as per test UI requirements.

Usage:

PassFailJFrame.builder()
                      .title("Test Instructions")
                      .instructions(INSTRUCTIONS)
                      .rows(int)
                      .columns(int)
                      .testUI(<TestClass::createAndShowUI>)
                      .positionTestUI(<TestClass::positionMultiTestUI>)
                      .build()
                      .awaitAndCheck();

where positionMultiTestUI is the implementation for positioning of multiple test windows for PositionWindows.positionTestWindows(List<? extends Window> testWindows,InstructionUI instructionUI)

@aivanov-jdk has demonstrated custom test UI positioning in this PR:
8294156: Demo positioning of multiple test windows

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-8340210: Add positionTestUI() to PassFailJFrame.Builder (Enhancement - P4)

Reviewers

Alexey Ivanov (@aivanov-jdk - Reviewer)
Alexander Zvegintsev (@azvegint - Reviewer)

Contributors

Alexey Ivanov <aivanov@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21023

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-09-16T19:12:57Z

👋 Welcome back honkar! 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 · 2024-09-16T19:13:02Z

@honkar-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:

8340210: Add positionTestUI() to PassFailJFrame.Builder

Co-authored-by: Alexey Ivanov <aivanov@openjdk.org>
Reviewed-by: aivanov, azvegint

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

eabfc6e: 8337563: NMT: rename MEMFLAGS to MemTag
d588182: 8338686: App classpath mismatch if a jar from the Class-Path attribute is on the classpath
5dc9723: 8340323: Test jdk/classfile/OptionsTest.java fails after JDK-8340200
90e92f9: 8339790: Support Intel APX setzucc instruction
28d009c: 8339934: Simplify Math.scalb(double) method
3e14fb9: 8340200: Misspelled constant AttributesProcessingOption.DROP_UNSTABLE_ATRIBUTES
64e3a9e: 8339574: Behavior of File.is{Directory,File,Hidden} is not documented with respect to symlinks
f877016: 8315273: (fs) Path.toRealPath(LinkOption.NOFOLLOW_LINKS) fails when "../../" follows a link (win)
b39e6a8: 8329816: Add SLEEF version 3.6.1
80db6e7: 8339648: ZGC: Division by zero in rule_major_allocation_rate
... and 110 more: https://git.openjdk.org/jdk/compare/f0e84b7617aebc421483f36bb7d0b14d0fc39616...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 · 2024-09-16T19:13:51Z

@honkar-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.

honkar-jdk · 2024-09-16T19:14:21Z

/contributor add @aivanov-jdk

openjdk · 2024-09-16T19:15:54Z

@honkar-jdk
Contributor Alexey Ivanov <aivanov@openjdk.org> successfully added.

mlbridge · 2024-09-16T19:18:43Z

Webrevs

aivanov-jdk

Looks good to me.

It was previously discussed in #15665 when I added testUI. The interface and the field in the builder were defined but there was no way to pass an implementation of PositionWindows.

Since the position implementation is done in test code it allows flexibility as the user can add custom positioning code per test UI requirements.

There's an enhancement JDK-8317116: Provide layouts for multiple test UI in PassFailJFrame to provide a set of common layouts for tests to re-use. It would avoid code duplication for the most common layouts like columns or rows to the right or bottom of the instruction UI.

honkar-jdk · 2024-09-16T21:00:12Z

There's an enhancement JDK-8317116: Provide layouts for multiple test UI in PassFailJFrame to provide a set of common layouts for tests to re-use. It would avoid code duplication for the most common layouts like columns or rows to the right or bottom of the instruction UI.

Update PR description with the PR link related to JDK-8317116 in case anyone wants to explore custom positioning of multiple UIs.

honkar-jdk · 2024-09-16T21:31:24Z

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java

+            if (windowListCreator == null || testWindows == null) {
+                throw new IllegalStateException("Neither a windows list creator "
+                                                + "nor a list of test windows "
+                                                + "has been provided");
+            }


@aivanov-jdk Running into exception when this check is added here. I believe Builder's windowListCreator and testWindows are not assigned yet at this stage.

And since this check is already in-place here: validate(), I'm removing it from positionTestUI().

The condition should've been &&: the error message is correct, the exception was supposed to be thrown if both windowListCreator and testWindows is null. The condition with || is always true.

Yet I agree to remove this condition from here as it enforces the order of calls, which is not strictly required, even though it would common to add a window creator before setting positioning.

honkar-jdk · 2024-09-16T23:53:09Z

@azvegint You may want to review this PR, as it is related to #15721

azvegint

I think that this can be improved by adding some predefined positionTestUI functions, like in the #15721 (we can borrow them from it).
For example, consider adding PositionWindowsLib.java(or maybe a nested public static class for PassFailJFrame to avoid extra @build clause), which has functions something like

.positionTestUI(PositionWindowsLib::positionHH) // aka TwoWindowsHH.positionTestUI
.positionTestUI(PositionWindowsLib::positionHV) // aka TwoWindowsHV.positionTestUI
...

It'll be easier for a test developer to just take a ready-to-use function (which in most cases will be the same across multiple tests),
and it'll be easier for the test reviewer because there will be less code to review.

aivanov-jdk · 2024-09-17T09:21:24Z

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java

+         * @throws IllegalStateException if the {@code positionWindows} field
+         *              is already set, or if neither a windows list creator
+         *              nor a list of test windows has been provided


Suggested change

* @throws IllegalStateException if the {@code positionWindows} field

* is already set, or if neither a windows list creator

* nor a list of test windows has been provided

* @throws IllegalStateException if the {@code positionWindows} field

* is already set

aivanov-jdk · 2024-09-17T09:45:03Z

I think that this can be improved by adding some predefined positionTestUI functions, like in the #15721 (we can borrow them from it).

I agree that we should — and we will. Yet I don't see missing default layouts as stopper from integrating this PR.

Adding a library of common layouts will take more time — to add layouts and to review — and @honkar-jdk wants to use the feature right now. It's better to do it with what's currently available rather than implement tricks to position two test UI windows. @kumarabhi006 also asked about the feature.

Once tests are created, or even right now, we can submit bugs to update the test and use a common layout rather than one in the test and link those bugs as blocked by JDK-8317116.

There's already a test java/awt/event/KeyEvent/AcceleratorTest/AcceleratorTest.java which uses two test UI windows, yet it doesn't do anything to position them:

jdk/test/jdk/java/awt/event/KeyEvent/AcceleratorTest/AcceleratorTest.java

Lines 157 to 161 in c6721a0

    
           PassFailJFrame.positionTestWindow(jfr, PassFailJFrame.Position.HORIZONTAL); 
        
           jfr.setSize(200, 200); 
        
           PassFailJFrame.positionTestWindow(output, PassFailJFrame.Position.HORIZONTAL); 
        
           output.setSize(200, 200);

This test could be updated to use logging area instead of two test windows. Either way works.

For example, consider adding PositionWindowsLib.java(or maybe a nested public static class for PassFailJFrame to avoid extra @build clause)

I've thought about it. We can add the most common layout right into PassFailJFrame to avoid additional @build tag. It could even be in a separate class, as you suggest, if it's referenced by PassFailJFrame, it will be automatically compiled by jtreg.

I think we should add convenience methods positionTestUIRows, positionTestUIColumns, it'll make using the common layouts even simpler. If the convenience methods reference another class, it should be compiled automatically; if it doesn't work, we will put the layouts as a nested class or as methods directly into PassFailJFrame.

aivanov-jdk · 2024-09-17T09:47:13Z

There has been no need for multiple test windows for nearly a year, it's not the most used feature. This is why I think we can add common layouts later rather than rush it right now.

A few tests are also easy to update, and these tests would be a live demo for the improvement.

aivanov-jdk

I wanted to request the changes instead: the description of the method needs updating as the result of removing the check for window creator or window list.

honkar-jdk · 2024-09-17T17:28:50Z

I think that this can be improved by adding some predefined positionTestUI functions, like in the #15721 (we can borrow them from it). For example, consider adding PositionWindowsLib.java(or maybe a nested public static class for PassFailJFrame to avoid extra @build clause), which has functions something like
.positionTestUI(PositionWindowsLib::positionHH) // aka TwoWindowsHH.positionTestUI
.positionTestUI(PositionWindowsLib::positionHV) // aka TwoWindowsHV.positionTestUI
...

@azvegint Valid suggestion and re-usability of common layouts is better than having to repeat it in the test code. This will be added as next step to PassFailJFrame iterative improvement as @aivanov-jdk suggested.

honkar-jdk · 2024-09-17T20:04:37Z

/integrate

openjdk · 2024-09-17T20:05:49Z

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

eabfc6e: 8337563: NMT: rename MEMFLAGS to MemTag
d588182: 8338686: App classpath mismatch if a jar from the Class-Path attribute is on the classpath
5dc9723: 8340323: Test jdk/classfile/OptionsTest.java fails after JDK-8340200
90e92f9: 8339790: Support Intel APX setzucc instruction
28d009c: 8339934: Simplify Math.scalb(double) method
3e14fb9: 8340200: Misspelled constant AttributesProcessingOption.DROP_UNSTABLE_ATRIBUTES
64e3a9e: 8339574: Behavior of File.is{Directory,File,Hidden} is not documented with respect to symlinks
f877016: 8315273: (fs) Path.toRealPath(LinkOption.NOFOLLOW_LINKS) fails when "../../" follows a link (win)
b39e6a8: 8329816: Add SLEEF version 3.6.1
80db6e7: 8339648: ZGC: Division by zero in rule_major_allocation_rate
... and 110 more: https://git.openjdk.org/jdk/compare/f0e84b7617aebc421483f36bb7d0b14d0fc39616...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-09-17T20:05:55Z

@honkar-jdk Pushed as commit f0ae90f.

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

add positionTestUI() to PFJ builder pattern

081b3fe

openjdk bot added the rfr Pull request is ready for review label Sep 16, 2024

openjdk bot added the client client-libs-dev@openjdk.org label Sep 16, 2024

aivanov-jdk approved these changes Sep 16, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 16, 2024

honkar-jdk changed the title ~~JDK-8340210 : Add positionTestUI() to PassFailJFrame builder pattern~~ JDK-8340210 : Add positionTestUI() to PassFailJFrame.Builder Sep 16, 2024

openjdk bot added ready Pull request is ready to be integrated and removed ready Pull request is ready to be integrated labels Sep 16, 2024

removed errorenous check

5a331fd

honkar-jdk commented Sep 16, 2024

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Sep 16, 2024

azvegint reviewed Sep 17, 2024

View reviewed changes

aivanov-jdk approved these changes Sep 17, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 17, 2024

aivanov-jdk suggested changes Sep 17, 2024

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Sep 17, 2024

doc update

cb342a8

aivanov-jdk approved these changes Sep 17, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 17, 2024

azvegint approved these changes Sep 17, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Sep 17, 2024

openjdk bot closed this Sep 17, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 17, 2024

honkar-jdk mentioned this pull request Sep 18, 2024

8340015: Open source several AWT focus tests - series 7 #21008

Closed

3 tasks

JDK-8340210 : Add positionTestUI() to PassFailJFrame.Builder #21023

JDK-8340210 : Add positionTestUI() to PassFailJFrame.Builder #21023

Uh oh!

Conversation

honkar-jdk commented Sep 16, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Sep 16, 2024

Uh oh!

openjdk bot commented Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Sep 16, 2024

Uh oh!

honkar-jdk commented Sep 16, 2024

Uh oh!

openjdk bot commented Sep 16, 2024

Uh oh!

mlbridge bot commented Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

aivanov-jdk left a comment

Choose a reason for hiding this comment

Uh oh!

honkar-jdk commented Sep 16, 2024

Uh oh!

honkar-jdk Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

honkar-jdk commented Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azvegint left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

honkar-jdk Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aivanov-jdk commented Sep 17, 2024

Uh oh!

aivanov-jdk left a comment

Choose a reason for hiding this comment

Uh oh!

honkar-jdk commented Sep 17, 2024

Uh oh!

honkar-jdk commented Sep 17, 2024

Uh oh!

openjdk bot commented Sep 17, 2024

Uh oh!

openjdk bot commented Sep 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

honkar-jdk commented Sep 16, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 16, 2024 •

edited

Loading

mlbridge bot commented Sep 16, 2024 •

edited

Loading

honkar-jdk Sep 16, 2024 •

edited

Loading

honkar-jdk commented Sep 16, 2024 •

edited

Loading

azvegint left a comment •

edited

Loading

aivanov-jdk commented Sep 17, 2024 •

edited

Loading