Re: RFR: 8299423: JavaFX Mac system menubar leaks
On Fri, 30 Dec 2022 09:41:46 GMT, Florian Kirmaier wrote: > This PR fixes the leak in the mac system menu bar. > > Inside the native code, NewGlobalRef is called for the callable. > Which makes it into a "GC-Root" until DeleteGlobalRef is called. > > The DeleteGlobalRef is never called for the MenuEntry, if it's removed from > the menu without removing it's callable. > This PR adds logic, whether the Menu is inserted. If it's not inserted in a > Menu anymore, then DeleteGlobalRef is called, by calling `_setCallback` with > the callable "null". > > The unit test verifies, that this bug happened without this change, but no > longer happens with this change. Have you considered storing a weak global reference to the callback instead of a strong global reference? That might also solve the problem without keeping track of the callback in `MacMenuDelegate`. - PR: https://git.openjdk.org/jfx/pull/987
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages
On Sun, 11 Dec 2022 20:12:17 GMT, John Hendrikx wrote:
> Packages fixed:
> - com.sun.javafx.binding
> - com.sun.javafx.collections
> - javafx.beans
> - javafx.beans.binding
> - javafx.collections
> - javafx.collections.transformation
Second part of the review. I'm still looking at the changes to sorting. I think
that the problem stems from a more basic flaw that we can possible fix, and
that is that any `ObservableListWrapper` is a `SortedList` even when it's not.
A `SortedList` should require that its elements implement `Comparable`. The
only other class affected is the `Sequential` variant, so the scope here is
small.
While the current fix removes the warnings, I suspect we are just hiding the
flaw that these warnings exist for.
In addition `FXCollections::sort(ObservableList)` can change to
if (list instanceof SortableList sortableList) {
sortableList.sort();
and remove the `@SuppressWarnings("unchecked")`.
modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line
709:
> 707: private static class EmptyObservableList extends AbstractList
> implements ObservableList {
> 708:
> 709: private static final ListIterator iterator = new
> ListIterator<>() {
Isn't a better fix is to not make this class static and use ``? Other lists
create a new iterator on each invocation, but this is effectively a singleton.
In fact, `EmptyObservableList` doesn't need to be declared because it's called
only in one place, so it can be "inlined" when declaring the `EMPTY_LIST`
field. Maybe this is out of scope.
modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line
1640:
> 1638: @Override
> 1639: public Iterator iterator() {
> 1640: return new Iterator<>() {
Here the empty `Set` creates a listener on invocation, unlike in the list case.
Might want to keep a single pattern. I prefer the one with a singleton iterator
because the empty set itself is a singleton. Same comment about considering
"inlining" it.
modules/javafx.base/src/main/java/javafx/collections/transformation/TransformationList.java
line 94:
> 92: List currentSource = source;
> 93: while(currentSource instanceof TransformationList) {
> 94: currentSource = ((TransformationList ?>)currentSource).source;
Can use pattern matching for `instanceof`.
Also, can add a space after `while`.
modules/javafx.base/src/main/java/javafx/collections/transformation/TransformationList.java
line 143:
> 141: int idx = getSourceIndex(index);
> 142: while(currentSource != list && currentSource instanceof
> TransformationList) {
> 143: final TransformationList tSource =
> (TransformationList) currentSource;
Sam here.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages
On Mon, 26 Dec 2022 14:32:52 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/javafx/beans/binding/ListExpression.java
>> line 238:
>>
>>> 236: public Iterator iterator() {
>>> 237: final ObservableList list = get();
>>> 238: return (list == null)?
>>> ListExpression.emptyList().iterator() : list.iterator();
>>
>> You're using three slightly different ways of referring to the empty list:
>> * `ListExpression.emptyList()`
>> * `emptyList()`
>> * `EMPTY_LIST`
>>
>> What do you think about using the first option in all cases?
>
> I'm fine with that; the first two are equivalent, but in some cases I need to
> add the type witness to avoid a warning and you can only do that by adding
> the class name as well (ie. `emptyList()` is not allowed, but
> `ListExpression.emptyList()` is.
Why not use `FXCollections.emptyObservableList()` directly? If it's too
long, a convenvenience method can be used:
private static ObservableList emptyList() {
return FXCollections.emptyObservableList();
}
No need to hold the instance here again in a way that makes it lose its type
and do the same cast (and supress the warning) that `FXCollections` already
does.
Same with the other collections.
-
PR: https://git.openjdk.org/jfx/pull/972
RFR: 8299423: JavaFX Mac system menubar leaks
This PR fixes the leak in the mac system menu bar. Inside the native code, NewGlobalRef is called for the callable. Which makes it into a "GC-Root" until DeleteGlobalRef is called. The DeleteGlobalRef is never called for the MenuEntry, if it's removed from the menu without removing it's callable. This PR adds logic, whether the Menu is inserted. If it's not inserted in a Menu anymore, then DeleteGlobalRef is called, by calling `_setCallback` with the callable "null". The unit test verifies, that this bug happened without this change, but no longer happens with this change. - Commit messages: - JDK-8299423 - JDK-8299423 Changes: https://git.openjdk.org/jfx/pull/987/files Webrev: https://webrevs.openjdk.org/?repo=jfx=987=00 Issue: https://bugs.openjdk.org/browse/JDK-8299423 Stats: 132 lines in 2 files changed: 130 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/987.diff Fetch: git fetch https://git.openjdk.org/jfx pull/987/head:pull/987 PR: https://git.openjdk.org/jfx/pull/987
RFR: JDK-8299423: JavaFX Mac system menubar leaks
Hi Everyone, Please review this change: https://github.com/openjdk/jfx/pull/987 It fixes a newly found leak in the mac system menu bar Florian
