-
-
Notifications
You must be signed in to change notification settings - Fork 89
Rigorous unordered list detection and feature-complete indentation support #42
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
base: dev
Are you sure you want to change the base?
Conversation
|
Notably, this changes the original behavior of imgui_markdown from indenting by leadingSpaces / 2, to an actual markdown specification of indenting. This could be considered a breaking change that will cause side-effects for long term users of this library. However, I find the authenticity of markdown in a markdown library for ImGui to be paramount. The existing implementation of markdown using these leading spaces in this way is inauthentic and could be described as a sibling implementation of markdown. I understand that caution needs to be exercised to make sure things go as smoothly as possible, so if any ideas are presented, I'd be happy to discuss and implement them. |
|
Thanks for these PRs. This is a fairly big change and will take me a while to review, and I have ongoing work with a deadline for the next week. Note that we do consider a change of behaviour to be a breaking change. As such we'd need to add a config setting to enable this new formatting over the old one. ImGui_Markdown has always been intended as a simple fast version of Markdown, preferring simplicity over exact following of the spec. However if we can remain simple and have a version which follows the spec better that would be appreciated, but a silent breaking change is bad, many users may have markdown in their help window which they don't check when they do a build etc., and I don't want to force users to rewrite their mardown if there's no benefit for them in doing so. |
imgui_markdown.h
Outdated
|
|
||
| namespace ImGui | ||
| { | ||
| // Configuration - internal use only |
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.
Even if this is internal configuration it should go in the config with the default and marked internal similarly to how ImGui itself does. I also don't want to introduce unnamed namespaces under the ImGui namespace as a header.
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.
That's a good rationale. I can definitely make that change.
Yes, I figured this would be the case but I was hesitant to start implementing config setting until I had word from a maintainer. That way I can do so in a guided way that matches the expectations of the library. I'm assuming you'd want such a flag to be contained in MarkdownConfig? Also, this implementation of proper markdown indentations is pretty lightweight, all things considered. It does not introduce much overhead at all. Possibly the most notable overhead is the extra stack memory being used to store the indent lines. |
Yes, I think that's the best place.
Great. That seems reasonable. I won't be able to respond now for a while, but will review these in a week or so. |
d22367c to
ed4b763
Compare
7f3411b to
f175ad1
Compare
|
@dougbinks This PR has been updated to use the code from #41 and seems to be correct still through testing. Feel free to review and let me know of any changes to make. P.S. This PR has highlighted some interesting newline characteristics in markdown that I want to tackle in a future PR. |
|
Note I'll be a bit busy for a while, so no rush with any fixes. |
This PR makes the detection of unordered lists match markdown spec (+, -, * are all valid unordered list markers).
It also implements fully featured indentation according to markdown spec (GitHub etc), allowing for much more versatility and greater support of existing markdown files that likely don't match the original implementation of imgui_markdown.
As an example, I tested the formatting of a pretty messed up and complex markdown file to see that things worked as expected.
Given the following input (forgive the literal keyboard smashing):
The following render is created (GitHub on the left, imgui_markdown on the right):
For reference, this is how imgui_markdown renders this text on the main and dev branch:
This is obviously incorrect in ways that are not close to the actual markdown specification. Before this PR, trying to render markdown files from repositories on GitHub very often breaks in these ways.
Notably the vertical spacing is a bit wonky. This is fixed by #41 which I implemented as well. This PR is currently a draft because it relies on some fixes that exist in #41, so there is going to be merge conflicts I will have to resolve once #41 is merged into the dev branch.