Skip to content

Adjust calls to deprecated POI methods removed in POI 4.0.#640

Closed
WoozyG wants to merge 5 commits intovaadin:feature/poi-3.17-update-to-releasefrom
WoozyG:feature/poi-3.17-update-to-release
Closed

Adjust calls to deprecated POI methods removed in POI 4.0.#640
WoozyG wants to merge 5 commits intovaadin:feature/poi-3.17-update-to-releasefrom
WoozyG:feature/poi-3.17-update-to-release

Conversation

@WoozyG
Copy link
Contributor

@WoozyG WoozyG commented Nov 20, 2017

These changes are all 3.17 compatible, and enable downstream users to
drop in 4.0 builds as needed.

This is a way Spreadsheet users can get the conditional formatting fixes
done in 4.0 without further code changes, once this branch is merged
back and released.

Alternatively, a POI 4.0 build could be lobbied for, and this branch
could move up to that version.


This change is Reviewable

These changes are all 3.17 compatible, and enable downstream users to
drop in 4.0 builds as needed.

This is a way Spreadsheet users can get the conditional formatting fixes
done in 4.0 without further code changes, once this branch is merged
back and released.

Alternatively, a POI 4.0 build could be lobbied for, and this branch
could move up to that version.
@CLAassistant
Copy link

CLAassistant commented Nov 20, 2017

CLA assistant check
All committers have signed the CLA.

flawed, as it doesn't notice formula based chart titles, but that's
another issue.
@WoozyG
Copy link
Contributor Author

WoozyG commented Nov 22, 2017

Same test failures as #617, fixed in POI master.

Found a workbook case with > 10 conditional format rules per sheet,
which caused two different rules to use the same CSS index #.  Adjusted
ranges to more acceptable limits, updated comments.

Also added JavaDoc for the number scheme order rationale and upcoming
changes needed to support table formats.
@alexberazouski
Copy link

There is one new test failure in SheetFilterTableTest. Let's do not merge it to the with 3.17 update


Reviewed 4 of 4 files at r1, 1 of 1 files at r2.
Review status: 5 of 6 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@WoozyG
Copy link
Contributor Author

WoozyG commented Dec 7, 2017

I didn't touch any code near that failure, and indeed, that test passes for the build and me except in PhantomJS. Sounds suspiciously like a bug where PhantomJS isn't updating the DOM when other engines do? Is there a way to manually check this failure?

@WoozyG
Copy link
Contributor Author

WoozyG commented Dec 7, 2017

In fact, the second time I ran this test locally, it passed. Not sure what's going on.

@alexberazouski
Copy link

Yes, I see. But same test failed after build restart again


Review status: 5 of 6 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@WoozyG
Copy link
Contributor Author

WoozyG commented Dec 12, 2017

The build on Dec. 1, #1063, was the first one with this new test failure. The only change in that build was in JavaDoc. That obviously isn't the cause. None of the later changes affect that test either. That test passes for me locally. I suspect some change to the automated test harness that is affecting it.

That particular test is potentially susceptible to concurrency/timing affects, among other things.

@WoozyG WoozyG mentioned this pull request Dec 18, 2017
Simplest way to handle this was to reverse the iteration order, to match
the CSS evaluation order given the way Vaadin Spreadsheet handles shared
border styles.

Includes a new unit test and test file from the issue. 

Outside chance it may help the mystery unit test that's failing.
@WoozyG
Copy link
Contributor Author

WoozyG commented Dec 19, 2017

What happened with this test run? Looks like all the PhantomJS tests failed due to mismatched screenshots?

@alvarezguille
Copy link
Member

@WoozyG please reopen if this is still valid

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.

4 participants