Skip to content

fix for #651#654

Closed
ianscriven wants to merge 1 commit into
vaadin:masterfrom
ianscriven:fix-651
Closed

fix for #651#654
ianscriven wants to merge 1 commit into
vaadin:masterfrom
ianscriven:fix-651

Conversation

@ianscriven

@ianscriven ianscriven commented Dec 18, 2017

Copy link
Copy Markdown
Contributor

see #651

Fixes the issue by reversing the iteration order when updating cells (so top to bottom, right to left).


This change is Reviewable

…ell values are used for conditional formatting
@WoozyG

WoozyG commented Dec 18, 2017

Copy link
Copy Markdown
Contributor

This is indeed the correct fix, but there are two places it is needed - two implementations of the batch processor functional interface in CellValueManager.

I have both plus a unit test based on the sample file in #651. I can commit them here or in branch feature/poi-3.17-update-to-release, which I think is close to being merged to master, pending resolution of one last test? For that matter this change may affect that test.

@ianscriven

Copy link
Copy Markdown
Contributor Author

Hasn't that branch been merged as part of ed2822c? Or are there additional changes?

@WoozyG

WoozyG commented Dec 18, 2017

Copy link
Copy Markdown
Contributor

There are more changes pending, but a remaining unit test failure we haven't tracked down yet.

#640

@ianscriven

ianscriven commented Dec 18, 2017

Copy link
Copy Markdown
Contributor Author

Ok. I'm happy for this to be fixed in that branch, and it would make sense to do so if it will have an impact on unit tests.

@WoozyG

WoozyG commented Dec 18, 2017

Copy link
Copy Markdown
Contributor

I've committed the larger change (this one, plus reversing loop order on the other code path, plus this file as a unit test) to the other branch. Once the last unit test failure is resolved that should get merged back to master. At that point users could use future release builds with POI versions 3.17 through 4.0.x to pick up further formula evaluation bug fixes (there are two already in 4.0 master that affect some Vaadin unit tests).

@WoozyG

WoozyG commented Dec 19, 2017

Copy link
Copy Markdown
Contributor

I checked in the change, but something happened with the unit test run - all the PhantomJS screenshot comparison tests failed for some reason. Vaadin folks will have to tell us what happened there, TeamCity guest access doesn't have enough details.

@alvarezguille

Copy link
Copy Markdown
Member

This is now addressed in #701, let's review and merge the solution based on latest POI

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.

3 participants