On Fri, 25 Jul 2025 07:31:42 GMT, Prasanta Sadhukhan <psadhuk...@openjdk.org> wrote:
>> Issue is seen that a popup doesn't get closed when the component that >> invokes it, gets removed from the parent container. >> This is because the JPopupMenu does not listen to its invoker liefecycle >> thereby behaving as a standalone entity after creation. >> Fix is made to make sure popup listens to its invoker lifecycle by >> registering its PropertyChangeListener to the invoker and listens to the >> ["ancestor" property name ], >> https://github.com/openjdk/jdk/blob/441dbde2c3c915ffd916e39a5b4a91df5620d7f3/src/java.desktop/share/classes/javax/swing/JComponent.java#L4853-L4858 >> which will become null when removed, wherein we should dispose of the popup > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Use setVisible(false) instead of dispose, update test Changes requested by aivanov (Reviewer). src/java.desktop/share/classes/javax/swing/JPopupMenu.java line 129: > 127: transient Frame frame; > 128: private int desiredLocationX,desiredLocationY; > 129: transient private PropertyChangeListener propListener = Suggestion: private transient PropertyChangeListener propListener = Use blessed modifier order. src/java.desktop/share/classes/javax/swing/JPopupMenu.java line 129: > 127: transient Frame frame; > 128: private int desiredLocationX,desiredLocationY; > 129: transient private PropertyChangeListener propListener = I also suggest adding a blank line between location fields and `propListener` src/java.desktop/share/classes/javax/swing/JPopupMenu.java line 142: > 140: } > 141: } > 142: }; Anonymous class can be replaced with lambda¹: Suggestion: (e) -> { if (e.getOldValue() != null && e.getNewValue() == null && isVisible()) { setVisible(false); } }; If you use the named property listener, checking the name of the property can be safely dropped, and the nested `if` blocks could be chained with the `&&` operator. ¹ @DamonGuy [advocated using lambdas in such cases](https://github.com/openjdk/jdk/pull/22977#discussion_r1909302224). src/java.desktop/share/classes/javax/swing/JPopupMenu.java line 965: > 963: oldInvoker.removePropertyChangeListener(oldPropListener); > 964: } > 965: invoker.addPropertyChangeListener(propListener); Suggestion: if ((oldInvoker != this.invoker) && (ui != null)) { ui.uninstallUI(this); if (oldInvoker != null) { oldInvoker.removePropertyChangeListener(propListener); } invoker.addPropertyChangeListener(propListener); `oldPropListener` is not needed, the instance never changes. src/java.desktop/share/classes/javax/swing/JPopupMenu.java line 965: > 963: oldInvoker.removePropertyChangeListener(oldPropListener); > 964: } > 965: invoker.addPropertyChangeListener(propListener); Suggestion: if (oldInvoker != null) { oldInvoker.removePropertyChangeListener("ancestor", oldPropListener); } invoker.addPropertyChangeListener("ancestor", propListener); Use named property listener, it's more efficient since the listener won't be called for any other properties. src/java.desktop/share/classes/javax/swing/JPopupMenu.java line 1429: > 1427: } > 1428: if(indexCounter < maxCounter && values.elementAt(indexCounter). > 1429: equals("propListener")) { Suggestion: if(indexCounter < maxCounter && values.elementAt(indexCounter). equals("propListener")) { To align with the code above, wrapped `equals` has to use four-space indentation. ------------- PR Review: https://git.openjdk.org/jdk/pull/26407#pullrequestreview-3056144669 PR Review Comment: https://git.openjdk.org/jdk/pull/26407#discussion_r2231578655 PR Review Comment: https://git.openjdk.org/jdk/pull/26407#discussion_r2237281954 PR Review Comment: https://git.openjdk.org/jdk/pull/26407#discussion_r2237270173 PR Review Comment: https://git.openjdk.org/jdk/pull/26407#discussion_r2231584604 PR Review Comment: https://git.openjdk.org/jdk/pull/26407#discussion_r2237241406 PR Review Comment: https://git.openjdk.org/jdk/pull/26407#discussion_r2237237614
