Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: use the same cell reference constructor in order to ensure consistency #4634

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft

fix!: use the same cell reference constructor in order to ensure consistency #4634

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
245d39b
fix: use the same cell reference constructor in order to ensure consi…
ugur-vaadin Feb 2, 2023
2af83b6
Merge branch 'master' into 4588-spreadsheet-does-not-trigger-addcellv…
ugur-vaadin Feb 14, 2023
fe708df
fix: use getSheet from SpreadsheetCommand to get the sheet
ugur-vaadin Feb 14, 2023
6227ec5
fix: apply formatter
ugur-vaadin Feb 14, 2023
268ff65
fix: add cell reference without sheet name to cell value change event
ugur-vaadin Feb 14, 2023
066e1d2
Merge branch 'main' into 4588-spreadsheet-does-not-trigger-addcellval…
ugur-vaadin Feb 15, 2023
7105375
fix: add missing cell reference without sheet name to event
ugur-vaadin Feb 15, 2023
893994a
Merge branch 'main' into 4588-spreadsheet-does-not-trigger-addcellval…
ugur-vaadin Feb 15, 2023
31fa8ac
fix: update cell value change unit test
ugur-vaadin Feb 15, 2023
8f08024
Merge branch '4588-spreadsheet-does-not-trigger-addcellvaluechangelis…
ugur-vaadin Feb 15, 2023
9158159
Merge branch 'main' into 4588-spreadsheet-does-not-trigger-addcellval…
ugur-vaadin Feb 27, 2023
5476fdd
fix: add sheet name to more internal cell references
ugur-vaadin Feb 27, 2023
09928c6
Merge branch 'main' into 4588-spreadsheet-does-not-trigger-addcellval…
ugur-vaadin Jun 30, 2023
530e9e7
fix: introduce cellset to avoid selected cell duplication in event
ugur-vaadin Jul 3, 2023
27ce9d9
Merge branch 'main' into 4588-spreadsheet-does-not-trigger-addcellval…
ugur-vaadin Jul 3, 2023
f58821b
refactor: add contains checks for indexes and add javadocs
ugur-vaadin Jul 10, 2023
60dd913
Merge branch 'main' into 4588-spreadsheet-does-not-trigger-addcellval…
ugur-vaadin Jul 18, 2023
e20b0fe
refactor: rename api to minimize the need for refactoring
ugur-vaadin Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions ...rc/main/java/com/vaadin/flow/component/spreadsheet/tests/fixtures/PopupButtonFixture.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ public void loadFixture(final Spreadsheet spreadsheet) {
}
List<String> values = new ArrayList<>(VALUES);
CellReference ref = event.getSelectedCellReference();
CellReference newRef = new CellReference(ref.getRow(),
ref.getCol());
CellReference newRef = new CellReference(
spreadsheet.getActiveSheet().getSheetName(), ref.getRow(),
ref.getCol(), false, false);
DataValidationButton popupButton = new DataValidationButton(
spreadsheet, values);
popupButton.setUp();
Expand Down
59 changes: 38 additions & 21 deletions ...dsheet-flow/src/main/java/com/vaadin/flow/component/spreadsheet/CellSelectionManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,9 @@ public SelectionChangeEvent getLatestSelectionEvent() {
}

