Bug 58787 - [patch] Border utility to set cell styles around a range of cells
Summary: [patch] Border utility to set cell styles around a range of cells
Status: NEW
Alias: None
Product: POI
Classification: Unclassified
Component: SS Common (show other bugs)
Version: 3.15-dev
Hardware: PC All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords: PatchAvailable
Depends on: 58633 58879 59264
Blocks: 54593 55555
  Show dependency tree
 
Reported: 2016-01-01 16:43 UTC by Mark Murphy
Modified: 2019-01-06 13:01 UTC (History)
0 users



Attachments
Patch file generated via Ant script (6.18 KB, application/gzip)
2016-01-01 16:43 UTC, Mark Murphy
Details
Updated patch file containing quick guide modifications (4.90 KB, application/gzip)
2016-01-17 01:30 UTC, Mark Murphy
Details
Rebased and updated again (4.20 KB, application/gzip)
2016-01-17 01:57 UTC, Mark Murphy
Details
This is a SVN diff of the quick-guide.xml (4.11 KB, text/plain)
2016-01-17 05:25 UTC, Mark Murphy
Details
Completed ant patch including tests, examples, and documentation (7.43 KB, patch)
2016-03-14 02:05 UTC, Mark Murphy
Details | Diff
Patch file generated via Ant script (7.55 KB, patch)
2016-03-19 17:59 UTC, Mark Murphy
Details | Diff
Patch file generated by ant (7.55 KB, patch)
2016-06-14 01:49 UTC, Mark Murphy
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Murphy 2016-01-01 16:43:04 UTC
Created attachment 33391 [details]
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.
Comment 1 Mark Murphy 2016-01-01 16:55:59 UTC
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.
Comment 2 Javen O'Neal 2016-01-16 23:43:28 UTC
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.
Comment 3 Mark Murphy 2016-01-17 01:30:21 UTC
Created attachment 33455 [details]
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.
Comment 4 Mark Murphy 2016-01-17 01:41:12 UTC
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.
Comment 5 Mark Murphy 2016-01-17 01:57:53 UTC
Created attachment 33456 [details]
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.
Comment 6 Mark Murphy 2016-01-17 05:25:32 UTC
Created attachment 33457 [details]
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.
Comment 7 Mark Murphy 2016-01-17 14:42:37 UTC
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.
Comment 8 Javen O'Neal 2016-01-17 17:51:55 UTC
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.
Comment 9 Mark Murphy 2016-01-17 21:17:09 UTC
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.
Comment 10 Javen O'Neal 2016-01-22 05:41:19 UTC
Mark,

Did you take a look at RegionUtil to see if meets your needs? See Bug 54593.
Comment 11 Mark Murphy 2016-01-22 20:15:13 UTC
(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.
Comment 12 Javen O'Neal 2016-01-24 01:13:13 UTC
(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));
Comment 13 Mark Murphy 2016-03-12 15:04:50 UTC
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.
Comment 14 Nick Burch 2016-03-12 15:18:08 UTC
(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
Comment 15 Mark Murphy 2016-03-14 02:05:43 UTC
Created attachment 33667 [details]
Completed ant patch including tests, examples, and documentation
Comment 16 Mark Murphy 2016-03-14 02:08:16 UTC
(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.
Comment 17 Javen O'Neal 2016-03-14 04:21:26 UTC
(In reply to Mark Murphy from comment #15)
> Created attachment 33667 [details]
> Completed ant patch including tests, examples, and documentation

Attachment 33667 [details] 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
Comment 18 Mark Murphy 2016-03-14 11:43:25 UTC
(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.
Comment 19 Javen O'Neal 2016-03-14 16:24:30 UTC
(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.
Comment 20 Mark Murphy 2016-03-19 17:59:47 UTC
Created attachment 33684 [details]
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.
Comment 21 Mark Murphy 2016-03-24 03:32:14 UTC
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?
Comment 22 Javen O'Neal 2016-04-02 04:34:31 UTC
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 [details] 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!
Comment 23 Javen O'Neal 2016-04-02 20:25:45 UTC
(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.
Comment 24 Javen O'Neal 2016-04-06 04:10:33 UTC
Update this documentation as well: https://poi.apache.org/faq.html#faq-N100E3
Comment 25 Javen O'Neal 2016-06-11 06:48:28 UTC
(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
Comment 26 Javen O'Neal 2016-06-11 12:26:01 UTC
(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).
Comment 27 Javen O'Neal 2016-06-12 23:05:30 UTC
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);
>  }
Comment 28 Javen O'Neal 2016-06-13 01:05:05 UTC
(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.
Comment 29 Javen O'Neal 2016-06-13 01:31:02 UTC
(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.
Comment 30 Mark Murphy 2016-06-13 11:48:28 UTC
(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.
Comment 31 Mark Murphy 2016-06-13 12:00:53 UTC
(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.
Comment 32 Javen O'Neal 2016-06-13 15:52:59 UTC
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.
Comment 33 Mark Murphy 2016-06-13 16:12:07 UTC
(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.
Comment 34 Javen O'Neal 2016-06-13 18:16:34 UTC
(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.
Comment 35 Mark Murphy 2016-06-13 23:10:53 UTC
(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.
Comment 36 Mark Murphy 2016-06-14 00:39:02 UTC
(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.
Comment 37 Mark Murphy 2016-06-14 01:49:43 UTC
Created attachment 33948 [details]
Patch file generated by ant

This leaves 2. Additional unit tests to verify reduced style creation unfinished
Comment 38 Mark Murphy 2016-08-01 00:06:06 UTC
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.
Comment 39 Javen O'Neal 2016-08-01 04:16:38 UTC
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.
Comment 40 Javen O'Neal 2016-08-01 04:30:20 UTC
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
Comment 41 Mark Murphy 2016-08-01 04:36:09 UTC
(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.
Comment 42 Javen O'Neal 2016-08-01 04:55:00 UTC
(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.
Comment 43 Mark Murphy 2016-10-05 02:33:18 UTC
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.