Bug 58633 - [patch] Need to set multiple CellStyle properties at once
Summary: [patch] Need to set multiple CellStyle properties at once
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: SS Common (show other bugs)
Version: 3.13-FINAL
Hardware: PC All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords: PatchAvailable
Depends on:
Blocks: 58787
  Show dependency tree
 
Reported: 2015-11-22 02:00 UTC by Mark Murphy
Modified: 2016-01-16 08:07 UTC (History)
0 users



Attachments
Patch file (2.83 KB, text/plain)
2015-11-22 02:00 UTC, Mark Murphy
Details
Patch file with test cases (1.98 KB, application/gzip)
2015-11-23 23:05 UTC, Mark Murphy
Details
Quick Guide Update (107.85 KB, text/html)
2016-01-16 01:59 UTC, Mark Murphy
Details
This is the patch file for the quick guide (7.00 KB, text/plain)
2016-01-16 06:07 UTC, Mark Murphy
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Murphy 2015-11-22 02:00:18 UTC
Created attachment 33286 [details]
Patch file

Setting a single CellStyle property at a time using setCellStyleProperty causes a search through all the CellStyles for each property set, and creates a new CellStyle if a match is not found, even if that specific style will never be used. When more than one property needs to be set, for example when drawing borders, it would be more efficient both in the number of styles created, and in performance, to set them all in one shot.

I have included a patch to org.apache.poi.ss.util.CellUtil which adds a new method setCellStyleProperty(Cell, Workbook, Map<String, Object>) that adds a group of properties to the cell style in one shot. I also modified the existing method to call the new method with a map containing a single property. No test cases yet, just wanted to make sure I am on the right track.
Comment 1 Javen O'Neal 2015-11-22 03:54:31 UTC
Thanks for the patch. Could you add a unit test and/or write some example usage code?
Comment 2 Mark Murphy 2015-11-23 23:05:46 UTC
Created attachment 33288 [details]
Patch file with test cases

