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

Message interface declutter & mention / component abstraction #1835

Closed
wants to merge 8 commits into from
Closed

Message interface declutter & mention / component abstraction #1835

wants to merge 8 commits into from

Conversation

arynxd
Copy link
Contributor

@arynxd arynxd commented Sep 19, 2021

Pull Request Etiquette

Changes

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

Closes Issue: NaN

Description

This PR implements the new message mentions / components abstractions that are planned for v5. It will be draft until the checklist is complete. This is open for contribution.

  • Decide on nullability for getContentX
  • Decide on behavior on empty components / content
  • Decide on how this should impact internal Message abstractions (AbstractMessage / ReceivedMessage)
  • Decide on the abstraction layout of Message(Mentions/Components), potential interface?

TODO

  • Make getContentX nonnull
  • Throw on empty content access
  • Expose application flags through JDA

@DV8FromTheWorld DV8FromTheWorld marked this pull request as draft September 19, 2021 19:42
@arynxd
Copy link
Contributor Author

arynxd commented Sep 21, 2021

@DV8FromTheWorld would be good if you could comment on the task items.

import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

public class MessageComponents
Copy link
Member

Choose a reason for hiding this comment

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

Lets group the getters in this file by the types. (Buttons with buttons, Menus with menus)

@DV8FromTheWorld DV8FromTheWorld deleted the branch discord-jda:master November 30, 2021 00:07
@DV8FromTheWorld DV8FromTheWorld changed the base branch from v5 to master November 30, 2021 00:21
@MinnDevelopment MinnDevelopment added the status: documentation needed lacks documentation, either partially or completely label Jan 4, 2022
@MinnDevelopment
Copy link
Member

This will also need to address mentions in slash command interactions. Options of type STRING can contain mentions, which are resolved by discord.

Copy link
Contributor

@Sanduhr32 Sanduhr32 left a comment

Choose a reason for hiding this comment

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

@arynxd you still working on this?

this.jda = jda;
this.guild = guild;

this.users = Collections.unmodifiableList(processMentions(Message.MentionType.USER, new ArrayList<>(), true, this::matchUser));
Copy link
Contributor

Choose a reason for hiding this comment

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

You wrap a lot of collections returned by processMentions() with a Collections.unmodifiableList() call.
Maybe change the methods returned collection.

Suggested change
this.users = Collections.unmodifiableList(processMentions(Message.MentionType.USER, new ArrayList<>(), true, this::matchUser));
this.users = processMentions(Message.MentionType.USER, new ArrayList<>(), true, this::matchUser);

}
catch (NumberFormatException ignored) {}
}
return collection;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return collection;
return Collections.unmodifiableList(collection);

Comment on lines +67 to +70
this.roles = Collections.unmodifiableList(processMentions(Message.MentionType.ROLE, new ArrayList<>(), true, this::matchRole));
this.textChannels = Collections.unmodifiableList(processMentions(Message.MentionType.CHANNEL, new ArrayList<>(), true, it -> matchChannel(it, ChannelType.TEXT, TextChannel.class)));
this.voiceChannels = Collections.unmodifiableList(processMentions(Message.MentionType.CHANNEL, new ArrayList<>(), true, it -> matchChannel(it, ChannelType.VOICE, VoiceChannel.class)));
this.emotes = Collections.unmodifiableList(processMentions(Message.MentionType.EMOTE, new ArrayList<>(), true, this::matchEmote));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.roles = Collections.unmodifiableList(processMentions(Message.MentionType.ROLE, new ArrayList<>(), true, this::matchRole));
this.textChannels = Collections.unmodifiableList(processMentions(Message.MentionType.CHANNEL, new ArrayList<>(), true, it -> matchChannel(it, ChannelType.TEXT, TextChannel.class)));
this.voiceChannels = Collections.unmodifiableList(processMentions(Message.MentionType.CHANNEL, new ArrayList<>(), true, it -> matchChannel(it, ChannelType.VOICE, VoiceChannel.class)));
this.emotes = Collections.unmodifiableList(processMentions(Message.MentionType.EMOTE, new ArrayList<>(), true, this::matchEmote));
this.roles = processMentions(Message.MentionType.ROLE, new ArrayList<>(), true, this::matchRole);
this.textChannels = processMentions(Message.MentionType.CHANNEL, new ArrayList<>(), true, it -> matchChannel(it, ChannelType.TEXT, TextChannel.class));
this.voiceChannels = processMentions(Message.MentionType.CHANNEL, new ArrayList<>(), true, it -> matchChannel(it, ChannelType.VOICE, VoiceChannel.class));
this.emotes = processMentions(Message.MentionType.EMOTE, new ArrayList<>(), true, this::matchEmote);


if (guild == null)
{
this.members = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also wrap this.

Suggested change
this.members = new ArrayList<>();
this.members = Collections.unmodifiableList(new ArrayList<>());

@MinnDevelopment MinnDevelopment added this to the 5.0.0-alpha.5 milestone Jan 10, 2022
@Sanduhr32
Copy link
Contributor

This will also need to address mentions in slash command interactions. Options of type STRING can contain mentions, which are resolved by discord.

Should options of types user, channel, role & mentionable also be included in their respective lists?

@DV8FromTheWorld
Copy link
Member

Superseded by #2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: documentation needed lacks documentation, either partially or completely
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants