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.

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

Implement invite targets #1628

Merged

Conversation

anweisen
Copy link
Contributor

@anweisen anweisen commented May 22, 2021

Pull Request Etiquette

Changes

  • Internal code
  • Library interface (affecting end-user code)
  • Documentation
  • Other: _____

Closes Issue: NaN

Description

This pr provides us with the functionality to create invites for applications and streams.
This allows us to use the discord together features.

The feature is currently only available in the desktop app and the web version.
The invites will show up as normal voice channel invites in the Android and iOS versions.

Examples

Embedded Applications

You can create an invite to an embedded application using

VoiceChannel channel = ...;
channel.createInvite()
    .setTargetApplication("755600276941176913") // Application id for youtuber together
    .queue();

There are currently only a few applications you can use, i listed all I currently know with their application id, so you can try them out yourself if you want:

  • YouTube Together: 755600276941176913, 880218832743055411
  • Poker: 755827207812677713
  • Betrayal.io: 773336526917861400
  • Fishington.io: 814288819477020702
  • Chess / CG 2 Dev: 832012586023256104, 832012774040141894
  • Awkword: 879863881349087252
  • Spellcast: 852509694341283871
  • Doodlecrew: 878067389634314250
  • Wordsnack: 879863976006127627
  • Lettertile: 879863686565621790

Stream Invites

You can create an invite to an user stream using

VoiceChannel channel = ...;
channel.createInvite()
    .setTargetStream(user) // You can use an User object or the id of the user here
    .queue();

The user to whose stream the invite goes must be streaming in the channel on which the invite is being created.

Invite Properties

This pr also adds the following properties with the following getters to an Invite object

  • getTargetType(): Invite.TargetType
  • getTargetApplication(): Invite.EmbeddedApplication
  • getTargetUser(): User

@Andre601
Copy link
Contributor

Is this an actual planned feature of Discord?
The docs don't show any PRs for it...

@KxmischesDomi
Copy link

Is this an actual planned feature of Discord?
The docs don't show any PRs for it...

It is already implemented in the desktop app.
Look at this: https://www.reddit.com/r/discordapp/comments/jtnso2/social_party_games_on_discord/
There is still no way of creating these invites with the discord app.

@anweisen
Copy link
Contributor Author

anweisen commented May 23, 2021

Is this an actual planned feature of Discord?
The docs don't show any PRs for it...

It is already implemented in the desktop app.
Look at this: https://www.reddit.com/r/discordapp/comments/jtnso2/social_party_games_on_discord/
There is still no way of creating these invites with the discord app.

The corresponding properties of the invite object and in the creation request are already in the docs

@Sanduhr32
Copy link
Contributor

Question: Why do you used boxed longs instead of native once?

@anweisen
Copy link
Contributor Author

anweisen commented May 23, 2021

Question: Why do you used boxed longs instead of native once?

I thought it would be better to use them as they can be null and this property is not required
They were also used for other properties which are not required like maxAge and maxUses

@anweisen anweisen marked this pull request as ready for review May 23, 2021 19:32
@RedDaedalus
Copy link
Contributor

Is this an actual planned feature of Discord?
The docs don't show any PRs for it...

This is also enabled on the official Poker Night server.

@anweisen anweisen changed the title Initial pass on discord together Implement invite targets May 24, 2021
@DV8FromTheWorld DV8FromTheWorld added the status: freezer this is currently put on hold label May 25, 2021
@DV8FromTheWorld
Copy link
Member

JDA only implements features exposed in the discord public documentation. If this feature gets documented there then we can revisit this.

Setting this PR on freezer until then.

@MinnDevelopment MinnDevelopment added size: small and removed status: freezer this is currently put on hold labels Jun 12, 2021
Copy link
Member

@DV8FromTheWorld DV8FromTheWorld left a comment

Choose a reason for hiding this comment

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

I'm definitely nervous about this API.
All of these setters implicitly set the TargetType based on tangentially related data passed in.

