Skip to content

Commit 47c2ec3

Browse files
author
Jeanette Winzenburg
committed
8187229: Tree/TableCell: cancel event must return correct editing location
8269136: Tree/TablePosition: must not throw NPE on instantiating with null table Reviewed-by: mhanl, aghaisas
1 parent 0e7cf62 commit 47c2ec3

File tree

7 files changed

+359
-27
lines changed

7 files changed

+359
-27
lines changed

‎modules/javafx.controls/src/main/java/javafx/scene/control/TableCell.java

+5-5
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,9 @@ private ReadOnlyObjectWrapper<TableView<S>> tableViewPropertyImpl() {
299299
* *
300300
**************************************************************************/
301301

302+
// editing location at start of edit - fix for JDK-8187229
303+
private TablePosition<S, T> editingCellAtStartEdit;
304+
302305
/** {@inheritDoc} */
303306
@Override public void startEdit() {
304307
final TableView<S> table = getTableView();
@@ -333,6 +336,7 @@ private ReadOnlyObjectWrapper<TableView<S>> tableViewPropertyImpl() {
333336

334337
Event.fireEvent(column, editEvent);
335338
}
339+
editingCellAtStartEdit = new TablePosition<>(table, getIndex(), column);
336340
}
337341

338342
/** {@inheritDoc} */
@@ -384,7 +388,6 @@ private ReadOnlyObjectWrapper<TableView<S>> tableViewPropertyImpl() {
384388

385389
// reset the editing index on the TableView
386390
if (table != null) {
387-
TablePosition<S,?> editingCell = table.getEditingCell();
388391
if (updateEditingIndex) table.edit(-1, null);
389392

390393
// request focus back onto the table, only if the current focus
@@ -395,7 +398,7 @@ private ReadOnlyObjectWrapper<TableView<S>> tableViewPropertyImpl() {
395398

396399
CellEditEvent<S,?> editEvent = new CellEditEvent<>(
397400
table,
398-
editingCell,
401+
editingCellAtStartEdit,
399402
TableColumn.editCancelEvent(),
400403
null
401404
);
@@ -704,9 +707,6 @@ private void updateItem(int oldIndex) {
704707
super.layoutChildren();
705708
}
706709

707-
708-
709-
710710
/***************************************************************************
711711
* *
712712
* Expert API *

‎modules/javafx.controls/src/main/java/javafx/scene/control/TablePosition.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2011, 2017, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2011, 2021, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -70,7 +70,7 @@ public TablePosition(@NamedArg("tableView") TableView<S> tableView, @NamedArg("r
7070
super(row, tableColumn);
7171
this.controlRef = new WeakReference<>(tableView);
7272

73-
List<S> items = tableView.getItems();
73+
List<S> items = tableView != null ? tableView.getItems() : null;
7474
this.itemRef = new WeakReference<>(
7575
items != null && row >= 0 && row < items.size() ? items.get(row) : null);
7676

‎modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableCell.java

+5-4
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,9 @@ private ReadOnlyObjectWrapper<TreeTableView<S>> treeTableViewPropertyImpl() {
300300
* *
301301
**************************************************************************/
302302

303+
// editing location at start of edit - fix for JDK-8187229
304+
private TreeTablePosition<S, T> editingCellAtStartEdit = null;
305+
303306
/** {@inheritDoc} */
304307
@Override public void startEdit() {
305308
if (isEditing()) return;
@@ -336,6 +339,7 @@ private ReadOnlyObjectWrapper<TreeTableView<S>> treeTableViewPropertyImpl() {
336339

337340
Event.fireEvent(column, editEvent);
338341
}
342+
editingCellAtStartEdit = new TreeTablePosition<>(table, getIndex(), column);
339343
}
340344

341345
/** {@inheritDoc} */
@@ -390,9 +394,6 @@ private ReadOnlyObjectWrapper<TreeTableView<S>> treeTableViewPropertyImpl() {
390394

391395
// reset the editing index on the TableView
392396
if (table != null) {
393-
@SuppressWarnings("unchecked")
394-
TreeTablePosition<S,T> editingCell = (TreeTablePosition<S,T>) table.getEditingCell();
395-
396397
if (updateEditingIndex) table.edit(-1, null);
397398

398399
// request focus back onto the table, only if the current focus
@@ -403,7 +404,7 @@ private ReadOnlyObjectWrapper<TreeTableView<S>> treeTableViewPropertyImpl() {
403404

404405
CellEditEvent<S,T> editEvent = new CellEditEvent<S,T>(
405406
table,
406-
editingCell,
407+
editingCellAtStartEdit,
407408
TreeTableColumn.<S,T>editCancelEvent(),
408409
null
409410
);

‎modules/javafx.controls/src/main/java/javafx/scene/control/TreeTablePosition.java

+3-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2011, 2021, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -72,7 +72,8 @@ public TreeTablePosition(@NamedArg("treeTableView") TreeTableView<S> treeTableVi
7272
TreeTablePosition(@NamedArg("treeTableView") TreeTableView<S> treeTableView, @NamedArg("row") int row, @NamedArg("tableColumn") TreeTableColumn<S,T> tableColumn, boolean doLookup) {
7373
super(row, tableColumn);
7474
this.controlRef = new WeakReference<>(treeTableView);
75-
this.treeItemRef = new WeakReference<>(doLookup ? treeTableView.getTreeItem(row) : null);
75+
this.treeItemRef = new WeakReference<>(doLookup ?
76+
(treeTableView != null ? treeTableView.getTreeItem(row) : null) : null);
7677

7778
nonFixedColumnIndex = treeTableView == null || tableColumn == null ? -1 : treeTableView.getVisibleLeafIndex(tableColumn);
7879
}

‎modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java

+140-13
Original file line numberDiff line numberDiff line change
@@ -25,33 +25,44 @@
2525

2626
package test.javafx.scene.control;
2727

28-
import javafx.scene.control.TableRow;
29-
import javafx.scene.control.TreeTableRow;
30-
import javafx.scene.control.skin.TableCellSkin;
31-
import test.com.sun.javafx.scene.control.infrastructure.StageLoader;
32-
import test.com.sun.javafx.scene.control.infrastructure.VirtualFlowTestUtils;
33-
import javafx.collections.FXCollections;
34-
import javafx.collections.ObservableList;
35-
import javafx.scene.control.TableCell;
36-
import javafx.scene.control.TableCellShim;
37-
import javafx.scene.control.TableColumn;
38-
import javafx.scene.control.TableView;
28+
import java.lang.ref.WeakReference;
29+
import java.util.ArrayList;
30+
import java.util.List;
3931

4032
import org.junit.After;
4133
import org.junit.Before;
4234
import org.junit.Test;
4335

44-
import static test.com.sun.javafx.scene.control.infrastructure.ControlTestUtils.assertStyleClassContains;
36+
import com.sun.javafx.tk.Toolkit;
37+
4538
import static org.junit.Assert.*;
46-
import static org.junit.Assert.assertEquals;
39+
import static test.com.sun.javafx.scene.control.infrastructure.ControlTestUtils.*;
40+
import static test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory.*;
41+
42+
import javafx.beans.property.SimpleObjectProperty;
43+
import javafx.collections.FXCollections;
44+
import javafx.collections.ObservableList;
45+
import javafx.scene.control.MenuItem;
46+
import javafx.scene.control.TableCell;
47+
import javafx.scene.control.TableCellShim;
48+
import javafx.scene.control.TableColumn;
49+
import javafx.scene.control.TableColumn.CellEditEvent;
50+
import javafx.scene.control.TablePosition;
51+
import javafx.scene.control.TableRow;
52+
import javafx.scene.control.TableView;
53+
import javafx.scene.control.skin.TableCellSkin;
54+
import test.com.sun.javafx.scene.control.infrastructure.StageLoader;
55+
import test.com.sun.javafx.scene.control.infrastructure.VirtualFlowTestUtils;
4756

4857
/**
4958
*/
5059
public class TableCellTest {
5160
private TableCell<String,String> cell;
5261
private TableView<String> table;
62+
private TableColumn<String, String> editingColumn;
5363
private TableRow<String> row;
5464
private ObservableList<String> model;
65+
private StageLoader stageLoader;
5566

5667
@Before public void setup() {
5768
Thread.currentThread().setUncaughtExceptionHandler((thread, throwable) -> {
@@ -65,12 +76,14 @@ public class TableCellTest {
6576
cell = new TableCell<String,String>();
6677
model = FXCollections.observableArrayList("Four", "Five", "Fear"); // "Flop", "Food", "Fizz"
6778
table = new TableView<String>(model);
79+
editingColumn = new TableColumn<>("TEST");
6880

6981
row = new TableRow<>();
7082
}
7183

7284
@After
7385
public void cleanup() {
86+
if (stageLoader != null) stageLoader.dispose();
7487
Thread.currentThread().setUncaughtExceptionHandler(null);
7588
}
7689

@@ -402,6 +415,120 @@ public void testCellInUneditableColumnIsNotEditable() {
402415
assertFalse(cell.isEditing());
403416
}
404417

418+
/**
419+
* Basic config of table-/cell to allow testing of editEvents:
420+
* table is editable, has editingColumn and cell is configured with table and column.
421+
*/
422+
private void setupForEditing() {
423+
table.setEditable(true);
424+
table.getColumns().add(editingColumn);
425+
// FIXME: default cell (of tableColumn) needs not-null value for firing cancel
426+
editingColumn.setCellValueFactory(cc -> new SimpleObjectProperty<>(""));
427+
428+
cell.updateTableView(table);
429+
cell.updateTableColumn(editingColumn);
430+
}
431+
432+
@Test
433+
public void testEditCancelEventAfterCancelOnCell() {
434+
setupForEditing();
435+
int editingIndex = 1;
436+
cell.updateIndex(editingIndex);
437+
table.edit(editingIndex, editingColumn);
438+
TablePosition<?, ?> editingPosition = table.getEditingCell();
439+
List<CellEditEvent<?, ?>> events = new ArrayList<>();
440+
editingColumn.setOnEditCancel(events::add);
441+
cell.cancelEdit();
442+
assertEquals("column must have received editCancel", 1, events.size());
443+
assertEquals("editing location of cancel event", editingPosition, events.get(0).getTablePosition());
444+
}
445+
446+
@Test
447+
public void testEditCancelEventAfterCancelOnTable() {
448+
setupForEditing();
449+
int editingIndex = 1;
450+
cell.updateIndex(editingIndex);
451+
table.edit(editingIndex, editingColumn);
452+
TablePosition<?, ?> editingPosition = table.getEditingCell();
453+
List<CellEditEvent<?, ?>> events = new ArrayList<>();
454+
editingColumn.setOnEditCancel(events::add);
455+
table.edit(-1, null);
456+
assertEquals("column must have received editCancel", 1, events.size());
457+
assertEquals("editing location of cancel event", editingPosition, events.get(0).getTablePosition());
458+
}
459+
460+
@Test
461+
public void testEditCancelEventAfterCellReuse() {
462+
setupForEditing();
463+
int editingIndex = 1;
464+
cell.updateIndex(editingIndex);
465+
table.edit(editingIndex, editingColumn);
466+
TablePosition<?, ?> editingPosition = table.getEditingCell();
467+
List<CellEditEvent<?, ?>> events = new ArrayList<>();
468+
editingColumn.setOnEditCancel(events::add);
469+
cell.updateIndex(0);
470+
assertEquals("column must have received editCancel", 1, events.size());
471+
assertEquals("editing location of cancel event", editingPosition, events.get(0).getTablePosition());
472+
}
473+
474+
@Test
475+
public void testEditCancelEventAfterModifyItems() {
476+
setupForEditing();
477+
stageLoader = new StageLoader(table);
478+
int editingIndex = 1;
479+
table.edit(editingIndex, editingColumn);
480+
TablePosition<?, ?> editingPosition = table.getEditingCell();
481+
List<CellEditEvent<?, ?>> events = new ArrayList<>();
482+
editingColumn.setOnEditCancel(events::add);
483+
table.getItems().add(0, "added");
484+
Toolkit.getToolkit().firePulse();
485+
assertEquals("column must have received editCancel", 1, events.size());
486+
assertEquals("editing location of cancel event", editingPosition, events.get(0).getTablePosition());
487+
}
488+
489+
/**
490+
* Test that removing the editing item implicitly cancels an ongoing
491+
* edit and fires a correct cancel event.
492+
*/
493+
@Test
494+
public void testEditCancelEventAfterRemoveEditingItem() {
495+
setupForEditing();
496+
stageLoader = new StageLoader(table);
497+
int editingIndex = 1;
498+
table.edit(editingIndex, editingColumn);
499+
TablePosition<?, ?> editingPosition = table.getEditingCell();
500+
List<CellEditEvent<?, ?>> events = new ArrayList<>();
501+
editingColumn.setOnEditCancel(events::add);
502+
table.getItems().remove(editingIndex);
503+
Toolkit.getToolkit().firePulse();
504+
assertNull("sanity: editing terminated on items modification", table.getEditingCell());
505+
assertEquals("column must have received editCancel", 1, events.size());
506+
assertEquals("editing location of cancel event", editingPosition, events.get(0).getTablePosition());
507+
}
508+
509+
/**
510+
* Test that removing the editing item does not cause a memory leak.
511+
*/
512+
@Test
513+
public void testEditCancelMemoryLeakAfterRemoveEditingItem() {
514+
TableView<MenuItem> table = new TableView<>(FXCollections.observableArrayList(
515+
new MenuItem("some"), new MenuItem("other")));
516+
TableColumn<MenuItem, String> editingColumn = new TableColumn<>("Text");
517+
editingColumn.setCellValueFactory(cc -> new SimpleObjectProperty<>(""));
518+
table.setEditable(true);
519+
table.getColumns().add(editingColumn);
520+
stageLoader = new StageLoader(table);
521+
int editingIndex = 1;
522+
MenuItem editingItem = table.getItems().get(editingIndex);
523+
WeakReference<MenuItem> itemRef = new WeakReference<>(editingItem);
524+
table.edit(editingIndex, editingColumn);
525+
table.getItems().remove(editingIndex);
526+
editingItem = null;
527+
Toolkit.getToolkit().firePulse();
528+
attemptGC(itemRef);
529+
assertEquals("item must be gc'ed", null, itemRef.get());
530+
}
531+
405532
/**
406533
* Test that cell.cancelEdit can switch table editing off
407534
* even if a subclass violates its contract.

‎modules/javafx.controls/src/test/java/test/javafx/scene/control/TablePositionBaseTest.java

+60
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
/*
2+
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
3+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
4+
*
5+
* This code is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU General Public License version 2 only, as
7+
* published by the Free Software Foundation. Oracle designates this
8+
* particular file as subject to the "Classpath" exception as provided
9+
* by Oracle in the LICENSE file that accompanied this code.
10+
*
11+
* This code is distributed in the hope that it will be useful, but WITHOUT
12+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
13+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
14+
* version 2 for more details (a copy is included in the LICENSE file that
15+
* accompanied this code).
16+
*
17+
* You should have received a copy of the GNU General Public License version
18+
* 2 along with this work; if not, write to the Free Software Foundation,
19+
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
20+
*
21+
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
22+
* or visit www.oracle.com if you need additional information or have any
23+
* questions.
24+
*/
25+
26+
package test.javafx.scene.control;
27+
28+
import org.junit.Test;
29+
30+
import javafx.scene.control.TableColumn;
31+
import javafx.scene.control.TablePosition;
32+
import javafx.scene.control.TreeTableColumn;
33+
import javafx.scene.control.TreeTablePosition;
34+
35+
/**
36+
* Test Tree/TablePosition.
37+
*/
38+
public class TablePositionBaseTest {
39+
40+
//---------- TableView
41+
42+
/**
43+
* JDK-8269136: must not throw on null Table.
44+
*/
45+
@Test
46+
public void testNullTable() {
47+
new TablePosition<>(null, 2, new TableColumn<>());
48+
}
49+
50+
//------------- TreeTableView
51+
52+
/**
53+
* JDK-8269136: must not throw on null TreeTable.
54+
*/
55+
@Test
56+
public void testNullTreeTable() {
57+
new TreeTablePosition<>(null, 2, new TreeTableColumn<>());
58+
}
59+
60+
}

0 commit comments

Comments
 (0)