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

DocBook reader: Table width support #6791

Merged
merged 1 commit into from
Nov 21, 2020

Conversation

pyssling
Copy link
Contributor

Table width is not natively supported by docbook but is by
the docbook fo stylesheets through an XML processing instruction,

. Implement support for this instruction

in the DocBook reader.

Copy link
Collaborator

@tarleb tarleb left a comment

Choose a reason for hiding this comment

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

The code looks good, just the indentation of the first case statement appears to be off.
I don't know enough about DocBook to comment on the logic.

@pyssling pyssling force-pushed the docbook-table-width-support branch from 899d17f to 5d72047 Compare November 3, 2020 21:22
@pyssling
Copy link
Contributor Author

pyssling commented Nov 3, 2020

Thank you, done.

@pyssling pyssling force-pushed the docbook-table-width-support branch 2 times, most recently from c2d9cf5 to 6a3632a Compare November 5, 2020 22:36
@@ -960,12 +960,22 @@ parseBlock (Elem e) =
let aligns = case colspecs of
[] -> replicate numrows AlignDefault
cs -> map toAlignment cs
let parseWidth s = safeRead (T.filter (\x -> (x >= '0' && x <= '9')
|| x == '.') s)
let textWidth = case filterChild (named "?dbfo") e of
Copy link
Owner

Choose a reason for hiding this comment

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

What is this ?dbfo?

Copy link
Contributor Author

@pyssling pyssling Nov 7, 2020

Choose a reason for hiding this comment

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

It's an XML processing instruction that's used by the docbook stylesheets to specify the table width in proportion to text. The CALS table model (from 1995) never included something width in relation to text - so this is what people came up with it seems.
Edit: Should add that this stuff is really a reverse engineering of how the docbook stylesheets work - dbfo is most often accompanied by dbhtml and sometimes by dbman processing instructions. Documentation of the docbook stylesheet processing instructions can be found here: http://docbook.sourceforge.net/release/xsl/current/doc/pi/pi-fo.html

@@ -960,12 +960,22 @@ parseBlock (Elem e) =
let aligns = case colspecs of
[] -> replicate numrows AlignDefault
cs -> map toAlignment cs
let parseWidth s = safeRead (T.filter (\x -> (x >= '0' && x <= '9')
Copy link
Owner

Choose a reason for hiding this comment

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

Is this field just a decimal number with no units? If so, do we need to filter? If not, are we ignoring the units?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's percent - but the percent sign may or may not be there from what I've seen. I though this was safest - but there may be better ways.

@jgm
Copy link
Owner

jgm commented Nov 17, 2020

It would be nice to have a test of some kind.
At least a command test.

@pyssling pyssling force-pushed the docbook-table-width-support branch 2 times, most recently from def6a29 to 72f9907 Compare November 17, 2020 22:00
@pyssling
Copy link
Contributor Author

Added a testcase. Hopefully good now.

@jgm
Copy link
Owner

jgm commented Nov 19, 2020

Thanks -- I should have mentioned, we typically name the test cases after issues or PRs, so that it's easier to remember/look up what motivated them. Thus 6791.md.
Alternatively you could add a link to this PR in the text.

@pyssling pyssling force-pushed the docbook-table-width-support branch from 72f9907 to e5e53c3 Compare November 20, 2020 07:03
@pyssling
Copy link
Contributor Author

done.

Table width in relation to text width is not natively supported
by docbook but is by the docbook fo stylesheets through an XML
processing instruction, <?dbfo table-width="50%"?> .
Implement support for this instruction in the DocBook reader.
@pyssling pyssling force-pushed the docbook-table-width-support branch from e5e53c3 to c058cb8 Compare November 20, 2020 08:30
@jgm jgm merged commit 56ceaf4 into jgm:master Nov 21, 2020
@jgm
Copy link
Owner

jgm commented Nov 21, 2020

thanks!

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 this pull request may close these issues.

3 participants