[Bug 60902] New: .cloneCellStyle as a Workbook method

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug 60902] New: .cloneCellStyle as a Workbook method

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=60902

            Bug ID: 60902
           Summary: .cloneCellStyle as a Workbook method
           Product: POI
           Version: 3.16-dev
          Hardware: PC
            Status: NEW
          Severity: normal
          Priority: P2
         Component: XSSF
          Assignee: [hidden email]
          Reporter: [hidden email]
  Target Milestone: ---

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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 60902] .cloneCellStyle as a Workbook method

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=60902

--- Comment #1 from [hidden email] ---
Created attachment 34867
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=34867&action=edit
version 1, adds a new cloneCellStyle method to the workbook

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 60902] .cloneCellStyle as a Workbook method

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=60902

Nick Burch <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 OS|                            |All

--- Comment #2 from Nick Burch <[hidden email]> ---
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?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 60902] .cloneCellStyle as a Workbook method

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=60902

[hidden email] changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--- Comment #3 from [hidden email] ---
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?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 60902] .cloneCellStyle as a Workbook method

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=60902

--- Comment #4 from Javen O'Neal <[hidden email]> ---
> 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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 60902] .cloneCellStyle as a Workbook method

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=60902

--- Comment #5 from [hidden email] ---
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

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 60902] .cloneCellStyle as a Workbook method

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=60902

--- Comment #6 from [hidden email] ---
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?!

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 60902] .cloneCellStyle as a Workbook method

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=60902

[hidden email] changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #34867|0                           |1
        is obsolete|                            |

--- Comment #7 from [hidden email] ---
Created attachment 34869
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=34869&action=edit
version 2, let the user decide if the method should search the existing styles

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 60902] .cloneCellStyle as a Workbook method

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=60902

--- Comment #8 from Mark Murphy <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 60902] .cloneCellStyle as a Workbook method

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=60902

--- Comment #9 from Nick Burch <[hidden email]> ---
(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?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 60902] .cloneCellStyle as a Workbook method

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=60902

--- Comment #10 from Mark Murphy <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 60902] .cloneCellStyle as a Workbook method

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=60902

--- Comment #11 from [hidden email] ---
(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?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 60902] .cloneCellStyle as a Workbook method

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=60902

--- Comment #12 from Nick Burch <[hidden email]> ---
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

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 60902] .cloneCellStyle as a Workbook method

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=60902

--- Comment #13 from [hidden email] ---
(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?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 60902] .cloneCellStyle as a Workbook method

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=60902

Dominik Stadler <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]