SerenityOS / serenity

The Serenity Operating System 🐞
https://serenityos.org
BSD 2-Clause "Simplified" License
30.77k stars 3.18k forks source link

LibMarkdown: Inconsistent rendering of multiple single-line inline code-blocks #19534

Open AtkinsSJ opened 1 year ago

AtkinsSJ commented 1 year ago

This markdown:

## Name

checksum - helper program for calculating checksums

## Synopsis

`$ md5sum <file>`
`$ sha1sum <file>`
`$ sha256sum <file>`
`$ sha512sum <file>`

## Description

This program calculates and print specified checksum of files. It cannot be run directly, only
as `md5sum`, `sha1sum`, `sha256sum` or `sha512sum`.

produces different results in the terminal and HTML renderings:

image

Specifically, the single-line inline code blocks all become one line in terminal mode, but separate lines in HTML. Which is correct? I don't know! :tada:

timschumi commented 1 year ago

For the source code as-is, a single line is the expected output. The fact that it is a single line is hidden in the HTML output, as it can't fit both inline code blocks into the same line, essentially forcing a line break anyways. Making the window wider reveals that the output there is also just a single line.

The missing line breaks for HTML were fixed in #19537, actually rendering them correctly in the Terminal requires a separate bugfix, as manual line breaks simply get discarded there.

tcl3 commented 1 year ago

I've made a PR which changes the multiple inline code blocks in the checksum man page to a single multi-line code block. This renders correctly as HTML as well as in the terminal. This seems to be what other man pages do.

I also had a look at the underlying markdown rendering issue and I believe the problem is here:

https://github.com/SerenityOS/serenity/blob/0f040456a733eb411bf6f8438f68547c5f6703a4/Userland/Libraries/LibMarkdown/Text.cpp#L79-L86

Making the BreakNode:render_for_terminal() method output a newline also fixes the issue, but the indentation of the resulting markdown is slightly off.

I think making this fix this rendering inconsistency, but I didn't include it in the PR I've subimtted, as I couldn't find a man page where the rendering was affected (after changing checksum to use a multiline code block).

Is there a good reason why BreakNode behaves differently when producing output for the terminal? Should I include the rendering fix in the PR I've already submitted, or create a second one?

timschumi commented 1 year ago

Is there a good reason why BreakNode behaves differently when producing output for the terminal?

The reason why it is currently unimplemented is because the render_for_terminal doesn't seem to have a concept of outputting more than one line. All it can do is emit a newline as part of a line, but that results in the indentation being off. HTML doesn't have this problem because it mostly doesn't care about newlines in the source code, we can just emit a <br /> whereever we please and it will get rendered correctly.