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

[ugur-vaadin]

Description

In Apache POI, CellReference has multiple constructors. In the version that Vaadin V8 used, the constructor CellReference(Cell cell) did not use the sheet name from the Cell object. However, while having the same signature, this constructor started using the sheet name in the new versions. This creates inconsistencies when checking equality for CellReferences since we use different constructors in various locations.

Several options were discussed, including:

• Adding cell references with and without the sheet names to the event. This fixes Set.contains usages, however the set will have duplicates and any size comparison would be broken.
• Using the cell references that only use the sheet names. This will have the correct size without duplicates, however will require the Set.contains checks to be updated to include the sheet names. This might also be somewhat inconvenient. Using the constructors without sheet names (e.g "B2", "C3") is easier and the sheet name might be expected to determined from the context.
• Using the cell references that only use the sheet names internally, but introducing a new class CellSet that handles contains behaviour inside. This will be a breaking change for users that use the set directly or use other Set API.

This PR goes with this option. For the API:

• size() is present
• contains(...) is present
• getCells() is used to get the actual set of cells and perform any other operations

Note that currently, CellSet only supports cells from the same sheet. If necessary, another internal set can be initialized with all the combinations to provide accurate and fast response to contains(...) calls.

Fixes #4588

Type of change

• Bugfix
• Feature

Checklist

• I have read the contribution guide: https://vaadin.com/docs/latest/contributing/overview
• I have added a description following the guideline.
• The issue is created in the corresponding repository and I have referenced it.
• I have added tests to ensure my change is effective and works as intended.
• New and existing tests are passing locally with my change.
• I have performed self-review and corrected misspellings.

[sonatype-lift]

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 ]

[sissbruecker]

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.

[ugur-vaadin]

Addressed in the latest version

[sissbruecker]

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.

[ugur-vaadin]

Addressed in the latest version

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

No Coverage information
0.0% Duplication

[rolfsmeds]

As we are not aware of any users having problems due to this issue, perhaps it would be better to wait until the next major as it will introduce some breaking changes?

[yuriy-fix]

Added to the v25 breaking changes list to be revisited before v25.