Bug 60845 - [PATCH] copied cell style and CF
Summary: [PATCH] copied cell style and CF
Status: NEW
Alias: None
Product: POI
Classification: Unclassified
Component: XSSF (show other bugs)
Version: 3.16-dev
Hardware: PC All
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2017-03-10 23:47 UTC by dollinger.florian
Modified: 2018-12-30 16:41 UTC (History)
1 user (show)



Attachments
The resulting file, which is not correctly displayed in Excel, but in LibreOffice (6.38 KB, application/vnd.openxmlformats-officedocument.spreadsheetml.sheet)
2017-03-11 10:06 UTC, dollinger.florian
Details
corrupted styles.xml, which causes the described behaviour (2.56 KB, text/xml)
2017-03-11 11:02 UTC, dollinger.florian
Details
working styles.xml (1.93 KB, text/xml)
2017-03-11 11:03 UTC, dollinger.florian
Details
a first idea how to patch it (52.65 KB, patch)
2017-03-15 16:48 UTC, dollinger.florian
Details | Diff
patched equals function (6.45 KB, text/x-csrc)
2017-03-17 02:11 UTC, dollinger.florian
Details
patched equals function (5.44 KB, text/x-csrc)
2017-03-17 02:11 UTC, dollinger.florian
Details
patched equals function (21.84 KB, text/x-csrc)
2017-03-17 02:12 UTC, dollinger.florian
Details
patched equals functions for XSSFFont, XSSFCellFill and XSSFCellBorder (8.66 KB, application/zip)
2017-03-17 02:18 UTC, dollinger.florian
Details
patch as a diff file (git) (3.40 KB, patch)
2017-03-19 20:56 UTC, dollinger.florian
Details | Diff
better version (5.09 KB, patch)
2017-03-20 13:07 UTC, dollinger.florian
Details | Diff
corrected test cases (5.34 KB, patch)
2017-03-20 13:07 UTC, dollinger.florian
Details | Diff
version 3: non-forced font registration (17.77 KB, patch)
2017-03-20 13:48 UTC, dollinger.florian
Details | Diff
version 3: non-forced font registration (17.77 KB, patch)
2017-03-20 13:49 UTC, dollinger.florian
Details | Diff
corrected imports (17.71 KB, patch)
2017-03-20 13:55 UTC, dollinger.florian
Details | Diff
corrected imports (17.71 KB, patch)
2017-03-20 13:56 UTC, dollinger.florian
Details | Diff
testcases (11.50 KB, patch)
2017-03-20 13:57 UTC, dollinger.florian
Details | Diff
java source (17.71 KB, patch)
2017-03-20 13:58 UTC, dollinger.florian
Details | Diff
src patch, minor changes (17.68 KB, patch)
2017-03-21 06:57 UTC, dollinger.florian
Details | Diff
src, version 6: improved equals method (6.15 KB, patch)
2017-03-23 11:17 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-10 23:47:57 UTC
Two equal XLSX Files
Both are using conditional Formatting of the Background Color on e.g. Column 1
Now, copy one Row of File1 to File2 using:

for (int i = 0; i < sourceRow.getLastCellNum(); i++) {

    XSSFCellStyle newStyle = destSheet.getWorkbook().createCellStyle();
    newStyle.cloneStyleFrom(oldCell.getCellStyle());      
    newCell.setCellStyle(newStyle);


    // Set the cell data type
    newCell.setCellType(oldCell.getCellType());

    // Set the cell data value
    switch (oldCell.getCellType()) {
    case Cell.CELL_TYPE_BLANK:
        newCell.setCellValue(oldCell.getStringCellValue());
        break;
    case Cell.CELL_TYPE_BOOLEAN:
        newCell.setCellValue(oldCell.getBooleanCellValue());
        break;
    case Cell.CELL_TYPE_ERROR:
        newCell.setCellErrorValue(oldCell.getErrorCellValue());
        break;
    case Cell.CELL_TYPE_FORMULA:
        newCell.setCellFormula(oldCell.getCellFormula());
        break;
    case Cell.CELL_TYPE_NUMERIC:
        newCell.setCellValue(oldCell.getNumericCellValue());
        break;
    case Cell.CELL_TYPE_STRING:
        newCell.setCellValue(oldCell.getRichStringCellValue());
        break;
    }
}

