Copilot commented on code in PR #745: URL: https://github.com/apache/fesod/pull/745#discussion_r2644643702
########## fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/ods/OdsSheet.java: ########## @@ -0,0 +1,694 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.sheet.metadata.ods; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.Setter; +import org.apache.fesod.sheet.exception.ExcelGenerateException; +import org.apache.poi.ss.usermodel.AutoFilter; +import org.apache.poi.ss.usermodel.Cell; +import org.apache.poi.ss.usermodel.CellRange; +import org.apache.poi.ss.usermodel.CellStyle; +import org.apache.poi.ss.usermodel.Comment; +import org.apache.poi.ss.usermodel.DataValidation; +import org.apache.poi.ss.usermodel.DataValidationHelper; +import org.apache.poi.ss.usermodel.Drawing; +import org.apache.poi.ss.usermodel.Footer; +import org.apache.poi.ss.usermodel.Header; +import org.apache.poi.ss.usermodel.Hyperlink; +import org.apache.poi.ss.usermodel.PageMargin; +import org.apache.poi.ss.usermodel.PaneType; +import org.apache.poi.ss.usermodel.PrintSetup; +import org.apache.poi.ss.usermodel.Row; +import org.apache.poi.ss.usermodel.Sheet; +import org.apache.poi.ss.usermodel.SheetConditionalFormatting; +import org.apache.poi.ss.usermodel.Workbook; +import org.apache.poi.ss.util.CellAddress; +import org.apache.poi.ss.util.CellRangeAddress; +import org.apache.poi.ss.util.PaneInformation; +import org.odftoolkit.odfdom.doc.table.OdfTable; +import org.odftoolkit.odfdom.doc.table.OdfTableCell; +import org.odftoolkit.odfdom.doc.table.OdfTableRow; + +/** + * ODS sheet implementation for writing ODS files. + * + */ +@Getter +@Setter +@EqualsAndHashCode +public class OdsSheet implements Sheet { + + /** + * workbook + */ + private OdsWorkbook odsWorkbook; + + /** + * sheet name + */ + private String sheetName; Review Comment: This method overrides [Sheet.getSheetName](1); it is advisable to add an Override annotation. ########## fesod-sheet/src/main/java/org/apache/fesod/sheet/read/metadata/holder/ods/OdsReadWorkbookHolder.java: ########## @@ -0,0 +1,48 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.sheet.read.metadata.holder.ods; + +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.Setter; +import org.apache.fesod.sheet.read.metadata.ReadWorkbook; +import org.apache.fesod.sheet.read.metadata.holder.ReadWorkbookHolder; +import org.apache.fesod.sheet.support.ExcelTypeEnum; +import org.odftoolkit.odfdom.doc.OdfSpreadsheetDocument; + +/** + * ODS Workbook holder + * + */ +@Getter +@Setter +@EqualsAndHashCode Review Comment: This method overrides [ReadWorkbookHolder.canEqual](1); it is advisable to add an Override annotation. ########## fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/ods/OdsSheet.java: ########## @@ -0,0 +1,694 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.sheet.metadata.ods; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.Setter; +import org.apache.fesod.sheet.exception.ExcelGenerateException; +import org.apache.poi.ss.usermodel.AutoFilter; +import org.apache.poi.ss.usermodel.Cell; +import org.apache.poi.ss.usermodel.CellRange; +import org.apache.poi.ss.usermodel.CellStyle; +import org.apache.poi.ss.usermodel.Comment; +import org.apache.poi.ss.usermodel.DataValidation; +import org.apache.poi.ss.usermodel.DataValidationHelper; +import org.apache.poi.ss.usermodel.Drawing; +import org.apache.poi.ss.usermodel.Footer; +import org.apache.poi.ss.usermodel.Header; +import org.apache.poi.ss.usermodel.Hyperlink; +import org.apache.poi.ss.usermodel.PageMargin; +import org.apache.poi.ss.usermodel.PaneType; +import org.apache.poi.ss.usermodel.PrintSetup; +import org.apache.poi.ss.usermodel.Row; +import org.apache.poi.ss.usermodel.Sheet; +import org.apache.poi.ss.usermodel.SheetConditionalFormatting; +import org.apache.poi.ss.usermodel.Workbook; +import org.apache.poi.ss.util.CellAddress; +import org.apache.poi.ss.util.CellRangeAddress; +import org.apache.poi.ss.util.PaneInformation; +import org.odftoolkit.odfdom.doc.table.OdfTable; +import org.odftoolkit.odfdom.doc.table.OdfTableCell; +import org.odftoolkit.odfdom.doc.table.OdfTableRow; + +/** + * ODS sheet implementation for writing ODS files. + * + */ +@Getter +@Setter +@EqualsAndHashCode +public class OdsSheet implements Sheet { + + /** + * workbook + */ + private OdsWorkbook odsWorkbook; + + /** + * sheet name + */ + private String sheetName; + + /** + * row list + */ + private List<OdsRow> rowList; + + /** + * last row index + */ + private Integer lastRowIndex; + + /** + * ODF Table (created when flushing) + */ + private OdfTable odfTable; + + public OdsSheet(OdsWorkbook odsWorkbook, String sheetName) { + this.odsWorkbook = odsWorkbook; + this.sheetName = sheetName; + this.rowList = new ArrayList<>(); + this.lastRowIndex = -1; + } + + /** + * Flush all data to the ODF document. + */ + public void flushToDocument() { + try { + odfTable = OdfTable.newTable(odsWorkbook.getOdfDocument(), rowList.size(), getMaxColumnCount()); + odfTable.setTableName(sheetName); + + for (OdsRow odsRow : rowList) { + int rowIndex = odsRow.getRowIndex(); + OdfTableRow odfRow = odfTable.getRowByIndex(rowIndex); + + Iterator<Cell> cellIterator = odsRow.cellIterator(); + while (cellIterator.hasNext()) { + OdsCell odsCell = (OdsCell) cellIterator.next(); + int colIndex = odsCell.getColumnIndex(); + OdfTableCell odfCell = odfRow.getCellByIndex(colIndex); + + writeCellValue(odfCell, odsCell); + } + } + } catch (Exception e) { + throw new ExcelGenerateException("Failed to flush ODS sheet data", e); + } + } + + /** + * Get the maximum column count across all rows. + */ + private int getMaxColumnCount() { + int max = 1; + for (OdsRow row : rowList) { + if (row.getLastCellNum() > max) { + max = row.getLastCellNum(); + } + } + return max; + } + + /** + * Write cell value to ODF cell. + */ + private void writeCellValue(OdfTableCell odfCell, OdsCell odsCell) { + if (odsCell == null || odfCell == null) { + return; + } + + switch (odsCell.getCellType()) { + case STRING: + String stringValue = odsCell.getStringCellValue(); + if (stringValue != null) { + odfCell.setStringValue(stringValue); + } + break; + case NUMERIC: + if (odsCell.getDateValue() != null) { + // Handle date + odfCell.setDateValue(java.util.Calendar.getInstance()); + java.util.Calendar cal = java.util.Calendar.getInstance(); + cal.set( + odsCell.getDateValue().getYear(), + odsCell.getDateValue().getMonthValue() - 1, + odsCell.getDateValue().getDayOfMonth(), + odsCell.getDateValue().getHour(), + odsCell.getDateValue().getMinute(), + odsCell.getDateValue().getSecond()); Review Comment: The date handling logic has a potential issue with Calendar.set(). The getYear() method returns the full year (e.g., 2024), but when setting dates using Calendar fields without using Calendar.YEAR, Calendar.MONTH etc. constants, there could be confusion. Additionally, the milliseconds component of the LocalDateTime is not being set, which could result in precision loss. Consider using Calendar.set(year, month, dayOfMonth, hourOfDay, minute, second) or including milliseconds if time precision is important. ########## fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/ods/OdsCell.java: ########## @@ -0,0 +1,359 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.sheet.metadata.ods; + +import java.math.BigDecimal; +import java.time.LocalDateTime; +import java.time.ZoneId; +import java.util.Calendar; +import java.util.Date; +import lombok.AccessLevel; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.Setter; +import org.apache.fesod.sheet.enums.NumericCellTypeEnum; +import org.apache.fesod.sheet.metadata.data.FormulaData; +import org.apache.poi.ss.SpreadsheetVersion; +import org.apache.poi.ss.usermodel.CellBase; +import org.apache.poi.ss.usermodel.CellStyle; +import org.apache.poi.ss.usermodel.CellType; +import org.apache.poi.ss.usermodel.Comment; +import org.apache.poi.ss.usermodel.Hyperlink; +import org.apache.poi.ss.usermodel.RichTextString; +import org.apache.poi.ss.usermodel.Row; +import org.apache.poi.ss.usermodel.Sheet; +import org.apache.poi.ss.util.CellRangeAddress; + +/** + * ODS cell implementation for writing ODS files. + * + */ +@Getter +@Setter +@EqualsAndHashCode +public class OdsCell extends CellBase { + + /** + * column index + */ + @Getter(value = AccessLevel.NONE) + @Setter(value = AccessLevel.NONE) + private Integer columnIndex; + + /** + * cell type + */ + @Getter(value = AccessLevel.NONE) + @Setter(value = AccessLevel.NONE) + private CellType cellType; + + /** + * numeric cell type + */ + private NumericCellTypeEnum numericCellType; + + /** + * workbook + */ + private final OdsWorkbook odsWorkbook; + + /** + * sheet + */ + private final OdsSheet odsSheet; + + /** + * row + */ + private final OdsRow odsRow; + + /** + * {@link CellType#NUMERIC} + */ + private BigDecimal numberValue; + + /** + * {@link CellType#STRING} and {@link CellType#ERROR} {@link CellType#FORMULA} + */ + private String stringValue; + + /** + * {@link CellType#BOOLEAN} + */ + private Boolean booleanValue; + + /** + * {@link CellType#NUMERIC} + */ + private LocalDateTime dateValue; + + /** + * formula + */ + private FormulaData formulaData; + + /** + * rich text string + */ + private RichTextString richTextString; + + /** + * style + */ + private CellStyle cellStyle; + + public OdsCell(OdsWorkbook odsWorkbook, OdsSheet odsSheet, OdsRow odsRow, Integer columnIndex, CellType cellType) { + this.odsWorkbook = odsWorkbook; + this.odsSheet = odsSheet; + this.odsRow = odsRow; + this.columnIndex = columnIndex; + this.cellType = cellType; + if (this.cellType == null) { + this.cellType = CellType._NONE; + } + } + + @Override + protected void setCellTypeImpl(CellType cellType) { + this.cellType = cellType; + } + + @Override + protected void setCellFormulaImpl(String formula) { + FormulaData formulaData = new FormulaData(); + formulaData.setFormulaValue(formula); + this.formulaData = formulaData; + this.cellType = CellType.FORMULA; + } + + @Override + protected void removeFormulaImpl() { + this.formulaData = null; + } + + @Override + protected void setCellValueImpl(double value) { + numberValue = BigDecimal.valueOf(value); + this.cellType = CellType.NUMERIC; + } + + @Override + protected void setCellValueImpl(Date value) { + if (value == null) { + return; + } + this.dateValue = LocalDateTime.ofInstant(value.toInstant(), ZoneId.systemDefault()); + this.cellType = CellType.NUMERIC; + this.numericCellType = NumericCellTypeEnum.DATE; + } + + @Override + protected void setCellValueImpl(LocalDateTime value) { + this.dateValue = value; + this.cellType = CellType.NUMERIC; + this.numericCellType = NumericCellTypeEnum.DATE; + } + + @Override + protected void setCellValueImpl(Calendar value) { + if (value == null) { + return; + } + this.dateValue = LocalDateTime.ofInstant(value.toInstant(), ZoneId.systemDefault()); + this.cellType = CellType.NUMERIC; Review Comment: When setting a Calendar value, the numericCellType is not set to NumericCellTypeEnum.DATE, unlike the setCellValueImpl(Date) and setCellValueImpl(LocalDateTime) methods. This inconsistency could lead to the date not being properly formatted or handled as a date type in the ODS output. Add 'this.numericCellType = NumericCellTypeEnum.DATE;' after line 180 to maintain consistency with other date-setting methods. ```suggestion this.cellType = CellType.NUMERIC; this.numericCellType = NumericCellTypeEnum.DATE; ``` ########## fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/ods/OdsSheet.java: ########## @@ -0,0 +1,694 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.sheet.metadata.ods; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.Setter; +import org.apache.fesod.sheet.exception.ExcelGenerateException; +import org.apache.poi.ss.usermodel.AutoFilter; +import org.apache.poi.ss.usermodel.Cell; +import org.apache.poi.ss.usermodel.CellRange; +import org.apache.poi.ss.usermodel.CellStyle; +import org.apache.poi.ss.usermodel.Comment; +import org.apache.poi.ss.usermodel.DataValidation; +import org.apache.poi.ss.usermodel.DataValidationHelper; +import org.apache.poi.ss.usermodel.Drawing; +import org.apache.poi.ss.usermodel.Footer; +import org.apache.poi.ss.usermodel.Header; +import org.apache.poi.ss.usermodel.Hyperlink; +import org.apache.poi.ss.usermodel.PageMargin; +import org.apache.poi.ss.usermodel.PaneType; +import org.apache.poi.ss.usermodel.PrintSetup; +import org.apache.poi.ss.usermodel.Row; +import org.apache.poi.ss.usermodel.Sheet; +import org.apache.poi.ss.usermodel.SheetConditionalFormatting; +import org.apache.poi.ss.usermodel.Workbook; +import org.apache.poi.ss.util.CellAddress; +import org.apache.poi.ss.util.CellRangeAddress; +import org.apache.poi.ss.util.PaneInformation; +import org.odftoolkit.odfdom.doc.table.OdfTable; +import org.odftoolkit.odfdom.doc.table.OdfTableCell; +import org.odftoolkit.odfdom.doc.table.OdfTableRow; + +/** + * ODS sheet implementation for writing ODS files. + * + */ +@Getter +@Setter +@EqualsAndHashCode +public class OdsSheet implements Sheet { + + /** + * workbook + */ + private OdsWorkbook odsWorkbook; + + /** + * sheet name + */ + private String sheetName; + + /** + * row list + */ + private List<OdsRow> rowList; + + /** + * last row index + */ + private Integer lastRowIndex; + + /** + * ODF Table (created when flushing) + */ + private OdfTable odfTable; + + public OdsSheet(OdsWorkbook odsWorkbook, String sheetName) { + this.odsWorkbook = odsWorkbook; + this.sheetName = sheetName; + this.rowList = new ArrayList<>(); + this.lastRowIndex = -1; + } + + /** + * Flush all data to the ODF document. + */ + public void flushToDocument() { + try { + odfTable = OdfTable.newTable(odsWorkbook.getOdfDocument(), rowList.size(), getMaxColumnCount()); + odfTable.setTableName(sheetName); + + for (OdsRow odsRow : rowList) { + int rowIndex = odsRow.getRowIndex(); + OdfTableRow odfRow = odfTable.getRowByIndex(rowIndex); + + Iterator<Cell> cellIterator = odsRow.cellIterator(); + while (cellIterator.hasNext()) { + OdsCell odsCell = (OdsCell) cellIterator.next(); + int colIndex = odsCell.getColumnIndex(); + OdfTableCell odfCell = odfRow.getCellByIndex(colIndex); + + writeCellValue(odfCell, odsCell); + } + } + } catch (Exception e) { + throw new ExcelGenerateException("Failed to flush ODS sheet data", e); + } + } + + /** + * Get the maximum column count across all rows. + */ + private int getMaxColumnCount() { + int max = 1; + for (OdsRow row : rowList) { + if (row.getLastCellNum() > max) { + max = row.getLastCellNum(); + } + } + return max; + } + + /** + * Write cell value to ODF cell. + */ + private void writeCellValue(OdfTableCell odfCell, OdsCell odsCell) { + if (odsCell == null || odfCell == null) { + return; + } + + switch (odsCell.getCellType()) { + case STRING: + String stringValue = odsCell.getStringCellValue(); + if (stringValue != null) { + odfCell.setStringValue(stringValue); + } + break; + case NUMERIC: + if (odsCell.getDateValue() != null) { + // Handle date + odfCell.setDateValue(java.util.Calendar.getInstance()); Review Comment: In the writeCellValue method, when handling NUMERIC cell types with date values, there is redundant code. Line 154 sets the calendar value with Calendar.getInstance(), which is immediately overwritten on line 163. The first setDateValue call on line 154 should be removed as it serves no purpose and creates a temporary Calendar instance that is never used. ```suggestion ``` ########## fesod-sheet/src/main/java/org/apache/fesod/sheet/support/ExcelTypeEnum.java: ########## @@ -50,7 +50,12 @@ public enum ExcelTypeEnum { /** * xlsx */ - XLSX(".xlsx", new byte[] {80, 75, 3, 4}); + XLSX(".xlsx", new byte[] {80, 75, 3, 4}), + + /** + * ods (OpenDocument Spreadsheet) + */ + ODS(".ods", new byte[] {80, 75, 3, 4}); Review Comment: Both XLSX and ODS have the same magic bytes (80, 75, 3, 4 - ZIP file signature). This means ODS and XLSX cannot be distinguished by magic bytes alone in the recognitionExcelType method. When reading ODS files from an InputStream without explicit type specification, they will be incorrectly identified as XLSX files, causing parsing failures. Consider adding additional content inspection within the ZIP structure to differentiate between XLSX and ODS formats, or document that ODS files must be explicitly typed when reading from streams. ########## fesod-sheet/src/main/java/org/apache/fesod/sheet/read/metadata/holder/ods/OdsReadSheetHolder.java: ########## @@ -0,0 +1,41 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.sheet.read.metadata.holder.ods; + +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.Setter; +import org.apache.fesod.sheet.read.metadata.ReadSheet; +import org.apache.fesod.sheet.read.metadata.holder.ReadSheetHolder; +import org.apache.fesod.sheet.read.metadata.holder.ReadWorkbookHolder; + +/** + * ODS sheet holder + * + */ +@Getter +@Setter +@EqualsAndHashCode Review Comment: This method overrides [ReadSheetHolder.canEqual](1); it is advisable to add an Override annotation. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
