Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

handle NPE in justify text edge case #420 #421

Merged
merged 1 commit into from
Dec 4, 2019
Merged

handle NPE in justify text edge case #420 #421

merged 1 commit into from
Dec 4, 2019

Conversation

syjer
Copy link
Contributor

@syjer syjer commented Nov 29, 2019

hi @danfickle , this fix the issue #420 . From the debugging session, I see the issue is caused by a InlineText: "\n" which I guess is generated by the br element, but I can't get a clear picture.

If I understood correctly, the align logic is calling InlineText.calcTotalAdjustment without calling InlineText.countJustifiableChars on that InlineText element before (which has the InlineLayoutBox: "br" as a parent), thus not creating a "_counts" instance.

@danfickle
Copy link
Owner

Hi @syjer,

I'm going to merge this for the test, but I'm not sure if the a better fix might be to go ahead and call countJustifiableChars if _counts is null. I'll have to debug to see why it is not being called though.

Thanks for the PR and sorry for the delay in merging.

@danfickle danfickle merged commit 19887e0 into danfickle:open-dev-v1 Dec 4, 2019
@syjer
Copy link
Contributor Author

syjer commented Dec 4, 2019

call countJustifiableChars if _counts is null.

I was wondering the same. I think I tried to do that way, but I was missing a decent way to get the CharCounts counts parameter.

sorry for the delay in merging.

no problem at all :)

danfickle added a commit that referenced this pull request Dec 4, 2019
+ Further test for justified text with nested non-justified text.
+ Defence against divide by zero error.
+ Remove unneeded casts in justify method.
+ Add comments around #421
+ Fixes #420
@danfickle
Copy link
Owner

It runs out zero is the correct result! The only time it doesn't count characters beforehand is for non-justified content nested inside justified text, so returning zero adjustment for the non-justified inline text is correct.

The reason it showed up on <br/> is that this element is implemented in the default stylesheet with white-space: pre which does not get justified.

The key method to figure this out was:

    public void countJustifiableChars(CharCounts counts) {
        boolean justifyThis = getStyle().isTextJustify();
        for (Iterator<Object> i = getInlineChildren().iterator(); i.hasNext(); ) {
            Object o = i.next();
            if (o instanceof InlineLayoutBox) {
                ((InlineLayoutBox)o).countJustifiableChars(counts);
            } else if (o instanceof InlineText && justifyThis) {
                ((InlineText)o).countJustifiableChars(counts);
            }
        }
    }

Note: The justifyThis variable.

I added an extra test to make sure this was the case and it indeed breaks if we try counting chars when _counts is null.

Thanks again.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants