Skip to content

Fix XWPFDocument FindAndReplaceTextInParagraph#1524

Merged
tonyqus merged 8 commits into
nissl-lab:masterfrom
Kazbek:master
Apr 18, 2025
Merged

Fix XWPFDocument FindAndReplaceTextInParagraph#1524
tonyqus merged 8 commits into
nissl-lab:masterfrom
Kazbek:master

Conversation

@Kazbek
Copy link
Copy Markdown
Contributor

@Kazbek Kazbek commented Mar 27, 2025

My first attempt to contribute to NPOI.
Trying to fix my issue from 2021 #666. Work well for demo with bug and also passing single test in repo (FindAndReplaceText method).

@tonyqus tonyqus added this to the NPOI 2.7.4 milestone Mar 27, 2025
@tonyqus
Copy link
Copy Markdown
Member

tonyqus commented Mar 27, 2025

Please add at least one unit test for this fix.

@Kazbek
Copy link
Copy Markdown
Contributor Author

Kazbek commented Mar 27, 2025

Added test which cover bug demo case and a bit more variations.

@Kazbek
Copy link
Copy Markdown
Contributor Author

Kazbek commented Mar 27, 2025

I'm just realized that this method will not work when there are more than 2 occurance of replace text in paragraph. Will modify code and test for it. Idk if original method handle this or not.

…of oldValue in paragraph. Modified test to check it.
@Kazbek
Copy link
Copy Markdown
Contributor Author

Kazbek commented Mar 27, 2025

Now must work properly.

@tonyqus
Copy link
Copy Markdown
Member

tonyqus commented Mar 28, 2025

Btw, where are you from?

@Kazbek
Copy link
Copy Markdown
Contributor Author

Kazbek commented Mar 28, 2025

Russia

@tonyqus
Copy link
Copy Markdown
Member

tonyqus commented Mar 30, 2025

Unit tests failed. Please fix them

@Kazbek
Copy link
Copy Markdown
Contributor Author

Kazbek commented Mar 30, 2025

I screwed up a bit with the number of commits. The tests passed successfully on my computer. But as I understood, the error was that the text contained foreign characters, and the file was not in utf8 encoding. If you want I can make new PR with single commit.

@Kazbek
Copy link
Copy Markdown
Contributor Author

Kazbek commented Apr 1, 2025

Let me know if there's anything else you need me to do.

@tonyqus tonyqus requested a review from Copilot April 1, 2025 23:53
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses issue #666 by fixing the FindAndReplaceTextInParagraph functionality in XWPFDocument, as well as adding a test case to validate the replacement behavior.

  • Fixes text replacement within paragraphs in XWPFDocument.
  • Adds a new test case to verify that FindAndReplaceTextInParagraph works as expected.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
testcases/ooxml/XWPF/UserModel/TestXWPFDocument.cs Added a new test case to validate the updated FindAndReplaceText functionality.
ooxml/XWPF/Usermodel/XWPFDocument.cs Updated the FindAndReplaceTextInParagraph internal method to support recursive replacements using an additional parameter.
Comments suppressed due to low confidence (1)

ooxml/XWPF/Usermodel/XWPFDocument.cs:1895

  • [nitpick] The variable 'firstParagraph' (and similarly 'lastParagraph') may be misleading as it actually represents a run index rather than a paragraph index. Consider renaming them to 'firstRunIndex' and 'lastRunIndex' for improved clarity.
int firstParagraph = -1;

Comment on lines +210 to +211
XWPFDocument outputDocument = outputDocument = XWPFTestDataSamples.WriteOutAndReadBack(doc);

Copy link

Copilot AI Apr 1, 2025

Choose a reason for hiding this comment

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

Duplicate assignment to outputDocument detected. Please remove the redundant assignment to ensure clarity.

Suggested change
XWPFDocument outputDocument = outputDocument = XWPFTestDataSamples.WriteOutAndReadBack(doc);
XWPFDocument outputDocument = XWPFTestDataSamples.WriteOutAndReadBack(doc);

Copilot uses AI. Check for mistakes.
@tonyqus
Copy link
Copy Markdown
Member

tonyqus commented Apr 1, 2025

Can you help solve the conflict?

@tonyqus
Copy link
Copy Markdown
Member

tonyqus commented Apr 1, 2025

Wow, copilot review..... awesome!

@Kazbek
Copy link
Copy Markdown
Contributor Author

Kazbek commented Apr 2, 2025

Solved conflict and applied copilot suggestions.

@tonyqus
Copy link
Copy Markdown
Member

tonyqus commented Apr 18, 2025

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants