Improved headings and more accurate line parsing

[JoshuaMKW]

This PR aims to fundamentally improve the rendering of headers and overall vertical spacing within imgui_markdown, according to reasonable markdown specifications.

Notably, markdown discards isolate newlines when being rendered, but that was not implemented within imgui_markdown, causing fundamentally inaccurate rendering. This PR brings in that functionality and greatly improved the accuracy of markdown rendering in my testing.

The main focus of this PR, however, was the rendering of headers. By markdown specifications, if a header is the first line of the markdown file, it will not have preliminary padding applied. To support this, I have added a new field firstLine to ImGui::MarkdownFormatInfo which enables this functionality of headers to work. Additionally, the original implementation of header formatting in ImGui::defaultMarkdownFormatCallback was incorrect. After some adjustments, the spacing now matches that of GitHub's markdown renderer (within a margin of error, of course).

This is just the first of a few PRs I plan to bring to imgui_markdown to improve the rendering, as I am using this for my own ImGui application and have made some improvements to the formatting/rendering of markdown elements.

[dougbinks]

Thanks for this - my comments on your other PR also apply here.

[JoshuaMKW]

@dougbinks Now that we are beyond Christmas and such, are you in a position to review this PR? My other work within this repository is bottlenecked by waiting upon this so that's why I ask.

[dougbinks]

I am still very busy but having fetched, compiled, run & read the code I think the following change would be a good idea:

Rather than:

bool                    useComplexFormatting = false;

I think we should have a set of flags, something like:

int                   formatFlags = 0;
enum ImGuiMarkdownFormatFlags
{
    ImGuiMarkdownFormatFlags_None                        = 0,
    ImGuiMarkdownFormatFlags_DiscardExtraNewLines        = 1 << 0,
    ImGuiMarkdownFormatFlags_NoNewLineIfHeadingFirstLine = 1 << 1,
    ImGuiMarkdownFormatFlags_SeperatorDoesNotAdvance     = 1 << 2,
    ImGuiMarkdownFormatFlags_CommonMarkAll               = ImGuiMarkdownFormatFlags_DiscardExtraNewLines | ImGuiMarkdownFormatFlags_NoNewLineIfHeadingFirstLine | ImGuiMarkdownFormatFlags_SeperatorDoesNotAdvance;
};

and then use these flags for each functionality.

Additionally I think some minor changes can be made to make simplify the code, so the defaultMarkdownFormatCallback code can be simplified if we do the ImGui::NewLine() before the ImGui::PushFont in both cases and thus collapse the code down a little to something like:

            if( start_ )
            {
                if( 0 == ( markdownFormatInfo_.formatFlags & ImGuiMarkdownFormatFlags_NoNewLineIfHeadingFirstLine ) 
                    || !markdownFormatInfo_.firstLine )
                {
                    ImGui::NewLine();
                }
                if( fmt.font )
                {
#ifdef IMGUI_HAS_TEXTURES // used to detect dynamic font capability: https://github.com/ocornut/imgui/issues/8465#issuecomment-2701570771
                    ImGui::PushFont(fmt.font, fmt.fontSize > 0.0f ? fmt.fontSize : fmt.font->LegacySize);
#else
                    ImGui::PushFont(fmt.font);
#endif
                }
            }
            else
            {
                if( fmt.separator )
                {
                    // In markdown the separator does not advance the cursor
                    ImVec2 cursor = ImGui::GetCursorPos();
                    ImGui::Separator();
                    if( markdownFormatInfo_.formatFlags & ImGuiMarkdownFormatFlags_SeperatorDoesNotAdvance ) {
                        ImGui::SetCursorPos(cursor);
                    }
                }
                if( fmt.font )
                {
                    ImGui::PopFont();
                }
                ImGui::NewLine();
            }

What do you think of this approach? From our side we'd use a few of these flags but not all of them so the flexibility would be good.

[JoshuaMKW]

enum ImGuiMarkdownFormatFlags
{
    ImGuiMarkdownFormatFlags_None                        = 0,
    ImGuiMarkdownFormatFlags_DiscardExtraNewLines        = 1 << 0,
    ImGuiMarkdownFormatFlags_NoNewLineIfHeadingFirstLine = 1 << 1,
    ImGuiMarkdownFormatFlags_SeperatorDoesNotAdvance     = 1 << 2,
    ImGuiMarkdownFormatFlags_CommonMarkAll               = ImGuiMarkdownFormatFlags_DiscardExtraNewLines | ImGuiMarkdownFormatFlags_NoNewLineIfHeadingFirstLine | ImGuiMarkdownFormatFlags_SeperatorDoesNotAdvance;
};

Yes, I enjoy this idea a lot. I'll design it in the code to follow the same design philosophy as ImGui itself (int typedef, enum has trailing underscore and placed in the global scope, etc).

Question: How do you feel about something like this?

typedef int ImGuiMarkdownFormatFlags;

enum ImGuiMarkdownFormatFlags_
{
    ImGuiMarkdownFormatFlags_None = 0,
    ImGuiMarkdownFormatFlags_DiscardExtraNewLines = 1 << 0,         // (Accurate parsing) Provided markdown will discard all redundant newlines
    ImGuiMarkdownFormatFlags_NoNewLineIfHeadingFirstLine = 1 << 1,  // (Accurate parsing) Provided markdown will not format a newline after the first line if it is a heading
    ImGuiMarkdownFormatFlags_SeperatorDoesNotAdvance = 1 << 2,      // (Accurate parsing) Provided markdown will not advance to the next line after formatting a seperator
    ImGuiMarkdownFormatFlags_GithubStyle = ImGuiMarkdownFormatFlags_DiscardExtraNewLines | ImGuiMarkdownFormatFlags_NoNewLineIfHeadingFirstLine | ImGuiMarkdownFormatFlags_SeperatorDoesNotAdvance;
    ImGuiMarkdownFormatFlags_CommonMarkAll = ImGuiMarkdownFormatFlags_DiscardExtraNewLines | ImGuiMarkdownFormatFlags_NoNewLineIfHeadingFirstLine | ImGuiMarkdownFormatFlags_SeperatorDoesNotAdvance;
};