What happens when discord adds a TargetType 3 that also only needs a user?

Feels like something like setTarget(Target.STREAM.of(User)) and setTarget(Target.EMBEDDED_APPLICATION.of(long)) would be more likely to work further into the future.

Note: I don't like the above proposed builders, they were purely for example purposes. The point was the explicit targetting instead of the implicit targeting via tangential information

src/main/java/net/dv8tion/jda/api/entities/Invite.java Outdated Show resolved Hide resolved
src/main/java/net/dv8tion/jda/api/entities/Invite.java Outdated Show resolved Hide resolved
Comment on lines 83 to 98
case STREAM:
DataObject targetUserObject = content.getObject("target_user");
targetUser = getJDA().getEntityBuilder().createUser(targetUserObject);
application = null;
break;
case EMBEDDED_APPLICATION:
DataObject applicationObject = content.getObject("target_application");
application = new InviteImpl.EmbeddedApplicationImpl(
applicationObject.getString("icon", null), applicationObject.getString("name"), applicationObject.getString("description"),
applicationObject.getString("summary"), applicationObject.getLong("id"), applicationObject.getInt("max_participants", -1)
);
targetUser = null;
break;
default:
application = null;
targetUser = null;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
case STREAM:
DataObject targetUserObject = content.getObject("target_user");
targetUser = getJDA().getEntityBuilder().createUser(targetUserObject);
application = null;
break;
case EMBEDDED_APPLICATION:
DataObject applicationObject = content.getObject("target_application");
application = new InviteImpl.EmbeddedApplicationImpl(
applicationObject.getString("icon", null), applicationObject.getString("name"), applicationObject.getString("description"),
applicationObject.getString("summary"), applicationObject.getLong("id"), applicationObject.getInt("max_participants", -1)
);
targetUser = null;
break;
default:
application = null;
targetUser = null;
case STREAM:
DataObject targetUserObject = content.getObject("target_user");
targetUser = getJDA().getEntityBuilder().createUser(targetUserObject);
application = null;
break;
case EMBEDDED_APPLICATION:
DataObject applicationObject = content.getObject("target_application");
application = new InviteImpl.EmbeddedApplicationImpl(
applicationObject.getString("icon", null), applicationObject.getString("name"), applicationObject.getString("description"),
applicationObject.getString("summary"), applicationObject.getLong("id"), applicationObject.getInt("max_participants", -1)
);
targetUser = null;
break;
default:
application = null;
targetUser = null;

Copy link
Contributor Author

@anweisen anweisen Jun 15, 2021

Choose a reason for hiding this comment

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

I didn't indent these cases as they weren't indented in other switches I saw (eg in the EntityBuilder class) and the indent for them are set to false in the editorconfig. Does it make sense to indent them?

@anweisen
Copy link
Contributor Author

I'm definitely nervous about this API.
All of these setters implicitly set the TargetType based on tangentially related data passed in.

What happens when discord adds a TargetType 3 that also only needs a user?

Feels like something like setTarget(Target.STREAM.of(User)) and setTarget(Target.EMBEDDED_APPLICATION.of(long)) would be more likely to work further into the future.

Note: I don't like the above proposed builders, they were purely for example purposes. The point was the explicit targetting instead of the implicit targeting via tangential information

Maybe it would be better to use it like this:
setTargetType(TargetType.STREAM)
setTargetUser(user)

setTargetUser will then not set the target type to STREAM, so it has to be done by hand.
This would allows us to integrate a new type which uses the same setters.
But this approach is not ideal because you don't really know what you should/can set and what is for other target types.

Requested by DV8FromTheWorld

Co-authored-by: Austin Keener <keeneraustin@yahoo.com>
@MinnDevelopment
Copy link
Member

I think it's ok the way it is now, if in the future new target types get introduced we could add an overload that accepts a target type parameter. Currently, this is not needed since you can only have one target type per user/application anyway.

@carstendachsbacher
Copy link

