On Fri, 23 Jun 2023 08:41:25 GMT, Prasanta Sadhukhan <[email protected]> wrote:
>> Title buttons under Widows Classic L&F got their sizes from the XP desktop >> theme in which button width can be bigger than height. It is construed as XP >> bug where sizes aren't updated properly so it uses height units for width >> for XP and later windows. The proposed fix uses the [same >> technique](https://github.com/openjdk/jdk/blob/a0595761ef35c4eec8cb84326a869b9473cd5bba/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsInternalFrameTitlePane.java#L78-L82) >> for Classic and forces title buttons to be square and to fit the frame >> title in height. >> >> Before fix SwingSet2 demo (Windows Classic InternalFrame) >>  >> >> After fix >>  > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Fix for JDK-8139392 Looks good to me. The only concern is that UI Manager has a wrong width until `JInternalFrame` is shown or initialised. But it has been the case before the fix too. > Having said that, I guess XP theme also padding is not correct as can be seen > in this bug [JDK-8139392](https://bugs.openjdk.org/browse/JDK-8139392) which > I have rectified > > After fix >  Your latest changes also fix [JDK-8139392](https://bugs.openjdk.org/browse/JDK-8139392), do I get it right? If so, add it to the list of fixed issues. src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java line 292: > 290: width = > UIManager.getInt("InternalFrame.titleButtonHeight") + 2; > 291: } else { > 292: width = > UIManager.getInt("InternalFrame.titleButtonHeight") -2; Perhaps, add a space between `-2` (to be consistent with `+ 2`). You've updated the entire method `getIconWidth`, so it will look better. src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsInternalFrameTitlePane.java line 80: > 78: // Fix for XP bug where sometimes these sizes aren't updated > properly > 79: // Assume for now that height is correct and derive width > from height > 80: buttonWidth = buttonHeight+6; Perhaps? Suggestion: buttonWidth = buttonHeight + 6; For `height` above, it's `- 4`; no other instances of `-` or `+` inside the method, so spaces around `+` will be consistent. test/jdk/javax/swing/JInternalFrame/InternalFrameTitleButtonTest.java line 55: > 53: > 54: public static void main(String[] args) throws Exception { > 55: if (OSInfo.getOSType() == OSInfo.OSType.WINDOWS) { If you remove this line (OS check), the test would be easier to run as a stand-alone app. It's up to you though. test/jdk/javax/swing/JInternalFrame/InternalFrameTitleButtonTest.java line 104: > 102: if (icon.getIconHeight() > height - 4 || > 103: icon.getIconWidth() > height - 2) { > 104: throw new RuntimeException("Wrong title icon > size"); Suggestion: if (icon.getIconHeight() > height - 4 || icon.getIconWidth() > height - 2) { throw new RuntimeException("Wrong title icon size"); Wrapping the operator prevents ambiguity: continuation line or statement. ------------- Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14555#pullrequestreview-1495668370 PR Comment: https://git.openjdk.org/jdk/pull/14555#issuecomment-1604699016 PR Review Comment: https://git.openjdk.org/jdk/pull/14555#discussion_r1240160993 PR Review Comment: https://git.openjdk.org/jdk/pull/14555#discussion_r1240162708 PR Review Comment: https://git.openjdk.org/jdk/pull/14555#discussion_r1240155464 PR Review Comment: https://git.openjdk.org/jdk/pull/14555#discussion_r1240156462
