On Wed, 14 Feb 2024 19:11:42 GMT, Alexey Ivanov <[email protected]> wrote:
>> test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 1229:
>>
>>> 1227: * @throws IllegalArgumentException if {panelCreator} is
>>> {@code null}
>>> 1228: */
>>> 1229: public Builder splitUI(PanelCreator panelCreator) {
>>
>> Suggestion:
>>
>> public Builder splitUI(PanelCreator panelCreator, int
>> splitUIOrientation ) {
>>
>>
>>
>> Would it be easier if we use `splitUIOrientation` or an enum similar to
>> [Position](https://github.com/openjdk/jdk/blob/b823fa44508901a6bf39795ab18991d055a71b4e/test/jdk/java/awt/regtesthelpers/PassFailJFrame.java#L191)
>> to determine the split than have three different methods - `splitUI,
>> splitUIRight & splitUIBottom`? And eliminate the private `splitUI()` and
>> have the orientation logic within public `splitUI()`
>
> I don't think so… I find the `Position` cumbersome too.
>
> Using `JSplitPane.HORIZONTAL_SPLIT` or `JSplitPane.VERTICAL_SPLIT` in a
> public method of Builder exposes the internal detail that `JSplitPane` is
> used. What if we decide to implement it differently? After all, the UI could
> be placed into a horizontal or vertical
> [`Box`](https://docs.oracle.com/en/java/javase/21/docs/api/java.desktop/javax/swing/Box.html)?
> Passing an `int` as the orientation is not type-safe, we'll need to verify
> the parameter or let `JSplitPane` constructor do it.
>
> An enum is type-safe but it's cumbersome to type:
>
>
> splitUI(() -> new JPanel(), PassFailJFrame.SplitOrientation.HORIZONTAL)
> // or
> splitUI(() -> new JPanel(), JSplitPane.HORIZONTAL_SPLIT)
>
> does not look as good to me as
>
> splitUIRight(() -> new JPanel())
>
> `-Right` explicitly conveys the chosen position and is much shorter than the
> above samples with `enum` or constants from `JSplitPane`.
>
> Initially, I wanted to re-use `Position` but decided not to … for
> flexibility. Split UI could be combined with additional test UI windows… Not
> that I see a use case for such a use…
>
> The three methods make the `Builder` more crowded for the sake of brevity of
> its usage in the test code. I expect it would be `splitUI` in majority of
> cases.
In terms of flexibility & future extensibility (if any) your approach works
better.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17845#discussion_r1490152053
