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

Add support for guild welcome screens #1989

Closed
wants to merge 10 commits into from
Closed

Add support for guild welcome screens #1989

wants to merge 10 commits into from

Conversation

mlnrDev
Copy link
Contributor

@mlnrDev mlnrDev commented Jan 15, 2022

Pull Request Etiquette

Changes

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

Closes Issue: #1987

Description

this PR adds support for Guild welcome screens. this currently doesn't support modifying the welcome screen as I'm still figuring out a good design.

* @return The id of the emote that is used for this recommended channel or {@code null}
*/
@Nullable
public String getEmoteId()
Copy link
Contributor Author

@mlnrDev mlnrDev Jan 15, 2022

Choose a reason for hiding this comment

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

I'm not sure whether I should wrap this (while removing the getter below) into an Emoji object instead. The class constructor accepts the animated field which we can't know, but a recommended channel's emote can be animated.

Copy link
Member

Choose a reason for hiding this comment

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

We can't tell by the emote's id via something like a_ prefix or whatever?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, the object only contains the emote id and name, not the hash

*
* <p>Possible {@link net.dv8tion.jda.api.requests.ErrorResponse ErrorResponses} include:
* <ul>
* <li>{@link net.dv8tion.jda.api.requests.ErrorResponse#UNKNOWN_GUILD_WELCOME_SCREEN Unknown Guild Welcome Screen}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should convert this error into a null instead of forcing users to handle it as a user.

* @see Guild#retrieveWelcomeScreen()
* @see Invite.Guild#getWelcomeScreen()
*/
public class GuildWelcomeScreen
Copy link
Member

Choose a reason for hiding this comment

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

This should have a getGuild getter

@Nullable
public BaseGuildMessageChannel getChannel()
{
return (BaseGuildMessageChannel) api.getGuildChannelById(id);
Copy link
Member

Choose a reason for hiding this comment

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

This is questionable as I could see ForumChannels being a recommendable channel in the future.

* @return The id of the emote that is used for this recommended channel or {@code null}
*/
@Nullable
public String getEmoteId()
Copy link
Member

Choose a reason for hiding this comment

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

We can't tell by the emote's id via something like a_ prefix or whatever?

* @return The name of the emoji that is used for this recommended channel or {@code null}
*/
@Nullable
public String getEmojiName()
Copy link
Member

Choose a reason for hiding this comment

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

So this method can return a name or a unicode emoji? Yeah, not sure how to feel about that representation given the method naming..

* @return The welcome screen of this Guild or {@code null}
*/
@Nullable
GuildWelcomeScreen getWelcomeScreen();
Copy link
Member

Choose a reason for hiding this comment

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

So all resolved invites have this information, but the Guild itself doesnt? Curious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exactly, you have to retrieve it yourself.

Comment on lines +1905 to +1906
welcomeChannels.add(new GuildWelcomeScreen.Channel(api, welcomeChannelObj.getLong("channel_id"), welcomeChannelObj.getString("description"),
welcomeChannelObj.getString("emoji_id", null), welcomeChannelObj.getString("emoji_name", 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
welcomeChannels.add(new GuildWelcomeScreen.Channel(api, welcomeChannelObj.getLong("channel_id"), welcomeChannelObj.getString("description"),
welcomeChannelObj.getString("emoji_id", null), welcomeChannelObj.getString("emoji_name", null)));
welcomeChannels.add(new GuildWelcomeScreen.Channel(
api,
welcomeChannelObj.getLong("channel_id"),
welcomeChannelObj.getString("description"),
welcomeChannelObj.getString("emoji_id", null),
welcomeChannelObj.getString("emoji_name", null)
));

@DV8FromTheWorld
Copy link
Member

We should probably include creation/mutation of GuildWelcomeScreens in this PR instead of providing only read-only access.

We can likely use a simple builder system like we do with EntityBuilder to build it up.

@DV8FromTheWorld
Copy link
Member

Something else that I forgot to mention is that GuildWelcomeScreen should be inside Guild as Guild.WelcomeScreen.
This follows the pattern established by other constructs that make up the Guild itself.

@mlnrDev
Copy link
Contributor Author

mlnrDev commented Feb 5, 2022

Something else that I forgot to mention is that GuildWelcomeScreen should be inside Guild as Guild.WelcomeScreen. This follows the pattern established by other constructs that make up the Guild itself.

it's intentionally in its own file because it has an inner Channel class, so I didn't want it to look like this: Guild.WelcomeScreen.Channel

@DV8FromTheWorld
Copy link
Member

I don't personally see a problem with Guild.WelconeScreen.Channel.
The Guild.X classes represent constructs which are partials of a Guild's metadata. That metadata could be represented as Guild#getWelcomeScreenChannels, Guild#getWelcomeScreenDescription. We separate them only so that things logically make sense.

In contrast, something like GuildVoiceState or TextChannel are entities unto themselves. They are not metadata for the Guild.

@Aseeef
Copy link

Aseeef commented Apr 27, 2022

Hey any updates on this one?

@mlnrDev
Copy link
Contributor Author

mlnrDev commented Apr 27, 2022

Hey any updates on this one?

not right now, no. I won't have time for the next 2-3 weeks, so if someone wants to tackle this earlier, let me know.

@Yashar256
Copy link
Contributor

Yeah choosing a good design for modifying the welcome screen seems tricky. One possible design could be to have a welcome screen manager, with methods such as SortedSet<GuildWelcomeScreen.Channel> getWelcomeChannels() to see the order of the welcome screen channels while updating them, and methods such as void putChannelBeforeIndex(int index, GuildWelcomeScreen.Channel channel) and void putChannelAfterIndex(int index, GuildWelcomeScreen.Channel channel) to emulate the drag and drop ordering that people are used to in the client. And of course, more conventional methods for replacing a certain channel, removing a certain channel, etc.

@MinnDevelopment
Copy link
Member

Closing due to inactivity. If anyone wants to continue this based on the current master branch, feel free to open a new PR.

@mlnrDev mlnrDev deleted the feature/welcome-screen branch September 20, 2022 18:08
freya022 added a commit to freya022/JDA that referenced this pull request Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants