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

[Prasanta Sadhukhan]

On Fri, 25 Jul 2025 16:37:54 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Use setVisible(false) instead of dispose, update test
>
> 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.

ok

> 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).

ok..modified..

> 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.

fixed..

> 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.

ok

> src/java.desktop/share/classes/javax/swing/JPopupMenu.java line 1383:
> 
>> 1381:             values.addElement("propListener");
>> 1382:             values.addElement(propListener);
>> 1383:         }
> 
> Do we need to serialise `propListener`? What is the value in serialising the listener?
> 
> Will `invoker` serialise `propListener`? If it does, then we need to serialise it to be able to remove the listener after the components are deserialised and the `invoker` is changed.

There was failure in CI in some test citing NotSerializableException which was solved by serialising propListener

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/26407#discussion_r2239551917
PR Review Comment: https://git.openjdk.org/jdk/pull/26407#discussion_r2239553307
PR Review Comment: https://git.openjdk.org/jdk/pull/26407#discussion_r2239552668
PR Review Comment: https://git.openjdk.org/jdk/pull/26407#discussion_r2239552914
PR Review Comment: https://git.openjdk.org/jdk/pull/26407#discussion_r2239556446