Skip to content

Commit f3b7e64

Browse files
vaadin-botDiegoCardoso
vaadin-bot
and
DiegoCardoso
authored
fix: improve custom editors internal structure to prevent NPE errors (#7755) (#7772)
* fix: prevent trying to remove already removed Sometimes, the call to `removeCellCustomEditor` can be done to a widget already removed from its parent, which causes a client-side error. This change try to address that with some refactor on the custom editors internal map. The refactor aims to replace the keys that currently refer to the cell address with the editor ID itself. That way, in case the same editor is shared by more than one cell, the same `Slot` will be used. With the current logic, more than one `Slot` element would be created for the same editor, which could cause issues like, the editor not being shown because it was moved to the other `Slot` parent. Co-authored-by: Diego Cardoso <diego@vaadin.com>
1 parent 6ffaa56 commit f3b7e64

File tree

9 files changed

+179
-22
lines changed

9 files changed

+179
-22
lines changed

‎vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow-client/src/main/java/com/vaadin/addon/spreadsheet/client/CustomEditorEventListener.java

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,17 +28,25 @@
2828
public class CustomEditorEventListener implements EventListener {
2929

3030
private Slot slot;
31-
private String key;
31+
private String cellAddress;
3232
private SpreadsheetWidget widget;
3333

34-
public void init(Slot slot, String key) {
34+
public void init(Slot slot, String cellAddress) {
3535
this.slot = slot;
36-
this.key = key;
36+
this.cellAddress = cellAddress;
3737
Event.setEventListener(slot.getAssignedElement(), this);
3838
DOM.sinkEvents(slot.getAssignedElement(),
3939
Event.ONKEYDOWN | Event.FOCUSEVENTS);
4040
}
4141

42+
public void setCellAddress(String cellAddress) {
43+
this.cellAddress = cellAddress;
44+
}
45+
46+
public String getCellAddress() {
47+
return cellAddress;
48+
}
49+
4250
@Override
4351
public void onBrowserEvent(Event event) {
4452
switch (event.getTypeInt()) {
@@ -57,7 +65,7 @@ public void onBrowserEvent(Event event) {
5765
break;
5866
case Event.ONFOCUS:
5967
var jsniUtil = getSheetWidget().getSheetJsniUtil();
60-
jsniUtil.parseColRow(key);
68+
jsniUtil.parseColRow(cellAddress);
6169
var col = jsniUtil.getParsedCol();
6270
var row = jsniUtil.getParsedRow();
6371
getSheetWidget().setSelectedCell(col, row);

‎vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow-client/src/main/java/com/vaadin/addon/spreadsheet/client/SheetWidget.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5170,7 +5170,7 @@ public boolean isShowCustomEditorOnFocus() {
51705170
public void removeCustomCellEditor(String address,
51715171
Widget customEditorWidget) {
51725172

5173-
if (customEditorWidget == null) {
5173+
if (customEditorWidget == null || !customEditorWidget.isAttached()) {
51745174
return;
51755175
}
51765176

@@ -6012,7 +6012,7 @@ public Iterator<Widget> iterator() {
60126012
// This is for clearing of sheet from custom widgets
60136013
protected Collection<Widget> getCustomWidgetIterator() {
60146014
final List<Widget> emptyList = new ArrayList<Widget>();
6015-
if (customEditorWidget != null) {
6015+
if (customEditorWidget != null && customEditorWidget.isAttached()) {
60166016
emptyList.add(customEditorWidget);
60176017
}
60186018
emptyList.addAll(sheetOverlays.values());

‎vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow-client/src/main/java/com/vaadin/addon/spreadsheet/client/Slot.java

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ public class Slot extends Widget {
1616

1717
private final Element assignedElement;
1818
private boolean isElementFocused;
19+
private CustomEditorEventListener listener;
1920

2021
public Slot(String name, Element assignedElement, Element host) {
2122
this.assignedElement = assignedElement;
@@ -49,4 +50,12 @@ public boolean isElementFocused() {
4950
public void setElementFocused(boolean isElementFocused) {
5051
this.isElementFocused = isElementFocused;
5152
}
53+
54+
public CustomEditorEventListener getListener() {
55+
return listener;
56+
}
57+
58+
public void setListener(CustomEditorEventListener listener) {
59+
this.listener = listener;
60+
}
5261
}

‎vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow-client/src/main/java/com/vaadin/addon/spreadsheet/client/SpreadsheetConnector.java

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ public void onElementResize(ElementResizeEvent e) {
185185

186186
private Element host;
187187

188-
private HashMap<String, Widget> customEditors = null;
188+
private HashMap<String, Slot> customEditors = null;
189189

190190
// spreadsheet: we need the server side proxy
191191
public <T extends ServerRpc> T getProtectedRpcProxy(Class<T> rpcInterface) {
@@ -461,15 +461,16 @@ private void removeOldOverlays(Set<String> newOverlayKeys) {
461461
}
462462
}
463463

464-
public HashMap<String, Widget> getCustomEditors() {
464+
public HashMap<String, Slot> getCustomEditors() {
465465
return customEditors;
466466
}
467467

468468
private void setupCustomEditors() {
469469
if (getState().cellKeysToEditorIdMap == null) {
470-
if (getWidget().isShowCustomEditorOnFocus()) {
470+
if (!getWidget().isShowCustomEditorOnFocus()) {
471471
getWidget().removeCellCustomEditors(getCustomEditors());
472472
}
473+
customEditors = null;
473474
getWidget().setCustomEditorFactory(null);
474475
} else if (getWidget().getCustomEditorFactory() == null) {
475476
customEditors = new HashMap<>();
@@ -485,20 +486,23 @@ public boolean hasCustomEditor(String key) {
485486

486487
@Override
487488
public Widget getCustomEditor(String key) {
488-
if (customEditors.containsKey(key)) {
489-
return customEditors.get(key);
490-
}
491489
String editorId = getState().cellKeysToEditorIdMap
492490
.get(key);
491+
if (customEditors.containsKey(editorId)) {
492+
var slot = customEditors.get(editorId);
493+
slot.getListener().setCellAddress(key);
494+
return slot;
495+
}
493496
var editor = SheetJsniUtil.getVirtualChild(editorId,
494497
host.getPropertyString("appId"));
495498
Slot slot = new Slot("custom-editor-" + editorId,
496499
editor, host);
497-
customEditors.put(key, slot);
500+
customEditors.put(editorId, slot);
498501
CustomEditorEventListener listener = GWT
499502
.create(CustomEditorEventListener.class);
500503
listener.setSpreadsheetWidget(getWidget());
501504
listener.init(slot, key);
505+
slot.setListener(listener);
502506

503507
return slot;
504508
}

‎vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow-client/src/main/java/com/vaadin/addon/spreadsheet/client/SpreadsheetWidget.java

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -482,15 +482,16 @@ public void showCellCustomEditors(
482482
* @param customEditors
483483
* a map of cell keys to custom editor widgets
484484
*/
485-
public void removeCellCustomEditors(HashMap<String, Widget> customEditors) {
485+
public void removeCellCustomEditors(HashMap<String, Slot> customEditors) {
486486
if (customEditors == null || customEditors.isEmpty()) {
487487
return;
488488
}
489489

490-
for (var customEditor : customEditors.entrySet()) {
490+
for (var customEditor : customEditors.values()) {
491491
if (customEditor != null) {
492-
sheetWidget.removeCustomCellEditor(customEditor.getKey(),
493-
customEditor.getValue());
492+
sheetWidget.removeCustomCellEditor(
493+
customEditor.getListener().getCellAddress(),
494+
customEditor);
494495
}
495496
}
496497
}

‎vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow-integration-tests/src/main/java/com/vaadin/flow/component/spreadsheet/tests/fixtures/CustomEditorSharedFixture.java

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
/**
2+
* Copyright 2000-2025 Vaadin Ltd.
3+
*
4+
* This program is available under Vaadin Commercial License and Service Terms.
5+
*
6+
* See {@literal <https://vaadin.com/commercial-license-and-service-terms>} for the full
7+
* license.
8+
*/
9+
package com.vaadin.flow.component.spreadsheet.tests.fixtures;
10+
11+
import java.util.ArrayList;
12+
13+
import org.apache.poi.ss.usermodel.Cell;
14+
import org.apache.poi.ss.usermodel.Sheet;
15+
import org.apache.poi.ss.util.CellAddress;
16+
17+
import com.vaadin.flow.component.Component;
18+
import com.vaadin.flow.component.spreadsheet.Spreadsheet;
19+
import com.vaadin.flow.component.spreadsheet.SpreadsheetComponentFactory;
20+
import com.vaadin.flow.component.textfield.TextField;
21+
22+
public class CustomEditorSharedFixture implements SpreadsheetFixture {
23+
24+
static final int COMPONENT_ROW_INDEX = 1;
25+
static final int COMPONENT_COL_INDEX = 1;
26+
27+
@Override
28+
public void loadFixture(final Spreadsheet spreadsheet) {
29+
System.out.println("Loading CustomEditorRowFixture");
30+
spreadsheet.setSpreadsheetComponentFactory(new CustomEditorFactory());
31+
spreadsheet.setColumnWidth(0, 200);
32+
spreadsheet.setShowCustomEditorOnFocus(true);
33+
34+
var cells = new ArrayList<Cell>();
35+
cells.add(spreadsheet.createCell(COMPONENT_ROW_INDEX,
36+
COMPONENT_COL_INDEX, ""));
37+
cells.add(spreadsheet.createCell(COMPONENT_ROW_INDEX + 1,
38+
COMPONENT_COL_INDEX, ""));
39+
spreadsheet.refreshCells(cells);
40+
}
41+
42+
private static class CustomEditorFactory
43+
implements SpreadsheetComponentFactory {
44+
45+
private TextField textField;
46+
47+
@Override
48+
public Component getCustomComponentForCell(Cell cell, int rowIndex,
49+
int columnIndex, Spreadsheet spreadsheet, Sheet sheet) {
50+
return null;
51+
}
52+
53+
@Override
54+
public Component getCustomEditorForCell(Cell cell, final int rowIndex,
55+
final int columnIndex, final Spreadsheet spreadsheet,
56+
Sheet sheet) {
57+
if (!sheet.getSheetName().equals("Sheet1")
58+
|| rowIndex < COMPONENT_ROW_INDEX
59+
|| rowIndex > COMPONENT_ROW_INDEX + 1
60+
|| columnIndex != COMPONENT_COL_INDEX) {
61+
return null;
62+
}
63+
64+
if (textField == null) {
65+
textField = new TextField();
66+
textField.addValueChangeListener(e -> spreadsheet.refreshCells(
67+
spreadsheet.createCell(activeCell.getRow(),
68+
activeCell.getColumn(), e.getValue())));
69+
}
70+
return textField;
71+
}
72+
73+
private CellAddress activeCell;
74+
75+
@Override
76+
public void onCustomEditorDisplayed(Cell cell, int rowIndex,
77+
int columnIndex, Spreadsheet spreadsheet, Sheet sheet,
78+
Component customEditor) {
79+
if (cell == null) {
80+
return;
81+
}
82+
83+
activeCell = new CellAddress(rowIndex, columnIndex);
84+
85+
if (customEditor instanceof TextField editor) {
86+
editor.setValue(cell.getStringCellValue());
87+
}
88+
}
89+
}
90+
91+
}

‎vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow-integration-tests/src/main/java/com/vaadin/flow/component/spreadsheet/tests/fixtures/TestFixtures.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ public enum TestFixtures {
3636
Rename(RenameFixture.class),
3737
CreateSheet(SheetsFixture.class),
3838
CustomEditor(SimpleCustomEditorFixture.class),
39+
CustomEditorShared(CustomEditorSharedFixture.class),
3940
CustomEditorRow(CustomEditorRowFixture.class),
4041
Styles(StylesFixture.class),
4142
LockCell(LockCellFixture.class),

‎vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow-integration-tests/src/test/java/com/vaadin/flow/component/spreadsheet/test/CustomEditorIT.java

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,46 @@ public void customEditorIsVisible_sheetIsChanged_editorsRemoved() {
354354
}
355355
}
356356

357+
@Test
358+
public void customEditorShared_persistsValuesCorrectly() {
359+
createNewSpreadsheet();
360+
loadTestFixture(TestFixtures.CustomEditorShared);
361+
362+
// Test that moving focus between cells with shared custom editors
363+
// works correctly and values are persisted
364+
clickCell("B2");
365+
var maybeEditor = getInputInCustomEditorFromCell("B2");
366+
Assert.assertTrue(maybeEditor.isPresent());
367+
368+
clickCell("B3");
369+
maybeEditor = getInputInCustomEditorFromCell("B3");
370+
Assert.assertTrue(maybeEditor.isPresent());
371+
372+
clickCell("B2");
373+
maybeEditor = getInputInCustomEditorFromCell("B2");
374+
Assert.assertTrue(maybeEditor.isPresent());
375+
376+
clickCell("B2");
377+
maybeEditor = getInputInCustomEditorFromCell("B2");
378+
Assert.assertTrue(maybeEditor.isPresent());
379+
var editor = maybeEditor.get();
380+
editor.sendKeys("TestValueB2", Keys.ENTER);
381+
382+
clickCell("A1");
383+
getCommandExecutor().waitForVaadin();
384+
Assert.assertEquals("TestValueB2", getCellValue("B2"));
385+
386+
clickCell("B3");
387+
maybeEditor = getInputInCustomEditorFromCell("B3");
388+
Assert.assertTrue(maybeEditor.isPresent());
389+
editor = maybeEditor.get();
390+
editor.sendKeys("TestValueB3", Keys.ENTER);
391+
392+
clickCell("A1");
393+
getCommandExecutor().waitForVaadin();
394+
Assert.assertEquals("TestValueB3", getCellValue("B3"));
395+
}
396+
357397
@Test
358398
public void customEditorInFrozenCells_persistsValueOnVariousKeyActions()
359399
throws Exception {

‎vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow/src/main/java/com/vaadin/flow/component/spreadsheet/SpreadsheetComponentFactory.java

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -83,15 +83,18 @@ Component getCustomComponentForCell(Cell cell, int rowIndex,
8383
int columnIndex, Spreadsheet spreadsheet, Sheet sheet);
8484

8585
/**
86-
* Should return the custom component that is displayed in the cell when it
87-
* has been selected. Thus, the component replaces the default inline editor
86+
* Should return the custom component that is displayed in the cell when the
87+
* sheet is loaded or then the cell is selected when
88+
* {@link Spreadsheet#setShowCustomEditorOnFocus(boolean)} is enabled. has
89+
* been selected. Thus, the component replaces the default inline editor
8890
* functionality in the Spreadsheet. This method is called only for cells
8991
* that are <b>not locked</b> (a cell is considered locked when the sheet or
9092
* cell is protected or the cell is null).
9193
* <p>
92-
* If some cells share the same type of "editor", the same component
93-
* instance can be shared for all of those cells. As the component comes
94-
* visible in Cell X, the
94+
* If {@link Spreadsheet#setShowCustomEditorOnFocus(boolean)} is enabled and
95+
* some cells share the same type of "editor", the same component instance
96+
* can be shared for all of those cells. As the component comes visible in
97+
* Cell X, the
9598
* {@link #onCustomEditorDisplayed(Cell, int, int, Spreadsheet, Sheet, Component)}
9699
* is called with the appropriate parameters. This way, you can update the
97100
* editor component value accordingly to the currently selected cell.

0 commit comments

Comments
 (0)