RFR: 4346610: Adding JSeparator to JToolBar "pushes" buttons added after separator to edge [v2]

[Alexey Ivanov]

On Wed, 2 Aug 2023 06:36:29 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

>> Adding buttons in a JToolBar after a JSeparator will push the button to the far right of the frame instead of just after the separator
>> 
>> ![image](https://github.com/openjdk/jdk/assets/43534309/7fcad657-493f-4370-b046-b31c212a8aa7)
>> 
>> This is because BasicSeparatorUI does not define a maximum size. This leads to the separator getting all the extra width.
>> Fix is made to limit the separator's maximum size to the preferred size of corresponding orientation (i.e. width for VERTICAL and height for HORIZONTAL)
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review comments

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSeparatorUI.java line 32:

> 30: import java.awt.Graphics;
> 31: import java.awt.Insets;
> 32: import java.awt.Rectangle;

Neither `Insets`, nor `Rectangle` are used here.
Suggestion:

test/jdk/javax/swing/JToolBar/ToolBarSeparatorTest.java line 67:

> 65:         try {
> 66:             SwingUtilities.invokeAndWait(() -> {
> 67:                 frame = new JFrame("Troy's ToolBarTest");

Who is Troy?
Suggestion:

                frame = new JFrame("ToolBar Separator Test");

test/jdk/javax/swing/JToolBar/ToolBarSeparatorTest.java line 69:

> 67:                 frame = new JFrame("Troy's ToolBarTest");
> 68:                 toolBar = new JToolBar();
> 69:                 toolBar.setMargin(new Insets(0,0,0,0));

Suggestion:

                toolBar.setMargin(new Insets(0, 0, 0, 0));

test/jdk/javax/swing/JToolBar/ToolBarSeparatorTest.java line 71:

> 69:                 toolBar.setMargin(new Insets(0,0,0,0));
> 70:                 btn = new JButton("button 1");
> 71:                 toolBar.add(btn);

Suggestion:

                toolBar.add(new JButton("button 1"));

The field `btn` is not used for anything else any more, it can be removed.

test/jdk/javax/swing/JToolBar/ToolBarSeparatorTest.java line 88:

> 86:             SwingUtilities.invokeAndWait(() -> {
> 87:                 pt = toolBar.getLocationOnScreen();
> 88:                 size = toolBar.getSize();

Suggestion:

                toolBarBounds = new Rectangle(toolBar.getLocationOnScreen(),
                                              toolBar.getSize());

Where `toolBarBounds` is a static field of `Rectangle` which replaces both `pt` and `size`. Thus, you already have the rectangle to capture the screenshot if the test fails.

test/jdk/javax/swing/JToolBar/ToolBarSeparatorTest.java line 92:

> 90:                 sepPrefWidth = separator.getPreferredSize().width;
> 91:             });
> 92:             if (separator.getSize().width != separator.getPreferredSize().width) {

Suggestion:

            if (sepWidth != sepPrefWidth) {

test/jdk/javax/swing/JToolBar/ToolBarSeparatorTest.java line 94:

> 92:             if (separator.getSize().width != separator.getPreferredSize().width) {
> 93:                 System.out.println("size " + sepWidth);
> 94:                 System.out.println("preferredsize " + sepPrefWidth);

Suggestion:

                System.out.println("preferredSize " + sepPrefWidth);

test/jdk/javax/swing/JToolBar/ToolBarSeparatorTest.java line 97:

> 95:                 BufferedImage img = robot.createScreenCapture(
> 96:                     new Rectangle(pt.x, pt.y, size.width, size.height));
> 97:                 ImageIO.write(img, "png", new java.io.File("image.png"));

Theoretically, though unlikely, it may throw `IOException` which will hide the following `RuntimeException` which fails the test. But it's okay: the test fails one way or another.

test/jdk/javax/swing/JToolBar/ToolBarSeparatorTest.java line 98:

> 96:                     new Rectangle(pt.x, pt.y, size.width, size.height));
> 97:                 ImageIO.write(img, "png", new java.io.File("image.png"));
> 98:                 throw new RuntimeException("separator size is too wide");

Suggestion:

                throw new RuntimeException("Separator size is too wide");

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

PR Review: https://git.openjdk.org/jdk/pull/15054#pullrequestreview-1558867917
PR Review Comment: https://git.openjdk.org/jdk/pull/15054#discussion_r1281817083
PR Review Comment: https://git.openjdk.org/jdk/pull/15054#discussion_r1281822389
PR Review Comment: https://git.openjdk.org/jdk/pull/15054#discussion_r1281825105
PR Review Comment: https://git.openjdk.org/jdk/pull/15054#discussion_r1281824792
PR Review Comment: https://git.openjdk.org/jdk/pull/15054#discussion_r1281828508
PR Review Comment: https://git.openjdk.org/jdk/pull/15054#discussion_r1281821406
PR Review Comment: https://git.openjdk.org/jdk/pull/15054#discussion_r1281828757
PR Review Comment: https://git.openjdk.org/jdk/pull/15054#discussion_r1281831532
PR Review Comment: https://git.openjdk.org/jdk/pull/15054#discussion_r1281829099