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

Fixed issue where a zero length libre office document with a water mark ... #18

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
2 participants
@rojotek
Copy link

rojotek commented Feb 23, 2015

...image was causing an identifier out of range bug.

What's this PR do?

Changes the min value for the identifier manager in XWPFDocument to be 0, as the included test document had an identifier with value 0.

Document Sourced from libre office with a watermark and no text.

The newly added testcase will fail if the code change is removed.

Fixed issue where a zero length libre office document with a water ma…
…rk image was causing an identifier out of range bug.

@rojotek rojotek force-pushed the rojotek:zero-length-libre-office branch from f267480 to 4945461 Feb 23, 2015

@Gagravarr

This comment has been minimized.

Copy link
Contributor

Gagravarr commented Feb 24, 2015

Any chance you could update the unit test to also check that we correctly found the image in the header?

@rojotek

This comment has been minimized.

Copy link
Author

rojotek commented Feb 24, 2015

how does that look @Gagravarr ?

@Gagravarr

This comment has been minimized.

Copy link
Contributor

Gagravarr commented Feb 24, 2015

Is it worth also asserting that the text of the header is empty, do you think?

Also, I've just noticed the naming of things like getHeaderArray(int) which looks a bit odd to me, so I've started a thread on the dev list about it. Your opinion there would be helpful!

@rojotek

This comment has been minimized.

Copy link
Author

rojotek commented Feb 24, 2015

Asserting that the text is empty probably makes sense (so have coded it up :) ).

@Gagravarr

This comment has been minimized.

Copy link
Contributor

Gagravarr commented Feb 24, 2015

Purely style changes, but were it me:

  • Might be good to put all the properties checks together
  • Would probably be easier to read if you fetched the XWPFHeader once, then used that everywhere you check, rather than calling doc.getHeaderArray(0) each time
@rojotek

This comment has been minimized.

Copy link
Author

rojotek commented Feb 24, 2015

was that what you had in mind?

@Gagravarr

This comment has been minimized.

Copy link
Contributor

Gagravarr commented Feb 24, 2015

Perfect, thanks! Committed in r1661908.

@rojotek

This comment has been minimized.

Copy link
Author

rojotek commented Feb 24, 2015

Commited through svn and in trunk so closing this PR.

@rojotek rojotek closed this Feb 24, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.