8387301: ListView, ComboBox, TableView, TreeTableView fail when item type is a value class - #2250
8387301: ListView, ComboBox, TableView, TreeTableView fail when item type is a value class#2250kevinrushforth wants to merge 9 commits into
Conversation
|
👋 Welcome back kcr! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
| return true; | ||
| } else { | ||
| try { | ||
| System.err.println("[3] Calling Ojbects.hasIdentityMethod(obj)"); |
There was a problem hiding this comment.
Fixed. My fingers do that all the time!
kevinrushforth
changed the title
|
Reviewers: @andy-goryachev-oracle @arapte @dansmithcode Your comments would be welcome if you have time to look at this. |
|
The total number of required reviews for this PR has been set to 2 based on the presence of this label: |
| * | ||
| * @param <T> the type of the referent | ||
| */ | ||
| public class WeakReferenceWrapper<T> { |
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.
There was a problem hiding this comment.
WeakReference(T) should work as before
But that isn't the case.
| * <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. |
There was a problem hiding this comment.
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.
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 WeakReference deals with value objects, but it is uncertain if or when that might be.
| private boolean isFirstRun = true; | ||
|
|
||
| private WeakReference<S> oldRowItemRef; | ||
| private WeakReferenceWrapper<S> oldRowItemRef; |
There was a problem hiding this comment.
While it is ok to apply this workaround (WeakReferenceWrapper) here, this kind of change in the WeakReference behavior is just awful: we should never force the application developers (r a third party library developers) to make a change like this.
2 participants
This PR creates a WeakReferenceWrapper object to replace direct uses of WeakReference in controls where the referent is a user-supplied object of an unknown type.
As noted in JEP 401, which is now integrated into JDK 28, "The garbage collection APIs in java.lang.ref ... do not allow developers to manually manage value objects in the heap. Attempts to create Reference objects for value objects throw IdentityException at run time."
Several core JDK classes such as all of the primitive wrappers (e.g.,
Integer,Character),Optional,LocalDateTime, and a few others are now value types if JDK 28 is run with the--enable-previewoption.The
ListView,ComboBox,TableView, andTreeTableViewcontrols take a parameterized item type and hold items of that type. The following places in the implementation create weak references to an item. If that item type is a value class -- meaning that it does not have identity -- creating theWeakReferencefails.As noted in the JBS issue, there are 3 cases to consider.
SelectedItemsReadOnlyObservableList<E>--Eis the item type (created byMultipleSelectionModelBase<T>) :ListView,TableView,ComboBox(due to its skin creating aListView<T>) -- replace withWeakReferenceWrapperTablePosition<S,T>--Sis the item type :TableView-- the reference is unused, so I removed itTableCell<S,T>andTreeTableCell<S,T>--Sis the item type :TableView,TreeTableView-- replace withWeakReferenceWrapperThe new
WeakReferenceWrapperclass takes a referent of any type and either creates a WeakReference (if it has identity) or directly stores the reference (if it is null or does not have identity). I added a test for the wrapper.All of the controls tests pass with this fix. I did three test runs as follows:
--enable-preview--enable-previewWithout the fix, 31 controls tests fails on the 3rd run.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2250/head:pull/2250$ git checkout pull/2250Update a local copy of the PR:
$ git checkout pull/2250$ git pull https://git.openjdk.org/jfx.git pull/2250/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2250View PR using the GUI difftool:
$ git pr show -t 2250Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2250.diff
Using Webrev
Link to Webrev Comment