| Summary: | [patch] Need to set multiple CellStyle properties at once | ||
|---|---|---|---|
| Product: | POI | Reporter: | Mark Murphy <jmarkmurph> |
| Component: | SS Common | Assignee: | POI Developers List <dev> |
| Status: | RESOLVED FIXED | ||
| Severity: | enhancement | Keywords: | PatchAvailable |
| Priority: | P2 | ||
| Version: | 3.13-FINAL | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | All | ||
| Bug Depends on: | |||
| Bug Blocks: | 58787 | ||
| Attachments: |
Patch file
Patch file with test cases Quick Guide Update This is the patch file for the quick guide |
||
Thanks for the patch. Could you add a unit test and/or write some example usage code? Created attachment 33288 [details]
Patch file with test cases
generated from Ant
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? (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. 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! 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(). Added in r1722607 with a few minor javadoc modifications, and changed the new method to setCellStyleProperties(Cell, Map<String, Object>). 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 (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? I can do that add examples and quick guide paragraph. 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.
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 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.
(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. |
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.