Bug 57450: Patch submitted, request for feedback

[Stefan Thurnherr <st...@gmail.com>]

I've submitted a patch and added some discussion points to this enhancement
request over a week ago:

"A way to autosize columns on SXSSF"
https://bz.apache.org/bugzilla/show_bug.cgi?id=57450#c7

Could someone please have a look and give feedback? Thanks!
 stefan.

Re: Bug 57450: Patch submitted, request for feedback

[Javen O'Neal <ja...@gmail.com>]

Good point. I forgot about autosizing particular cells. This might be
worthy of its own bug so as not to restrict/complicate the implementation
of SXSSFSheet.

Consider two possible scenarios: 1) user wants to autosize two columns
using just the header text in one or two rows, 2) user wants to autosize
data in the rows under headers. If the user adds new rows to the
spreadsheet after tracking existing data rows, are the newly added rows
also tracked? Basically, tracked-by-default and not-tracked-by-default
behavior. Do you want to support both workflows in POI, or require the user
to resolve this?
On 9 Nov 2015 3:37 p.m., "ST" <st...@gmail.com> wrote:

> By having the possibility of tracking specific rows only, one gains the
> capability to exclude some rows from autosizing calculations. This is
> useful independently of type of Sheet. Example usecase is a row of column
> headings. This was also proposed by the issue reporter, see his latest
> comment [1]. Although not strictly related to the SXSSF autosizing bug, it
> would be a nice side-effect to gain.
>
> [1] https://bz.apache.org/bugzilla/show_bug.cgi?id=57450#c4
> Den 9 nov 2015 17:30 skrev "Javen O'Neal" <ja...@gmail.com>:
>
> > Tracking is simply unnecessary for XSSFSheet and HSSFSheet because the
> rows
> > never fall outside the memory window. Keep in mind that tracking rows
> uses
> > a little more memory and performs some expensive sizing logic that may
> not
> > get used. I'd keep the autosizing tracker at the SXSSF level since it
> only
> > applies for windowed memory sheets.
> >
> > I expect users needing to write themselves into the SXSSFSheet API when
> it
> > differs from the Sheet interface, and this is certainly one of those
> cases.
> > On 9 Nov 2015 05:14, "ST" <st...@gmail.com> wrote:
> >
> > > Javen,
> > >
> > > Regarding your critical feedback given in your first email of this
> email
> > > thread, I've updated my patch. But let's first have the other
> discussions
> > > triggered by your second email of this email thread.
> > >
> > > I actually like the tightly-coupled extreme of your vision: completely
> > > hiding the tracker logic behind the Sheet API. Internally we could
> still
> > > use a private class (same for all Sheet impls) to do the autosizing. I
> > can
> > > see the following benefits with this hide-behind-Sheet-API approach:
> > > * "Solves" the problem of backward compatibility: same window-dependant
> > > behaviour as before for SXSSFSheets, which can be fixed by using newly
> > > added API (see below)
> > > * Keeps same API for the different Sheet implementations
> > > * Will still allow to exclude some rows from autosize calculation
> > >
> > > I propose to add the following tracker API methods to the Sheet
> > interface:
> > > * trackRowForAutoSizing(Row)
> > > * autoSizeAllColumnsByTrackedRows()
> > > * autoSizeAllColumnsByTrackedRows(bool) // not sure if needed
> > > These will complement the two existing autosize methods:
> > > * autoSizeColumn(int) // keep this existing method
> > > * autoSizeColumn(int,bool) // keep this existing method
> > >
> > > What do you think about this?
> > >
> > > The quicker but uglier solution would be to add the new tracker API
> only
> > to
> > > SXSSFSheet. But this in my opinion defeats the whole idea of the Sheet
> > > interface since POI users will have to do the autosizing differently
> > based
> > > on the actual Sheet implementation.
> > >
> > > I'll give it a try when I find the time.
> > >   stefan.
> > >
> > >
> > >
> > >
> > > On Wed, Nov 4, 2015 at 6:37 AM, Javen O'Neal <ja...@gmail.com>
> > wrote:
> > >
> > > > Here's what I had in mind about keeping support for
> > > > SXSSFSheet.autoSizeColumn(int) and autoSizeColumn(int, bool):
> > > >
> > > > int rowAccessWindowSize = 1;
> > > > SXSSFWorkbook wb = new SXSSFWorkbook(rowAccessWindowSize);
> > > > SXSSFSheet sh = wb.createSheet();
> > > > AutosizeColumnTracker tracker = sh.getAutosizeColumnTracker(); //this
> > > > probably needs to be a singleton. Can be lazily-created.
> > > > // tell workbook to keep track of column widths as they *might* be
> > > > auto-sized at the end of the sheet
> > > >
> > > > boolean useMergedCells = false;
> > > > tracker.monitorColumn(0, useMergedCells);
> > > > tracker.monitorColumn(2, useMergedCells);
> > > > tracker.monitorColumn(4); // equivalent to monitorColumn(4, true);
> > > >
> > > > // create a bunch of rows and cells, write some values to the cells,
> > > merge
> > > > some cells across columns and rows so auto-size has something
> > meaningful
> > > to
> > > > do
> > > > Row row;
> > > > Cell cell;
> > > > for (int r=0; r<=10; r++) {
> > > >     row = sh.createRow(r);
> > > >     for (int c=0; c<=10; c++) {
> > > >         cell = row.createCell(0);
> > > >         cell.setCellValue("row=" + r + ", column=" + c + ",
> > > > somethingToMakeColumnsHaveDifferentWidth=" + ((int) Math.pow(r, c)));
> > > >     }
> > > > }
> > > > // ... etc
> > > >
> > > > sh.autoSizeColumn(0); // okay
> > > > sh.autoSizeColumn(0, false); // okay
> > > > sh.autoSizeColumn(0, true); // throws exception: column widths were
> > > > calculated using merged cell widths, but this pre-requisite is no
> > longer
> > > > valid.
> > > > sh.autoSizeColumn(1); // throws exception: column was not monitored
> for
> > > > auto-sizing
> > > > sh.autoSizeColumn(2); // okay
> > > >
> > > > // and if we want to allow a user to auto-size a column that wasn't
> > > > previously monitored, we might need to add an extra method that will
> > > > indicate that the user understands that auto-sizing is computed
> solely
> > > from
> > > > the row access window, and not to throw an
> UnmonitoredColumnForAutosize
> > > > (insert your favorite name here) exception.
> > > > autoSizeRowAccessWindowOnly = true;
> > > > sh.autoSizeColumn(0, false, autoSizeRowAccessWindowOnly); // okay:
> > makes
> > > > existing functionality available
> > > > // alternatively, if you want to allow the user to determine if a
> > column
> > > > gets autosized using merged cells or not at the time autoSizeColumn
> is
> > > > called, the AutosizeColumnTracker would need to calculate and
> remember
> > > the
> > > > widths for both cases (useMergedCells=true and useMergedCell=false).
> > > >
> > > >
> > > > // finish up
> > > > // ... etc
> > > > wb.write(new FileOutputStream('example.xlsx'));
> > > >
> > > >
> > > > I think the relationship between the tracker object and the
> SXSSFSheet
> > > > warrants more discussion, as there are several ways to do this.
> > > > Considerations:
> > > > * extensibility: how can the AutosizeColumnTracker class be
> subclassed
> > by
> > > > users of POI? how can the SXSSFSheet be subclassed by users of POI?
> > > Should
> > > > AutosizeColumnTracker be final or @Internal to avoid this question?
> > > > * reusability: is there a need to create one autosize column tracker
> > > object
> > > > that can be applied to multiple sheets (whether this is just the
> index
> > of
> > > > the monitored columns or also the column widths)?
> > > > * if sh.autoSizeColumn uses the tracker, either the tracker needs to
> be
> > > > passed in as a third argument or the sheet needs to know what tracker
> > it
> > > is
> > > > associated with. Every time a column goes outside the window, the
> > > > SXSSFSheet needs to know which tracker object its associated with so
> it
> > > can
> > > > update the column widths in the tracker (or maintain its own column
> > > > widths). In your code, you require the user to explicitly call
> > trackRow,
> > > > but XSSFSheet and HSSFSheet don't have a similar construct:
> auto-sizing
> > > > applies to ALL rows and this is not user configurable.
> > > > * My vision of the AutosizeColumnTracker may be too tightly coupled
> > with
> > > > the SXSSFSheet. At the tightly-coupled extreme, AutosizeColumnTracker
> > may
> > > > be a private class (or just a private TreeMap member variable) inside
> > of
> > > > SXSSFSheet that maintains a TreeMap of tracked columns and column
> > widths;
> > > > all intents are registered directly on the SXSSFSheet:
> > > > sxssfSheet.monitorColumnForAutosizing(0);
> > > >
> > > > Thoughts?
> > > > Javen
> > > >
> > >
> >
>

Re: Bug 57450: Patch submitted, request for feedback

[ST <st...@gmail.com>]

By having the possibility of tracking specific rows only, one gains the
capability to exclude some rows from autosizing calculations. This is
useful independently of type of Sheet. Example usecase is a row of column
headings. This was also proposed by the issue reporter, see his latest
comment [1]. Although not strictly related to the SXSSF autosizing bug, it
would be a nice side-effect to gain.

[1] https://bz.apache.org/bugzilla/show_bug.cgi?id=57450#c4
Den 9 nov 2015 17:30 skrev "Javen O'Neal" <ja...@gmail.com>:

> Tracking is simply unnecessary for XSSFSheet and HSSFSheet because the rows
> never fall outside the memory window. Keep in mind that tracking rows uses
> a little more memory and performs some expensive sizing logic that may not
> get used. I'd keep the autosizing tracker at the SXSSF level since it only
> applies for windowed memory sheets.
>
> I expect users needing to write themselves into the SXSSFSheet API when it
> differs from the Sheet interface, and this is certainly one of those cases.
> On 9 Nov 2015 05:14, "ST" <st...@gmail.com> wrote:
>
> > Javen,
> >
> > Regarding your critical feedback given in your first email of this email
> > thread, I've updated my patch. But let's first have the other discussions
> > triggered by your second email of this email thread.
> >
> > I actually like the tightly-coupled extreme of your vision: completely
> > hiding the tracker logic behind the Sheet API. Internally we could still
> > use a private class (same for all Sheet impls) to do the autosizing. I
> can
> > see the following benefits with this hide-behind-Sheet-API approach:
> > * "Solves" the problem of backward compatibility: same window-dependant
> > behaviour as before for SXSSFSheets, which can be fixed by using newly
> > added API (see below)
> > * Keeps same API for the different Sheet implementations
> > * Will still allow to exclude some rows from autosize calculation
> >
> > I propose to add the following tracker API methods to the Sheet
> interface:
> > * trackRowForAutoSizing(Row)
> > * autoSizeAllColumnsByTrackedRows()
> > * autoSizeAllColumnsByTrackedRows(bool) // not sure if needed
> > These will complement the two existing autosize methods:
> > * autoSizeColumn(int) // keep this existing method
> > * autoSizeColumn(int,bool) // keep this existing method
> >
> > What do you think about this?
> >
> > The quicker but uglier solution would be to add the new tracker API only
> to
> > SXSSFSheet. But this in my opinion defeats the whole idea of the Sheet
> > interface since POI users will have to do the autosizing differently
> based
> > on the actual Sheet implementation.
> >
> > I'll give it a try when I find the time.
> >   stefan.
> >
> >
> >
> >
> > On Wed, Nov 4, 2015 at 6:37 AM, Javen O'Neal <ja...@gmail.com>
> wrote:
> >
> > > Here's what I had in mind about keeping support for
> > > SXSSFSheet.autoSizeColumn(int) and autoSizeColumn(int, bool):
> > >
> > > int rowAccessWindowSize = 1;
> > > SXSSFWorkbook wb = new SXSSFWorkbook(rowAccessWindowSize);
> > > SXSSFSheet sh = wb.createSheet();
> > > AutosizeColumnTracker tracker = sh.getAutosizeColumnTracker(); //this
> > > probably needs to be a singleton. Can be lazily-created.
> > > // tell workbook to keep track of column widths as they *might* be
> > > auto-sized at the end of the sheet
> > >
> > > boolean useMergedCells = false;
> > > tracker.monitorColumn(0, useMergedCells);
> > > tracker.monitorColumn(2, useMergedCells);
> > > tracker.monitorColumn(4); // equivalent to monitorColumn(4, true);
> > >
> > > // create a bunch of rows and cells, write some values to the cells,
> > merge
> > > some cells across columns and rows so auto-size has something
> meaningful
> > to
> > > do
> > > Row row;
> > > Cell cell;
> > > for (int r=0; r<=10; r++) {
> > >     row = sh.createRow(r);
> > >     for (int c=0; c<=10; c++) {
> > >         cell = row.createCell(0);
> > >         cell.setCellValue("row=" + r + ", column=" + c + ",
> > > somethingToMakeColumnsHaveDifferentWidth=" + ((int) Math.pow(r, c)));
> > >     }
> > > }
> > > // ... etc
> > >
> > > sh.autoSizeColumn(0); // okay
> > > sh.autoSizeColumn(0, false); // okay
> > > sh.autoSizeColumn(0, true); // throws exception: column widths were
> > > calculated using merged cell widths, but this pre-requisite is no
> longer
> > > valid.
> > > sh.autoSizeColumn(1); // throws exception: column was not monitored for
> > > auto-sizing
> > > sh.autoSizeColumn(2); // okay
> > >
> > > // and if we want to allow a user to auto-size a column that wasn't
> > > previously monitored, we might need to add an extra method that will
> > > indicate that the user understands that auto-sizing is computed solely
> > from
> > > the row access window, and not to throw an UnmonitoredColumnForAutosize
> > > (insert your favorite name here) exception.
> > > autoSizeRowAccessWindowOnly = true;
> > > sh.autoSizeColumn(0, false, autoSizeRowAccessWindowOnly); // okay:
> makes
> > > existing functionality available
> > > // alternatively, if you want to allow the user to determine if a
> column
> > > gets autosized using merged cells or not at the time autoSizeColumn is
> > > called, the AutosizeColumnTracker would need to calculate and remember
> > the
> > > widths for both cases (useMergedCells=true and useMergedCell=false).
> > >
> > >
> > > // finish up
> > > // ... etc
> > > wb.write(new FileOutputStream('example.xlsx'));
> > >
> > >
> > > I think the relationship between the tracker object and the SXSSFSheet
> > > warrants more discussion, as there are several ways to do this.
> > > Considerations:
> > > * extensibility: how can the AutosizeColumnTracker class be subclassed
> by
> > > users of POI? how can the SXSSFSheet be subclassed by users of POI?
> > Should
> > > AutosizeColumnTracker be final or @Internal to avoid this question?
> > > * reusability: is there a need to create one autosize column tracker
> > object
> > > that can be applied to multiple sheets (whether this is just the index
> of
> > > the monitored columns or also the column widths)?
> > > * if sh.autoSizeColumn uses the tracker, either the tracker needs to be
> > > passed in as a third argument or the sheet needs to know what tracker
> it
> > is
> > > associated with. Every time a column goes outside the window, the
> > > SXSSFSheet needs to know which tracker object its associated with so it
> > can
> > > update the column widths in the tracker (or maintain its own column
> > > widths). In your code, you require the user to explicitly call
> trackRow,
> > > but XSSFSheet and HSSFSheet don't have a similar construct: auto-sizing
> > > applies to ALL rows and this is not user configurable.
> > > * My vision of the AutosizeColumnTracker may be too tightly coupled
> with
> > > the SXSSFSheet. At the tightly-coupled extreme, AutosizeColumnTracker
> may
> > > be a private class (or just a private TreeMap member variable) inside
> of
> > > SXSSFSheet that maintains a TreeMap of tracked columns and column
> widths;
> > > all intents are registered directly on the SXSSFSheet:
> > > sxssfSheet.monitorColumnForAutosizing(0);
> > >
> > > Thoughts?
> > > Javen
> > >
> >
>

Re: Bug 57450: Patch submitted, request for feedback

[Javen O'Neal <ja...@gmail.com>]

Tracking is simply unnecessary for XSSFSheet and HSSFSheet because the rows
never fall outside the memory window. Keep in mind that tracking rows uses
a little more memory and performs some expensive sizing logic that may not
get used. I'd keep the autosizing tracker at the SXSSF level since it only
applies for windowed memory sheets.

I expect users needing to write themselves into the SXSSFSheet API when it
differs from the Sheet interface, and this is certainly one of those cases.
On 9 Nov 2015 05:14, "ST" <st...@gmail.com> wrote:

> Javen,
>
> Regarding your critical feedback given in your first email of this email
> thread, I've updated my patch. But let's first have the other discussions
> triggered by your second email of this email thread.
>
> I actually like the tightly-coupled extreme of your vision: completely
> hiding the tracker logic behind the Sheet API. Internally we could still
> use a private class (same for all Sheet impls) to do the autosizing. I can
> see the following benefits with this hide-behind-Sheet-API approach:
> * "Solves" the problem of backward compatibility: same window-dependant
> behaviour as before for SXSSFSheets, which can be fixed by using newly
> added API (see below)
> * Keeps same API for the different Sheet implementations
> * Will still allow to exclude some rows from autosize calculation
>
> I propose to add the following tracker API methods to the Sheet interface:
> * trackRowForAutoSizing(Row)
> * autoSizeAllColumnsByTrackedRows()
> * autoSizeAllColumnsByTrackedRows(bool) // not sure if needed
> These will complement the two existing autosize methods:
> * autoSizeColumn(int) // keep this existing method
> * autoSizeColumn(int,bool) // keep this existing method
>
> What do you think about this?
>
> The quicker but uglier solution would be to add the new tracker API only to
> SXSSFSheet. But this in my opinion defeats the whole idea of the Sheet
> interface since POI users will have to do the autosizing differently based
> on the actual Sheet implementation.
>
> I'll give it a try when I find the time.
>   stefan.
>
>
>
>
> On Wed, Nov 4, 2015 at 6:37 AM, Javen O'Neal <ja...@gmail.com> wrote:
>
> > Here's what I had in mind about keeping support for
> > SXSSFSheet.autoSizeColumn(int) and autoSizeColumn(int, bool):
> >
> > int rowAccessWindowSize = 1;
> > SXSSFWorkbook wb = new SXSSFWorkbook(rowAccessWindowSize);
> > SXSSFSheet sh = wb.createSheet();
> > AutosizeColumnTracker tracker = sh.getAutosizeColumnTracker(); //this
> > probably needs to be a singleton. Can be lazily-created.
> > // tell workbook to keep track of column widths as they *might* be
> > auto-sized at the end of the sheet
> >
> > boolean useMergedCells = false;
> > tracker.monitorColumn(0, useMergedCells);
> > tracker.monitorColumn(2, useMergedCells);
> > tracker.monitorColumn(4); // equivalent to monitorColumn(4, true);
> >
> > // create a bunch of rows and cells, write some values to the cells,
> merge
> > some cells across columns and rows so auto-size has something meaningful
> to
> > do
> > Row row;
> > Cell cell;
> > for (int r=0; r<=10; r++) {
> >     row = sh.createRow(r);
> >     for (int c=0; c<=10; c++) {
> >         cell = row.createCell(0);
> >         cell.setCellValue("row=" + r + ", column=" + c + ",
> > somethingToMakeColumnsHaveDifferentWidth=" + ((int) Math.pow(r, c)));
> >     }
> > }
> > // ... etc
> >
> > sh.autoSizeColumn(0); // okay
> > sh.autoSizeColumn(0, false); // okay
> > sh.autoSizeColumn(0, true); // throws exception: column widths were
> > calculated using merged cell widths, but this pre-requisite is no longer
> > valid.
> > sh.autoSizeColumn(1); // throws exception: column was not monitored for
> > auto-sizing
> > sh.autoSizeColumn(2); // okay
> >
> > // and if we want to allow a user to auto-size a column that wasn't
> > previously monitored, we might need to add an extra method that will
> > indicate that the user understands that auto-sizing is computed solely
> from
> > the row access window, and not to throw an UnmonitoredColumnForAutosize
> > (insert your favorite name here) exception.
> > autoSizeRowAccessWindowOnly = true;
> > sh.autoSizeColumn(0, false, autoSizeRowAccessWindowOnly); // okay: makes
> > existing functionality available
> > // alternatively, if you want to allow the user to determine if a column
> > gets autosized using merged cells or not at the time autoSizeColumn is
> > called, the AutosizeColumnTracker would need to calculate and remember
> the
> > widths for both cases (useMergedCells=true and useMergedCell=false).
> >
> >
> > // finish up
> > // ... etc
> > wb.write(new FileOutputStream('example.xlsx'));
> >
> >
> > I think the relationship between the tracker object and the SXSSFSheet
> > warrants more discussion, as there are several ways to do this.
> > Considerations:
> > * extensibility: how can the AutosizeColumnTracker class be subclassed by
> > users of POI? how can the SXSSFSheet be subclassed by users of POI?
> Should
> > AutosizeColumnTracker be final or @Internal to avoid this question?
> > * reusability: is there a need to create one autosize column tracker
> object
> > that can be applied to multiple sheets (whether this is just the index of
> > the monitored columns or also the column widths)?
> > * if sh.autoSizeColumn uses the tracker, either the tracker needs to be
> > passed in as a third argument or the sheet needs to know what tracker it
> is
> > associated with. Every time a column goes outside the window, the
> > SXSSFSheet needs to know which tracker object its associated with so it
> can
> > update the column widths in the tracker (or maintain its own column
> > widths). In your code, you require the user to explicitly call trackRow,
> > but XSSFSheet and HSSFSheet don't have a similar construct: auto-sizing
> > applies to ALL rows and this is not user configurable.
> > * My vision of the AutosizeColumnTracker may be too tightly coupled with
> > the SXSSFSheet. At the tightly-coupled extreme, AutosizeColumnTracker may
> > be a private class (or just a private TreeMap member variable) inside of
> > SXSSFSheet that maintains a TreeMap of tracked columns and column widths;
> > all intents are registered directly on the SXSSFSheet:
> > sxssfSheet.monitorColumnForAutosizing(0);
> >
> > Thoughts?
> > Javen
> >
>

Re: Bug 57450: Patch submitted, request for feedback

[ST <st...@gmail.com>]

Javen,

Regarding your critical feedback given in your first email of this email
thread, I've updated my patch. But let's first have the other discussions
triggered by your second email of this email thread.

I actually like the tightly-coupled extreme of your vision: completely
hiding the tracker logic behind the Sheet API. Internally we could still
use a private class (same for all Sheet impls) to do the autosizing. I can
see the following benefits with this hide-behind-Sheet-API approach:
* "Solves" the problem of backward compatibility: same window-dependant
behaviour as before for SXSSFSheets, which can be fixed by using newly
added API (see below)
* Keeps same API for the different Sheet implementations
* Will still allow to exclude some rows from autosize calculation

I propose to add the following tracker API methods to the Sheet interface:
* trackRowForAutoSizing(Row)
* autoSizeAllColumnsByTrackedRows()
* autoSizeAllColumnsByTrackedRows(bool) // not sure if needed
These will complement the two existing autosize methods:
* autoSizeColumn(int) // keep this existing method
* autoSizeColumn(int,bool) // keep this existing method

What do you think about this?

The quicker but uglier solution would be to add the new tracker API only to
SXSSFSheet. But this in my opinion defeats the whole idea of the Sheet
interface since POI users will have to do the autosizing differently based
on the actual Sheet implementation.

I'll give it a try when I find the time.
  stefan.




On Wed, Nov 4, 2015 at 6:37 AM, Javen O'Neal <ja...@gmail.com> wrote:

> Here's what I had in mind about keeping support for
> SXSSFSheet.autoSizeColumn(int) and autoSizeColumn(int, bool):
>
> int rowAccessWindowSize = 1;
> SXSSFWorkbook wb = new SXSSFWorkbook(rowAccessWindowSize);
> SXSSFSheet sh = wb.createSheet();
> AutosizeColumnTracker tracker = sh.getAutosizeColumnTracker(); //this
> probably needs to be a singleton. Can be lazily-created.
> // tell workbook to keep track of column widths as they *might* be
> auto-sized at the end of the sheet
>
> boolean useMergedCells = false;
> tracker.monitorColumn(0, useMergedCells);
> tracker.monitorColumn(2, useMergedCells);
> tracker.monitorColumn(4); // equivalent to monitorColumn(4, true);
>
> // create a bunch of rows and cells, write some values to the cells, merge
> some cells across columns and rows so auto-size has something meaningful to
> do
> Row row;
> Cell cell;
> for (int r=0; r<=10; r++) {
>     row = sh.createRow(r);
>     for (int c=0; c<=10; c++) {
>         cell = row.createCell(0);
>         cell.setCellValue("row=" + r + ", column=" + c + ",
> somethingToMakeColumnsHaveDifferentWidth=" + ((int) Math.pow(r, c)));
>     }
> }
> // ... etc
>
> sh.autoSizeColumn(0); // okay
> sh.autoSizeColumn(0, false); // okay
> sh.autoSizeColumn(0, true); // throws exception: column widths were
> calculated using merged cell widths, but this pre-requisite is no longer
> valid.
> sh.autoSizeColumn(1); // throws exception: column was not monitored for
> auto-sizing
> sh.autoSizeColumn(2); // okay
>
> // and if we want to allow a user to auto-size a column that wasn't
> previously monitored, we might need to add an extra method that will
> indicate that the user understands that auto-sizing is computed solely from
> the row access window, and not to throw an UnmonitoredColumnForAutosize
> (insert your favorite name here) exception.
> autoSizeRowAccessWindowOnly = true;
> sh.autoSizeColumn(0, false, autoSizeRowAccessWindowOnly); // okay: makes
> existing functionality available
> // alternatively, if you want to allow the user to determine if a column
> gets autosized using merged cells or not at the time autoSizeColumn is
> called, the AutosizeColumnTracker would need to calculate and remember the
> widths for both cases (useMergedCells=true and useMergedCell=false).
>
>
> // finish up
> // ... etc
> wb.write(new FileOutputStream('example.xlsx'));
>
>
> I think the relationship between the tracker object and the SXSSFSheet
> warrants more discussion, as there are several ways to do this.
> Considerations:
> * extensibility: how can the AutosizeColumnTracker class be subclassed by
> users of POI? how can the SXSSFSheet be subclassed by users of POI? Should
> AutosizeColumnTracker be final or @Internal to avoid this question?
> * reusability: is there a need to create one autosize column tracker object
> that can be applied to multiple sheets (whether this is just the index of
> the monitored columns or also the column widths)?
> * if sh.autoSizeColumn uses the tracker, either the tracker needs to be
> passed in as a third argument or the sheet needs to know what tracker it is
> associated with. Every time a column goes outside the window, the
> SXSSFSheet needs to know which tracker object its associated with so it can
> update the column widths in the tracker (or maintain its own column
> widths). In your code, you require the user to explicitly call trackRow,
> but XSSFSheet and HSSFSheet don't have a similar construct: auto-sizing
> applies to ALL rows and this is not user configurable.
> * My vision of the AutosizeColumnTracker may be too tightly coupled with
> the SXSSFSheet. At the tightly-coupled extreme, AutosizeColumnTracker may
> be a private class (or just a private TreeMap member variable) inside of
> SXSSFSheet that maintains a TreeMap of tracked columns and column widths;
> all intents are registered directly on the SXSSFSheet:
> sxssfSheet.monitorColumnForAutosizing(0);
>
> Thoughts?
> Javen
>

Re: Bug 57450: Patch submitted, request for feedback

[Javen O'Neal <ja...@gmail.com>]

Here's what I had in mind about keeping support for
SXSSFSheet.autoSizeColumn(int) and autoSizeColumn(int, bool):

int rowAccessWindowSize = 1;
SXSSFWorkbook wb = new SXSSFWorkbook(rowAccessWindowSize);
SXSSFSheet sh = wb.createSheet();
AutosizeColumnTracker tracker = sh.getAutosizeColumnTracker(); //this
probably needs to be a singleton. Can be lazily-created.
// tell workbook to keep track of column widths as they *might* be
auto-sized at the end of the sheet

boolean useMergedCells = false;
tracker.monitorColumn(0, useMergedCells);
tracker.monitorColumn(2, useMergedCells);
tracker.monitorColumn(4); // equivalent to monitorColumn(4, true);

// create a bunch of rows and cells, write some values to the cells, merge
some cells across columns and rows so auto-size has something meaningful to
do
Row row;
Cell cell;
for (int r=0; r<=10; r++) {
    row = sh.createRow(r);
    for (int c=0; c<=10; c++) {
        cell = row.createCell(0);
        cell.setCellValue("row=" + r + ", column=" + c + ",
somethingToMakeColumnsHaveDifferentWidth=" + ((int) Math.pow(r, c)));
    }
}
// ... etc

sh.autoSizeColumn(0); // okay
sh.autoSizeColumn(0, false); // okay
sh.autoSizeColumn(0, true); // throws exception: column widths were
calculated using merged cell widths, but this pre-requisite is no longer
valid.
sh.autoSizeColumn(1); // throws exception: column was not monitored for
auto-sizing
sh.autoSizeColumn(2); // okay

// and if we want to allow a user to auto-size a column that wasn't
previously monitored, we might need to add an extra method that will
indicate that the user understands that auto-sizing is computed solely from
the row access window, and not to throw an UnmonitoredColumnForAutosize
(insert your favorite name here) exception.
autoSizeRowAccessWindowOnly = true;
sh.autoSizeColumn(0, false, autoSizeRowAccessWindowOnly); // okay: makes
existing functionality available
// alternatively, if you want to allow the user to determine if a column
gets autosized using merged cells or not at the time autoSizeColumn is
called, the AutosizeColumnTracker would need to calculate and remember the
widths for both cases (useMergedCells=true and useMergedCell=false).


// finish up
// ... etc
wb.write(new FileOutputStream('example.xlsx'));


I think the relationship between the tracker object and the SXSSFSheet
warrants more discussion, as there are several ways to do this.
Considerations:
* extensibility: how can the AutosizeColumnTracker class be subclassed by
users of POI? how can the SXSSFSheet be subclassed by users of POI? Should
AutosizeColumnTracker be final or @Internal to avoid this question?
* reusability: is there a need to create one autosize column tracker object
that can be applied to multiple sheets (whether this is just the index of
the monitored columns or also the column widths)?
* if sh.autoSizeColumn uses the tracker, either the tracker needs to be
passed in as a third argument or the sheet needs to know what tracker it is
associated with. Every time a column goes outside the window, the
SXSSFSheet needs to know which tracker object its associated with so it can
update the column widths in the tracker (or maintain its own column
widths). In your code, you require the user to explicitly call trackRow,
but XSSFSheet and HSSFSheet don't have a similar construct: auto-sizing
applies to ALL rows and this is not user configurable.
* My vision of the AutosizeColumnTracker may be too tightly coupled with
the SXSSFSheet. At the tightly-coupled extreme, AutosizeColumnTracker may
be a private class (or just a private TreeMap member variable) inside of
SXSSFSheet that maintains a TreeMap of tracked columns and column widths;
all intents are registered directly on the SXSSFSheet:
sxssfSheet.monitorColumnForAutosizing(0);

Thoughts?
Javen

Re: Bug 57450: Patch submitted, request for feedback

[ST <st...@gmail.com>]

Javen,

Thanks for the feedback, I will look into it.

Regarding SXSSF#autoSizeColumn(): the current implementation works fine
and calculates autosize based on the content of the current window. So no
problem with keeping the current implementation. But isn't that confusing
for users? I'd prefer to throw an exception to "force" users to use either
the
fully supported autosize method, or none at all.



On Tue, Nov 3, 2015 at 6:48 AM, Javen O'Neal <ja...@gmail.com> wrote:

> Stefan,
>
> First off, thanks for the hard work and thought you put into improving the
> streaming workbook interface. It's certainly a challenge to work with
> columns on such a row-centric interface.
>
> Here's some critical feedback on attachment 33207 from comment 7 of bug
> 57450, mostly pertaining to the AutosizeColumnTracker class.
>
> In AutosizeColumnTracker:
>
> You should iterate in determinstic ascending order when iterating over
> monitoredColumns to calculate maxColumnWidths. The easiest way to do this
> is to change monitoredColumns from TreeSet to HashSet.
>
> You are maintaining two data structures, monitoredColumns and
> maxColumnWidths, however monitoredColumns just equals the keys of
> maxColumnWidths, so you can eliminate monitoredColumns altogether, using a
> TreeMap for maxColumnWidths.
>
> You have a method to add one column at a time to the columns-to-monitor
> set. It might be helpful to provide the following methods:
> monitorColumn(int columnIndex)
> monitorColumns(Collection<Integer> columnIndices)
> unmonitorColumn(int columnIndex)
> unmonitorColumn(Collection<Integer> columnIndices)
> The latter two may be needed if a user needs to scan rows in a sheet to
> determine if a column needs to be autosized. If the user doesn't monitor
> the columns from the very first row, the autosizing result may be incorrect
> if cells in the first row need to be wider than any remaining rows. For
> execution speed, it's best to prune unnecessary column width calculations
> as soon as the user knows they're no longer interested in it.
>
> applyAutosizeToColumn seems clunky--the user needs to remember which
> columns they want to autosize, and auto-size them one-by-one. Can you
> replace this with or add an autosizeMonitoredColumns method so users don't
> need to maintain duplicate data structures of monitored columns?
>
> I think your code works correctly for this, but did you test for both
> cases: current column width is too wide and current column width is too
> narrow?
>
> You can't deprecate SXXSFSheet#autoSizeColumn(int) or #autoSizeColumn(int,
> boolean) because these are both required by the Sheet interface[1]. You can
> replace these with a javadoc that says "not implemented for SXSSFSheets,
> use {@link AutosizeColumnTracker} instead" and throw an
> UnsupportedOperationException or RuntimeException, but it'd be even better
> if you could figure out how to keep these methods (Do they currently only
> work if the entire sheet fits within the window? Do they currently only
> autosize based on the content that is in the current window? Do they
> currently not work at all?)
>
> AutosizeColumnTracker should have unit tests (ideally 100% coverage) and
> example code in the users guide. Writing both has the added benefit that it
> gives you another chance to see how this class would be used by client code
> and identify the weak points of its interface.
>
> I will commit the minor stuff (mostly typos) that were identified in this
> patch.
>
> [1]
>
> https://poi.apache.org/apidocs/org/apache/poi/ss/usermodel/Sheet.html#autoSizeColumn%28int%29
>
>
> On Mon, Nov 2, 2015 at 12:00 PM, Stefan Thurnherr <
> st.mailinglists@gmail.com
> > wrote:
>
> > I've submitted a patch and added some discussion points to this
> enhancement
> > request over a week ago:
> >
> > "A way to autosize columns on SXSSF"
> > https://bz.apache.org/bugzilla/show_bug.cgi?id=57450#c7
> >
> > Could someone please have a look and give feedback? Thanks!
> >  stefan.
> >
>

Re: Bug 57450: Patch submitted, request for feedback

[Javen O'Neal <ja...@gmail.com>]

Stefan,

First off, thanks for the hard work and thought you put into improving the
streaming workbook interface. It's certainly a challenge to work with
columns on such a row-centric interface.

Here's some critical feedback on attachment 33207 from comment 7 of bug
57450, mostly pertaining to the AutosizeColumnTracker class.

In AutosizeColumnTracker:

You should iterate in determinstic ascending order when iterating over
monitoredColumns to calculate maxColumnWidths. The easiest way to do this
is to change monitoredColumns from TreeSet to HashSet.

You are maintaining two data structures, monitoredColumns and
maxColumnWidths, however monitoredColumns just equals the keys of
maxColumnWidths, so you can eliminate monitoredColumns altogether, using a
TreeMap for maxColumnWidths.

You have a method to add one column at a time to the columns-to-monitor
set. It might be helpful to provide the following methods:
monitorColumn(int columnIndex)
monitorColumns(Collection<Integer> columnIndices)
unmonitorColumn(int columnIndex)
unmonitorColumn(Collection<Integer> columnIndices)
The latter two may be needed if a user needs to scan rows in a sheet to
determine if a column needs to be autosized. If the user doesn't monitor
the columns from the very first row, the autosizing result may be incorrect
if cells in the first row need to be wider than any remaining rows. For
execution speed, it's best to prune unnecessary column width calculations
as soon as the user knows they're no longer interested in it.

applyAutosizeToColumn seems clunky--the user needs to remember which
columns they want to autosize, and auto-size them one-by-one. Can you
replace this with or add an autosizeMonitoredColumns method so users don't
need to maintain duplicate data structures of monitored columns?

I think your code works correctly for this, but did you test for both
cases: current column width is too wide and current column width is too
narrow?

You can't deprecate SXXSFSheet#autoSizeColumn(int) or #autoSizeColumn(int,
boolean) because these are both required by the Sheet interface[1]. You can
replace these with a javadoc that says "not implemented for SXSSFSheets,
use {@link AutosizeColumnTracker} instead" and throw an
UnsupportedOperationException or RuntimeException, but it'd be even better
if you could figure out how to keep these methods (Do they currently only
work if the entire sheet fits within the window? Do they currently only
autosize based on the content that is in the current window? Do they
currently not work at all?)

AutosizeColumnTracker should have unit tests (ideally 100% coverage) and
example code in the users guide. Writing both has the added benefit that it
gives you another chance to see how this class would be used by client code
and identify the weak points of its interface.

I will commit the minor stuff (mostly typos) that were identified in this
patch.

[1]
https://poi.apache.org/apidocs/org/apache/poi/ss/usermodel/Sheet.html#autoSizeColumn%28int%29


On Mon, Nov 2, 2015 at 12:00 PM, Stefan Thurnherr <st.mailinglists@gmail.com
> wrote:

> I've submitted a patch and added some discussion points to this enhancement
> request over a week ago:
>
> "A way to autosize columns on SXSSF"
> https://bz.apache.org/bugzilla/show_bug.cgi?id=57450#c7
>
> Could someone please have a look and give feedback? Thanks!
>  stefan.
>