On Wed, 2 Aug 2023 06:36:29 GMT, Prasanta Sadhukhan <[email protected]> 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 >> >>  >> >> 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
