Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redo Conditional Formatting for #461 #701

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

WoozyG
Copy link
Contributor

@WoozyG WoozyG commented Mar 15, 2019

between POI updates and this new attempt at pushing my local conditional
formatting work, I have much more properly supported. Note however I've identified at least one POI regression (which I apparently introduced), noted below.

Curious to see how the automated tests run for this, looks good on my end.

Now, cells where a conditional rule applies, and that rule has a number
format different than the original cell format, display using the
conditional format.

Also addresses the list of issues identified in bug #461 and bug #651. This incorporates work done in #640 but never finished and merged.

Having this in Spreadsheet master means I can next send a PR for bug
#529, adding support for table styles, both built-in and custom themed.

The Excel official sample file showing how to use conditional
formatting, which is included in the unit tests, now passes for more
sheets. I see excellent equivalence except for:

Features Spreadsheet doesn't support yet:

  • icon sets (sheet Quarters, Bike Rating, FY Months, Regional Sales)
  • data bars (Mountains)
  • gradients (Category Sales)

Likely POI bugs in conditional formatting evaluation when rule formulas
have relative cell references (I can reproduce the first few I tried in
POI unit tests):

  • something goes wrong with styles on Compare to Totals. This works on older POI versions, so I broke it in 4.0 somehow. I'll fix it. We are probably about to a patch release there anyway. Then Spreadsheet 3.0 can use the patched POI as its base version.

  • Style not updating when values change to match conditional rules on
    Products3

  • all rows show as matching on Customers2, instead of only unique rows.
    Could be a formula evaluation bug, or Conditional Formatting evaluation
    bug.


This change is Reviewable

between POI updates and this new attempt at pushing my local conditional
formatting work, I have much more properly supported.

Now, cells where a conditional rule applies, and that rule has a number
format different than the original cell format, display using the
conditional format.

Also addresses the list of issues identified in bug vaadin#461 and bug vaadin#651.

Having this in Spreadsheet master means I can next send a PR for bug
vaadin#529, adding support for table styles, both built-in and custom themed.

The Excel official sample file showing how to use conditional
formatting, which is included in the unit tests, now passes for more
sheets.  I see excellent equivalence except for:

Features Spreadsheet doesn't support yet:

* icon sets (sheet Quarters, Bike Rating, FY Months, Regional Sales)
* data bars (Mountains)
* gradients (Category Sales)

Likely POI bugs in conditional formatting evaluation when rule formulas
have relative cell references (I can reproduce the first few I tried in
POI unit tests):

* something goes wrong with styles on Compare to Totals
* Syle not updating when values change to match conditional rules on
Products3
* all rows show as matching on Customers2, instead of only unique rows.
Could be a formula evaluation bug, or Conditional Formatting evaluation
bug.
@WoozyG
Copy link
Contributor Author

WoozyG commented Mar 16, 2019

Yes, these 3 sheets are all the same bug, due to a math error I made in POI.

I've fixed POI bug #63264 in trunk. It will be fixed in the next dot release. Don't have a date for that yet, but if Vaadin wants it, I can start the process to get one out in the next few weeks.

@WoozyG
Copy link
Contributor Author

WoozyG commented Mar 18, 2019

There will be a new POI release, 4.1, in the next couple weeks. We should take that. It includes my fixes for conditional formatting, my implementation of SUBTOTAL() variations that skip hidden rows, which trips up a lot of sample workbooks, and maybe most important, some fixes for working with POI in deployment environments with JDK 9+. Those are mostly in the underlying XMLBeans project POI has taken over, which is what is used to parse and manipulate the OOXML in XLSX files.

I think I'll be the release manager for that, so I'll update this PR with a dependency update when it is publicly available.

@alvarezguille alvarezguille mentioned this pull request Mar 19, 2019
@WoozyG
Copy link
Contributor Author

WoozyG commented Apr 8, 2019

Released POI 4.1.0, will be available in the next day or two. At that point I'll commit a dependency update and we'll see where we are with these changes.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants