Bug 60902 - .cloneCellStyle as a Workbook method
Summary: .cloneCellStyle as a Workbook method
Status: NEW
Alias: None
Product: POI
Classification: Unclassified
Component: XSSF (show other bugs)
Version: 3.16-dev
Hardware: PC All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-22 14:15 UTC by dollinger.florian
Modified: 2017-08-19 16:39 UTC (History)
1 user (show)



Attachments
version 1, adds a new cloneCellStyle method to the workbook (4.67 KB, patch)
2017-03-22 14:16 UTC, dollinger.florian
Details | Diff
version 2, let the user decide if the method should search the existing styles (5.71 KB, patch)
2017-03-23 10:12 UTC, dollinger.florian
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description dollinger.florian 2017-03-22 14:15:12 UTC
Since cloneCellStyle is a method for a CellStyle, you always have to create a new CellStyle in order to clone a Style from another workbook, even if the style already exists - this may lead to a lot of redundant styles.

I suggest a new method on the XSSFWorkbook called cloneCellStyle:

/**
* Searches in the current Workbook for an equivalent CellStyle to the given one (from another Workbook).
* If none exists, it will be created.
*
* @param src The CellStyle that is going to be cloned
* @return CellStyle The created or looked up CellStyle in the current Workbook
*/
public XSSFCellStyle cloneCellStyle(XSSFCellStyle src) {
  // ... TODO ...
}

I will attach a first (already working) patch.
Comment 1 dollinger.florian 2017-03-22 14:16:20 UTC
Created attachment 34867 [details]
version 1, adds a new cloneCellStyle method to the workbook
Comment 2 Nick Burch 2017-03-22 14:49:36 UTC
The HSSF way is to use https://poi.apache.org/apidocs/org/apache/poi/hssf/usermodel/HSSFOptimiser.html afterwards to tidy up duplicates. Might we be better off doing something similar for xssf instead?
Comment 3 dollinger.florian 2017-03-22 16:03:20 UTC
Ah okay! Hum, i don't know - personally i would prefer a method which is "intelligent" enough to check itself if a new object already exists, otherwise it feels a bit like "making a mess and tidy up afterwards".

The only reason I can think of (why the optimizer method is preferable) is for performance.

Any other thoughts?
Comment 4 Javen O'Neal 2017-03-23 06:20:44 UTC
> The only reason I can think of (why the optimizer method is preferable) is
> for performance.
Performance is a big reason. It's always tricky as a library developer to make choices that impact our users. For example, should I be choosing whether we should cache values at the expense of memory in order to make code run faster for certain cases (and perhaps slower for others)? In general I prefer to give the choice to the user, but try to do so without presenting an overwhelming API. HSSFOptimizer is probably a O(nĀ²) operation, which could be done at the user's convenience, rather than executing O(n) comparisons every time a style is changed.

CellUtil.setCellStyleProperties attempts to find a matching style before creating a new style. Maybe you could implement something along those lines, so a user can choose to eat the cost when modifying a cell style or when saving.
Comment 5 dollinger.florian 2017-03-23 06:35:29 UTC
I see, what's about another parameter "clean":

public XSSFCellStyle cloneCellStyle(XSSFCellStyle src, boolean clean) {...}

