Created attachment 29023 [details] zip containing patch, test case, and test data [Patch] Getter for repeating rows and columns Hello, class Workbook provides a method setRepeatingRowsAndColumns(int,int,int,int,int), but no corresponding getter. The provided patch adds the following methods to class Sheet: - CellRangeAddress getRepeatingRows() - CellRangeAddress getRepeatingColumns() A few notes & thoughts: 1) the method setRepeatingRowsAndColumns(...) is defined in Workbook, although repeating rows/columns are actually Sheet properties. The rationale for the assignment is given in the Javadoc: "... This is function is included in the workbook because it creates/modifies name records which are stored at the workbook level..." For this are purely technical reasons, I would rather declare this method @deprecated and move it to class Sheet, in order to improve class coherence. 2) It would be preferrable to split setRepeatingRowsAndColumns(...) into two methods setRepeatingRows(String rowRangeRef) and setRepeatingColumns(String columnRangeRef), as this maps more directly to the user interface fields, the getters, and it avoids slightly puzzling -1 parameters when the user only wants to define either repeating rows or columnns. E.g.: setRepeatingRows("2:3") or setRepeatingColumns("A:A") A null parameter would indicate that repeating rows/columns should be removed. 3) Regarding the returned class, CellRangeAddress: it appears that both AreaReference and CellRangeAddress have some limitations when it comes to handling Excel Version 97 versus 2007: both classes are a bit shaky when they are to decide if a cell range spans full rows/columns, as the different Excel versions support different maximum rows and columns. CellReference, which is used by both CellRangeAddress and AreaReference, has an inconspicuous comment which says : "...// TODO - "-1" is a special value being temporarily used for whole row and whole column area references. .." This is a quite informal specification for what I think is an important convention, as it offers a way to declare a whole row or column range in a spreadsheet version-independent way. Thus, I decided to stress this feature when using CellRangeAddress. However, the -1 convention seems to be implemented only in a sporadic selection of methods. E.g. CellRangeAddress.getNumberOfCells() returns wondrous results when operating with -1. 3.1) A side note on CellRangeAddress and AreaReference: it seems that the reference classes do not exploit the full power that a rich class hierarchy could offer: E.g. in various contexts, a CellRangeAdress (or AreaReference) parameter is not exactly what is permitted, but only an apporximation. An elaboorate reference class hierarchy could declare valid values more precisely. E.g.: CellSetRef <---+---- CellRangeRef <---------+----- CellRef | \ \ | \ \ +----- RowSetRef <----]\[-- RowRangeRef <--- RowRef | \ | \ +---ColumnSetRef <--- ColumnRangeRef <----- ColumnRef Regards, Joachim
Thanks for the excellent patch, applied in r1364061. > > 1) the method setRepeatingRowsAndColumns(...) is defined in Workbook, > although repeating rows/columns are actually Sheet properties. The rationale > for the assignment is given in the Javadoc: > "... This is function is included in the workbook because it > creates/modifies name records which are stored at the workbook level..." > For this are purely technical reasons, I would rather declare this method > @deprecated and move it to class Sheet, in order to improve class coherence. > I'm OK to deprecate Workbook.setRepeatingRowsAndColumns. If we make this change we will need to change poi-examples to use the new methods. > 2) It would be preferrable to split setRepeatingRowsAndColumns(...) into two > methods setRepeatingRows(String rowRangeRef) and setRepeatingColumns(String > columnRangeRef), as this maps more directly to the user interface fields, > the getters, and it avoids slightly puzzling -1 parameters when the user > only wants to define either repeating rows or columnns. E.g.: > setRepeatingRows("2:3") or setRepeatingColumns("A:A") > A null parameter would indicate that repeating rows/columns should be > removed. > Sounds good. I'd prefer setRepeatingRows(CellRangeAddress rowRangeRef) , in this case the type of the argument is the same in both getter and setter. A null paramater is certainly much more user-friendly than passing -1 . > 3) Regarding the returned class, CellRangeAddress: it appears that both > AreaReference and CellRangeAddress have some limitations when it comes to > handling Excel Version 97 versus 2007: both classes are a bit shaky when > they are to decide if a cell range spans full rows/columns, as the different > Excel versions support different maximum rows and columns. > CellReference, which is used by both CellRangeAddress and AreaReference, has > an inconspicuous comment which says : > "...// TODO - "-1" is a special value being temporarily used for whole row > and whole column area references. .." > This is a quite informal specification for what I think is an important > convention, as it offers a way to declare a whole row or column range in a > spreadsheet version-independent way. Thus, I decided to stress this feature > when using CellRangeAddress. However, the -1 convention seems to be > implemented only in a sporadic selection of methods. E.g. > CellRangeAddress.getNumberOfCells() returns wondrous results when operating > with -1. > I see that this fix is in the patch. Thanks. > 3.1) A side note on CellRangeAddress and AreaReference: it seems that the > reference classes do not exploit the full power that a rich class hierarchy > could offer: > E.g. in various contexts, a CellRangeAdress (or AreaReference) parameter is > not exactly what is permitted, but only an apporximation. > An elaboorate reference class hierarchy could declare valid values more > precisely. E.g.: > > CellSetRef <---+---- CellRangeRef <---------+----- CellRef > | \ \ > | \ \ > +----- RowSetRef <----]\[-- RowRangeRef <--- RowRef > | \ > | \ > +---ColumnSetRef <--- ColumnRangeRef <----- ColumnRef > Is CellSetRef a subclass of CellRangeAddress ? I'd rather stay with current design and tighten it up to throw IllegalArgumentException if a column range is passed instead of a row range, etc. You are welcome to uplaod a patch with (1) and (2). Please feel free to re-open this ticket or create a new one . Regards, Yegor
Created attachment 29151 [details] Patch file
Hi Yegor, I finally had time to deal with the setRepatingRowsAndColumns() method. It is now relocated to class Sheet, and split into two methods that expect a CellRangeAddress parameter, according to your suggestion. Unfortunately, a bunch of additional get, add, and remove methods for the HSSFName/XSSFName handling was needed, but as they are all package visible, the user interface should not be to much cluttered by this. Regards, Joachim
Thanks for the follow-up, patch applied in r1369290 Regards, Yegor
*** Bug 46800 has been marked as a duplicate of this bug. ***