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

Add support for rowspan/colspan to tables #642

Merged
merged 3 commits into from
Mar 29, 2021

Conversation

utzig
Copy link
Contributor

@utzig utzig commented Feb 15, 2021

Better descriptions where added to the commits themselves. After this is merged, the only remaining support that needs to be added to tables is align. It also fixes the issues previously seen with the latex writer.

@utzig utzig force-pushed the table-spans branch 3 times, most recently from 277d8b0 to 451ad15 Compare February 15, 2021 23:46
@vermeeren vermeeren self-requested a review February 16, 2021 14:38
@vermeeren vermeeren self-assigned this Feb 16, 2021
@vermeeren vermeeren added code Source code enhancement Improvements, additions (also cosmetics) labels Feb 16, 2021
Copy link
Collaborator

@vermeeren vermeeren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@utzig Changes so far look good to me, but I don't observe any changes on the table example page and make pdf does throw an exception. Presumably still work in progress?

@utzig
Copy link
Contributor Author

utzig commented Feb 16, 2021

@utzig Changes so far look good to me, but I don't observe any changes on the table example page and make pdf does throw an exception. Presumably still work in progress?

Not WIP, all the tables should render correctly with this PR, they do here, and I don't see issues with "make pdf" locally. Are you sure you tested with the PR's commits?

@vermeeren
Copy link
Collaborator

Not WIP, all the tables should render correctly with this PR, they do here, and I don't see issues with "make pdf" locally. Are you sure you tested with the PR's commits?

@utzig commit 451ad15, Sphinx 3.0.2, make pdf throws:

writing... failed
Exception occurred while building, starting debugger:
Traceback (most recent call last):
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/cmd/build.py", line 280, in build_main
    app.build(args.force_all, filenames)
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/application.py", line 348, in build
    self.builder.build_update()
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/builders/__init__.py", line 294, in build_update
    self.build(['__all__'], to_build)
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/builders/__init__.py", line 361, in build
    self.write(docnames, list(updated_docnames), method)
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/builders/latex/__init__.py", line 301, in write
    docwriter.write(doctree, destination)
  File "/home/melvin/.local/lib/python3.7/site-packages/docutils/writers/__init__.py", line 78, in write
    self.translate()
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/writers/latex.py", line 102, in translate
    self.document.walkabout(visitor)
  File "/home/melvin/.local/lib/python3.7/site-packages/docutils/nodes.py", line 214, in walkabout
    if child.walkabout(visitor):
  File "/home/melvin/.local/lib/python3.7/site-packages/docutils/nodes.py", line 214, in walkabout
    if child.walkabout(visitor):
  File "/home/melvin/.local/lib/python3.7/site-packages/docutils/nodes.py", line 214, in walkabout
    if child.walkabout(visitor):
  [Previous line repeated 9 more times]
  File "/home/melvin/.local/lib/python3.7/site-packages/docutils/nodes.py", line 227, in walkabout
    visitor.dispatch_departure(self)
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/util/docutils.py", line 485, in dispatch_departure
    method(node)
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/writers/latex.py", line 947, in depart_row
    underlined = [cell.row + cell.height == self.table.row + 1 for cell in cells]
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/writers/latex.py", line 947, in <listcomp>
    underlined = [cell.row + cell.height == self.table.row + 1 for cell in cells]
AttributeError: 'NoneType' object has no attribute 'row'
> /home/melvin/.local/lib/python3.7/site-packages/sphinx/writers/latex.py(947)<listcomp>()
-> underlined = [cell.row + cell.height == self.table.row + 1 for cell in cells]
(Pdb)

Tables render the same as https://breathe.readthedocs.io/en/latest/tables.html with make html.

What Sphinx version do you have locally? I am thinking my ancient version is to blame here.

@utzig
Copy link
Contributor Author

utzig commented Feb 16, 2021

Not WIP, all the tables should render correctly with this PR, they do here, and I don't see issues with "make pdf" locally. Are you sure you tested with the PR's commits?

@utzig commit 451ad15, Sphinx 3.0.2, make pdf throws:

writing... failed
Exception occurred while building, starting debugger:
Traceback (most recent call last):
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/cmd/build.py", line 280, in build_main
    app.build(args.force_all, filenames)
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/application.py", line 348, in build
    self.builder.build_update()
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/builders/__init__.py", line 294, in build_update
    self.build(['__all__'], to_build)
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/builders/__init__.py", line 361, in build
    self.write(docnames, list(updated_docnames), method)
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/builders/latex/__init__.py", line 301, in write
    docwriter.write(doctree, destination)
  File "/home/melvin/.local/lib/python3.7/site-packages/docutils/writers/__init__.py", line 78, in write
    self.translate()
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/writers/latex.py", line 102, in translate
    self.document.walkabout(visitor)
  File "/home/melvin/.local/lib/python3.7/site-packages/docutils/nodes.py", line 214, in walkabout
    if child.walkabout(visitor):
  File "/home/melvin/.local/lib/python3.7/site-packages/docutils/nodes.py", line 214, in walkabout
    if child.walkabout(visitor):
  File "/home/melvin/.local/lib/python3.7/site-packages/docutils/nodes.py", line 214, in walkabout
    if child.walkabout(visitor):
  [Previous line repeated 9 more times]
  File "/home/melvin/.local/lib/python3.7/site-packages/docutils/nodes.py", line 227, in walkabout
    visitor.dispatch_departure(self)
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/util/docutils.py", line 485, in dispatch_departure
    method(node)
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/writers/latex.py", line 947, in depart_row
    underlined = [cell.row + cell.height == self.table.row + 1 for cell in cells]
  File "/home/melvin/.local/lib/python3.7/site-packages/sphinx/writers/latex.py", line 947, in <listcomp>
    underlined = [cell.row + cell.height == self.table.row + 1 for cell in cells]
