On Thu, 11 Aug 2022 22:27:39 GMT, Harshitha Onkar <hon...@openjdk.org> wrote:

>> 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.
>
> Harshitha Onkar has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   review changes

A few comments which are mostly (or all?) nit picking about the javadoc of the 
method.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 265:

> 263:     /**
> 264:      * Position the instruction frame with testWindow (testcase created
> 265:      * window) by the specified position. If testWindow is null, only

The pre-existing docs don't read very well to me and some of it doesn't make 
sense, so in this update
we should try to fix that.

First sentence would be better as 
"Positions the instruction frame relative to the test window as specified by 
the {@code position} parameter.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 269:

> 267:      * parameter.
> 268:      * Note: This method should be invoked from the method that creates
> 269:      * testWindow. At test-level, the testWindow must be made visible

I think we should remove that Note sentence - surely the calling method is 
irrelevant. 
 It is sufficient to explain that 
"This method should be called before making the test window visible".
But why is that ?

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 272:

> 270:      * only after calling this method.
> 271:      *
> 272:      * @param testWindow test window that the test is created. It can be 
> null.

"It can be null" -> "May be {@code null}"

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 273:

> 271:      *
> 272:      * @param testWindow test window that the test is created. It can be 
> null.
> 273:      * @param position  position can either be:

position must be one of

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 278:

> 276:      *                  center and the test window (if not null) is 
> placed to
> 277:      *                  the right of the instruction frame.
> 278:      *

"vertical center" means half way between the top and the bottom which is 
absolutely not what you mean, is it ?

horizontal center surely ?

But what if the test window is > 0.5 the width of the screen ? Now it is off 
the edge .. wouldn't it be better
to move the instruction window to the left to accommodate it ?
Although that might be tricky if we don't know its size yet ..

-------------

PR: https://git.openjdk.org/jdk/pull/9525

Reply via email to