-
Notifications
You must be signed in to change notification settings - Fork 21
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
Markdown Rendering in the What's New Panel #432
Conversation
Co-authored-by: Zac Sweers <[email protected]>
Co-authored-by: Zac Sweers <[email protected]>
…ange_log_parser # Conflicts: # skate-plugin/src/test/kotlin/com/slack/sgp/intellij/ChangeLogParserTest.kt
This comment was marked as outdated.
This comment was marked as outdated.
skate-plugin/build.gradle.kts
Outdated
java { | ||
sourceCompatibility = JavaVersion.VERSION_19 | ||
targetCompatibility = JavaVersion.VERSION_19 | ||
} |
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.
🤔
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.
Yeah, I keep on seeing this in every PR I make. I don't know why it keeps popping up? I think I deleted it though.
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.
It's something with your IDE settings, you should be using JDK 20 but your IDE keeps trying to change this to JDK 19
// Make the file configurable? | ||
// Only show when changed | ||
// Only show latest changes | ||
val settings = project.service<SkatePluginSettings>() |
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.
Can do this in a separate PR but let's make sure we connect these pieces to only show changes since the last update
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.
Yes for sure, should I delete these todo comments for now?
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.
I'd say leave 'em until we actually connect the parser to the previously seen state
skate-plugin/src/main/kotlin/com/slack/sgp/intellij/SkateService.kt
Outdated
Show resolved
Hide resolved
skate-plugin/src/main/kotlin/com/slack/sgp/intellij/ui/WhatsNewPanelFactory.kt
Outdated
Show resolved
Hide resolved
val file = LightVirtualFile("changelog.md", markdownFileString) | ||
val panel = MarkdownJCEFHtmlPanel(project, file) | ||
Disposer.register(parentDisposable, panel) | ||
val html = runReadAction { |
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.
for my own knowledge, what's runReadAction
do?
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.
I believe that runReadAction
is so we can read the markdown content and convert it to html. Here is some documentation for it too: https://plugins.jetbrains.com/docs/intellij/general-threading-rules.html#read-access, and it's used in: https://intellij-support.jetbrains.com/hc/en-us/community/posts/10856308259090-Markdown-Preview-Swing-Component.
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.
Got it, so looking at that first one it's more that it guarantees safe access if we're not on the main thread?
skate-plugin/src/main/kotlin/com/slack/sgp/intellij/ui/WhatsNewPanelFactory.kt
Outdated
Show resolved
Hide resolved
} | ||
|
||
// Control Panel that takes in the current project, markdown string, and a Disposable. | ||
// The Disposable is necessary to prevent a substantial memory leak while working with |
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.
We don't do anything with the parent disposable above though, could you elaborate on the leak it's preventing and how?
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.
I had actually gotten that information from here: https://intellij-support.jetbrains.com/hc/en-us/community/posts/10856308259090-Markdown-Preview-Swing-Component. This is also where I got how to implement MarkdownJCEFHtmlPanel
.
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.
So this only prevents a leak if we dispose the panel somewhere, but I don't see that happening in this code. It looks like we only create the disposable. A disposable is a common concept used for cases where you have some process or resource and you want to be able to "dispose" of it later. We use it a lot in RxJava too. It's only doing its job if we call dispose()
somewhere though, so I think we still have more to do here
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.
Should I be disposing it in SkateService
after the tool window is closed maybe?
This is a PR for markdown file (.md) rendering and formatting for the "What's New at Slack!" side panel in the Skate plugin. This change went through a few iterations because I was having trouble using intelliJ's markdown plugin in Skate, so I had done custom CSS formatting before. However, now I was able to use MarkdownJCEFHtmlPanel to properly format the md files. Here is what it looks like as it depends on the formatting for the markdown plugin with intelliJ: