RFR: 4938801: The popup does not go when the component is removed. [v5]

[Alexey Ivanov]

On Fri, 25 Jul 2025 07:31:42 GMT, Prasanta Sadhukhan <psadhukhan at 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