https://issues.apache.org/bugzilla/show_bug.cgi?id=53500
Yegor Kozlov <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #1 from Yegor Kozlov <[email protected]> --- 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 -- You are receiving this mail because: You are the assignee for the bug. --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
