On Thu, 15 Jan 2026 22:21:07 GMT, Christopher Schnick <[email protected]>
wrote:
>> This PR replaces various catch blocks for NPEs by proper null checks. It
>> looks like a lot but most of the binding changes are just variants of the
>> same approach. The test cases have been adapted to not check for NPEs
>> anymore.
>>
>> The PrismFontFactory change to a RuntimeException was made because I don't
>> see how the try block can throw an NPE.
>
> Christopher Schnick has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add test clarifying generics handling
Overall looks good to me. Some minor comments mostly.
modules/javafx.base/src/main/java/com/sun/javafx/binding/SelectBinding.java
line 147:
> 145: Logging.getLogger().warning("Value of select-binding has
> wrong type, expected Boolean. Returning default value.");
> 146: } else {
> 147: Logging.getLogger().fine("Value of select binding is
> null, returning default value");
Very minor: Maybe to follow the message above, we could have the ` returning
default value` in a new sentence as well. So: `Value of select binding is null.
Returning default value`. (Same below)
While writing this, I wonder if we also want to add `expected XXX` here, or
something similar? Might be a good hint for debugging.
modules/javafx.base/src/main/java/javafx/beans/binding/Bindings.java line 7474:
> 7472: @Override
> 7473: protected V computeValue() {
> 7474: final V val = getMapValue(op, key);
Minor: This can be returned directly (same below, `final V val` is not needed)
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
line 39:
> 37: import java.util.Random;
> 38:
> 39: import com.sun.javafx.binding.SelectBinding;
`SelectBinding` and `SimpleObjectProperty` are unused here
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
line 415:
> 413: // This does not throw an exception on get() due to generic type
> erasure
> 414: Object returnedPerson = objectBinding.get();
> 415: Assertions.assertEquals(person1, returnedPerson);
Minor: The `Assertions` can be removed since `assertEquals` is imported
statically.
modules/javafx.graphics/src/main/java/com/sun/javafx/css/ParsedValueImpl.java
line 674:
> 672: units = Enum.valueOf(SizeUnits.class, unitStr);
> 673: } catch (IllegalArgumentException iae) {
> 674: System.err.println(iae.toString());
The `toString()` can be removed here. Reported by IntelliJ, quoting:
`In these cases, conversion to string will be handled by the underlying library
methods, and the explicit call to toString() is not needed. Removing redundant
toString() calls can occasionally even improve performance and reduce object
allocations.`
modules/javafx.graphics/src/main/java/com/sun/javafx/font/PrismFontFactory.java
line 39:
> 37: import java.util.Iterator;
> 38: import java.util.Locale;
> 39: import java.util.NoSuchElementException;
Unused Import
modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line
1634:
> 1632: }
> 1633: return SKIP;
> 1634: } catch (IllegalArgumentException | NullPointerException
> iae) {
Minor: Variable should probably be renamed
-------------
PR Review: https://git.openjdk.org/jfx/pull/2032#pullrequestreview-3937907224
PR Review Comment: https://git.openjdk.org/jfx/pull/2032#discussion_r2925780221
PR Review Comment: https://git.openjdk.org/jfx/pull/2032#discussion_r2925795368
PR Review Comment: https://git.openjdk.org/jfx/pull/2032#discussion_r2925817694
PR Review Comment: https://git.openjdk.org/jfx/pull/2032#discussion_r2925822191
PR Review Comment: https://git.openjdk.org/jfx/pull/2032#discussion_r2925844943
PR Review Comment: https://git.openjdk.org/jfx/pull/2032#discussion_r2925846606
PR Review Comment: https://git.openjdk.org/jfx/pull/2032#discussion_r2925759132
