Re: [Integrated] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag
Changeset: 5a0e71b8 Author:Robert Lichtenberger Committer: Ambarish Rapte Date: 2020-01-29 06:25:21 + URL: https://git.openjdk.java.net/jfx/commit/5a0e71b8 8237372: NullPointerException in TabPaneSkin.stopDrag Reviewed-by: arapte ! modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java ! modules/javafx.controls/src/test/java/test/javafx/scene/control/TabPaneTest.java
Re: [Rev 03] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag
On Tue, 28 Jan 2020 16:04:59 GMT, Ambarish Rapte wrote: >> The pull request has been updated with 1 additional commit. > > The fix looks good to me. > Verified that the new test fails before and passes after fix and observed no > test failures. I don't have any additional comment, so this is good to go with 1 reviewer. @arapte can sponsor this. - PR: https://git.openjdk.java.net/jfx/pull/89
Re: [Rev 03] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag
On Tue, 28 Jan 2020 18:04:54 GMT, Robert Lichtenberger wrote: >> Test simulates a single mouse-released event. >> Fix simply guards against the null case. > > The pull request has been updated with 1 additional commit. The fix looks good to me. Verified that the new test fails before and passes after fix and observed no test failures. - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/89
Re: [Rev 03] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag
> Test simulates a single mouse-released event. > Fix simply guards against the null case. The pull request has been updated with 1 additional commit. - Added commits: - 39a61821: 8237372: NullPointerException in TabPaneSkin.stopDrag Changes: - all: https://git.openjdk.java.net/jfx/pull/89/files - new: https://git.openjdk.java.net/jfx/pull/89/files/a54a5306..39a61821 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/89/webrev.03 - incr: https://webrevs.openjdk.java.net/jfx/89/webrev.02-03 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/89.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/89/head:pull/89 PR: https://git.openjdk.java.net/jfx/pull/89
Re: [Rev 02] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag
On Tue, 28 Jan 2020 12:38:32 GMT, Ambarish Rapte wrote: >> The pull request has been updated with 1 additional commit. > > modules/javafx.controls/src/test/java/test/javafx/scene/control/TabPaneTest.java > line 57: > >> 56: import javafx.scene.input.KeyEvent; >> 57: import javafx.scene.input.MouseButton; >> 58: import javafx.scene.input.MouseEvent; > > This import it not used, please remove. Removed import and extra empty line. - PR: https://git.openjdk.java.net/jfx/pull/89
Re: [Rev 02] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag
On Tue, 28 Jan 2020 12:39:20 GMT, Robert Lichtenberger
wrote:
>> Test simulates a single mouse-released event.
>> Fix simply guards against the null case.
>
> The pull request has been updated with 1 additional commit.
Fix looks good, suggested minor changes in test.
modules/javafx.controls/src/test/java/test/javafx/scene/control/TabPaneTest.java
line 484:
> 483:
> 484:
> 485: @Test public void setRotateGraphicAndSeeValueIsReflectedInModel() {
Please remove the additional empty line from here.
modules/javafx.controls/src/test/java/test/javafx/scene/control/TabPaneTest.java
line 57:
> 56: import javafx.scene.input.KeyEvent;
> 57: import javafx.scene.input.MouseButton;
> 58: import javafx.scene.input.MouseEvent;
This import it not used, please remove.
-
Changes requested by arapte (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/89
Re: [Rev 01] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag
On Fri, 24 Jan 2020 15:41:20 GMT, Kevin Rushforth wrote:
>> The pull request has been updated with 1 additional commit.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
> line 1994:
>
>> 1993: private ListChangeListener childListener = new
>> ListChangeListener() {
>> 1994: @Override
>> 1995: public void onChanged(Change change) {
>
> Please revert.
Reverted the Overrides-Annotations inserted by eclipse.
-
PR: https://git.openjdk.java.net/jfx/pull/89
Re: [Rev 02] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag
> Test simulates a single mouse-released event. > Fix simply guards against the null case. The pull request has been updated with 1 additional commit. - Added commits: - a54a5306: 8237372: NullPointerException in TabPaneSkin.stopDrag Changes: - all: https://git.openjdk.java.net/jfx/pull/89/files - new: https://git.openjdk.java.net/jfx/pull/89/files/30290116..a54a5306 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/89/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/89/webrev.01-02 Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/89.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/89/head:pull/89 PR: https://git.openjdk.java.net/jfx/pull/89
Re: [Rev 01] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag
On Fri, 24 Jan 2020 15:43:25 GMT, Robert Lichtenberger
wrote:
>> Test simulates a single mouse-released event.
>> Fix simply guards against the null case.
>
> The pull request has been updated with 1 additional commit.
This is a simple enough change that 1 reviewer will suffice -- @arapte will
review.
I did notice some unrelated changes that should be reverted.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
line 1957:
> 1956: }
> 1957: @Override
> 1958: protected void interpolate(double frac) {
This is unrelated to your change. Please revert it.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
line 1974:
> 1973: }
> 1974: @Override
> 1975: protected void interpolate(double frac) {
Please revert.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
line 1994:
> 1993: private ListChangeListener childListener = new
> ListChangeListener() {
> 1994: @Override
> 1995: public void onChanged(Change change) {
Please revert.
-
PR: https://git.openjdk.java.net/jfx/pull/89
Re: [Rev 01] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag
> Test simulates a single mouse-released event. > Fix simply guards against the null case. The pull request has been updated with 1 additional commit. - Added commits: - 30290116: 8237372: NullPointerException in TabPaneSkin.stopDrag Changes: - all: https://git.openjdk.java.net/jfx/pull/89/files - new: https://git.openjdk.java.net/jfx/pull/89/files/923a63b2..30290116 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/89/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/89/webrev.00-01 Stats: 7 lines in 1 file changed: 3 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/89.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/89/head:pull/89 PR: https://git.openjdk.java.net/jfx/pull/89
Re: [Rev 01] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag
On Wed, 22 Jan 2020 08:01:49 GMT, Ambarish Rapte wrote:
>> The pull request has been updated with 1 additional commit.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
> line 2216:
>
>> 2215: // Animate tab header being dragged to its final position.
>> 2216: if (dragTabHeader != null) {
>> 2217: dragHeaderSourceX = dragTabHeader.getLayoutX();
>
> This is a situation when reorder was not started.
> So a check as `else if (dragState == DragState.REORDER)` seems more correct
> instead of `null` check. With this `else if` the `return;` on line 2213 can
> be removed.
You're right, this makes the method cleaner and easier to understand. I've
adapted the PR.
-
PR: https://git.openjdk.java.net/jfx/pull/89
Re: RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag
On Mon, 20 Jan 2020 10:00:55 GMT, Robert Lichtenberger
wrote:
> Test simulates a single mouse-released event.
> Fix simply guards against the null case.
The scenario mentioned in
[JDK-8237372](https://bugs.openjdk.java.net/browse/JDK-8237372) seems valid
scenario. However I am not sure if the scenario can be avoided or not.
Irrespective of that, the code in question should be corrected. Suggested a
minor change.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
line 2216:
> 2215: // Animate tab header being dragged to its final position.
> 2216: if (dragTabHeader != null) {
> 2217: dragHeaderSourceX = dragTabHeader.getLayoutX();
This is a situation when reorder was not started.
So a check as `else if (dragState == DragState.REORDER)` seems more correct
instead of `null` check. With this `else if` the `return;` on line 2213 can be
removed.
-
PR: https://git.openjdk.java.net/jfx/pull/89
Re: RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag
On Mon, 20 Jan 2020 10:00:55 GMT, Robert Lichtenberger wrote: > Test simulates a single mouse-released event. > Fix simply guards against the null case. @arapte - can you review this? The only question I have is whether is is expected that `dragTabHeader` can be null. If we understand why it is null, and that it isn't an error for it to be null, then the fix is fine. If its being null is unexpected, then the fix might just be masking the real problem. There isn't enough information in this PR to be able to evaluate this. - PR: https://git.openjdk.java.net/jfx/pull/89
RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag
Test simulates a single mouse-released event. Fix simply guards against the null case. - Commits: - 923a63b2: 8237372: NullPointerException in TabPaneSkin.stopDrag Changes: https://git.openjdk.java.net/jfx/pull/89/files Webrev: https://webrevs.openjdk.java.net/jfx/89/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8237372 Stats: 29 lines in 2 files changed: 21 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jfx/pull/89.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/89/head:pull/89 PR: https://git.openjdk.java.net/jfx/pull/89
