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

Allow getting ephemeral followups #533

Merged
merged 10 commits into from
Mar 5, 2022
Merged

Allow getting ephemeral followups #533

merged 10 commits into from
Mar 5, 2022

Conversation

lukellmann
Copy link
Member

@lukellmann lukellmann commented Feb 12, 2022 •
edited
Loading

Changes

  • change return type of getFollowupMessage[OrNull]() in InteractionResponseBehavior and EntitySuppliers from PublicFollowupMessage to FollowupMessage:
    You can not only get public followups but also ephemeral ones, this isn't documented yet though (it was for a short time but got reverted). Instead of hard coding the visibility of the followup message for these methods decide on the actual type by the flags of the message. This is safe for any changes Discord decides to make.
  • rename message type ApplicationCommand to ChatInputCommand (changed in discord/discord-api-docs@3973676)
  • change FollowupMessageCreateBuilder.ephemeral from var to val to prevent something like this:
val followup: PublicFollowupMessage = interactionResponse.followUpPublic {
    content = "foo"
    ephemeral = true // this will result in an ephemeral followup but the function still returns PublicFollowupMessage
}
followup.delete() // this will fail, you can't delete ephemeral followups

Additions

@lukellmann
Copy link
Member Author

Let's see what happens to this: discord/discord-api-docs#4496

@lukellmann
Copy link
Member Author

Let's see what happens to this: discord/discord-api-docs#4496

Ok, seems like getting ephemeral followup messages is not official (yet?) and subject to change.

I would still keep the type change from PublicFollowupMessage to FollowupMessage for these functions. If we receive a message with the ephemeral flag set it is ephemeral and not public.

@lukellmann
Copy link
Member Author

waiting for #531

@lukellmann lukellmann marked this pull request as draft February 12, 2022 16:20
@lukellmann
Merge branch '0.8.x' into changes/get-ephemeral-followups
43b53c0 
# Conflicts:
#	core/src/main/kotlin/behavior/interaction/response/InteractionResponseBehavior.kt
#	core/src/main/kotlin/entity/Message.kt
#	core/src/main/kotlin/entity/interaction/FollowupMessage.kt
#	core/src/main/kotlin/event/interaction/ApplicationCreate.kt
#	core/src/main/kotlin/supplier/CacheEntitySupplier.kt
#	core/src/main/kotlin/supplier/EntitySupplier.kt
#	core/src/main/kotlin/supplier/FallbackEntitySupplier.kt
#	core/src/main/kotlin/supplier/RestEntitySupplier.kt
#	core/src/main/kotlin/supplier/StoreEntitySupplier.kt
@lukellmann
make it work after merge of interaction refactor
7622361
@lukellmann
delete empty file
976d126
@lukellmann
override withStrategy
39e23f7
@lukellmann
delete another empty file
3e5d679
@lukellmann lukellmann marked this pull request as ready for review February 21, 2022 23:56
@lukellmann
Copy link
Member Author

This is now ready, I merged the interaction refactor into this.

@HopeBaron
Copy link
Member

does getting them means we can delete?

@lukellmann
Copy link
Member Author

does getting them means we can delete?

Nope, that did not work the last time I checked. So we still need the type distinction between public/ephemeral. The user can just check with is PublicFollowupMessage and then delete.

HopeBaron
HopeBaron reviewed Feb 24, 2022
View reviewed changes
core/src/main/kotlin/supplier/RestEntitySupplier.kt
@@ -440,11 +440,11 @@ public class RestEntitySupplier(public val kord: Kord) : EntitySupplier {
applicationId: Snowflake,
interactionToken: String,
messageId: Snowflake,
): PublicFollowupMessage? = catchNotFound {
Copy link
Member

Choose a reason for hiding this comment

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

Why is it that the types here changed

Copy link
Member Author

Choose a reason for hiding this comment

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

When the endpoint returns a message with the ephemeral flag it shouldn't be a PublicFollowupMessage.

Copy link
Member

Choose a reason for hiding this comment

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

so we are going to use flags to determine ephemeral and public?

Copy link
Member Author

@lukellmann lukellmann Feb 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That's exactly what the FollowupMessage() function does. The edit extension function for FollowupMessageBehavior already did this before (see here).

Copy link
Member Author

Choose a reason for hiding this comment

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

Or is there a downside to this that I'm missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then what's the point of the distinction?

Copy link
Member

Choose a reason for hiding this comment

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

you know Discord have the ephmeral flag; we don't usually compare by types and flags for entites
e.g DmChannel and TextChannel; they have their own types
Ephemeral and Public are parts of the concepts of interactions; also it's nice to express the distinction as an is rather than ==; it reads better and being a firm type makes it good.

On the other hand; your PR seems to have changed some of the parts that are going to change in V10 (which are not that hard to migrate to discord/discord-api-docs#4510

not sure abotu the attachment patch thing yet tho

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok makes sense to have separate types for this.

So the thing you suggest now is to migrate all methods on followup messages to FollowupMessageBehavior (and including a warning that it might not work for ephemerals)?

On the other hand; your PR seems to have changed some of the parts that are going to change in V10 (which are not that hard to migrate to discord/discord-api-docs#4510

Not sure which changes exactly you are referring to here.

Copy link
Member

Choose a reason for hiding this comment

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

[ALL VERSIONS] application.summary now returns an empty string. This field will be removed in v11

although we need a separate PR for this

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, this could be in scope for #530

@lukellmann
Copy link
Member Author

Discord removed the ability to edit ephemeral followups two days ago (see discord/discord-api-docs#4414). Should we remove this function for ephemeral followups then?

@lukellmann
Copy link
Member Author

Discord removed the ability to edit ephemeral followups two days ago (see discord/discord-api-docs#4414). Should we remove this function for ephemeral followups then?

Seems like they reverted that, it's working again.

@HopeBaron
Copy link
Member

Any official word on it?

@lukellmann
Copy link
Member Author

Any official word on it?

I've taken this from @DRSchlaubi (https://discord.com/channels/556525343595298817/631147109311053844/949635018844692530), nothing official.

@lukellmann
Copy link
Member Author

lukellmann commented Mar 5, 2022 •
edited
Loading

I also found this message: https://discord.com/channels/613425648685547541/788586647142793246/947415392530337812
Response to where this info came from: https://discord.com/channels/613425648685547541/788586647142793246/949673099161382912

@HopeBaron
Copy link
Member

Since there is no change on this yet ig we can reintroduce them; this is kinda annoying

@lukellmann
Copy link
Member Author

I didn't even remove the ability to edit ephemeral followups in the first place since it wasn't clear what would happen next, so we are all set in this regard.

@HopeBaron HopeBaron merged commit 9bdb20e into kordlib:0.8.x Mar 5, 2022
@lukellmann lukellmann deleted the changes/get-ephemeral-followups branch March 5, 2022 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HopeBaron HopeBaron HopeBaron left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lukellmann @HopeBaron