-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
JDK-8290469: Add new positioning options to PassFailJFrame test framework #9525
Conversation
👋 Welcome back honkar! A progress list of the required criteria for merging this PR into |
@honkar-jdk The following label will be automatically applied to this pull request:
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. |
Webrevs
|
*/ | ||
public static void positionTestWindow(Window testWindow, Position position) { | ||
Dimension screenSize = Toolkit.getDefaultToolkit().getScreenSize(); | ||
|
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.
In this method we "randomly" use getX(), or getLocation().x and getY() or and getLocation().y
Why can't we be consistent and pick one style ? Seems like getX() and getY() would fit best here.
* VERTICAL - both test instruction frame and test window | ||
* as arranged up and down. | ||
* TOP_LEFT_CORNER - test instruction frame positioned at | ||
* top left corner with main test window beside it. |
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.
All of these descriptions are lacking in explaining what goes where
eg
HORIZONTAL should say the instruction window will be placed so that the right edge is to the left of the screen's horizontal centre and the test window will be placed just to the right of it.
TOP_LEFT_CORNER should say the instruction window will be placed so that its top left corner is at the top left corner of the screen and the test window will be placed just to the right of the instruction window.
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.
@prrace I have slightly changed the description as per your suggestion. Please let me know if any changes are required.
HORIZONTAL - the test instruction frame is positioned such that its right edge aligns with screen's vertical center and the test window is placed to the right of the instruction frame.
VERTICAL - the test instruction frame is positioned such that its bottom edge aligns with the screen's horizontal center and the test window is placed below the instruction frame.
TOP_LEFT_CORNER - the test instruction frame is positioned such that its top left corner is at the top left corner of the screen and the test window is placed to the right of the instruction frame. (Taskbar placed at the top of the screen in this example)
} else if (position.equals(Position.VERTICAL)) { | ||
int newY = ((screenSize.height / 2) - frame.getHeight()); | ||
frame.setLocation(frame.getX(), newY); | ||
|
||
testWindow.setLocation(frame.getX(), | ||
(frame.getLocation().y + frame.getHeight() + 5)); | ||
} | ||
else if (position.equals(Position.TOP_LEFT_CORNER)) { | ||
frame.setLocation(0,0); |
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 am very sceptical of this. With a toolbar on the left and a menu bar on the top of the screen I bet you
are neither at x=0 or y=0.
And this code is not allowing the desktop to reposition it and report the updated REAL position back to you
The result will be overlapping windows.
I'm actually dubious of the existing code for much the same reason but it just likely hasn't been seen as a problem there because positioning in the middle of the screen usually gets you where you asked for.
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.
@prrace Thank you for reviewing. I do see your point - when taskbars and menu bars are located on top or left side of the screen, they would overlap the test instruction frame.
Two possible solutions:
-
Going with a fixed amount of offset on where to position the top-left corner of the instruction frame window. Probably this would not be ideal considering different screen settings and platforms.
-
The other approach would be to use GraphicsEnvironment.getMaximumWindowBounds() to get the displayable window size and position the instruction frame accordingly ?
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.
More than that - this code needs to call setlocation, and do "whatever" to make sure that is actually pushed to the "window manager" and the REAL location of the window come back. The way the code is written it sets a couple of vars and then reads back its own values .. no chance of finding the REAL location
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.
In the recent update the following points are addressed:
- Screen insets are used to account of taskbar position and the placement of the instruction frame at top-left corner
- Position option description update
Changes regarding pushing the latest position to window manager are still being evaluated and in progress.
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.
Following is a proposed solution to push the latest position changes to the window manager.
The get and setLocation calls are wrapped in invokeLater(). Robot.waitForIdle() are added between setLocation() and the subsequent getLocation() calls. The waitForIdle() calls ensures that any pending location updates are pushed to the window manager before the new location of the frame is in turn used to position the testWindow.
In multiple test window cases ONLY the test instruction frame and the main/root test window are positioned using positionTestWindow()
, the rest of the windows are positioned within EDT at test-level.
Impact of the proposed fix:
With this change positionTestWindow()
can no longer be called on EDT at test-level, hence any existing manual tests calling this method on EDT requires changes.
For example: In this particular test, the following line PassFailJFrame.positionTestWindow(frame,PassFailJFrame.Position.HORIZONTAL)
should be called after createAndShowGUI()
PassFailJFrame.positionTestWindow(frame, |
public static void positionTestWindow(Window testWindow, Position position) throws AWTException {
if (isEventDispatchThread()) {
throw new Exception("positionTestWindow() should not be called on EDT");
}
robot = new Robot();
Dimension screenSize = Toolkit.getDefaultToolkit().getScreenSize();
if (position.equals(Position.HORIZONTAL)) {
SwingUtilities.invokeAndWait(() -> {
int newX = ((screenSize.width / 2) - frame.getWidth());
frame.setLocation(newX, frame.getY());
});
// waits until the queue is flushed
robot.waitForIdle();
robot.delay(300);
SwingUtilities.invokeAndWait(() -> testWindow.setLocation((frame.getX() +
frame.getWidth() + 5), frame.getY()));
} else if (position.equals(Position.VERTICAL)) {
SwingUtilities.invokeAndWait(() -> {
int newY = ((screenSize.height / 2) - frame.getHeight());
frame.setLocation(frame.getX(), newY);
});
robot.waitForIdle();
robot.delay(300);
SwingUtilities.invokeAndWait(() -> testWindow.setLocation(frame.getX(),
(frame.getY() + frame.getHeight() + 5)));
} else if (position.equals(Position.TOP_LEFT_CORNER)) {
SwingUtilities.invokeAndWait(() -> {
GraphicsConfiguration gc = GraphicsEnvironment.getLocalGraphicsEnvironment()
.getDefaultScreenDevice().getDefaultConfiguration();
Insets screenInsets = Toolkit.getDefaultToolkit().getScreenInsets(gc);
frame.setLocation(screenInsets.left, screenInsets.top);
});
robot.waitForIdle();
robot.delay(300);
SwingUtilities.invokeAndWait(() -> testWindow.setLocation((frame.getX() +
frame.getWidth() + 5), frame.getY()));
}
}
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 should discuss that impact.
This method is useful to simplify the logic of individual tests but is it now becoming awkward to use ?
Or is it something that we should have required from the beginning ?
And why invokeLater and not invokeAndWait ? I'd have expected Later to not work for this case ..
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 interleaving does seem to make the code more complex. And at test-level, it causes some confusion as few methods need to be called within EDT and few out of EDT.
Initially there were just two positions - HORIZONTAL, VERTICAL and the issue wasn't evident probably, but with multiple frames scenario and TOP_LEFT corner positioning this issue became more evident.
I chose invokeLater earlier because waitForIdle
is called after it and it waits until the queue is empty. But I think invokeAndWait would be a better option here. In the above code snippet (proposed fix) invokeLater()
is now replaced with invokeAndWait()
.
Additionally, I want check if anyone has a simpler and much cleaner approach. At this stage evaluating and rewriting the framework (if required) is manageable. In future, as more and more manual test cases happen to use this framework, the amount of rework needed might be more.
Another possible approach - Use of JSPLITPANE , details added below.
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.
@prrace Updated the PR - added Toolkit.sync and Thread.sleep. Additionally, added few more changes as mentioned here - #9525 (comment) & #9525 (comment)
Another approach would be to use a JSplitPane to have the instruction frame and the main test frame as one window. Left half/top as instruction frame and Right half/ bottom as the main test window and this also provides HORIZONTAL_SPLIT or VERTICAL_SPLIT orientation equivalent to HORIZONTAL and VERTICAL positioning of PassFailJFrame. Advantage of this approach:
|
I can confirm that on Linux / Gnome with menu bar at the top and Calling Toolkit.sync() isn't enough to ensure it is always correct. Calling isVisible() or isShowing() don't help. The simplest thing that works every time for me is to add a short sleep() public class F { public static void main(String[] args) throws Exception { public static void doit() {
} |
@prrace Thank you for checking it on Linux. Adding a delay would be the best option to get the updated location values in this scenario then. Another approach that I had in mind was to change PassFailJFrame to use a JSplitPane (single window) for instructions and test window. This way there is no hassle of managing the positioning of 2 windows as described above #9525 (comment) Please let me know your suggestions on JSplitPane approach. |
I think it very unlikely a JSplitPane would satisfy all test requirements. |
@prrace Do you mean - a multiple frame test scenario or any other scenario in particular? We could readjust the splits (divider location) based on different test requirements and make the test window optional (if required). |
Well .. there surely must be test scenarios where a Frame is required. Perhaps the test moves it, iconifies, needs I can imagine that it might be interesting to add a new version that works with a JPanel as the container for the test and let a test author decide if they want to use that for future tests. |
@prrace I see your point and the broader range of testing requirements that might need to be supported by the test framework in future such as iconifying/ minimizing test frames. Earlier when I suggested JSplitPane, I was looking it from ease of positioning, disposing and that most of the manual tests had more or less - one instruction frame and one test window which could be achieved using the JSplitPane. I probably missed analyzing the part about broader support of framework for future test cases.
|
I don't think In some tests, the test UI consists of a single button only. I think a single window for such test UI would work better than two separate and independent frames. One frame is easier to manage. I was thinking about merging instructions and test UI from the start. Yet using two frames / windows seemed easier to implement. Some test cases may not fit into either pattern. There's always the option to create test UI as needed. Yet positioning of the instruction frame may be an issue. If test creates windows / dialogs owned by the instruction frame, AWT ensures the test UI windows / dialogs are above the instruction frame; however, if test UI is large enough, it could completely overlap the instructions. I guess some test cases may even be easier to implement and maintain with its own custom UI. Too many options in |
After evaluating possible alternative solutions as well as suggestions provided by @prrace & @aivanov-jdk, I think introducing small delay (Thread.sleep) would be the best option - for syncing the updated position to window manager and keeping in mind the broader range of testing requirement that the framework needs to support for now. |
The recent commit has the following changes -
|
Added null check for In some tests testWindow is not required, for example TrayIconScalingTest.java in which case we can call |
public static void positionTestWindow(Window testWindow, Position position) { | ||
Dimension screenSize = Toolkit.getDefaultToolkit().getScreenSize(); | ||
|
||
// to get the screen insets inorder to position the frame by taking into | ||
// account the location of taskbar/menubars on screen | ||
GraphicsConfiguration gc = GraphicsEnvironment.getLocalGraphicsEnvironment() | ||
.getDefaultScreenDevice().getDefaultConfiguration(); | ||
Insets screenInsets = Toolkit.getDefaultToolkit().getScreenInsets(gc); | ||
|
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 suggest separating the logic to calculate coordinates into a separate function, or save them into a local variable.
Then call sync
and sleep
. Or this sequence could be extracted into its own helper method to avoid code duplication, it's repeated at least twice.
Shall this method call window.setVisible(true)
automatically?
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.
@aivanov-jdk As suggested, separating the common code into a helper method would make the code look cleaner.
Earlier I did think about having the setVisible for both the testWindow and instruction frame within the positionWindow but usually at the test-level, the user would be in the habit of creating the testWindow and setting its visibility to true when creating test UI. Hence left it to be set at test-level. I can change it, if this approach is better.
Either way we need to make minor changes to existing manual tests - remove (in case we add testWindow.setVisible(true) to PassFailJFrame) or reposition (in case we retain it at test-level) setVisible() call.
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.
Toolkit.sync + Thread.sleep extracted to a separate helper method.
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.
Earlier I did think about having the setVisible for both the testWindow and instruction frame within the positionWindow but usually at the test-level, the user would be in the habit of creating the testWindow and setting its visibility to true when creating test UI. Hence left it to be set at test-level. I can change it, if this approach is better.
Either way we need to make minor changes to existing manual tests - remove (in case we add testWindow.setVisible(true) to PassFailJFrame) or reposition (in case we retain it at test-level) setVisible() call.
@aivanov-jdk I have retained setVisible()
for testWindow on test side (where test UI is created) for reasons as described above. Please let me know in case it is better to have it within positionWindow()
.
Toolkit.getDefaultToolkit().sync(); | ||
try { | ||
Thread.sleep(500); | ||
} catch (InterruptedException e) { | ||
e.printStackTrace(); | ||
} |
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 safe to call on EDT?
If positionTestWindow
is not to be called on EDT, should it be enforced?
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 intention is that it can be called either way - on or off.
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 that possible? Toolkit thread is not EDT, therefore native events are processed, it's probably enough to update the frame coordinates while EDT is blocked and doesn't handle events.
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.
Can you expand on what you mean ?
What's the problem with native events being processed ? We want that.
And this obviously isn't being called on the toolkit thread.
This change isn't doing anything to that method that might change threading reqts or rules of use.
Any problems already existed. What is being changed (apart from adding the new API) is just to make it more robust.
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 intention is that it can be called either way - on or off.
@aivanov-jdk As @prrace mentioned, the main reason for opting - Toolkit.sync + Thread.sleep approach instead of robot.waitForIdle() was to give the test user the flexibility to call positionTestWindow
on or off EDT. (since test cases can contain either AWT or Swing components). #9525 (comment)
With waitForIdle()
approach we had to wrap parts of the code within EDT and part of it out of EDT, this was confusing and complex.
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.
@prrace I'm just trying to understand how it works and whether it changes threading requirements. Thread.sleep
blocks the EDT, no events are handled. If processing native events is enough to update the frame coordinates, that's good.
Thank you both for clarifying.
Based on this discussion #9525 (comment), Added the term "approximately positions" to positionTestWindow() javadoc. |
/integrate |
@honkar-jdk |
The above suggestion was meant to be added by @lawrence-andrew as part of the test framework redesign discussion #9525 (comment) |
* window as specified by the {@code position} parameter. If testWindow | ||
* is null, only the instruction frame is positioned according to | ||
* {@code position} parameter. This method should be called before making | ||
* the test window visible. |
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.
Since this condition is documented, should we also implement check to handle it by calling isShowing() and throw some warning/exception if its visible.
We do the opposite for getLocationOnScreen() where we throw exception if the compoment is not showing.
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.
Since this condition is documented, should we also implement check to handle it by calling isShowing() and throw some warning/exception if its visible. We do the opposite for getLocationOnScreen() where we throw exception if the compoment is not showing.
It's more like a recommendation rather than a requirement. It is to avoid flickering when the window gets displayed for a very short period of time at its default position before being moved to its final position.
The same had happened to the instruction frame before Harshitha moved setVisible(true)
.
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.
Yes, as described by @aivanov-jdk ,this was added to avoid flickering (happens when windows move from the initial to the final position).
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
Capturing the entire screen is a better option. The test may create other windows, the image of the entire screen would be more useful. |
/integrate |
@honkar-jdk |
/sponsor |
Going to push as commit 568be58.
Your commit was automatically rebased without conflicts. |
@aivanov-jdk @honkar-jdk Pushed as commit 568be58. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Additional position setting (TOP_LEFT_CORNER) and a method to obtain bounds of test instruction frame are added to PassFailJFrame to handle positioning of multiple test frames.
In scenarios where multiple test windows might be present, the test windows might overlap the instruction frame. In order to fix this TOP_LEFT_CORNER position option is added that positions the test instruction frame at top left corner with main test window below it.
Additionally
getInstructionFrameBounds()
is added to obtain the position and dimensions of test instruction frame.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9525/head:pull/9525
$ git checkout pull/9525
Update a local copy of the PR:
$ git checkout pull/9525
$ git pull https://git.openjdk.org/jdk pull/9525/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9525
View PR using the GUI difftool:
$ git pr show -t 9525
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9525.diff