-
Notifications
You must be signed in to change notification settings - Fork 375
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
Nested html tags use styles of all elements in stack #433
base: master
Are you sure you want to change the base?
Conversation
a481950
to
9a3927f
Compare
9a3927f
to
bdab62a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the COMBINED ContainerType.
Otherwise just some formatting issues.
|
||
/** | ||
* SAX content handler used to parse HTML content and call the right method of {@link IDocumentHandler} according the | ||
* HTML content. | ||
*/ | ||
public class HTMLTextStylingContentHandler | ||
extends DefaultHandler | ||
public class HTMLTextStylingContentHandler extends DefaultHandler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert the formatting changes in the whole file
propertiesList.addAll( spansStack ); | ||
ContainerProperties result = null; | ||
for(ContainerProperties properties : propertiesList) { | ||
if (result == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incnsistent formatting (whitespace around braces, curly braces in new line)
public static ContainerProperties combine( ContainerProperties p1, ContainerProperties p2 ) | ||
{ | ||
ContainerProperties result = new ContainerProperties( | ||
p1.getType() == p2.getType() ? p1.getType() : ContainerType.COMBINED ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this implementation lead to problems in ODT when combining Paragraphs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just checked the ODTDocumentHandler.java
currently it doesn't support styles from spans at all. So it really makes no difference. There is probably an Issue or there should be an Issue reading the Span Styles as well and when that is implemented the containerType would be read as well.
@MalteJoe what do you think about this PR? |
@angelozerr As the Formatting Guidelines mentioned in https://github.com/opensagres/xdocreport/wiki/CodeFormatAndConvention are outdated and may be replaced with https://github.com/palantir/palantir-java-format at some point, I will close my comments regarding formatting. There's still one open comment, but it's too old for me to remember what exactly it was about, as I haven't been working with xdocreport for a while now. |
While working on fixing Issue #432 I noticed a Bug when using nested spans with styles:
This HTML Code should produce a bold and italic text
But because only the first item in the stack is used when converting to docx the result is only italic:
should be: