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

Port POI patches #1434

Merged
merged 9 commits into from
Nov 11, 2024
Merged

Port POI patches #1434

merged 9 commits into from
Nov 11, 2024

Conversation

antony-liu
Copy link
Collaborator

@antony-liu antony-liu commented Oct 20, 2024

POI patches:

  • bug 60260: parse unicode sheet names
  • bug 60601: unset hyperlinks after removing them individually from the DOM
  • bug 60601: allow SXSSFITestDataProvider.writeOutAndReadBack(Workbook wb) to work on SXSSFWorkbooks and XSSFWorkbooks
  • bug 60601: hyperlinks have to be removed one by one to avoid other errors
  • bug 60601: unlink hyperlinks from a sheet if all hyperlinks have been removed. Patch from Joachim Piketz
  • refactor duplicated code in TestXSSFSheet#setColumnGroupCollapsed; remove unnecessary int to short casts

…m the DOM

bug 60601: allow SXSSFITestDataProvider.writeOutAndReadBack(Workbook wb) to work on SXSSFWorkbooks and XSSFWorkbooks
bug 60601: hyperlinks have to be removed one by one to avoid other errors
bug 60601: unlink hyperlinks from a sheet if all hyperlinks have been removed. Patch from Joachim Piketz
@tonyqus tonyqus changed the title Port POI pathces Port POI patches Oct 20, 2024
@tonyqus tonyqus added this to the NPOI 2.7.2 milestone Oct 21, 2024
@tonyqus tonyqus added the bug label Oct 21, 2024
Copy link
Collaborator

@Bykiev Bykiev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tonyqus tonyqus modified the milestones: NPOI 2.7.2, NPOI 2.7.3 Nov 7, 2024
@tonyqus
Copy link
Member

tonyqus commented Nov 8, 2024

@antony-liu Since #1329 merge, there are some conflicts in TestXSSFSheet.cs. Can you help resolve the conflicts? I'm not so familar with the group column logic

@antony-liu
Copy link
Collaborator Author

There are two functions IsSetHyperlinks and IsSetHyperLinks in class CT_Worksheet. They have the same function。And IsSetHyperLinks is never used, so remove it.

@tonyqus
Copy link
Member

tonyqus commented Nov 9, 2024

What's the plan for TestSetColumnGroupCollapsed? I think @artem-iron has made it work. Has any bug been ported from POI 3.16?

@antony-liu
Copy link
Collaborator Author

I think it's due to outdated code implementation of XSSFSheet.GroupColumn and XSSFSheet.SetColumnGroupCollapsed. It is completely different from POI 3.16, I tried porting it, but it would cause some other test cases to fail.

I plan to start porting after completing POI 3.16.

@tonyqus
Copy link
Member

tonyqus commented Nov 10, 2024

@antony-liu I don't see you changed any code about TestSetColumnGroupCollapsed in the file changes. Is it caused by the incorrect merge? Can we follow Artem's implementation? At least, his changes don't break the unit test.

@tonyqus
Copy link
Member

tonyqus commented Nov 10, 2024

The merge is wrong. You should not remove the Assert code from Artem.

Can you rollback and remerge? And then re-enable TestSetColumnGroupCollapsed test.

Another choice is that you recreate a branch based on master and reapply all the rest commits to the branch. And don't touch anything about ColumnGroupCollapse

@antony-liu
Copy link
Collaborator Author

OK. I have reverted it, and enabled the test TestSetColumnGroupCollapsed.

The commit 1ade0af change code of TestSetColumnGroupCollapsed. But it is not compatible with method XSSFSheet.GroupColumn and XSSFSheet.SetColumnGroupCollapsed.

@tonyqus
Copy link
Member

tonyqus commented Nov 11, 2024

LGTM

@tonyqus tonyqus merged commit ba872d5 into nissl-lab:master Nov 11, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants