feat: treat markdown content in overview text better#1193
feat: treat markdown content in overview text better#1193rozPierog wants to merge 3 commits intodamontecres:mainfrom
Conversation
|
@rozPierog when you submit a PR that resolves an issue, please use the correct keywords like "fixes" or "resolves" according to https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests to autoclose issues when your PR gets merged so we keep the issue page tidy. |
damontecres
left a comment
There was a problem hiding this comment.
I don't see it specifically mentioned in Jellyfin's docs as supported, but it looks like the web UI actually renders the overview as Markdown.
Fortunately, Wholphin already has a markdown library which includes a Markdown composable. See an example used here.
My only concern with using the Markdown composable is the loss of the maxLines parameter, so there might be layout issues that need to be addressed as well.
|
@damontecres I see that Markdown library that you are using is also exposing |
Nice, that has potential. I am concerned about parsing the markdown in compose code though since that would be on the main thread. At the very least use But it might be good to parse the content into the Take a look at the |
|
I'm worried that this won't be as easy as it look at the start. We cannot build markdown annotated strings outside of the And even if I create reimplementation of the Markdown component then I would have control over paragraph rendering, but given that Markdown can have multiple paragraphs, and maxLines would apply to them separately it seems much less useful. I think best approach would be to
That's additional string manipulation, possibly it could be precomputed into |
|
I think I'm overthinking this and trying to pre-optimize. I think most overview texts will be plaintext and overhead of the markdown parsing plaintext should be tiny. So, @rozPierog I think you can implement it with the And if it looks good, I'll run the profiler with some sample overviews and see if there's any significant performance change. |
|
I think it got lost in the noise in my previous comment, but I cannot use |
I poked around more in the library and its docs and it would be very complicated to use So I think your proposal is an easy solution that solves most of the problem. |
- Add TextUtils.kt with String.stripMarkdown to cleanly show media overview text
|
@damontecres I added a stripping util, added tests around it, and moved the ItemDetailsDialogInfo overview rendering to Markdown component. Could you take a look if that makes sense to you? |
| if (overview.isNotNullOrBlank()) { | ||
| Text( | ||
| text = overview, | ||
| text = overview.stripMarkdown(), |
There was a problem hiding this comment.
I don't think the overview needs any processing here. I would just leave it as is.
Doing the processing here will run on the main thread. And pre-computing it will a bit more memory overhead which I'm trying to limit.
Description
Strips markdown tags from Overview text, and renders overview text in Markdown component inside ItemDetailsDialogInfo.kt
Related issues
fixes #1186
Testing
Open overview text of an episode/series/movie that contains html tags (e.g
<br>)Screenshots
AI or LLM usage
LLMs were used to create comprehensive regex list of markdown to strip, as well as in preparing the unit tests cases