Any updates on this? I would love to use this feature

@Xirado
Copy link
Contributor

Xirado commented Aug 27, 2021

Any updates on this? I would love to use this feature

@carstendachsbacher In the meantime you can use something like this
https://github.com/Xirado/HTLDiscordBot/blob/master/src/main/java/at/Xirado/htl/bot/commands/VoiceTargetCommand.java#L38

Copy link
Member

@MinnDevelopment MinnDevelopment left a comment

Choose a reason for hiding this comment

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

I think we can merge this. @DV8FromTheWorld any reason to still hold this up?

src/main/java/net/dv8tion/jda/api/entities/Invite.java Outdated Show resolved Hide resolved
Copy link
Member

@DV8FromTheWorld DV8FromTheWorld left a comment

Choose a reason for hiding this comment

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

Some questions

src/main/java/net/dv8tion/jda/api/entities/Invite.java Outdated Show resolved Hide resolved
src/main/java/net/dv8tion/jda/api/entities/Invite.java Outdated Show resolved Hide resolved
@DV8FromTheWorld
Copy link
Member

After all this time I still don't think that having setTargetApplication and setTargetUser and their respective getters are the correct move.

setTarget(InviteTarget.APPLICATION.fromId(...)) and getTarget().getType/getUser/getApplication seems better.

Theres also nothing that indicates to the users that .setTargetUser(...).setTargetApplication(...) is wrong.

@DV8FromTheWorld DV8FromTheWorld added the Required for v4 LTS defines that something is slated for inclusion in the final v4 LTS builds label Sep 19, 2021
@Xirado
Copy link
Contributor

Xirado commented Sep 21, 2021

It seems like Discord removed the ability to use these applications. The invite link is no longer clickable.

My mistake

@MinnDevelopment
Copy link
Member

@Xirado works for me

@Xirado
Copy link
Contributor

Xirado commented Sep 21, 2021

@MinnDevelopment Very weird. Maybe it's just my client? Me and my friends can't click the invite link even though it says Poker below it

My mistake

Copy link
Member

@MinnDevelopment MinnDevelopment left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. We spoke internally and came up with some changes that should hopefully be acceptable to everyone.

Copy link
Member

@MinnDevelopment MinnDevelopment left a comment

Choose a reason for hiding this comment

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

Just a few more points, and then it should be ready to go

src/main/java/net/dv8tion/jda/api/entities/Invite.java Outdated Show resolved Hide resolved
src/main/java/net/dv8tion/jda/api/entities/Invite.java Outdated Show resolved Hide resolved
src/main/java/net/dv8tion/jda/api/entities/Invite.java Outdated Show resolved Hide resolved
src/main/java/net/dv8tion/jda/api/entities/Invite.java Outdated Show resolved Hide resolved
src/main/java/net/dv8tion/jda/api/entities/Invite.java Outdated Show resolved Hide resolved
@MinnDevelopment
Copy link
Member

One thing I forgot, please also add Invite#getTargetType so you can use it in a switch without a null-check.

Copy link
Member

@MinnDevelopment MinnDevelopment left a comment

Choose a reason for hiding this comment

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

Thanks for your efforts! LGTM

@DManstrator
Copy link
Contributor

There are currently only a few applications you can use, i listed all I currently know with their application id, so you can try them out yourself if you want:

YouTube Together: 755600276941176913
Poker: 755827207812677713
Betrayal.io: 773336526917861400
Fishington.io: 814288819477020702
Chess / CG 2 Dev: 832012586023256104

How about creating an enum for that and allow it as a parameter for setTargetApplication for a better usability?

@MinnDevelopment
Copy link
Member

@DManstrator we don't want to provide an enum for undocumented and potentially temporary applications. If more applications exist in the future, we don't want to curate such a list either.

@DManstrator
Copy link
Contributor

Didn't knew it was still undocumented, makes sense then not to have that enum. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Required for v4 LTS defines that something is slated for inclusion in the final v4 LTS builds type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet