Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add contentDisplay and contentStripped to Message #521

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add contentDisplay and contentStripped to Message #521

wants to merge 3 commits into from

Conversation

DRSchlaubi
Copy link
Member

JDA has this functionality, and it has been requested

@DRSchlaubi DRSchlaubi added the enhancement New feature or request label Feb 5, 2022
@DRSchlaubi DRSchlaubi mentioned this pull request Feb 5, 2022
Closed
lukellmann
lukellmann requested changes Feb 6, 2022
View reviewed changes
common/src/main/kotlin/DiscordMarkdownFlavourDescriptor.kt Show resolved Hide resolved
common/src/main/kotlin/DiscordMarkdownFlavourDescriptor.kt Show resolved Hide resolved
core/src/main/kotlin/Markdown.kt Show resolved Hide resolved
core/src/main/kotlin/entity/Message.kt Show resolved Hide resolved
core/src/main/kotlin/entity/Message.kt Show resolved Hide resolved
core/src/main/kotlin/entity/Message.kt
* - Roles to their @RoleName format
* - Emotes (not emojis!) to their :name: format.
*/
public suspend fun contentDisplay(): String = contentDisplay.await()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about timestamps?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Timestamps don't really make sense because they depend on a specific user's time and don't look the same for all users

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, aware of that, just thought I would point this out, even though I can't come up with a solution.

core/src/test/kotlin/MarkdownEscapeTest.kt
import kotlin.test.assertEquals

@TestInstance(TestInstance.Lifecycle.PER_CLASS)
class MarkdownEscapeTest {
Copy link
Member

@lukellmann lukellmann Feb 6, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also add a test for strikethrough, bold, bold italics and multiline block quotes? https://support.discord.com/hc/en-us/articles/210298617-Markdown-Text-101-Chat-Formatting-Bold-Italic-Underline-

core/src/test/kotlin/MarkdownStripTest.kt Show resolved Hide resolved
core/src/test/kotlin/MarkdownStripTest.kt
import kotlin.test.assertEquals

@TestInstance(TestInstance.Lifecycle.PER_CLASS)
class MarkdownStripTest {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as for MarkdownEscapeTest: maybe also tests for bold etc.

@lukellmann
Copy link
Member

Right now I can think a few things that could be probelmatic with this implementation:

  • Message.contentDisplay() will always replace mentions etc. even if they are escaped by \ or inside a code block
  • Messge.contentDisplay() does not handle spoiler tags or timestamps
  • String.parseMarkdown() will not recognize spoiler tags, mentions etc. and therefore cannot escape or strip them (tough I'm not sure if it would be desired behavior)

@DRSchlaubi
Copy link
Member Author

Message.contentDisplay() will always replace mentions etc. even if they are escaped by \ or inside a code block

Are you sure that those mentions will appear in message.mentionedUsers if they're escaped?

@lukellmann
Copy link
Member

Message.contentDisplay() will always replace mentions etc. even if they are escaped by \ or inside a code block

Are you sure that those mentions will appear in message.mentionedUsers if they're escaped?

Oh, you've got a point there, didn't think about that. Probably not.
But it still seems to be a problem with emotes.

@DRSchlaubi
Copy link
Member Author

Problem with spoilers is, that | isn't a valid markdown token and changing the lexer is hard outside the markdown parser itself

@HopeBaron
Copy link
Member

I still can't figure out if this should be in the library or could be some sort of separate thing

@DRSchlaubi
Copy link
Member Author

JDA had a similar utility and we cannot make it a lazy property of Message from another module

@HopeBaron HopeBaron added the on hold Something is preventing this issue from being resolved label Apr 9, 2022
lukellmann
lukellmann reviewed Feb 16, 2023
View reviewed changes
gradle/libs.versions.toml
@@ -70,6 +70,7 @@ atomicfu-plugin = { module = "org.jetbrains.kotlinx:atomicfu-gradle-plugin", ver
binary-compatibility-validator-plugin = { module = "org.jetbrains.kotlinx:binary-compatibility-validator", version.ref = "binary-compatibility-validator" }
ksp-plugin = { module = "com.google.devtools.ksp:symbol-processing-gradle-plugin", version.ref = "ksp" }

markdown = { group = "org.jetbrains", name = "markdown", version = "0.2.4" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be under the other section? it's not a plugin, right?

@lukellmann
Copy link
Member

./gradlew apiDump is missing :)

@DRSchlaubi
Copy link
Member Author

./gradlew apiDump is missing :)

Dang keep forgetting that :)

@lukellmann lukellmann changed the base branch from 0.8.x to 0.9.x March 25, 2023 17:39
@lukellmann lukellmann changed the base branch from 0.9.x to main June 17, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukellmann lukellmann lukellmann requested changes

@HopeBaron HopeBaron Awaiting requested review from HopeBaron

Assignees
No one assigned
Labels
enhancement New feature or request on hold Something is preventing this issue from being resolved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@DRSchlaubi @lukellmann @HopeBaron