The Row is copied, the values, the size, also the background colors and so on.
But the conditional formatting does not longer work for the
copied rows. "Not working" means: the cells background is white, even if the value is the right one.

If you use the cell formatting options (in excel) and simply click on "No Color" for Filling (which is already selected), and confirm it with OK, it works.
Comment 1 dollinger.florian 2017-03-11 01:18:29 UTC
there is a workaround, if you set the FillPattern by hand via

bla.setFillPattern(FillPatternType.SOLID_FOREGROUND);

for the cells which are in the range of some CF-rule, then the conditional formatting works. the problem then is, that the cells background becomes BLACK if the CF-rule(s) do not fire.
Comment 2 dollinger.florian 2017-03-11 10:06:54 UTC
Created attachment 34815 [details]
The resulting file, which is not correctly displayed in Excel, but in LibreOffice

The same problem holds for

Map<String, Object> prop = new HashMap<>();
prop.put(CellUtil.ROTATION, oldCell.getCellStyle().getRotation());
CellUtil.setCellStyleProperties(newCell, prop);

therefore it is not a cloneStyle issue I guess.
Comment 3 dollinger.florian 2017-03-11 11:02:52 UTC
Created attachment 34816 [details]
corrupted styles.xml, which causes the described behaviour

i was able to find out which file in the .zip filestructure causes the problem, it is the styles.xml, i will also add another one - which was created by excel on the same basis - the latter is working
Comment 4 dollinger.florian 2017-03-11 11:03:16 UTC
Created attachment 34817 [details]
working styles.xml
Comment 5 dollinger.florian 2017-03-12 19:11:01 UTC
Okay, the problem is the following:

Excel applies conditional formatting only to two sorts of cells:
1) those who have the default fillId=0
2) those who have a patternType like "solid"

But POI creates a new one like:

<patternFill>
<fgColor indexed="64"/>
<bgColor indexed="64"/>
</patternFill>

which does not fit any of those two sorts.
Comment 7 dollinger.florian 2017-03-15 10:40:21 UTC
Thanks for all your replies!
I finally found out where the problem is:

1) Excel shows conditional formatting if, and only if, the cell has fillId=0 (the default NONE one) OR if the fillStylePattern != NONE.
Thats a bug in Excel, not in POI.

2) The problem is, that Excel is de facto "Standard".
POI calls like this to set the new Style: setCellStyleProperties >> setFormatProperties >> setFillPattern

Here is what setFillPattern looks like:

Java Code:
public void setFillPattern(FillPatternType pattern) {
        CTFill ct = getCTFill();
        CTPatternFill ctptrn = ct.isSetPatternFill() ? ct.getPatternFill() : ct.addNewPatternFill();
 
        if (pattern == FillPatternType.NO_FILL && ctptrn.isSetPatternType()) {
                ctptrn.unsetPatternType();
        } else {
                ctptrn.setPatternType(STPatternType.Enum.forInt(pattern.getCode() + 1));
        }
 
        addFill(ct);
}


It (POI) does simply not check if the FillStyle does already exist, it always adds a new one. And in general, this is then not fullfilling condtions in 1)
Comment 8 dollinger.florian 2017-03-15 16:48:57 UTC
Created attachment 34835 [details]
a first idea how to patch it

A first approach to fix the issue, works for now but is still in progress. The idea is to search the styles.xml for already existing fills before adding a new one with the same properties.
Comment 9 dollinger.florian 2017-03-16 23:40:44 UTC
Okay, I again looked at the code and problem I described at first is caused by the following thing:

cloneStyleFrom adds the styles from the original to the destination like that:

---

CTFill fill = CTFill.Factory.parse(
    src.getCTFill().toString(), DEFAULT_XML_OPTIONS
);
                  
addFill(fill);

---

I dont't know why, but that results in the following CTFill-String:

<xml-fragment xmlns:main="http://schemas.openxmlformats.org/spreadsheetml/2006/main" xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" xmlns:x14ac="http://schemas.microsoft.com/office/spreadsheetml/2009/9/ac" xmlns:x16r2="http://schemas.microsoft.com/office/spreadsheetml/2015/02/main">
  <main:patternFill patternType="none"/>
