On Thu, 27 Jul 2023 14:30:23 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) Changes requested by aivanov (Reviewer). src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSeparatorUI.java line 1: > 1: /* Could you please expand the wildcard imports and update the copyright year, please? src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSeparatorUI.java line 155: > 153: > 154: public Dimension getMinimumSize( JComponent c ) { return null; } > 155: public Dimension getMaximumSize( JComponent c ) { A blank line before the method would visually separate it from the previous method. The updated implementation isn't trivial. src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSeparatorUI.java line 162: > 160: else { > 161: return new Dimension(Integer.MAX_VALUE, d.height); > 162: } `WindowsToolBarSeparatorUI` uses `Short.MAX_VALUE` instead of `Interger`: https://github.com/openjdk/jdk/blob/ee3e0917b393b879a543060ace2537be84f20e82/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsToolBarSeparatorUI.java#L74-L81 Should we follow the pattern here? In Java Code Style, the `else` keyword should be on the same line as the closing brace. test/jdk/javax/swing/JToolBar/ToolBarSeparatorTest.java line 71: > 69: toolBar.add(btn); > 70: toolBar.add(new JButton("button 2")); > 71: toolBar.add(new JSeparator(SwingConstants.VERTICAL)); Would it be easier to save `JSeparator` into a field, then get its size and compare it to its preferred size? if (separator.getSize().width != separator.getPreferredSize().width) { // Fail the test because the separator is too wide } ------------- PR Review: https://git.openjdk.org/jdk/pull/15054#pullrequestreview-1557022453 PR Review Comment: https://git.openjdk.org/jdk/pull/15054#discussion_r1280662321 PR Review Comment: https://git.openjdk.org/jdk/pull/15054#discussion_r1280655962 PR Review Comment: https://git.openjdk.org/jdk/pull/15054#discussion_r1280661723 PR Review Comment: https://git.openjdk.org/jdk/pull/15054#discussion_r1280666570
