fix: introduce cellset to avoid selected cell duplication in event

Author: ugur-vaadin

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

@@ -30,7 +30,7 @@ public class PopupButtonFixture implements SpreadsheetFixture {
     @Override
     public void loadFixture(final Spreadsheet spreadsheet) {
         spreadsheet.addSelectionChangeListener(event -> {
-            if (event.getAllSelectedCells().size() != 1) {
+            if (event.getAllSelectedCells().getCellCount() != 1) {
                 return;
             }
             List<String> values = new ArrayList<>(VALUES);

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

@@ -0,0 +1,36 @@
+package com.vaadin.flow.component.spreadsheet;
+
+import org.apache.poi.ss.util.CellReference;
+
+import java.util.Collections;
+import java.util.Set;
+
+public class CellSet {
+
+    private final Set<CellReference> cells;
+
+    public CellSet(Set<CellReference> cells) {
+        this.cells = cells;
+    }
+
+    public Set<CellReference> getCells() {
+        return Collections.unmodifiableSet(cells);
+    }
+
+    public int getCellCount() {
+        return cells.size();
+    }
+
+    public boolean containsCell(CellReference cell) {
+        if (cells.isEmpty()) {
+            return false;
+        }
+        if (cell.getSheetName() == null) {
+            CellReference cellWithSheetName = new CellReference(
+                    cells.iterator().next().getSheetName(), cell.getRow(),
+                    cell.getCol(), cell.isRowAbsolute(), cell.isColAbsolute());
+            return cells.contains(cellWithSheetName);
+        }
+        return cells.contains(cell);
+    }
+}

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

@@ -783,14 +783,9 @@ private String getFormattedCellValue(Cell cell) {
     }
 
     private void fireCellValueChangeEvent(Cell cell) {
-        Set<CellReference> cells = new HashSet<CellReference>();
+        Set<CellReference> cells = new HashSet<>();
         CellReference ref = new CellReference(cell);
         cells.add(ref);
-        if (ref.getSheetName() != null) {
-            CellReference refWithoutSheetName = new CellReference(ref.getRow(),
-                    ref.getCol());
-            cells.add(refWithoutSheetName);
-        }
         spreadsheet.fireEvent(new CellValueChangeEvent(spreadsheet, cells));
     }
 
@@ -800,13 +795,6 @@ private void fireFormulaValueChangeEvent(Set<CellReference> changedCells) {
     }
 
     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));
     }

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

@@ -2857,16 +2857,14 @@ public void shiftRows(int startRow, int endRow, int n,
                     getHiddenRowIndexes());
             if (row == null) {
                 valueManager.updateDeletedRowsInClientCache(rowIndex, rowIndex);
-                if (_hiddenRowIndexes.contains(rowIndex)) {
-                    _hiddenRowIndexes.remove(rowIndex);
-                }
+                _hiddenRowIndexes.remove(rowIndex);
                 for (int c = 0; c < getCols(); c++) {
                     styler.clearCellStyle(r, c);
                 }
             } else {
                 if (row.getZeroHeight()) {
                     _hiddenRowIndexes.add(rowIndex);
-                } else if (_hiddenRowIndexes.contains(rowIndex)) {
+                } else {
                     _hiddenRowIndexes.remove(rowIndex);
                 }
                 for (int c = 0; c < getCols(); c++) {
@@ -5242,15 +5240,15 @@ public void setRowColHeadingsVisible(boolean visible) {
      */
     public abstract static class ValueChangeEvent
             extends ComponentEvent<Component> {
-        private final Set<CellReference> changedCells;
+        private final CellSet changedCells;
 
         public ValueChangeEvent(Component source,
                 Set<CellReference> changedCells) {
             super(source, false);
-            this.changedCells = changedCells;
+            this.changedCells = new CellSet(changedCells);
         }
 
-        public Set<CellReference> getChangedCells() {
+        public CellSet getChangedCells() {
             return changedCells;
         }
     }
@@ -5374,21 +5372,18 @@ public List<CellRangeAddress> getCellRangeAddresses() {
          * @return A combination of all selected cells, regardless of selection
          *         mode. Doesn't contain duplicates.
          */
-        public Set<CellReference> getAllSelectedCells() {
-            return Spreadsheet.getAllSelectedCells(selectedCellReference,
-                    individualSelectedCells, cellRangeAddresses);
-
+        public CellSet getAllSelectedCells() {
+            return new CellSet(
+                    Spreadsheet.getAllSelectedCells(selectedCellReference,
+                            individualSelectedCells, cellRangeAddresses));
         }
     }
 
     private static Set<CellReference> getAllSelectedCells(
             CellReference selectedCellReference,
             List<CellReference> individualSelectedCells,
             List<CellRangeAddress> cellRangeAddresses) {
-        Set<CellReference> cells = new HashSet<CellReference>();
-        for (CellReference r : individualSelectedCells) {
-            cells.add(r);
-        }
+        Set<CellReference> cells = new HashSet<>(individualSelectedCells);
         cells.add(selectedCellReference);
 
         if (cellRangeAddresses != null) {
@@ -5594,10 +5589,9 @@ public CellReference getSelectedCellReference() {
     public Set<CellReference> getSelectedCellReferences() {
         SelectionChangeEvent event = selectionManager.getLatestSelectionEvent();
         if (event == null) {
-            return new HashSet<CellReference>();
-        } else {
-            return event.getAllSelectedCells();
+            return new HashSet<>();
         }
+        return event.getAllSelectedCells().getCells();
     }
 
     /**

vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow/src/main/java/com/vaadin/flow/component/spreadsheet/charts/converter/xssfreader/AbstractSeriesReader.java

@@ -293,7 +293,7 @@ void onValueChange(final List<CellReference> referencedCells,
             return;
         }
 
-        for (CellReference changedCell : event.getChangedCells()) {
+        for (CellReference changedCell : event.getChangedCells().getCells()) {
             // getChangedCell erroneously provides relative cell refs
             // if this gets fixed, this conversion method should be
             // removed

vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow/src/test/java/com/vaadin/flow/component/spreadsheet/tests/CellValueChangeEventOnFormulaChangeTest.java

@@ -11,9 +11,9 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 
-import java.util.LinkedList;
-import java.util.List;
+import java.util.concurrent.atomic.AtomicReference;
 
+import com.vaadin.flow.component.spreadsheet.CellSet;
 import org.apache.poi.ss.usermodel.Row;
 import org.apache.poi.ss.usermodel.Sheet;
 import org.apache.poi.ss.usermodel.Workbook;
@@ -51,20 +51,22 @@ public void setup() {
      */
     @Test
     public void formulaChangeResultingInSameValue() {
-        List<CellReference> changedCells = new LinkedList<>();
+        AtomicReference<CellSet> changedCells = new AtomicReference<>();
 
         spreadsheet.addCellValueChangeListener(
-                event -> changedCells.addAll(event.getChangedCells()));
+                event -> changedCells.set(event.getChangedCells()));
 
         spreadsheet.setSelection("C1");
         // B1 is 0, so the result doesn't change
         spreadsheet.getCellValueManager().onCellValueChange(3, 1, "=A1+2*B1");
 
-        assertEquals("There should be 2 changed cells", 2, changedCells.size());
+        assertEquals("There should be one changed cell", 1,
+                changedCells.get().getCellCount());
         assertTrue("The changed cells should include C1 with sheet name",
-                changedCells.contains(new CellReference("Sheet0!C1")));
+                changedCells.get()
+                        .containsCell(new CellReference("Sheet0!C1")));
         assertTrue("The changed cells should include C1 without sheet name",
-                changedCells.contains(new CellReference("C1")));
+                changedCells.get().containsCell(new CellReference("C1")));
     }
 
 }

vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow/src/test/java/com/vaadin/flow/component/spreadsheet/tests/FormulasTest.java

@@ -83,9 +83,9 @@ public void formulaValueChangeListener_invokedOnFormulaValueChange() {
         spreadsheet.refreshCells(A1, A2);
 
         // Check that the event was fired with the correct values
-        Assert.assertEquals(event.get().getChangedCells().size(), 1);
-        Assert.assertEquals(event.get().getChangedCells().iterator().next()
-                .formatAsString(), "Sheet1!A1");
+        Assert.assertEquals(event.get().getChangedCells().getCellCount(), 1);
+        Assert.assertEquals(event.get().getChangedCells().getCells().iterator()
+                .next().formatAsString(), "Sheet1!A1");
         // Sanity check for the forumula cell effective value
         Assert.assertEquals(2.0, A1.getNumericCellValue(), 0.0);
     }