</xml-fragment>

which is definitely not the same as

<patternFill patternType="none" xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main" xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" xmlns:x14ac="http://schemas.microsoft.com/office/spreadsheetml/2009/9/ac" xmlns:x16r2="http://schemas.microsoft.com/office/spreadsheetml/2015/02/main"/>

at least not syntactically.

---

addFill (StylesTable.java) now looks if the new FillStyle is already in the Set by fills.indexOf(newFill).

indexOf is based on equals of XSSFCellFill, which does a string-Comparison!

---

One way to patch that would be to change the equals function to a semantic comparison like:

public boolean equals(Object o) {
  if (!(o instanceof XSSFCellFill)) return false;

  XSSFCellFill cf = (XSSFCellFill) o;
        
        
  return (
    this.getFillBackgroundColor() == cf.getFillBackgroundColor()
    && this.getFillForegroundColor() == cf.getFillForegroundColor()
    && this.getPatternType() == cf.getPatternType()
  );
}

The downside is the deteriorated performance I guess. And you have to do the same on XSSFFont.java and XSSFCellBorder.java (and maybe more).

---

Another way is to convert the "fragments" into normal XML inside cloneStyleFrom(),
I am still looking for a way to do that reliable (using xmlbeans).
Comment 10 dollinger.florian 2017-03-17 00:21:02 UTC
The XML-Fragment results from the copy() function in CTFill!


 private CTFill getCTFill(){
  CTFill ct;
  // bug 56295: handle missing applyFill attribute as "true" because Excel does as well
  if(!_cellXf.isSetApplyFill() || _cellXf.getApplyFill()) {
    int fillIndex = (int)_cellXf.getFillId();
    XSSFCellFill cf = _stylesSource.getFillAt(fillIndex);
            
    ct = (CTFill)cf.getCTFill().copy();
    // ^^^^^^
            
  } else {
      ct = CTFill.Factory.newInstance();
  }
  return ct;
}
Comment 11 dollinger.florian 2017-03-17 02:11:26 UTC
Created attachment 34837 [details]
patched equals function
Comment 12 dollinger.florian 2017-03-17 02:11:51 UTC
Created attachment 34838 [details]
patched equals function
Comment 13 dollinger.florian 2017-03-17 02:12:47 UTC
Created attachment 34839 [details]
patched equals function
Comment 14 dollinger.florian 2017-03-17 02:18:03 UTC
Created attachment 34840 [details]
patched equals functions for XSSFFont, XSSFCellFill and XSSFCellBorder

This patch changes the equals functions to a semantic one, rather than the former syntactic one
Comment 15 Dominik Stadler 2017-03-19 07:43:03 UTC
Please note that providing a diff instead of the whole source of the files would be better as the whole files can quickly become outdated due to other changes to the codebase and thus will make applying the changes more and more difficult over time. 

See http://poi.apache.org/guidelines.html#SubmittingPatches for some automated ways of creating the patch-files.

Also ideally some additional unit-tests accompany a patch so the changes are verified and stay in place correctly through future changes to the code.
Comment 16 dollinger.florian 2017-03-19 20:56:15 UTC
Created attachment 34846 [details]
patch as a diff file (git)

Hey Dominik,
thanks for the advice, i added the patch diff file - i am not very familiar with jUnit, but I will give it a try. Btw.: Should the current repository jUnit tests run without any errors?
Comment 17 dollinger.florian 2017-03-19 20:56:50 UTC
back to NEW
Comment 18 Dominik Stadler 2017-03-19 21:34:20 UTC
Yes, unit-tests are expected to run fine always, we had some failures on Windows recently, but they should be fixed again. 

If they fail for you, please post on the dev-mailing-list so we can discuss if it is a dev-setup-issue or a problem in the tests themselves.
Comment 19 dollinger.florian 2017-03-20 13:07:34 UTC
Created attachment 34847 [details]
better version
Comment 20 dollinger.florian 2017-03-20 13:07:58 UTC
Created attachment 34848 [details]
corrected test cases
Comment 21 dollinger.florian 2017-03-20 13:48:49 UTC
Created attachment 34849 [details]
version 3: non-forced font registration

