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

getLastChildElement() always returns first element or null #146

Closed
RHarryH opened this issue Jan 3, 2022 · 2 comments · Fixed by #153
Closed

getLastChildElement() always returns first element or null #146

RHarryH opened this issue Jan 3, 2022 · 2 comments · Fixed by #153
Milestone

Comments

@RHarryH
Copy link

RHarryH commented Jan 3, 2022

Hello,

It looks like there is a bug in OdfElement.getLastChildElement() method:

public OdfElement getLastChildElement() {
    OdfElement lastElementChild = null;
    NodeList nodeList = this.getChildNodes();
    Node node = nodeList.item(0);
    for (int i = nodeList.getLength(); i >= 0; i--) {
      if (node instanceof OdfElement) {
        lastElementChild = (OdfElement) node;
        break;
      }
    }
    return lastElementChild;
  }

The loop there does noting. Only if first child node will be and instance of OdfElement, the node will be returned. Otherwise, the null value will be returned. I think the correct code should be as below:

public OdfElement getLastChildElement() {
    OdfElement lastElementChild = null;
    NodeList nodeList = this.getChildNodes();
    for (int i = nodeList.getLength(); i >= 0; i--) {
      Node node = nodeList.item(i);
      if (node instanceof OdfElement) {
        lastElementChild = (OdfElement) node;
        break;
      }
    }
    return lastElementChild;
  }

Please correct me if my understanding of this method is wrong but I think its name is self explanatory.

@svanteschubert
Copy link
Contributor

@RHarryH A thousand thanks, pretty cool catch! :-)

Michael pointed out this issue today (we both have overseen it for the past 3 weeks) our apologies for this - likely it got lost in the email flood after the holiday season (will aim to get a mail filter for these to stop this problem)...

This is a very simple patch, I copy-pasted it, did a build and pushed, but in general any pull-request (PR) gives us pre-build and saves us some time :-)
Last but not least, if you have a use case and could add please a test case that would make the old source fail and the new one work this would be very much appreciated. But the fix itself is already a huge improvement :-)

Thanks again!
Svante

@RHarryH
Copy link
Author

RHarryH commented Jan 24, 2022

@svanteschubert thanks for fixing that! To be honest this is my second issue I've ever reported on the GitHub and I wasn't confident enough to make my own PR :)

For now I'm using a workaround in my code so I'll try to find it, isolate an example and add it there.

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 a pull request may close this issue.

3 participants