8387301: ListView, ComboBox, TableView, TreeTableView fail when item type is a value class #2250
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| /* | ||
| * Copyright (c) 2026, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
| * under the terms of the GNU General Public License version 2 only, as | ||
| * published by the Free Software Foundation. Oracle designates this | ||
| * particular file as subject to the "Classpath" exception as provided | ||
| * by Oracle in the LICENSE file that accompanied this code. | ||
| * | ||
| * This code is distributed in the hope that it will be useful, but WITHOUT | ||
| * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
| * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
| * version 2 for more details (a copy is included in the LICENSE file that | ||
| * accompanied this code). | ||
| * | ||
| * You should have received a copy of the GNU General Public License version | ||
| * 2 along with this work; if not, write to the Free Software Foundation, | ||
| * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| * | ||
| * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
| * or visit www.oracle.com if you need additional information or have any | ||
| * questions. | ||
| */ | ||
|
|
||
| package com.sun.javafx.scene.control; | ||
|
|
||
| import java.lang.ref.WeakReference; | ||
| import java.lang.reflect.InvocationTargetException; | ||
| import java.lang.reflect.Method; | ||
| import java.util.Objects; | ||
|
|
||
| /** | ||
| * Wrapper class that holds a reference to an object with or without identity. | ||
| * If the object is non-null and has identity, a weak reference is created and | ||
| * stored; otherwise it holds a reference to the object itself. | ||
| * <p> | ||
| * In the case of a value object, the referent is never collected, so it is only | ||
| * suitable for uses that do not rely on the object being placed onto a reference | ||
| * queue. | ||
| * | ||
| * @param <T> the type of the referent | ||
| */ | ||
| public class WeakReferenceWrapper<T> { | ||
|
Contributor
There was a problem hiding this comment. An application or a library should not be forced to do this, in my opinion. WeakReference(T) should work as before - in case of a value object it should hold the reference indefinitely because it's the same behavior as before. It may work differently with the WeakReference(T,ReferenceQueue) constructor which is ok because it would affect a much smaller space of use cases. It is probably ok to do it right now, to avoid javafx breaking with the value objects preview enabled.
Member
Author
There was a problem hiding this comment.
But that isn't the case. |
||
|
|
||
| private static final Method hasIdentityMethod; | ||
| private final T obj; | ||
| private final WeakReference<T> ref; | ||
|
|
||
| static { | ||
| Method meth; | ||
| try { | ||
| meth = Objects.class.getMethod("hasIdentity", Object.class); | ||
| } catch (NoSuchMethodException ex) { | ||
| meth = null; | ||
| } | ||
| hasIdentityMethod = meth; | ||
| } | ||
|
|
||
| /** | ||
| * Helper method that reflectively calls Objects.hasIdentity to determine | ||
| * whether we should create and hold a weak reference to the given object. | ||
| * If {@code obj} is null, treat it as having no identity (matching JDK 28's | ||
| * behavior with or without {@code --enable-preview}) and return false. | ||
| * If {@code obj} is not null, check whether {@code Objects.hasIdentity} exists: | ||
| * if the method doesn't exist or cannot be invoked, return true; otherwise, | ||
| * call {@code Objects.hasIdentity(obj)} reflectively and return its value. | ||
| */ | ||
| private static boolean useWeakRef(Object obj) { | ||
| if (obj == null) { | ||
| return false; | ||
| } else if (hasIdentityMethod == null) { | ||
| return true; | ||
| } else { | ||
| try { | ||
| return (Boolean)hasIdentityMethod.invoke(null, obj); | ||
| } catch (IllegalAccessException | InvocationTargetException ex) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Creates a new reference that refers to the given object. | ||
| * If the object is non-null and has identity, a weak reference is created and | ||
| * stored; otherwise it holds a reference to the object itself. | ||
| * | ||
| * @param obj the object this reference will refer to | ||
| */ | ||
| public WeakReferenceWrapper(T obj) { | ||
| if (useWeakRef(obj)) { | ||
| this.obj = null; | ||
| this.ref = new WeakReference<>(obj); | ||
| } else { | ||
| this.obj = obj; | ||
| this.ref = null; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns this reference object's referent. If the referent is held in a weak reference, | ||
| * and this reference object has been cleared by the garbage collector, then this method | ||
| * returns null. | ||
| * | ||
| * @return the object to which this reference refers, or null if this reference object has been cleared | ||
| */ | ||
| public T get() { | ||
| return ref != null ? ref.get() : obj; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| /* | ||
| * Copyright (c) 2026, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
| * under the terms of the GNU General Public License version 2 only, as | ||
| * published by the Free Software Foundation. Oracle designates this | ||
| * particular file as subject to the "Classpath" exception as provided | ||
| * by Oracle in the LICENSE file that accompanied this code. | ||
| * | ||
| * This code is distributed in the hope that it will be useful, but WITHOUT | ||
| * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
| * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
| * version 2 for more details (a copy is included in the LICENSE file that | ||
| * accompanied this code). | ||
| * | ||
| * You should have received a copy of the GNU General Public License version | ||
| * 2 along with this work; if not, write to the Free Software Foundation, | ||
| * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| * | ||
| * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
| * or visit www.oracle.com if you need additional information or have any | ||
| * questions. | ||
| */ | ||
| package test.com.sun.javafx.scene.control; | ||
|
|
||
| import com.sun.javafx.scene.control.WeakReferenceWrapper; | ||
| import java.lang.ref.WeakReference; | ||
| import org.junit.jupiter.api.Test; | ||
| import test.util.memory.JMemoryBuddy; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
| /* | ||
| * Test WeakReferenceWrapper utility. | ||
| */ | ||
| public class WeakReferenceWrapperTest { | ||
|
|
||
| // POJO with identity | ||
| static class POJO { | ||
| final int i; | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return i; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object other) { | ||
| if (other instanceof POJO otherPOJO) { | ||
| return i == otherPOJO.i; | ||
| } else { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| POJO(int i) { | ||
| this.i = i; | ||
| } | ||
| } | ||
|
|
||
| // Test WeakReferenceWrapper with null, Integer (could be a value object), | ||
| // String (always an identity object), and a POJO (identity object). | ||
| @Test | ||
| public void testWeakReferenceWrapper() { | ||
| var nullRef = new WeakReferenceWrapper<Object>(null); | ||
| assertNull(nullRef.get()); | ||
|
|
||
| Integer i = 123; | ||
| var intRef = new WeakReferenceWrapper<Integer>(i); | ||
| assertEquals(i, intRef.get()); | ||
|
|
||
| String str = "abc"; | ||
| var strRef = new WeakReferenceWrapper<String>(str); | ||
| assertEquals(str, strRef.get()); | ||
|
|
||
| POJO pojo = new POJO(456); | ||
| var pojoRef = new WeakReferenceWrapper<POJO>(pojo); | ||
| assertEquals(pojo, pojoRef.get()); | ||
| } | ||
|
|
||
| // Test that a WeakReferenceWrapper of an identity object holds the object | ||
| // weakly and does not prevent the object from being collected. | ||
| @Test | ||
| public void testWeakReferenceToIdentityObjIsCollectable() { | ||
| var pojo = new POJO(789); | ||
| var pojoWeakRef = new WeakReference<POJO>(pojo); | ||
| var pojoRef = new WeakReferenceWrapper<POJO>(pojo); | ||
|
|
||
| JMemoryBuddy.assertNotCollectable(pojoWeakRef); | ||
| assertNotNull(pojoRef.get()); | ||
| assertSame(pojo, pojoRef.get()); | ||
|
|
||
| pojo = null; | ||
| JMemoryBuddy.assertCollectable(pojoWeakRef); | ||
| assertNull(pojoRef.get()); | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might suggest to include the JBS number for when we need to undo / redo things due to inevitable change in the value objects JEP (and possibly a link to the JEP itself).
Perhaps also say a couple of words about the fact that this class should not exist had they decided to make the WeakReference implementation handle this case transparently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no JBS issue or JEP that will eliminate the need for this. The restriction is intentional with no current plan to change it.
I could add a comment that this class might become unnecessary in the future, if a there is a change in the way
WeakReferencedeals with value objects, but it is uncertain if or when that might be.