poi-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Javen O'Neal" <javenon...@gmail.com>
Subject Re: Bug 57450: Patch submitted, request for feedback
Date Tue, 03 Nov 2015 05:48:59 GMT
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.
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message