JDK-8221987: javafx.graphics/javafx.stage.Window$TKBoundsConfigurator.apply (#408) #431
Conversation
|
Issue: #408 |
|
This looks like a safe and obvious fix to me. I have tagged @arapte to do the formal review. |
| newWW, newWH, newCW, newCH, | ||
| newXG, newYG, | ||
| newRX, newRY); | ||
| } |
There was a problem hiding this comment.
The set of local variables will not be needed when peer is null.
Please consider change as below,
public void apply() {
if (dirty) {
if (peer != null) {
// Snapshot values and then reset() before we call down
// as we may end up with recursive calls back up with
// new values that must be recorded as dirty.
boolean xSet = !Double.isNaN(x);
float newX = xSet ? (float) x : 0f;
boolean ySet = !Double.isNaN(y);
float newY = ySet ? (float) y : 0f;
float newWW = (float) windowWidth;
float newWH = (float) windowHeight;
float newCW = (float) clientWidth;
float newCH = (float) clientHeight;
float newXG = xGravity;
float newYG = yGravity;
float newRX = (float) renderScaleX;
float newRY = (float) renderScaleY;
reset();
peer.setBounds(newX, newY, xSet, ySet,
newWW, newWH, newCW, newCH,
newXG, newYG,
newRX, newRY);
} else {
reset();
}
}
}
There was a problem hiding this comment.
A second alternative would be to return early from the method if peer is null, leaving the rest of the method unchanged.
public void apply() {
if (dirty) {
if (peer == null) {
reset();
return;
}
...
}
I don't have a strong opinion. Any of the three choices (the original fix, Ambarish's proposed change, or my variation on that) are fine with me.
There was a problem hiding this comment.
A second alternative would be to return early from the method if peer is null, leaving the rest of the method unchanged.
public void apply() { if (dirty) { if (peer == null) { reset(); return; } ... }I don't have a strong opinion. Any of the three choices (the original fix, Ambarish's proposed change, or my variation on that) are fine with me.
I find the proposal from @kevinrushforth clearer.
|
I could observe the issue with, ControlsFX 8.40.14 & ControlsFX 9.0.0, downloaded from here https://github.com/controlsfx/controlsfx |
@arapte There are RC. You can find them here: |
|
@kevinrushforth thanks for your code review. I integrated your suggestion. |
STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
Use org.controlsfx.samples.HelloNotifications from https://github.com/controlsfx/controlsfx/blob/master/controlsfx-samples/src/main/java/org/controlsfx/samples/HelloNotifications.java and press very frequent on "Top-right notification" then the NPE will occur. It is independent of the os. We tested it on windows and mac.