You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@poi.apache.org by bu...@apache.org on 2016/01/01 17:43:04 UTC
[Bug 58787] New: [patch] Border Drawing utility that does not create
unnecessary styles
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Bug ID: 58787
Summary: [patch] Border Drawing utility that does not create
unnecessary styles
Product: POI
Version: 3.14-dev
Hardware: PC
Status: NEW
Severity: normal
Priority: P2
Component: SS Common
Assignee: dev@poi.apache.org
Reporter: jmarkmurph@yahoo.com
Created attachment 33391
--> https://bz.apache.org/bugzilla/attachment.cgi?id=33391&action=edit
Patch file generated via Ant script
This new utility can be used to create "CellBorders" that can then be applied
to one or more sheets of the workbook without creating unnecessary intermediate
styles. In addition, this patch includes some helper methods to allow a
workbook to report it's own SpreadsheetVersion.
The patch has an example file that includes color borders, and removing
borders, and tests for changes to CellUtil.setCellProperty.
This patch depends on 58633.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [PATCH] Border Drawing utility that does not create
unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Dominik Stadler <do...@gmx.at> changed:
What |Removed |Added
----------------------------------------------------------------------------
Summary|[patch] Border Drawing |[PATCH] Border Drawing
|utility that does not |utility that does not
|create unnecessary styles |create unnecessary styles
Severity|normal |enhancement
Keywords| |PatchAvailable
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [In progress] Border Drawing utility that does not
create unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #14 from Nick Burch <ap...@gagravarr.org> ---
(In reply to Mark Murphy from comment #13)
> back to working on getting the last bits in here, and as I was working on
> it, I noticed that CellUtil.setCellProperty has a Workbook parameter, but if
> it doesn't match the Workbook for the cell and exception is thrown.
> Shouldn't we, instead, add a version of CellUtil.setCellProperty without the
> Workbook parameter and depricate the old one? That Workbook parameter is
> ignored except for the check against the cell.
setCellStyleProperties doesn't need/take a Workbook, so bringing
setCellStylePropery into line with that makes sense to me
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #23 from Javen O'Neal <on...@apache.org> ---
(In reply to Javen O'Neal from comment #22)
> 7. Create a blocker to this bug: a re-write/deprecation of short borderTypes in
> favor of enums.
Opened bug 59264 to take care of this.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #27 from Javen O'Neal <on...@apache.org> ---
Mark,
When adding a border color, if a border line style is not set in the property
template, the code sets it. Was there any reason for using drawTopBorder rather
than directly adding the property?
> private void drawTopBorderColor(CellRangeAddress range, short color) {
> int row = range.getFirstRow();
> int firstCol = range.getFirstColumn();
> int lastCol = range.getLastColumn();
> for (int i = firstCol; i <= lastCol; i++) {
> CellAddress cell = new CellAddress(row, i);
> // if BORDER_TOP is not set on BorderPropertyTemplate, make a thin border so that there's something to color
> if (borderIsNotSet(cell, CellUtil.BORDER_TOP)) {
> - drawTopBorder(new CellRangeAddress(row, row, i, i), BorderStyle.THIN);
> + addProperty(cell, CellUtil.BORDER_TOP, BorderStyle.THIN);
> }
> addProperty(cell, CellUtil.TOP_BORDER_COLOR, color);
> }
> }
Also, what should BorderPropertyTemplate do if the border line style is set to
NONE and then drawTopBorderColor is called? Should it change the BorderStyle to
THIN?
> private boolean borderIsNotSet(CellAddress cell, String borderDirection) {
> Object borderLineStyle = getTemplateProperty(cell, borderDirection);
> - return (borderLineStyle == null);
> + return (borderLineStyle == null) || (borderLineStyle == BorderStyle.NONE);
> }
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Javen O'Neal <on...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Summary|[patch] Border Drawing |[patch] Border utility to
|utility that does not |set cell styles around a
|create unnecessary styles |range of cells
--- Comment #22 from Javen O'Neal <on...@apache.org> ---
Take a look at the following files:
Common SS:
- src/java/org/apache/poi/ss/util/RegionUtil.java
- set the same border style or color on an outer edge of a cell region
- almost the same as attachment 33684 except does not handle ALL, NONE, and
interior edges of a region
- src/java/org/apache/poi/ss/usermodel/BorderFormatting.java
- get/set border styles (dot, dash, etc) and colors
- implementing classes:
- src/java/org/apache/poi/hssf/usermodel/HSSFBorderFormatting.java
bound to a workbook, CFRuleRecord, and BorderFormatting*
- src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFBorderFormatting.java
bound to a CTBorder
- src/java/org/apache/poi/ss/util/CellUtil.java
- constants for the names of border properties to be used as keys in
get/setCellStyleProperties
- get/setCellStyleProperties, use a Map that is detached from cells or
styles, then when setting cell style properties, search through existing styles
and add a new style only if no identical style exists.
HSSF:
- src/java/org/apache/poi/hssf/record/cf/BorderFormatting.java
- a class to handle the bit-stuffing for records used by
HSSFBorderFormatting
HSSF Examples:
- src/examples/src/org/apache/poi/hssf/usermodel/examples/Borders.java
- a cell style attached to (potentially) multiple cells can be modified one
attribute at a time (border line style, border line color)
HWPF:
- src/scratchpad/src/org/apache/poi/hwpf/usermodel/BorderCode.java
- create a border object that is detached from other objects (paragraph,
table)
- specifies border line width, border style (dot, dash, etc), border color,
border space between text and border, etc, presumably for Text Boxes?
XWPF:
- src/ooxml/java/org/apache/poi/xwpf/usermodel/Borders.java
- an enum to hold the ~191 border styles for Word documents
Some of these features were written without enums, so it's pretty messy. Going
forward, it's worthwhile to make these methods enum-friendly (and therefore
type safe).
All of the classes listed above operate on a single border format that could be
applied to a single cell, except for RegionUtil.
RegionUtil sets the styles with CellUtil.setCellStyleProperty immediately,
while PropertyTemplate maintains an internal Map to a Sheet, and explicitly
applies the styles to a sheet with applyBorders.
If two regions do not overlap and the style of each region is homogeneous,
RegionUtil may create *slightly* more intermediate styles (the corners) than
PropertyTemplate. If two regions overlap, RegionUtil is likely to create many
more styles than necessary.
Here's what needs done:
1. DrawingBorders example: Use CellRangeAddress("B2:D5") to make it easier to
see what's going on
2. Unit test: add a test case counting up the number of styles that are in a
workbook before and after. Do the same for RegionUtil and demonstrate that
PropertyTemplate creates significantly fewer intermediate styles. Either look
at total number of cell styles (wb.getNumCellStyles) or number of cell styles
in the styles table that are not referenced by any cells.
3. Try to find more descriptive names for PropertyTemplate and Extent.
BorderUtil/BorderPropertyTemplate and
BorderArrangement/BorderRegionEdges/BorderExtent.
4. Try to integrate your PropertyTemplate code into RegionUtil (might be easier
to copy RegionUtil into PropertyTemplate and rename PropertyTemplate to
RegionUtil).
5. My guess is that the inspiration for this class is to build up a template
that you could stamp onto multiple similarly-formatted sheets. This is a more
common usecase for POI because we deal with computer-generated templates, but
it might not be the most common use case (certainly for entry-level
applications). See if you can make this class more approachable to a wider
audience. If not, then maybe it's better to let BorderPropertyTemplate remain a
separate (higher-level) class from RegionUtil, and not plan to replace
RegionUtil.
6. The _propertyTemplate map is stuck/hidden from a user. What if the user
wants to build up a base template, and fork it with two variations? Potential
solution: add a PropertyTemplate(PropertyTemplate other) constructor that would
deep-copy the Map (I think the Java consensus is that copy constructors are
preferred over clone because it's easier to subclass).
7. Create a blocker to this bug: a re-write/deprecation of short borderTypes in
favor of enums. It'd be a shame to have a shiny new class that uses
non-type-safe borderTypes or bloat your class with short and enum variants of
each method
Most of the SS utilities are static (stateless) methods, but I think your
encapsulation of a complex data structure makes sense.
Please look through the classes that I mentioned, especially CellUtil and
RegionUtil to see if there's anything you can leverage.
Thanks for the hard work!
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border Drawing utility that does not create
unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #16 from Mark Murphy <jm...@yahoo.com> ---
(In reply to Nick Burch from comment #14)
> (In reply to Mark Murphy from comment #13)
> > back to working on getting the last bits in here, and as I was working on
> > it, I noticed that CellUtil.setCellProperty has a Workbook parameter, but if
> > it doesn't match the Workbook for the cell and exception is thrown.
> > Shouldn't we, instead, add a version of CellUtil.setCellProperty without the
> > Workbook parameter and depricate the old one? That Workbook parameter is
> > ignored except for the check against the cell.
>
> setCellStyleProperties doesn't need/take a Workbook, so bringing
> setCellStylePropery into line with that makes sense to me
I am going to do this in a new bug since it is independent of this particular
bug, and I really want to avoid scope creep.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border Drawing utility that does not create
unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #1 from Mark Murphy <jm...@yahoo.com> ---
A remaining issue, though not sure how important it is. This process creates
unnecessary <border> elements in the XLSX file. Largely because
CellUtil.setFormatProperties adds each piece of the border individually instead
of adding them in a single shot. This is likely to be the case no matter how
borders are drawn. A fix to this would be to hold all border properties in
their own HashMap and process that in one piece in
CellUtil.setFormatProperties. I have not done this. The same issue occurs with
fills, but not in the border drawing process.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border Drawing utility that does not create
unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Mark Murphy <jm...@yahoo.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
OS| |All
Depends on| |58633
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #30 from Mark Murphy <jm...@yahoo.com> ---
(In reply to Javen O'Neal from comment #26)
> (In reply to Javen O'Neal from comment #22)
> > 1. DrawingBorders example: Use CellRangeAddress("B2:D5") to make it easier
> > to see what's going on
> Applied in r1747888
>
> > 3. Try to find more descriptive names for PropertyTemplate and Extent.
> Applied in r1747884. Using BorderPropertyTemplate and BorderExtent.
>
> > 7. Create a blocker to this bug: a re-write/deprecation of short borderTypes
> > in favor of enums.
> Applied in r1747868. I converted borderTypes from short to BorderStyles enum.
> The colors will remain as short values, since they are indices into the
> color table (indexed+custom).
Sorry for taking so long to send my patches to you. If I had added my changes,
you wouldn't have had to do so much work. Instead of BorderPropertyTemplate I
was using CellStyleTemplate as it would be useful for other CellStyle
attributes beside Border attributes. For example I had planned to add fills to
it as well. I moved away from BorderPropertyTemplate since that name seems
limiting to me. If I had sent my patch sooner, you would have known that. My
fault, but just to prevent too many from using BorderPropertyTemplate, we may
want to change that sooner rather than later.
Thanks for taking your time to work on this.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #28 from Javen O'Neal <on...@apache.org> ---
(In reply to Javen O'Neal from comment #22)
> 6. The _propertyTemplate map is stuck/hidden from a user. What if the user
> wants to build up a base template, and fork it with two variations?
> Potential solution: add a PropertyTemplate(PropertyTemplate other)
> constructor that would deep-copy the Map
Applied in r1748071.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border Drawing utility that does not create
unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Javen O'Neal <on...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |NEEDINFO
--- Comment #17 from Javen O'Neal <on...@apache.org> ---
(In reply to Mark Murphy from comment #15)
> Created attachment 33667 [details]
> Completed ant patch including tests, examples, and documentation
Attachment 33667 doesn't compile because it imports OOXML classes (appears to
only be for javadoc linking). OOXML classes aren't compiled or available to
org.apache.poi.ss.util. Replace these {@link} references with regular text
references and delete the imports and it'll be good. If you have code that
relies on OOXML classes, you have to write Voldemort code or take advantage of
inheritance of methods that are resolved at run-time [1]. If you want to check
that your code compiles (and passes unit tests) before generating the patch
file, run "ant test" or "ant clean test".
The solution we've accepted for junit tests is to either duplicate the test in
TestHSSFWorkbook, TestXSSFWorkbook, and TestSXSSFWorkbook, or write a single
test (preferred) in BaseTestWorkbook that uses testDataProvider.createWorkbook.
In the case of org.apache.poi.ss.util junit tests, we've usually picked
HSSFWorkbook as the class to test the methods (for the same OOXML availability
reason as above). I'd like to rearrange the org.apache.poi.ss.util tests to run
with HSSF, XSSF, and SXSSF instances to make sure they all work (either with
junit testcase parameterization or with ITestDataProvider as we have done for
org.apache.poi.hssf.usermodel, .xssf.usermodel, and .xssf.streaming unit
tests). Anything in org.apache.poi.ss.util should probably work on all 3 kinds
of workbooks, with the same behavior (unless a different behavior makes sense),
but this is outside the scope of this bug.
You mentioned in the javadocs that PropertyTemplate would replace RegionUtil. I
haven't compared the two classes side-by-side yet. Do you mean that
PropertyTemplate is a higher-level wrapper around RegionUtil, and that most
people using RegionUtil would likely be more interested in PropertyTemplate? Or
is PropertyTemplate a rewrite of RegionUtil, fixing inconsistencies in the
provided methods? I'm a fan of code reduction on my personal projects, but I
try to keep POI backwards compatible if at all possible, and retire features 2+
releases after announcing deprecation (if possible). Your patch didn't include
any changes to RegionUtil. What are your long-term plans for this class?
Deprecate it? Merge PropertyTemplate into RegionUtil (or vice versa, the less
backwards-compatible option of the two)? Call RegionUtil from PropertyTemplate?
Thanks for the hard work you've put into this bug.
[1] Using class inheritance rather than if-then statements to avoid
compile-time dependencies. Added TestDataProvider.createWorkbook(int
rowAccessSize) and TestDataProvider.trackAllColumnsForAutosizing, which don't
do anything special for HSSFWorkbook or XSSFWorkbook, but have special behavior
for SXSSFWorkbook: https://svn.apache.org/viewvc?view=revision&revision=1730997
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #36 from Mark Murphy <jm...@yahoo.com> ---
(In reply to comment #27)
> private boolean borderIsNotSet(CellAddress cell, String borderDirection) {
> Object borderLineStyle = getTemplateProperty(cell, borderDirection);
> - return (borderLineStyle == null);
> + return (borderLineStyle == null) || (borderLineStyle == BorderStyle.NONE);
> }
Changed this to borderIsSet() because I generally do not like negative logic
flags. That is, booleans with something like Not in the name. These flags tend
to map a value like Found to False, and Not Found to True. Is the object
notFound? Yes the object is notFound, or No the object is not notFound. Too
many nots. I prefer to put the negation in the operator.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #37 from Mark Murphy <jm...@yahoo.com> ---
Created attachment 33948
--> https://bz.apache.org/bugzilla/attachment.cgi?id=33948&action=edit
Patch file generated by ant
This leaves 2. Additional unit tests to verify reduced style creation
unfinished
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
ithan <it...@gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |ithanriza@gmail.com
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [In progress] Border Drawing utility that does not
create unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Mark Murphy <jm...@yahoo.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #33455|0 |1
is obsolete| |
--- Comment #5 from Mark Murphy <jm...@yahoo.com> ---
Created attachment 33456
--> https://bz.apache.org/bugzilla/attachment.cgi?id=33456&action=edit
Rebased and updated again
This really only contains a new class CellBorders and a Quick Guide and
Examples. Not real sure how to write unit tests for this since I need to look
at the generated spreadsheet to determine if it worked or not. I could use some
coaching on that.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [In progress] Border Drawing utility that does not
create unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #4 from Mark Murphy <jm...@yahoo.com> ---
If you use the drawing borders example I added to the Quick Guide for
setCellProperties, it will not create extra styles. This enhancement really
adds some additional helpers around border drawing that will make it even
easier.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #29 from Javen O'Neal <on...@apache.org> ---
(In reply to Javen O'Neal from comment #25)
> (In reply to Mark Murphy from comment #20)
> > Created attachment 33684 [details]
> > Patch file generated via Ant script
>
> Applied in r1747851 to branches/ss_border_property_template
Merged to trunk in r1748075. Deleted branches/ss_border_property_template in
r1748076.
Remaining:
* comment 27 (question about draw(Top|Bottom|Left|Right)BorderColor)
* 2. Unit test: add a test case counting up the number of styles that are in a
workbook before and after. Do the same for RegionUtil and demonstrate that
PropertyTemplate creates significantly fewer intermediate styles. Either look
at total number of cell styles (wb.getNumCellStyles) or number of cell styles
in the styles table that are not referenced by any cells.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border Drawing utility that does not create
unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #18 from Mark Murphy <jm...@yahoo.com> ---
(In reply to Javen O'Neal from comment #17)
> If you want to check that your code compiles (and passes unit tests) before
> generating the patch file, run "ant test" or "ant clean test".
"ant test" unit tests fail before it gets to my test, so I am just running my
tests manually.
> Anything in
> org.apache.poi.ss.util should probably work on all 3 kinds of workbooks,
> with the same behavior (unless a different behavior makes sense)
Not real sure how this would make sense with this utility. I don't know enough
about how SXSSF works to make this work for that type of spreadsheet. It
requires the sheet to be in memory all at once, or at least the portion that
the template is being applied against.
> You mentioned in the javadocs that PropertyTemplate would replace
> RegionUtil. I haven't compared the two classes side-by-side yet. Do you mean
> that PropertyTemplate is a higher-level wrapper around RegionUtil, and that
> most people using RegionUtil would likely be more interested in
> PropertyTemplate? Or is PropertyTemplate a rewrite of RegionUtil, fixing
> inconsistencies in the provided methods?
My thought is that RegionUtil would be deprecated in favor of PropertyTemplate.
PropertyTemplate is most accurately a rewrite of RegionUtil with an eye toward
reducing the number of CellStyles created while borders are being written. It
is possible that RegionUtil could be adjusted to use the PropertyTemplate
class, but that would still create excess styles as each border is added to the
CellStyle one at a time in RegionUtil. I would favor dropping it since it does
nothing else, and renaming PropertyTemplate to RegionUtil makes little sense
since RegionUtil is a "static only" class, but PropertyTemplate is not. The two
classes work differently. The one advantage RegionUtil has over
PropertyTemplate is that it can be used with SXSSF since it can be used with a
limited set of rows.
Now that I am thinking of it more, maybe PropertyTemplate can indeed be used
with SXSSFWorkbooks by adding an option to apply the template to just the rows
in memory, and ignoring template rows that are not in memory. Or by adding a
feature to allow applying a smaller template to a relative position in the
sheet.
As far as the unavailable classes in the links, and test, I will look at some
other tests, and rewrite mine to conform. I did not notice the inability to
compile, probably because I don't have something configured correctly in
eclipse. I will figure that out.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [PATCH] Border Drawing utility that does not create
unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Bug 58787 depends on bug 58633, which changed state.
Bug 58633 Summary: [patch] Need to set multiple CellStyle properties at once
https://bz.apache.org/bugzilla/show_bug.cgi?id=58633
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [In progress] Border Drawing utility that does not
create unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #7 from Mark Murphy <jm...@yahoo.com> ---
I created a new object for this named CellBorder because I am working on cell
borders. However, I can see this being useful for defining and applying a bunch
of fills to a region as well. And, for that matter, there are other cell style
properties that could be applied in this manner. So I am conflicted on whether
this should be in a new class called CellBorder, or in the RegionUtil class. I
would need a slightly different approach to put it in the RegionUtil class.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Javen O'Neal <on...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Blocks| |54593
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [In progress] Border Drawing utility that does not
create unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #10 from Javen O'Neal <on...@apache.org> ---
Mark,
Did you take a look at RegionUtil to see if meets your needs? See Bug 54593.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Mark Murphy <jm...@yahoo.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #33684|0 |1
is obsolete| |
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #41 from Mark Murphy <jm...@yahoo.com> ---
(In reply to Javen O'Neal from comment #40)
> Two other changes:
> # Rather than saving all property template values as Shorts, I stored
> higher-level objects (BorderStyle and BorderExtent) when available. This
> user code, makes unit tests, and error messages easier to read while also
> allowing us to deprecate and remove meaningless code/id fields on enums.
> > public Object getTemplateProperty(CellAddress cell, String property)
> from [1]
>
> # As a marginal performance improvement, I used 4 Map.containsKey calls
> rather than for-looping over all the keys in cell style template to count
> getNumBorders and getNumBorderColors [2].
>
> [1]
> https://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/ss/util/
> BorderPropertyTemplate.java?revision=1748075&view=markup&pathrev=1748292#l928
> [2]
> https://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/ss/util/
> BorderPropertyTemplate.java?revision=1748075&view=markup&pathrev=1748292#l888
Never mind, I wasn't ready anyway, something has changed in my environment
since I last tested, and I can't make things work now. Rokie mistake, won't
happen again.
I have reverted things until I get it cleared up in my environment. I will look
at your suggestions as well. Thanks for those.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #39 from Javen O'Neal <on...@apache.org> ---
When there is no significant speed or memory benefit to using (int row, int
col) arguments over a more descriptive CellAddress argument to a public
function, only the function using the CellAddress argument should be in the
public API.
This keeps classes smaller and easier to find the function that needs to be
used, even if it moves the CellAddress object creation from the POI library to
user code. This is probably a good thing because it encourages reuse of the
CellAddress objects (hiding the object creation in the method hides that cost).
I would recommend deleting the following methods:
> public int getNumBorders(int row, int col)
> public BorderStyle getBorderStyle(int row, int col, String property)
> public int getNumBorderColors(int row, int col)
> public short getTemplateProperty(int row, int col, String property)
Every POI entry point is:
* another function to test
* another potential bug (null pointer and range check being the most likely)
* mental burden for the user to figure out which variant of a function to use.
These can be completely removed (no deprecation warning) without a deprecation
warning up to 3.15 final release.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [PATCH] Border Drawing utility that does not create
unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Bug 58787 depends on bug 58879, which changed state.
Bug 58879 Summary: Return SpreadsheetVersion from Workbook
https://bz.apache.org/bugzilla/show_bug.cgi?id=58879
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #42 from Javen O'Neal <on...@apache.org> ---
(In reply to Mark Murphy from comment #41)
> something has changed in my environment since I last tested
Try ant clean jenkins. Sometimes outdated artifacts don't get rebuilt when they
should (I've been bitten on test-ss and test-ooxml-ss before), probably due to
problems in the ant rules I wrote into build.xml.
If you want, create an svn branch to commit your changes and merge when done. I
believe we can create a Jenkins job for branches.
If you're comfortable with git or git-svn, local commits is another good
option.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [PATCH] Border Drawing utility that does not create
unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Javen O'Neal <on...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |NEEDINFO
--- Comment #2 from Javen O'Neal <on...@apache.org> ---
It looks like all the changes in patch.tar.gz/patch.txt were committed in bug
58633 and bug 58879.
I think the two tests from TestCellUtil.java were committed as part of bug
58633. If not, could you rebase your changes, and rewrite to use junit4 instead
of junit3?
For CellBorder.java, you might want to use an enum here so that CellBorders can
be used in switch statements in Java 6, and also improves code quality due to
type checking. See SpreadsheetVersion's implementation if you need an example.
For another example, see o.a.p.ss.usermodel.HorizontalAlignment. Functions in
CellBorder.java should have unit tests before this is committed.
Per comment 1, if creating unnecessary intermediate <border> elements is still
an issue after bug 58633, please write a unit test for that.
Thanks for the DrawingBorders example!
==== Side-note about JUnit testing in POI ====
You'll find two different flavors of unit tests in Apache POI
There's junit3:
import junit.framework.TestCase;
public class TestSomeClass extends TestCase {
public void testSomeMethod() { }
}
and junit4:
import org.unit.Test;
public class TestSomeClass {
@Test
public void testSomeMethod() { }
}
We're slowly trying to convert our junit3 tests over to junit4. There are a few
reasons for this:
* junit4 has new features that are more flexible
* inheritance: we don't need to inherit from TestCase, making it easier to
inherit from a base class that makes the tests more concise (see
BaseTestWorkbook, TestXSSFWorkbook)
* Don't need to build and maintain test suite classes--that is, classes that
just list the TestCase classes to be run.
New tests should be written in junit4.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [In progress] Border Drawing utility that does not
create unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #11 from Mark Murphy <jm...@yahoo.com> ---
(In reply to Javen O'Neal from comment #10)
> Mark,
>
> Did you take a look at RegionUtil to see if meets your needs? See Bug 54593.
Yes, in fact RegionUtil is the source of my previous queries. But RegionUtil is
slow since it takes a one at a time approach at setting cell properties. Thus
creating a lot of unused intermediate CellStyles. I didn't see it until I
created a moderately sized sheet. That was when I set about creating
setCellProperties(). But RegionUtil can't really utilize that method the way it
is written. CellBorder takes the approach of putting together the borders in a
separate object, then applying them to the sheet all at once. This should
perform significantly better. I am not sure if we should upgrade RegionUtil or
remove it. RegionUtil is a better name because it can include fills as well,
but it needs a better implementation. CellBorder does everything RegionUtil
does, but performs better, and may use less memory because all those unused
intermediate styles aren't created.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border Drawing utility that does not create
unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Mark Murphy <jm...@yahoo.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEEDINFO |NEW
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #33 from Mark Murphy <jm...@yahoo.com> ---
(In reply to Javen O'Neal from comment #32)
> I can roll back the changes for BorderPropertyTemplate and add
> CellStyleTemplate. I like that idea better. Maybe rewrite RegionUtil as well
> if you want a class that operates on a cell range rather than a single cell.
I have some ideas for RegionUtil that would take advantage of
CellStyleTemplate. I just need to decide how to implement them. We could set up
some methods that would draw a full grid, or horizontal lines or vertical
lines, or full grid with an outline. Potentially other combinations, but what
is the best way to do that. Then when CellStyleTemplate supports fills we could
add even/odd banding in RegionUtil. Still meditating on that. This is just to
make CellStyleTemplate more approachable. Maybe to keep things simple we should
just have an enum of predefined CellStyleTemplates accessible from RegionUtil
with a single call, and anything more complex would call for direct use of the
CellStyleTemplate.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [In progress] Border Drawing utility that does not
create unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #8 from Javen O'Neal <on...@apache.org> ---
In general, I prefer creating inheritable classes rather than util functions
because
* behavior can be overridden in an object-oriented way
* features are more discoverable through Javadocs. People have to know CellUtil
exists and contains some goodies. Util classes get disorganized, turning into a
junk drawer where it's hard to search through the class to find what you want.
Splitting up a util class solves the junk drawer problem but invites the
Wall-e spork clarification problem: if a function could belong in either of two
util classes, which one does it go in, and if people expect it in the opposite
one, possibly creating a new function that duplicates functionality.
Util functions make sense in some situations: return the maximum row number for
sheets in a workbook, or return diff two workbooks, or interleave the rows of
two spreadsheets (where there isn't an implied directionality, so that object
and subject in `object.action(subject)` are swappable.
In general, having higher level data structures/class allows higher level
client code, and that is a good thing.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #24 from Javen O'Neal <on...@apache.org> ---
Update this documentation as well: https://poi.apache.org/faq.html#faq-N100E3
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [In progress] Border Drawing utility that does not
create unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Javen O'Neal <on...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Summary|[PATCH] Border Drawing |[In progress] Border
|utility that does not |Drawing utility that does
|create unnecessary styles |not create unnecessary
| |styles
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [PATCH] Border Drawing utility that does not create
unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Javen O'Neal <on...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Depends on| |58879
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #40 from Javen O'Neal <on...@apache.org> ---
Two other changes:
# Rather than saving all property template values as Shorts, I stored
higher-level objects (BorderStyle and BorderExtent) when available. This user
code, makes unit tests, and error messages easier to read while also allowing
us to deprecate and remove meaningless code/id fields on enums.
> public Object getTemplateProperty(CellAddress cell, String property)
from [1]
# As a marginal performance improvement, I used 4 Map.containsKey calls rather
than for-looping over all the keys in cell style template to count
getNumBorders and getNumBorderColors [2].
[1]
https://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/ss/util/BorderPropertyTemplate.java?revision=1748075&view=markup&pathrev=1748292#l928
[2]
https://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/ss/util/BorderPropertyTemplate.java?revision=1748075&view=markup&pathrev=1748292#l888
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Mark Murphy <jm...@yahoo.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Blocks| |55555
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #35 from Mark Murphy <jm...@yahoo.com> ---
(In reply to comment #27)
> When adding a border color, if a border line style is not set in the property
> template, the code sets it. Was there any reason for using drawTopBorder rather
> than directly adding the property?
Now that I can look at my code, this is a style thing. While it would work to
just add the code here, if a bug were to rear it's ugly head in
drawTopBorder(), we would potentially have two places to make the correction.
One in drawTopBorderColor(), and one in drawTopBorder(). Notice that there is
already one side case where we are removing a border by setting it to NONE
which adds extra code. Granted the need to ensure a border exists to apply a
color to would not ever be setting the top border to NONE, but say another side
case appears? My time programming RPG has given me a strong appreciation for
the DRY concepts because I frequently experience what happens when that isn't
followed.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [In progress] Border Drawing utility that does not
create unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Mark Murphy <jm...@yahoo.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #33456|0 |1
is obsolete| |
Attachment #33457|0 |1
is obsolete| |
--- Comment #15 from Mark Murphy <jm...@yahoo.com> ---
Created attachment 33667
--> https://bz.apache.org/bugzilla/attachment.cgi?id=33667&action=edit
Completed ant patch including tests, examples, and documentation
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #38 from Mark Murphy <jm...@yahoo.com> ---
r1754691 addresses issues 1,3, and 6 from comment 22.
Remaining is issue 2.
As far as issues 4 and 5 go, I plan to add additional attributes in the
CellStyleTemplate class, and I am considering ways to integrate it into the
RegionUtil class, but have not yet determined how that would work.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border Drawing utility that does not create
unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Mark Murphy <jm...@yahoo.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #33667|0 |1
is obsolete| |
--- Comment #20 from Mark Murphy <jm...@yahoo.com> ---
Created attachment 33684
--> https://bz.apache.org/bugzilla/attachment.cgi?id=33684&action=edit
Patch file generated via Ant script
I corrected the links to remove XSSF references, and compiled and tested via
ant. Funny thing is, if I run ant test from the console, all tests succeed. It
I run ant test via eclipse, the first test fails. But if I manually run junit
on that test inside eclipse, the test passes. Not sure what is stopping the ant
task from completing properly. Maybe someone that knows more about ant than I
can look at it and determine what the issue is. I am using eclipse 4.5 with
Java 8. Ant 1.9 passes the tests when I run it from the console.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border Drawing utility that does not create
unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #19 from Javen O'Neal <on...@apache.org> ---
(In reply to Mark Murphy from comment #18)
> "ant test" unit tests fail before it gets to my test, so I am just running
> my tests manually.
Ant builds should pass if you've updated to the latest code. The build is
usually green, so either you checked out the code between when dungeons checked
in a breaking change and a fix, have an incomplete checkout, or one of your own
changes is breaking the build. See https://builds.apache.org/job/POI/
> Now that I am thinking of it more, maybe PropertyTemplate can indeed be used
> with SXSSFWorkbooks by adding an option to apply the template to just the
> rows in memory, and ignoring template rows that are not in memory. Or by
> adding a feature to allow applying a smaller template to a relative position
> in the sheet.
You could throw an error if the any cells to modify the border on are outside
the current window. You could theoretically queue up modifications to any cell
that is below the window and apply the changes once the row enters the access
window, but we don't have a precedent for this. This would consume more RAM,
which defeats the purpose of SXSSF. It's fair to have SXSSF functions mandate
rows that need access are in the access window, and bow out otherwise.
> I did not notice the inability to compile, probably because I
> don't have something configured correctly in eclipse. I will
> figure that out.
I develop in Eclipse or vim and run tests on the command line. When I run tests
in Eclipse, it runs additional stress tests that "ant test" doesn't run.
Sometimes these extra tests fail or take extra time.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #43 from Mark Murphy <jm...@apache.org> ---
r1763338 addresses issues 1,3, 6, and 7 from comment 22.
Remaining is issue 2.
As far as issues 4 and 5 go, I plan to add additional attributes in the
CellStyleTemplate class, and I am considering ways to integrate it into the
RegionUtil class, but have not yet determined how that would work.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [In progress] Border Drawing utility that does not
create unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #13 from Mark Murphy <jm...@yahoo.com> ---
back to working on getting the last bits in here, and as I was working on it, I
noticed that CellUtil.setCellProperty has a Workbook parameter, but if it
doesn't match the Workbook for the cell and exception is thrown. Shouldn't we,
instead, add a version of CellUtil.setCellProperty without the Workbook
parameter and depricate the old one? That Workbook parameter is ignored except
for the check against the cell.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [In progress] Border Drawing utility that does not
create unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #12 from Javen O'Neal <on...@apache.org> ---
(In reply to Mark Murphy from comment #5)
> Not real sure how to write unit tests for this since I need to
> look at the generated spreadsheet to determine if it worked or not. I could
> use some coaching on that.
Sometimes writing unit tests is tough. At the bare minimum, a unit test can be
used to show someone how the code was intended to be used (this works great as
example code if you exclude the assertions). Unit tests should say what your
code should and should not do. For example, createBorder(region, ALL) should
create a left border on all cells in the first column of region, and should not
create a left border on any column to the right of the first column in the
region.
You can and should review your results by opening the file in Excel, but that's
probably the last time your feature will be tested using Excel unless there's a
bug.
Your unit test will have to assume some functionality is implemented
correctly--which is fair to say about anything that the test wasn't explicitly
written to test. This might mean writing:
cell = row.createCell(0);
//blank/empty cells don't have any style to start with.
assertEquals(0, Util.getNumOfBorders(cell));
Util.setBorder(cell, LEFT);
assertTrue(Util.isBorderSet(cell, LEFT));
// the right border should not be set
assertFalse(Util.isBorderSet(cell, RIGHT));
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #34 from Javen O'Neal <on...@apache.org> ---
(In reply to Javen O'Neal from comment #32)
> I can roll back the changes for BorderPropertyTemplate and add
> CellStyleTemplate. I like that idea better. Maybe rewrite RegionUtil as well
> if you want a class that operates on a cell range rather than a single cell.
Rolled back in r1748293, r1748294, and r1748295.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [In progress] Border Drawing utility that does not
create unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Mark Murphy <jm...@yahoo.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #33391|0 |1
is obsolete| |
--- Comment #3 from Mark Murphy <jm...@yahoo.com> ---
Created attachment 33455
--> https://bz.apache.org/bugzilla/attachment.cgi?id=33455&action=edit
Updated patch file containing quick guide modifications
I added a Quick guide paragraph for this enhancement and regenerated the patch
file. I also removed a redundant reference to the workbook object.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #26 from Javen O'Neal <on...@apache.org> ---
(In reply to Javen O'Neal from comment #22)
> 1. DrawingBorders example: Use CellRangeAddress("B2:D5") to make it easier
> to see what's going on
Applied in r1747888
> 3. Try to find more descriptive names for PropertyTemplate and Extent.
Applied in r1747884. Using BorderPropertyTemplate and BorderExtent.
> 7. Create a blocker to this bug: a re-write/deprecation of short borderTypes
> in favor of enums.
Applied in r1747868. I converted borderTypes from short to BorderStyles enum.
The colors will remain as short values, since they are indices into the color
table (indexed+custom).
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Javen O'Neal <on...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Depends on| |59264
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [In progress] Border Drawing utility that does not
create unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #9 from Mark Murphy <jm...@yahoo.com> ---
Perhaps it should be called CellStyleTemplate instead of CellBorder. Then we
could have methods that add fills and other CellStyle properties, and then
apply them all at once.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [In progress] Border Drawing utility that does not
create unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #6 from Mark Murphy <jm...@yahoo.com> ---
Created attachment 33457
--> https://bz.apache.org/bugzilla/attachment.cgi?id=33457&action=edit
This is a SVN diff of the quick-guide.xml
I used SVN to create a diff of the quick-guide.xml with the description of how
to use CellBorder.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [In progress] Border Drawing utility that does not
create unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Mark Murphy <jm...@yahoo.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEEDINFO |NEW
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #31 from Mark Murphy <jm...@yahoo.com> ---
(In reply to Javen O'Neal from comment #27)
> Mark,
>
> When adding a border color, if a border line style is not set in the
> property template, the code sets it. Was there any reason for using
> drawTopBorder rather than directly adding the property?
>
> > private void drawTopBorderColor(CellRangeAddress range, short color) {
> > int row = range.getFirstRow();
> > int firstCol = range.getFirstColumn();
> > int lastCol = range.getLastColumn();
> > for (int i = firstCol; i <= lastCol; i++) {
> > CellAddress cell = new CellAddress(row, i);
> > // if BORDER_TOP is not set on BorderPropertyTemplate, make a thin border so that there's something to color
> > if (borderIsNotSet(cell, CellUtil.BORDER_TOP)) {
> > - drawTopBorder(new CellRangeAddress(row, row, i, i), BorderStyle.THIN);
> > + addProperty(cell, CellUtil.BORDER_TOP, BorderStyle.THIN);
> > }
> > addProperty(cell, CellUtil.TOP_BORDER_COLOR, color);
> > }
> > }
>
> Also, what should BorderPropertyTemplate do if the border line style is set
> to NONE and then drawTopBorderColor is called? Should it change the
> BorderStyle to THIN?
> > private boolean borderIsNotSet(CellAddress cell, String borderDirection) {
> > Object borderLineStyle = getTemplateProperty(cell, borderDirection);
> > - return (borderLineStyle == null);
> > + return (borderLineStyle == null) || (borderLineStyle == BorderStyle.NONE);
> > }
Why use drawTopBorder? I don't have the code in front of me right now, but
probably to be consistent with the others. For interior borders there is an
issue with page breaks, if you do not have both the top border on one cell, and
the bottom border of the cell above it, either the line at the bottom of the
page or the line at the top of the next page will be missing from the printed
document. This would not apply to top borders, but I suspect that all the color
methods just use the draw method to ensure any special edge cases like that are
handled.
What to do about setting color for NONE border? I guess that the least
surprising option would be to add a THIN border.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #25 from Javen O'Neal <on...@apache.org> ---
(In reply to Mark Murphy from comment #20)
> Created attachment 33684 [details]
> Patch file generated via Ant script
Applied in r1747851 to branches/ss_border_property_template
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border Drawing utility that does not create
unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Mark Murphy <jm...@yahoo.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Summary|[In progress] Border |[patch] Border Drawing
|Drawing utility that does |utility that does not
|not create unnecessary |create unnecessary styles
|styles |
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
--- Comment #32 from Javen O'Neal <on...@apache.org> ---
I can roll back the changes for BorderPropertyTemplate and add
CellStyleTemplate. I like that idea better. Maybe rewrite RegionUtil as well if
you want a class that operates on a cell range rather than a single cell.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border utility to set cell styles around a range
of cells
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Bug 58787 depends on bug 59264, which changed state.
Bug 59264 Summary: Unify interface for setting cell border line styles using BorderStyle enum instead of short code
https://bz.apache.org/bugzilla/show_bug.cgi?id=59264
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 58787] [patch] Border Drawing utility that does not create
unnecessary styles
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Mark Murphy <jm...@yahoo.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Version|3.14-dev |3.15-dev
--- Comment #21 from Mark Murphy <jm...@yahoo.com> ---
Any thoughts about the most recent patch? I am trying to set up my environment
so that I have one change per project. So, I noticed that puts a wonky name in
the eclipse project file. Probably not optimal. How do you deal with this?
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org