boolean isCellInsideSelection(int row, int column) {
CellReference cellReference = new CellReference(row - 1, column - 1);
CellReference cellReference = new CellReference(
spreadsheet.getActiveSheet().getSheetName(), row - 1,
column - 1, false, false);
boolean inside = cellReference.equals(selectedCellReference)
|| individualSelectedCells.contains(cellReference);
if (!inside) {
Expand Down Expand Up @@ -173,7 +175,9 @@ protected void reloadCurrentSelection() {
*/
protected void onCellSelected(int row, int column,
boolean discardOldRangeSelection) {
CellReference cellReference = new CellReference(row - 1, column - 1);
CellReference cellReference = new CellReference(
spreadsheet.getActiveSheet().getSheetName(), row - 1,
column - 1, false, false);
CellReference previousCellReference = selectedCellReference;
if (!cellReference.equals(previousCellReference)
|| discardOldRangeSelection && (!cellRangeAddresses.isEmpty()
Expand Down Expand Up @@ -217,8 +221,9 @@ protected void onSheetAddressChanged(String value,
region.col1 - 1, region.col2 - 1);
}
handleCellRangeSelection(cra);
selectedCellReference = new CellReference(cra.getFirstRow(),
cra.getFirstColumn());
selectedCellReference = new CellReference(
spreadsheet.getActiveSheet().getSheetName(),
cra.getFirstRow(), cra.getFirstColumn(), false, false);
paintedCellRange = cra;
cellRangeAddresses.clear();
cellRangeAddresses.add(cra);
Expand All @@ -234,8 +239,10 @@ protected void onSheetAddressChanged(String value,
.createCorrectCellRangeAddress(region.row1,
region.col1, region.row2, region.col2);
handleCellRangeSelection(cra);
selectedCellReference = new CellReference(cra.getFirstRow(),
cra.getFirstColumn());
selectedCellReference = new CellReference(
spreadsheet.getActiveSheet().getSheetName(),
cra.getFirstRow(), cra.getFirstColumn(), false,
false);
paintedCellRange = cra;
cellRangeAddresses.clear();
cellRangeAddresses.add(cra);
Expand Down Expand Up @@ -382,8 +389,9 @@ protected void handleCellRangeSelection(CellRangeAddress cra) {

protected void handleCellRangeSelection(String name, CellRangeAddress cra) {

final CellReference firstCell = new CellReference(cra.getFirstRow(),
cra.getFirstColumn());
final CellReference firstCell = new CellReference(
spreadsheet.getActiveSheet().getSheetName(), cra.getFirstRow(),
cra.getFirstColumn(), false, false);

handleCellRangeSelection(name, firstCell, cra, true);
}
Expand Down Expand Up @@ -463,8 +471,9 @@ protected void onCellRangePainted(int selectedCellRow,
cellRangeAddresses.clear();
individualSelectedCells.clear();

selectedCellReference = new CellReference(selectedCellRow - 1,
selectedCellColumn - 1);
selectedCellReference = new CellReference(
spreadsheet.getActiveSheet().getSheetName(),
selectedCellRow - 1, selectedCellColumn - 1, false, false);

CellRangeAddress cra = spreadsheet.createCorrectCellRangeAddress(row1,
col1, row2, col2);
Expand Down Expand Up @@ -507,7 +516,9 @@ protected void onCellAddToSelectionAndSelected(int row, int column) {
individualSelectedCells.add(selectedCellReference);
}
handleCellSelection(row, column);
selectedCellReference = new CellReference(row - 1, column - 1);
selectedCellReference = new CellReference(
spreadsheet.getActiveSheet().getSheetName(), row - 1,
column - 1, false, false);
spreadsheet.loadCustomEditorOnSelectedCell();
if (individualSelectedCells.contains(selectedCellReference)) {
individualSelectedCells.remove(
Expand Down Expand Up @@ -558,8 +569,9 @@ protected void onCellsAddedToRangeSelection(int row1, int col1, int row2,
*/
protected void onRowSelected(int row, int firstColumnIndex) {
handleCellSelection(row, firstColumnIndex);
selectedCellReference = new CellReference(row - 1,
firstColumnIndex - 1);
selectedCellReference = new CellReference(
spreadsheet.getActiveSheet().getSheetName(), row - 1,
firstColumnIndex - 1, false, false);
spreadsheet.loadCustomEditorOnSelectedCell();
cellRangeAddresses.clear();
individualSelectedCells.clear();
Expand Down Expand Up @@ -592,8 +604,9 @@ protected void onRowAddedToRangeSelection(int row, int firstColumnIndex) {
individualSelectedCells.add(selectedCellReference);
}
handleCellSelection(row, firstColumnIndex);
selectedCellReference = new CellReference(row - 1,
firstColumnIndex - 1);
selectedCellReference = new CellReference(
spreadsheet.getActiveSheet().getSheetName(), row - 1,
firstColumnIndex - 1, false, false);
spreadsheet.loadCustomEditorOnSelectedCell();
cellRangeAddresses.add(spreadsheet.createCorrectCellRangeAddress(row, 1,
row, spreadsheet.getColumns()));
Expand All @@ -612,8 +625,9 @@ protected void onRowAddedToRangeSelection(int row, int firstColumnIndex) {
*/
protected void onColumnSelected(int firstRowIndex, int column) {
handleCellSelection(firstRowIndex, column);
selectedCellReference = new CellReference(firstRowIndex - 1,
column - 1);
selectedCellReference = new CellReference(
spreadsheet.getActiveSheet().getSheetName(), firstRowIndex - 1,
column - 1, false, false);
spreadsheet.loadCustomEditorOnSelectedCell();
cellRangeAddresses.clear();
individualSelectedCells.clear();
Expand Down Expand Up @@ -646,8 +660,9 @@ protected void onColumnAddedToSelection(int firstRowIndex, int column) {
individualSelectedCells.add(selectedCellReference);
}
handleCellSelection(firstRowIndex, column);
selectedCellReference = new CellReference(firstRowIndex - 1,
column - 1);
selectedCellReference = new CellReference(
spreadsheet.getActiveSheet().getSheetName(), firstRowIndex - 1,
column - 1, false, false);
spreadsheet.loadCustomEditorOnSelectedCell();
cellRangeAddresses.add(spreadsheet.createCorrectCellRangeAddress(1,
column, spreadsheet.getRows(), column));
Expand Down Expand Up @@ -676,8 +691,10 @@ protected void mergedRegionAdded(CellRangeAddress region) {
handleCellAddressChange(region.getFirstRow() + 1,
region.getFirstColumn() + 1, false);
}
selectedCellReference = new CellReference(region.getFirstRow(),
region.getFirstColumn());
selectedCellReference = new CellReference(
spreadsheet.getActiveSheet().getSheetName(),
region.getFirstRow(), region.getFirstColumn(), false,
false);
fire = true;
}
for (Iterator<CellRangeAddress> i = cellRangeAddresses.iterator(); i
Expand Down
8 changes: 6 additions & 2 deletions ...dsheet-flow/src/main/java/com/vaadin/flow/component/spreadsheet/CellSelectionShifter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,9 @@ private void fireCellValueChangeEvent(CellRangeAddress region) {
for (int x = region.getFirstColumn(); x <= region
.getLastColumn(); x++) {
for (int y = region.getFirstRow(); y <= region.getLastRow(); y++) {
cells.add(new CellReference(y, x));
cells.add(new CellReference(
spreadsheet.getActiveSheet().getSheetName(), y, x,
false, false));
}
}
spreadsheet.fireEvent(new CellValueChangeEvent(spreadsheet, cells));
Expand Down Expand Up @@ -399,8 +401,10 @@ public void onSelectionDecreasePainted(int r, int c) {
if (!SpreadsheetUtil.isCellInRange(selectedCellReference,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

18% of developers fix this issue

NULL_DEREFERENCE: object selectedCellReference last assigned on line 398 could be null and is dereferenced by call to isCellInRange(...) at line 401.

ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

newPaintedCellRange)) {
selectedCellReference = new CellReference(
spreadsheet.getActiveSheet().getSheetName(),
newPaintedCellRange.getFirstRow(),
newPaintedCellRange.getFirstColumn());
newPaintedCellRange.getFirstColumn(), false,
false);
}
getCellSelectionManager().handleCellRangeSelection(
selectedCellReference, newPaintedCellRange, false);
Expand Down
4 changes: 3 additions & 1 deletion ...preadsheet-flow/src/main/java/com/vaadin/flow/component/spreadsheet/CellValueManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,9 @@ public void onCellValueChange(int col, int row, String value) {

// capture cell value to history
CellValueCommand command = new CellValueCommand(spreadsheet);
command.captureCellValues(new CellReference(row - 1, col - 1));
command.captureCellValues(
new CellReference(spreadsheet.getActiveSheet().getSheetName(),
row - 1, col - 1, false, false));
spreadsheet.getSpreadsheetHistoryManager().addCommand(command);
boolean updateHyperlinks = false;
boolean formulaChanged = false;
Expand Down
16 changes: 11 additions & 5 deletions ...din-spreadsheet-flow/src/main/java/com/vaadin/flow/component/spreadsheet/Spreadsheet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,8 @@ void setNamedRanges(List<String> namedRanges) {
void onPopupButtonClick(int row, int column) {
PopupButton popup = sheetPopupButtons
.get(SpreadsheetUtil.relativeToAbsolute(this,
new CellReference(row - 1, column - 1)));
new CellReference(getActiveSheet().getSheetName(),
row - 1, column - 1, false, false)));
if (popup != null) {
popup.openPopup();
}
Expand All @@ -652,7 +653,8 @@ void onPopupButtonClick(int row, int column) {
void onPopupClose(int row, int column) {
PopupButton popup = sheetPopupButtons
.get(SpreadsheetUtil.relativeToAbsolute(this,
new CellReference(row - 1, column - 1)));
new CellReference(getActiveSheet().getSheetName(),
row - 1, column - 1, false, false)));

if (popup != null) {
popup.closePopup();
Expand Down Expand Up @@ -2922,7 +2924,8 @@ private void rowsMoved(int first, int last, int n) {
} else if (numberOfRowsAboveWasChanged(row, last, first)) {
int newRow = cell.getRow() + n;
int col = cell.getCol();
CellReference newCell = new CellReference(newRow, col, true,
CellReference newCell = new CellReference(
getActiveSheet().getSheetName(), newRow, col, true,
true);
pbutton.setCellReference(newCell);
updated.put(newCell, pbutton);
Expand Down Expand Up @@ -4760,7 +4763,8 @@ public void setPopup(String cellAddress, PopupButton popupButton) {
* removes the pop-up button for the target cell.
*/
public void setPopup(int row, int col, PopupButton popupButton) {
setPopup(new CellReference(row, col), popupButton);
setPopup(new CellReference(getActiveSheet().getSheetName(), row, col,
false, false), popupButton);
}

/**
Expand Down Expand Up @@ -5184,7 +5188,9 @@ private static Set<CellReference> getAllSelectedCells(

for (int x = a.getFirstColumn(); x <= a.getLastColumn(); x++) {
for (int y = a.getFirstRow(); y <= a.getLastRow(); y++) {
cells.add(new CellReference(y, x));
cells.add(new CellReference(
selectedCellReference.getSheetName(), y, x,
false, false));
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion ...heet-flow/src/main/java/com/vaadin/flow/component/spreadsheet/SpreadsheetHandlerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,9 @@ private void fireCellValueChangeEvent(CellRangeAddress region) {
for (int x = region.getFirstColumn(); x <= region
.getLastColumn(); x++) {
for (int y = region.getFirstRow(); y <= region.getLastRow(); y++) {
cells.add(new CellReference(y, x));
cells.add(new CellReference(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue with the overall change to include spreadsheet names into the cell references is that if a developer is already using cell references without the sheet name, the comparison will now fail. And that's quite easy to achieve, as new CellReference("A2") is a lot easier to use than the other alternatives that include the sheet name. It's very likely a breaking change.

We could consider adding both types of references (with and without sheet name) to the events. For example, for cell value change events we could modify the logic here:

flow-components/vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow/src/main/java/com/vaadin/flow/component/spreadsheet/CellValueManager.java

Lines 796 to 799 in 245d39b

private void fireCellValueChangeEvent(Set<CellReference> changedCells) {
spreadsheet
.fireEvent(new CellValueChangeEvent(spreadsheet, changedCells));
}

To something like:

    private void fireCellValueChangeEvent(Set<CellReference> changedCells) {
        List<CellReference> cellRefsWithSheetName = changedCells
                .stream()
                .filter(ref -> ref.getSheetName() != null)
                .toList();
        cellRefsWithSheetName.forEach(ref -> {
            CellReference refWithoutSheetName = new CellReference(ref.getRow(), ref.getCol());
            changedCells.add(refWithoutSheetName);
        });
        spreadsheet
                .fireEvent(new CellValueChangeEvent(spreadsheet, changedCells));
    }

The downside would be that a change to a single cell will report two changes in the event. Though that might be the lesser evil than relying on which CellReference constructor someone is using.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in the latest version

spreadsheet.getActiveSheet().getSheetName(), y, x,
false, false));
}
}
fireCellValueChangeEvent(cells);
Expand Down
6 changes: 4 additions & 2 deletions ...w/src/main/java/com/vaadin/flow/component/spreadsheet/command/CellShiftValuesCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,10 @@ public CellReference getSelectedCellReference() {
.isCellInRange(selectedCellReference, paintedCellRange)) {
return selectedCellReference;
} else {
return new CellReference(paintedCellRange.getFirstRow(),
paintedCellRange.getFirstColumn());
return new CellReference(
spreadsheet.getActiveSheet().getSheetName(),
paintedCellRange.getFirstRow(),
paintedCellRange.getFirstColumn(), false, false);
}
}

Expand Down
11 changes: 7 additions & 4 deletions ...et-flow/src/main/java/com/vaadin/flow/component/spreadsheet/command/CellValueCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,8 @@ public void captureCellRangeValues(CellRangeAddress... cellRanges) {

@Override
public CellReference getSelectedCellReference() {
return new CellReference(selectedCellRow, selectedcellCol);
return new CellReference(spreadsheet.getActiveSheet().getSheetName(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For commands, it seems safer to use SpreadsheetCommand.getSheet(). Currently that always delegates to Spreadsheet.getActivesheet(), but there's a chance that the implementation might change at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in the latest version

selectedCellRow, selectedcellCol, false, false);
}

@Override
Expand Down Expand Up @@ -339,13 +340,15 @@ public Set<CellReference> getChangedCells() {
for (Object o : values) {
if (o instanceof CellValue) {
CellValue cellValue = (CellValue) o;
changedCells
.add(new CellReference(cellValue.row, cellValue.col));
changedCells.add(new CellReference(
spreadsheet.getActiveSheet().getSheetName(),
cellValue.row, cellValue.col, false, false));
} else {
CellRangeValue cellRangeValue = (CellRangeValue) o;
for (int r = cellRangeValue.row1; r <= cellRangeValue.row2; r++) {
for (int c = cellRangeValue.col1; c <= cellRangeValue.col2; c++) {
changedCells.add(new CellReference(r, c));
changedCells.add(new CellReference(
getSheet().getSheetName(), r, c, false, false));
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion ...src/main/java/com/vaadin/flow/component/spreadsheet/command/RowInsertOrDeleteCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ public void execute() {

@Override
public CellReference getSelectedCellReference() {
return new CellReference(row, 0);
return new CellReference(spreadsheet.getActiveSheet().getSheetName(),
row, 0, false, false);
}

@Override
Expand Down