Fix Markdown rendering issues #458
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The vast majority of Markdown use cases will be displaying static text, so trying to optimise for an editor-like scenario is counter-productive. This PR sets the flag to false by default. This commit also cleans up the MarkdownProcessor code and kdocs.
The value classes we used to use would rely on the CommonMark models for this, but none of the CommonMark models actually implement equals() and hashCode(), leading to issues in tests and in Compose.
Now links point to GitHub, so the app doesn't crash when you click them.
Tests have been updated to match the new behaviour, that's closer to the CommonMark one. Now we don't carry unparsed inline Markdown anymore, but rather we fully parse it in advance, so rendering it later is easier and doesn't require running parsing in the UI anymore. This is a tradeoff in memory usage for speed of rendering, and it makes sense in the context of most Markdown text being static and only parsed once, then kept on screen. Inline extensions are now possible, but not tested. This makes #325 possible, since the extension now can be moved to its own module. Note: please don't look too much at the test changes. They're _verbose_, and are only there to ensure CommonMark specs compliance.
This is not tested, and not used yet, so it may or may not need tweaks down the line. We'll find out when we implement the first extension.
hamen
reviewed
Jul 19, 2024
| @@ -130,7 +130,7 @@ The standalone theme can be used in any Compose for Desktop app. You use it as a | |||
| to your heart's content. By default, it matches the official Int UI specs. | |||
|
|
|||
| For an example on how to set up a standalone app, you can refer to | |||
| the [`standalone` sample](samples/standalone/build.gradle.kts). | |||
| the [`standalone` sample](https://github.com/JetBrains/jewel/blob/main/samples/standalone/build.gradle.kts). | |||
There was a problem hiding this comment.
I thought about doing this when I worked on links 👍🏻
obask
approved these changes
Jul 19, 2024
markdown/core/src/main/kotlin/org/jetbrains/jewel/markdown/InlineMarkdown.kt
Show resolved
Hide resolved
markdown/core/src/main/kotlin/org/jetbrains/jewel/markdown/processing/ProcessingUtil.kt
Show resolved
Hide resolved
This was referenced Jul 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes a number of things in the Markdown rendering pipeline:
1. We will not optimise for edits by default anymore
The vast majority of Markdown usages are in UI text, which will never change. Having optimise for edits enabled by default added an enormous overhead since the same processor instance was shared across all usages, and we were doing pointless work every time some UI text was shown. All strings in this case are different, and they don't change often (or at all).
The flag is now off by default. Users who want to implement an editor preview will need to provide their own processor instance with the flag set (see standalone sample).
2. Models implement equals and hashCode
Using value classes had a small rendering performance and memory improvement, but it meant that we relied on CommonMark's models for equality checking and hashcode calculation. CommonMark's models do not implement these methods, so we were stuck with reference checks, which even if the content was identical would result in a recomposition.
This PR gets rid of the value classes and switches to normal classes, backed by Poko autogenerated equals/hashCode/toString. Inline Markdown is not passed through as a string anymore, to be rendered into inline models during UI rendering. Now the rendering phase only needs to visit our models, making it significantly faster.
Also, models don't contain business logic anymore; it's the processor's job to know how to translate CommonMark models to our own.
A huge amount of changes in this PR are needed to adapt tests accordingly. Feel free to ignore the changes in
MarkdownProcessorDocumentParsingTest, they're all mechanical changes but functionally equivalent.3. Inline extensions are now (tentatively) supported
There is a new extension API for custom inline node processing and rendering. We have not tested it yet, nor we have used it yet. However, it should make possible to implement #325 properly in a separate module, instead of dumping it in markdown-core.
I expect the inline rendering API might need some more work before being fully functional; we'll see as we start using it.
4. Code cleanup
✨