AttributeError: 'NoneType' object has no attribute 'row'
> /home/melvin/.local/lib/python3.7/site-packages/sphinx/writers/latex.py(947)<listcomp>()
-> underlined = [cell.row + cell.height == self.table.row + 1 for cell in cells]
(Pdb)

Tables render the same as https://breathe.readthedocs.io/en/latest/tables.html with make html.

What Sphinx version do you have locally? I am thinking my ancient version is to blame here.

3.4.3

@utzig
Copy link
Contributor Author

utzig commented Feb 16, 2021

I tried with Sphinx==3.0.2 and it works fine as well, what docutils version do you have?

@vermeeren
Copy link
Collaborator

vermeeren commented Feb 16, 2021

I tried with Sphinx==3.0.2 and it works fine as well, what docutils version do you have?

@utzig docutils 0.16

(Bit short on time, will have more proper reply and testing hopefully soon.)

@utzig
Copy link
Contributor Author

utzig commented Feb 16, 2021

I tried with Sphinx==3.0.2 and it works fine as well, what docutils version do you have?

@utzig docutils 0.16

(Bit short on time, will have more proper reply and testing hopefully soon.)

Same problem here! I found out the problem, Doxygen>=1.9.0 is required because the previous versions didn't have rowspan, colspan and align, so tables are not rendered correctly with those. Regarding the issues with "make all" I will see if I can fix it today or in the next few days.

@vermeeren
Copy link
Collaborator

Same problem here! I found out the problem, Doxygen>=1.9.0 is required because the previous versions didn't have rowspan, colspan and align, so tables are not rendered correctly with those.

@utzig Ah yes of course, my Doxygen is version 1.8.13 as I'm running Debian stable (buster).

Regarding the issues with "make all" I will see if I can fix it today or in the next few days.

I can't recall anything about this, is there some problem with this in master?

@utzig
Copy link
Contributor Author

utzig commented Feb 16, 2021

Same problem here! I found out the problem, Doxygen>=1.9.0 is required because the previous versions didn't have rowspan, colspan and align, so tables are not rendered correctly with those.

@utzig Ah yes of course, my Doxygen is version 1.8.13 as I'm running Debian stable (buster).

Regarding the issues with "make all" I will see if I can fix it today or in the next few days.

I can't recall anything about this, is there some problem with this in master?

I believe the issue is with my parser changes, or maybe rendering.

@utzig
Copy link
Contributor Author

utzig commented Feb 16, 2021

This is related to the same problem you suggested in the previous PR to use .. only: html (#638 (review)). I had previously removed that requirement on this PR but now I dropped the commit and it should finish "make all" again. If you have Doxygen>=1.9.0 (tbh I don't know which exact version, but 1.8.17 does not work!) it will even render the tables correctly, otherwise you get same results as before because there were no parameters in the XML to be started with. The latex writer I will have to investigate further to find out why it is breaking when there are spans in tables.

@utzig
Copy link
Contributor Author

utzig commented Feb 17, 2021

The failures in the Sphinx latex writer happen on this line: https://github.com/sphinx-doc/sphinx/blob/3.x/sphinx/writers/latex.py#L963

Since self.table.colcount is the number of the columns, when there is span at least some rows won't have colcount amount of columns and it will have None in the last element's in the list. So basically I think the latex writer does not support spanning, but I need to check with input directly from rST.

@vermeeren
Copy link
Collaborator

So basically I think the latex writer does not support spanning, but I need to check with input directly from rST.

@utzig Do you have some news about this? If not, I think we should merge this as-is keeping the .. only:: html to avoid broken PDF output with old Doxygen versions, as changes themselves are good improvements either way.

Expand parser to support parsing extra "entry" parameters from recent
Doxygen versions, namely "align", "rowspan" and "colspan". This should
allow the renderer to correctly render tables once updated.

Signed-off-by: Fabio Utzig <[email protected]>
"rowspan" is unable to span across different "thead", and "tbody"
elements which make it not useful with the standard row parsing
routines. This commit adds a final step when parsing a table to join all
"thead" and "tbody" rows into just two set of elements.

This still has the limitation that a rowspan from a header won't
span to body elements, but this is not the most usual situation.

Signed-off-by: Fabio Utzig <[email protected]>
@utzig
Copy link
Contributor Author

utzig commented Mar 29, 2021

So basically I think the latex writer does not support spanning, but I need to check with input directly from rST.

@utzig Do you have some news about this? If not, I think we should merge this as-is keeping the .. only:: html to avoid broken PDF output with old Doxygen versions, as changes themselves are good improvements either way.

To correct myself, the Latex writer does actually support spans, the problem with the code I pointed previously is that it expects that valid spans exist, and in that case all correct data structures are built in memory; which is not possible if using invalid Doxygen XML (Doxygen<=1.8.18). I tried adding some fault handling for it last month, with a warning about the broken Doxygen XML, but wasn't very happy on how to handle it properly, so it's better to just render it wrong in this case and know about it. The html only build filters are already in place so this should be passing the CI and ready to merge.

Copy link
Collaborator

@vermeeren vermeeren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@utzig Thanks for the details, I agree it's better to render wrong in these cases. Created #665 to raise the Doxygen dependency version after Debian bullseye is stable, from that point onwards anything older is considered legacy anyway.

Will merge and make a release in a sec.

@michaeljones michaeljones merged commit 466a524 into breathe-doc:master Mar 29, 2021
michaeljones pushed a commit that referenced this pull request Mar 29, 2021
@utzig utzig deleted the table-spans branch March 29, 2021 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Source code enhancement Improvements, additions (also cosmetics)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants