On Fri, 25 Jul 2025 16:37:54 GMT, Alexey Ivanov <aiva...@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