Specifically ImGuiMarkdownFormatFlags_GithubStyle. Here is the thought process: ImGuiMarkdownFlags may or may not include flags in the future that are not specifically designed for accurate rendering. Not only that but we may want to target other popular Markdown formatting styles in the future. By designating a combinatorial enum to a specific style, it not only gives us the future proofing of being able to discriminate flags specifically for a style, but it also makes it much clearer to a client of this library as to how they should achieve this accurate "GitHub" markdown formatting style. It also serves as a bit of contextual documentation for the individual flags this way.

[dougbinks]

I'll design it in the code to follow the same design philosophy as ImGui itself (int typedef, enum has trailing underscore and placed in the global scope, etc).

Yes, that would be best.

Question: How do you feel about something like this?

That's great!

[JoshuaMKW]

Alright, this PR is now ready to be merged whenever you are ready.

[dougbinks]

It looks like there's one minor missing test, see notes above.

I'll be likely too busy for the next two days but will merge this as soon as I can once that's fixed.

[JoshuaMKW]

It looks like there's one minor missing test, see notes above.

I'll be likely too busy for the next two days but will merge this as soon as I can once that's fixed.

Sorry, what is this minor missing test? If you mean the lack of || !markdownFormatInfo_.firstLine, that's because I found that this actually produced inaccurate results compared to simply not having it in the conditional.

If this is what you pointed out, then we should still be good to go. Otherwise I'll need you to point it out to me if you can.

[dougbinks]

If you mean the lack of || !markdownFormatInfo_.firstLine, that's because I found that this actually produced inaccurate results compared to simply not having it in the conditional.

Yes I mean this test.

If this is what you pointed out, then we should still be good to go. Otherwise I'll need you to point it out to me if you can.

Without this test there is no reason for the firstLine variable and we should change the flag ImGuiMarkdownFormatFlags_NoNewLineIfHeadingFirstLine to something like ImGuiMarkdownFormatFlags_NoNewLineBeforeHeading.

[JoshuaMKW]

I have made the requested changes and tested the PR, it all looks good!

[dougbinks]

Could you undo the formatting changes in 23966fe which are unrelated to any functionally modified code? It might be easiest to simply redo that commit without the formatting change.

If I do move this header to ImGui style formatting (which I find hard to read) then I would prefer to do this outside of a functional change. At the moment it is difficult for me to review the PR due to amount of formatting changes.

[JoshuaMKW]

Could you undo the formatting changes in 23966fe which are unrelated to any functionally modified code? It might be easiest to simply redo that commit without the formatting change.

If I do move this header to ImGui style formatting (which I find hard to read) then I would prefer to do this outside of a functional change. At the moment it is difficult for me to review the PR due to amount of formatting changes.

Oh, yes of course I can take care of that. Sorry my IDE likes to auto format (configurations for my own projects) and sometimes I forget to reverse the formatting before pushing the commit. I should have this fixed soon.

On the topic of formatting, maybe it would be worth including a .clang-format file in the future? You can set it up however you like and require future contributions to run the formatter before pushing a commit. I use clang format for my own projects. (The typical IDE default is to override any IDE formatting configuration with whatever .clang-format specifies)

[dougbinks]

Thanks!

On the topic of formatting I'd rather not add clang autoformat for now.

[JoshuaMKW]

Thanks!

On the topic of formatting I'd rather not add clang autoformat for now.

For sure. Just to clarify, I meant at some unspecified point in the future, not this PR. Regardless I will follow your command in that regard haha. I'm preparing for a proposal due tomorrow so it may be a bit before I get that commit done 😅

[dougbinks]

I'm preparing for a proposal due tomorrow so it may be a bit before I get that commit done 😅

Good luck, and no rush for the PR!

[JoshuaMKW]

@dougbinks I have redone that particular commit to remove the formatting changes I accidentally included, and I took the liberty of consolidating the last commit that did a redundant formatting fix into this new commit. It's been force pushed to erase the original two commits, so the commit history is a bit nicer.

Also she said yes, so hooray to that lol!

[JoshuaMKW]

Which, now that the library is looking forward to a set of extensible feature flags, how do you feel about PRs like #14 being merged into imgui_markdown as default off, much like the features presented by this PR and #42 ? The prior rationale was to keep the library lightweight and simple, which I do agree with. However now that we can easily filter features by flag, it gives the library the fun opportunity to reconsider these features/characteristics without abandoning the original philosophy.

[dougbinks]

I'll look at the PR merge shortly as I'm very busy for the next couple of days.

For the other PRs feature flags do sound like a good way to handle these, though we need to ensure the library remains fast.

I have been planning a revised version of imgui_markdown at some point which can pre-parse text into an array of tokens which can then be used to render the output, or the tokens can be rendered immediately for the same behaviour as current. This could allow more complex parsing, indeed I was considering using a library like Sundown or md4c with an imgui backend for full markdown compliance. However this is a larger undertaking so extending the current code is reasonable so long as we don't decrease performance or significantly increase complexity.