On Mon, 21 Jul 2025 02:16:27 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

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/javax/swing/JPopupMenu.java line 962:

> 960:             ui.uninstallUI(this);
> 961:             if (oldInvoker != null) {
> 962:                 oldInvoker.removePropertyChangeListener(propListener);

This won't remove the install `propListener` because a new instance of the 
listener is created each time the method is called.

test/jdk/javax/swing/JPopupMenu/TestPopupInvoker.java line 56:

> 54:     static volatile Rectangle size;
> 55: 
> 56:     public static class MyThread implements Runnable {

Do you even need another thread?

The test is driven from the main thread, and you can remove the component from 
container on the main thread.

Perhaps, a better version of the test would remove the component when the event 
from the popup menu that it's now displayed is received. If the popup is hidden 
as the result of component removal, another event should be received. Thus, the 
test would depend on the events and latches instead of on the delays.

test/jdk/javax/swing/JPopupMenu/TestPopupInvoker.java line 68:

> 66:             System.out.println("Starting 3 second countdown...");
> 67:             try{
> 68:                 Thread.currentThread().sleep(3000);

Suggestion:

                Thread.sleep(3000);

`sleep` is a static method, no instance of `Thread` is needed.

test/jdk/javax/swing/JPopupMenu/TestPopupInvoker.java line 71:

> 69:             } catch (Exception e) {}
> 70:             System.out.println("Removing popup invoker from the 
> container.");
> 71:             box.remove(invo);

This breaks Swing contract that components must be accessed via EDT only.

test/jdk/javax/swing/JPopupMenu/TestPopupInvoker.java line 92:

> 90:                     jpm.add("Three");
> 91:                 }
> 92:                 jpm.show(label, 0, 0);

The popup can be shown programmatically without mouse click: `invokeAndWait(() 
-> jpm.show())` after the initial `waitForIdle` and delay.

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

PR Review: https://git.openjdk.org/jdk/pull/26407#pullrequestreview-3038970876
PR Review Comment: https://git.openjdk.org/jdk/pull/26407#discussion_r2219765853
PR Review Comment: https://git.openjdk.org/jdk/pull/26407#discussion_r2219783806
PR Review Comment: https://git.openjdk.org/jdk/pull/26407#discussion_r2219767297
PR Review Comment: https://git.openjdk.org/jdk/pull/26407#discussion_r2219772804
PR Review Comment: https://git.openjdk.org/jdk/pull/26407#discussion_r2219786884

Reply via email to