again a new version with a non-forced font registration (does not create a new one if the cloned one is available)
Comment 22 dollinger.florian 2017-03-20 13:49:37 UTC
Created attachment 34850 [details]
version 3: non-forced font registration

again a new version with a non-forced font registration (does not create a new one if the cloned one is available)
Comment 23 dollinger.florian 2017-03-20 13:55:54 UTC
Created attachment 34851 [details]
corrected imports
Comment 24 dollinger.florian 2017-03-20 13:56:42 UTC
Created attachment 34852 [details]
corrected imports

marked as patch and obsolete versions
Comment 25 dollinger.florian 2017-03-20 13:57:15 UTC
Created attachment 34853 [details]
testcases
Comment 26 dollinger.florian 2017-03-20 13:58:20 UTC
Created attachment 34854 [details]
java source

looks like i am too stupid to use bugzilla xD
Comment 27 dollinger.florian 2017-03-21 06:57:35 UTC
Created attachment 34856 [details]
src patch, minor changes
Comment 28 dollinger.florian 2017-03-23 11:17:39 UTC
Created attachment 34871 [details]
src, version 6: improved equals method
Comment 29 Mark Murphy 2017-03-23 11:45:06 UTC
Comment on attachment 34871 [details]
src, version 6: improved equals method

What is the purpose of this adjustment, It appears multiple times in your changes.

-        if(!(o instanceof XSSFFont)) return false;
+        
+        if (this == o) 
+            return true;
+        if (o == null) 
+            return false; 
+        if (o.getClass() != getClass()) 
+            return false;

How does the new code provide a different outcome? Aren't you just replacing a java construct with a bit of reflection?
Comment 30 dollinger.florian 2017-03-23 19:39:17 UTC
For:

+        if (this == o) 
+            return true;

I would say it's a bit faster if the object is identical to o (in terms of "same" not just "equal") - since the method returns immediately then. But maybe that's not the case too often...


The other two if's is just what i personally like
+        if (o == null) 
+            return false; 
+        if (o.getClass() != getClass()) 
+            return false;

Do you think it's slower in summary? It's not necessary to do it that way... I just thought that the Patch is not applied yet and I can modify it however I want xD
Comment 31 Mark Murphy 2017-03-23 19:57:43 UTC
(In reply to dollinger.florian from comment #30)
> For:
> 
> +        if (this == o) 
> +            return true;
> 
> I would say it's a bit faster if the object is identical to o (in terms of
> "same" not just "equal") - since the method returns immediately then. But
> maybe that's not the case too often...
> 
> 
> The other two if's is just what i personally like
> +        if (o == null) 
> +            return false; 
> +        if (o.getClass() != getClass()) 
> +            return false;
> 
> Do you think it's slower in summary? It's not necessary to do it that way...
> I just thought that the Patch is not applied yet and I can modify it however
> I want xD

Well, there is a great debate about which is correct, but in this case, sticking with instanceof seems correct to me simply because changing to getClass is an unnecessary breaking change. Unless there is a bug introduced by the instanceof, I would keep it.
Comment 32 dollinger.florian 2017-03-24 06:34:40 UTC
Well, I was thinking since yesterday why and which one is preferable over the other and came to the conclusion that the method I used is maybe a bit more reliable, since

a.getClass() == b.getClass()

returns TRUE only, if the two Classes are really the same. In contrast,

a instanceOf bClass

returns also true, if a is of a subclass of b.
I think that violates the equals contract on transitivity.

I also found a website telling the same: http://onewebsql.com/blog/on-equals
Comment 33 Dominik Stadler 2018-12-30 16:41:46 UTC
Applied most of the patch via r1849969, I left out the changes to object-equals-checks, I wanted to avoid changing the behavior for now here. 

We very rarely see derived classes for these objects and thus the differences are likely more academic than of actual relevance. 

For performance, I would only change it if some objective performance measurement shows that it is contributing a lot to overall run time. "it should be faster" is often a bad guidance for doing performance optimizations.