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

HSSFWorkbook.getAllEmbeddedObjects wasn't getting all embedded objects #2

Closed
wants to merge 2 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@trejkaz

trejkaz commented May 21, 2013

HSSFWorkbook.getAllEmbeddedObjects wasn't getting all embedded objects, only the top-level ones. This fixes it to get them recursively.

Update HSSFWorkbook.java
HSSFWorkbook.getAllEmbeddedObjects wasn't getting all embedded objects, only the top-level ones.
@Gagravarr

This comment has been minimized.

Contributor

Gagravarr commented Jun 12, 2013

I may be missing something, but I can't see how your new method ends up getting called?

Also, any chance of a unit test for this (likely with a test file), which shows that some objects were missed before, but are now being fetched?

@trejkaz

This comment has been minimized.

trejkaz commented Jun 13, 2013

Now that you ask it, I'm not sure if the diff is complete or not. I'll have to check our offline copy again. The summary of the issue was that the original code only returned the images at the top-level, not those inside groups.

As for samples, the only one I have, I'm not allowed to share.

@Gagravarr

This comment has been minimized.

Contributor

Gagravarr commented Jun 13, 2013

If you do manage to dig out the full patch, that'd be handy. What you could then do is write a very small program that opens each test file we have in turn, and prints out the number of embedded parts. Run that with pre and post fixed jars, if one of them shows a different number then we have an existing test file we can use for the unit test!

@trejkaz

This comment has been minimized.

trejkaz commented Jun 14, 2013

https://gist.github.com/trejkaz/5779009 shows results before and after the patch.

I guess there are more sample files than anyone expected - v3.9 only found embedded objects in 4 files and with the fix, 10 files have embedded objects.

@Gagravarr

This comment has been minimized.

Contributor

Gagravarr commented Jun 14, 2013

Looks like files will tend to either have one kind or the other then - I spot lots of files we're now finding things in, but none we found before getting more

I've applied your two patches in r1493001, along with two new unit tests based on your gist, thanks!

@asfgit asfgit closed this in 5d79479 Jan 4, 2016

pjfanning pushed a commit to pjfanning/poi that referenced this pull request Jan 24, 2018

Merge pull request apache#2 from pjfanning/zmau-trunk
Rebase with apache/poi trunk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment