Mailing-List: contact dev-help@poi.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "POI Developers List" <dev@poi.apache.org>
MIME-Version: 1.0
In-Reply-To: 
 <CAM+TppJWzhz50TExdHRAzPyFbMz8hgCKuTT=LxReq+koBE4bxQ@mail.gmail.com>
References: 
 <CABwQARuqZTsba5iU0LO3230PXvh_5XxQ3xanyF2wUHn1+nY9kQ@mail.gmail.com>
	<CAM+TppJWzhz50TExdHRAzPyFbMz8hgCKuTT=LxReq+koBE4bxQ@mail.gmail.com>
Date: Tue, 3 Nov 2015 22:53:23 +0100
Message-ID: 
 <CABwQARsLQXgYv8dn+6kX9mkF0fPFiF+1-zuK-pOdxw-iHB0WWg@mail.gmail.com>
Subject: Re: Bug 57450: Patch submitted, request for feedback
From: ST <st.mailinglists@gmail.com>
To: POI Developers List <dev@poi.apache.org>
Content-Type: multipart/alternative; boundary=089e010d851042b3370523a9ec3c

--089e010d851042b3370523a9ec3c
Content-Type: text/plain; charset=UTF-8

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 <javenoneal@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.
> >
>

--089e010d851042b3370523a9ec3c--