* @param src The CellStyle that is going to be cloned
* @param clean If set to true, this method attempts to find a already existing CellStyle in the current workbook which has the same properties as the given one.
* @return CellStyle The created or looked up CellStyle in the current Workbook
Comment 6 dollinger.florian 2017-03-23 06:38:15 UTC
The main problem with CellUtil.setCellStyleProperties is that it is nearly useless (at least in my case) for XSSF workbooks - since only indexed colors are copied, no fonts, and so on (see bug 60895).
I will try to fix that someday but for now I think  Workbook.cloneCellStyle() is not a bad idea?!
Comment 7 dollinger.florian 2017-03-23 10:12:02 UTC
Created attachment 34869 [details]
version 2, let the user decide if the method should search the existing styles
Comment 8 Mark Murphy 2017-03-23 11:15:57 UTC
(In reply to dollinger.florian from comment #6)
> The main problem with CellUtil.setCellStyleProperties is that it is nearly
> useless (at least in my case) for XSSF workbooks - since only indexed colors
> are copied, no fonts, and so on (see bug 60895).
> I will try to fix that someday but for now I think 
> Workbook.cloneCellStyle() is not a bad idea?!

setCellStyleProperties() doesn't copy anything. You have to put the properties you want in the map. that can include fonts if you choose. It does not handle colors other than indexed colors though, but that is due to the fact that HSSF only supports indexed colors, and CellUtil.setCellStyleProperties is part of the converged interface. POI would be better served if you found a way to handle the XSSF color differences in the converged interface rather than create a whole new set of methods. Probably need a way to convert themed colors to indexed colors if the user tries to use themed colors with an HSSF sheet. Other than themed colors, CellUtil.setStyleProperties() supports all the CellStyle attributes for XSSF.
Comment 9 Nick Burch 2017-03-23 11:47:12 UTC
(In reply to Mark Murphy from comment #8)
> setCellStyleProperties() doesn't copy anything. You have to put the
> properties you want in the map. that can include fonts if you choose. It
> does not handle colors other than indexed colors though, but that is due to
> the fact that HSSF only supports indexed colors, and
> CellUtil.setCellStyleProperties is part of the converged interface.

There's Common SS support for XSSF-style colours though - they're used in both XSSF and in some newer bits of HSSF like newer Conditional Formatting. The class you'd want to use is ExtendedColor - 
https://poi.apache.org/apidocs/org/apache/poi/ss/usermodel/ExtendedColor.html

Not sure if you could easily write totally generic code though, eg what happens if you give a custom red colour to HSSF? Error? Do what Excel does and down-mix to the nearest colour that XLS knows about?
Comment 10 Mark Murphy 2017-03-23 12:36:22 UTC
(In reply to Nick Burch from comment #9)

> Not sure if you could easily write totally generic code though, eg what
> happens if you give a custom red colour to HSSF? Error? Do what Excel does
> and down-mix to the nearest colour that XLS knows about?

That is one way to deal with it, but if there is nothing close, you could also use custom slots to add the color to the XLS pallet. I wonder if color matching is easy in Java. Or, there should be a suitable color matching algorithm that we can use.
Comment 11 dollinger.florian 2017-03-23 20:01:05 UTC
(In reply to Mark Murphy from comment #8)
> setCellStyleProperties() doesn't copy anything. You have to put the
> properties you want in the map. that can include fonts if you choose. It
> does not handle colors other than indexed colors though, but that is due to
> the fact that HSSF only supports indexed colors, and
> CellUtil.setCellStyleProperties is part of the converged interface. POI
> would be better served if you found a way to handle the XSSF color
> differences in the converged interface rather than create a whole new set of
> methods. Probably need a way to convert themed colors to indexed colors if
> the user tries to use themed colors with an HSSF sheet. Other than themed
> colors, CellUtil.setStyleProperties() supports all the CellStyle attributes
> for XSSF.

Yes, sorry for the inaccuracy.

1) .setCellStyleProperties() is not the best choice for my use case. I want to copy cells including their properties from one workbook to another (since I am developing a table merging tool for my company).  I tried to use .cloneCellStyle() at first, but that somehow crashed the conditional formatting in the destination workbook/worksheet (bug 60845).

2) I also tried .setCellStyleProperties(), but I gave it up (for now) since colors are not yet fully supported, and fonts have to be copied manually, and also because of some other problems (bug 60895)

3) I then fixed the issue in 1), just to realize that, using .cloneCellStyle(), I will end up with many many redundant cellStyles. That's why i thought that a "global" .cloneCellStyle() is not a bad idea (bug 60902, this one)


But, yes - It is a good idea to extend .setCellStyleProperties() to XSSF, it would be also nice to have something like .getCellStyleProperties() - but thats a whole bunch of work and that's something I cannot do in the next weeks or months.

But what speaks against the solution above?
Comment 12 Nick Burch 2017-03-23 20:18:45 UTC
For your use-case, copying from one workbook to another, I'd probably lean towards maintaining a Map<CellStyle,CellStyle> in your code + calling cloneStyleFrom for any you don't have. You'd then grab the cell style from the source workbook, grab the existing cloned style in the destination workbook if it was there, or clone+store+use if not. (Map is source workbook style to destination workbook style)

Sounds like we've got some bugs to solve in the existing cloneStyleFrom before that approach could work though
Comment 13 dollinger.florian 2017-03-24 06:41:01 UTC
(In reply to Nick Burch from comment #12)
> For your use-case, copying from one workbook to another, I'd probably lean
> towards maintaining a Map<CellStyle,CellStyle> in your code + calling
> cloneStyleFrom for any you don't have. You'd then grab the cell style from
> the source workbook, grab the existing cloned style in the destination
> workbook if it was there, or clone+store+use if not. (Map is source workbook
> style to destination workbook style)
> 
> Sounds like we've got some bugs to solve in the existing cloneStyleFrom
> before that approach could work though

I will try that, thank you!

But again, what's the position of the developer team?
If I supply a fully functional and well integrated .cloneCellStyle() on workbooks, will you accept it?