generated from Ant
Comment 3 Mark Murphy 2015-11-30 15:50:01 UTC
I am finished with this patch, do I need to set the status to resolved, or does someone else do that when it is added to the project?
Comment 4 Javen O'Neal 2015-11-30 18:10:15 UTC
(In reply to Mark Murphy from comment #3)
> I am finished with this patch, do I need to set the status to resolved, or
> does someone else do that when it is added to the project?

tldr: leave it open. A committer will close the bug once it's been committed and documented.

Here's the general workflow for the Apache POI project:

Indicate that your patches are finished and ready for inclusion using [PATCH] in the bug title and PatchAvailable in the keywords field (helps the committers find the bug more easily). (Already done)

A committer will review your changes, identify any problems (doesn't compile, fails existing unit tests, needs new unit tests, missing javadocs) and let you know if any changes need done. Depending on how much work needs done and how busy the committer is, he or she may ask you to make the changes or will make the changes his/herself.

Only once the bug is deemed duplicate/invalid/won't fix or the suggested changes are committed and the appropriate documentation is updated do we close the bug.

It may take us a while to getting around to reviewing and committing this, so patience is key. Some bugs take a few years to get committed--please don't wait that long without politely bugging us. In general, the simpler and shorter the change, the less time it takes to be included.
Comment 5 Nick Burch 2015-11-30 22:44:44 UTC
I'd suggest using Collections.singletonMap for the string,object setter in CellUtils

Tests wise, might be good to cover one or two more cases, especially if there isn't a lot of unit testing currently for that class

An example around using CellUtil for this sort of thing, for the examples package, might be good too!
Comment 6 Javen O'Neal 2016-01-01 22:55:13 UTC
This is a nitpick from setCellStyleProperty(Cell, Workbook, String, Object), but how should this function behave if the cell does not belong to the workbook? If this is undefined, then this function should either throw an exception or not have a workbook argument.

If someone can explain a usecase where cell does not belong to workbook, and can describe what the desired behavior should be, I'll leave this as is. Otherwise, I think we should deprecate setCellStyleProperty(Cell, Workbook, String, Object) and replace with setCellStyleProperty(Cell, String, Object), and determine the workbook object inside the function with cell.getSheet().getWorkbook().
Comment 7 Javen O'Neal 2016-01-02 04:30:33 UTC
Added in r1722607 with a few minor javadoc modifications, and changed the new method to setCellStyleProperties(Cell, Map<String, Object>).
Comment 8 Javen O'Neal 2016-01-02 04:44:18 UTC
After setCellStyleProperty or setCellStyleProperties, if any cell styles are no longer used in the workbook, there needs to be a method to remove cell styles from the workbook.

I mentioned this (commented out) in the Javadocs for setCellStyleProperties [1]. If POI currently supports this, the Javadocs should refer to this. If not, the functions should be created. For example, WorkbookUtil.removeUnusedCellStyles(Workbook wb) or WorkbookUtil.removeStyleFromWorkbookIfUnused(CellStyle style, Workbook wb). Not sure if this belongs in CellUtil, WorkbookUtil, CellStyleUtil, or as methods on Workbook or the styles source (StylesTable for XSSF, not sure for HSSF).

I decided against having this cleanup done in setCellStyleProperty/Properties assuming that the cleanup would run in linear time with respect to number of cells in a workbook.

[1] https://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/ss/util/CellUtil.java?view=markup&pathrev=1722607#l187
Comment 9 Javen O'Neal 2016-01-02 11:20:48 UTC
(In reply to Nick Burch from comment #5)
> Tests wise, might be good to cover one or two more cases, especially if
> there isn't a lot of unit testing currently for that class

Is r1722618 what you had in mind here, or were you thinking about more unit tests for setCellStyleProperty/Properties?

> An example around using CellUtil for this sort of thing, for the examples 
> package, might be good too!

We still need to add this to examples package and/or quick guide. I think CellUtil.setCellStyleProperties is a secret to most people--it was to me until I saw this bug. I added links to this function in Cell.setCellStyle javadoc in hopes to direct more here. This utility function minimizes the need for a style optimizer, which consolidates styles, like HSSFStyleOptimizer (currently nothing available for XSSF), since duplicate styles are avoided in the first place.

Mark, would you like to take the task of writing example code in the examples package and a paragraph showing how to use this in the quick-guide?
Comment 10 Mark Murphy 2016-01-02 23:53:03 UTC
I can do that add examples and quick guide paragraph.
Comment 11 Mark Murphy 2016-01-16 01:59:09 UTC
Created attachment 33450 [details]
Quick Guide Update

This is the updated Quick Guide page which adds examples for drawing a red medium weight border around cell B2, and also around and inside the range D4:F6.
Comment 12 Javen O'Neal 2016-01-16 04:52:14 UTC
Did you by any chance happen to edit the quick-guide.xml source [1], or did you make your additions to quick-guide.html [2,3]? We use Apache Forrest to convert the xml to the html site.

If not, it's no big deal to transcribe your changes to the XML file. Just a little more work for both of us :)

[1] XML Source: http://svn.apache.org/viewvc/poi/site/src/documentation/content/xdocs/spreadsheet/quick-guide.xml?view=log
[2] Generated HTML: http://svn.apache.org/viewvc/poi/site/publish/spreadsheet/quick-guide.html?view=log
[3] Site: http://poi.apache.org/spreadsheet/quick-guide.html
Comment 13 Mark Murphy 2016-01-16 06:07:51 UTC
Created attachment 33453 [details]
This is the patch file for the quick guide

Not sure if I am doing this correctly, but I have too many changes in my project to just create a patch with Ant. I transcribed my changes from the HTML to the XML file, and created a patch file for just that one file.
Comment 14 Javen O'Neal 2016-01-16 08:07:42 UTC
(In reply to Mark Murphy from comment #13)
> Created attachment 33453 [details]
> This is the patch file for the quick guide

Thanks for your code and documentation contributions! Spreadsheet quick guide updated in r1724929.

I left out the whitespace changes in other sections from attachment 33453 [details]. I'm not picky about inconsistent whitespace in the documentation, unlike code.

Changelog updated in r1724933.

> Not sure if I am doing this correctly, but I have too many changes in my project 
> to just create a patch with Ant. I transcribed my changes from the HTML to the 
> XML file, and created a patch file for just that one file.

"svn diff" is probably easiest here.
I have dozens of working copies of POI on my computer--each for a bug. This works when the changes are unrelated or independent, but it sounds like the changes you have are dependent on each other.

I think that is everything for this bug. Please reopen if I forgot something.