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 Message and Event receivers #345

Merged
merged 9 commits into from
Jan 27, 2022
Merged

Conversation

Zabuzard
Copy link
Member

@Zabuzard Zabuzard commented Jan 17, 2022

Overview

This implements and closes long awaited #236 and #237 by adding two new interfaces MessageReceiver (+ MessageReceiverAdapter) and EventReceiver to the system.

Additionally, some refactoring had to be done to keep a good flow. First of all, SlashCommand, MessageReceiver and EventReceiver (and soon also Routine, see #235 ) are summarized under a marker interface called Feature. This marks all features that our bot core system supports.

Consequently, the previous starting class Commands has been renamed to Features and is now the origin of all features, not only commands (we already moved other stuff there in the past to prepare for this).

While at it, the existing class RejoinMuteListener has been adapted and now implements EventReceiver instead.

Details

System-wise, Features#createFeatures returns a list of all features to BotCore and it will iterate and filter it based on the type (a sealed interface would be awesome here to not miss any future type).

For SlashCommands, nothing changed. EventReceivers are directly forwarded to jda.addEventListener, since that interface extends JDAs EventListener. MessageReceivers subscribe to channels they are interested in by a REGEX pattern matching channel names, BotCore takes care of the message event forwarding by iterating a simple Map<Pattern, MessageReceiver>.

Examples

Quick example for a MessageReceiver, via its MessageReceiverAdapter helper:

class Foo extends MessageReceiverAdapter {
  public Foo() {
    super(Pattern.compile("lobby"));
  }
  
  @Override
  void onMessageReceived(GuildMessageReceivedEvent event) {
    event.getMessage()
      .addReaction("🐸")
      .queue();
  }
}

a quick example for a EventListener:

class Foo implements EventReceiver {
  @Override
  public void onEvent(GenericEvent event) {
    if (event instanceof GuildMessageReceivedEvent messageEvent) {
      System.out.println("Message was sent!");
    }
  }
}

and in both cases a simple addition in Features.java:

features.add(new Foo());

Remarks

When merging this, we have to go through our Wiki and do some minor adjusments caused by the renaming of Commands (now Features).

This PR competes against #341, which is a different design-approach to this problem, in which SlashCommands get the event listening capabilities directly instead of creating separate flows for them. (Which I am not a fan of, since this leads to creation of a behemoth class that can do everything instead of separating capabilities and concerns - SRP.)

@borgrel
Copy link
Contributor

borgrel commented Jan 18, 2022

While this adds great functionality, it defeats the original design of the bot?

The bot has a SINGLE command listener and manually validates / splits all incoming slash commands to drastically reduce overhead (by only having the correct command respond to the event instead of letting each command manually filter out invalid commands they dont need to respond to)

The message system should work the same way? A single listener that internally redirects where applicable?

Currently every message listener responds to all message events and have to manually filter the messages they are not interested in, instead of only receiving messages they do want.

@Zabuzard
Copy link
Member Author

While this adds great functionality, it defeats the original design of the bot?

I am not sure I get your concern. I am in no way saying that people should use the new interfaces when all they want to do is to create a slash command. In particular, you should rarely have to use EventReceiver and instead use the alternatives SlashCommand and MessageReceiver if possible.

But it is also clear that there are a number of commands that do need more capabilities than just being a slash command (your help system is a good example for this). Hence why we need to introduce those features as well.
People can of course also combine things, Foo implements SlashCommand, MessageReceiver, if needed.

Or is your concern more about the fact that we have jda.addEventListener(...) on all the message/event receivers instead of letting them all go into BotCore and routing them ourselves? In that case, I was indeed also concidering this but at the end I couldn't see any benefit to this (other than introducing possible bugs). After all, why should we reinvent the existing routing logic inside JDA with our own if we have no benefit. Maybe you see benefit that I overlooked. For the slash commands there was clearly benefit since we had a need to decorate our slash command events with extra information.
The good thing is though that the detail of who routes the message/event receiver events is hidden inside BotCore, so it could be changed at any time if needed without impacting any user.

@borgrel
Copy link
Contributor

borgrel commented Jan 19, 2022

Or is your concern more about the fact that we have jda.addEventListener(...) on all the message/event receivers instead of letting them all go into BotCore and routing them ourselves?

This

The good thing is though that the detail of who routes the message/event receiver events is hidden inside BotCore, so it could be changed at any time if needed without impacting any user.

True enough, but not if we want to add internal routing

The fact is that few of our message listeners will want / need global message listening. for example the free system will only need to listen on the help channels. Illum's suggestion of a dad bot would only need to listen on lobby and geek speak. if the bot gets functionality that blocks ppl from talking in snippets (by deleting any posts after the first by the same user in a 24hr period) it would only need to listen to messages in the snippets channel.

The way it currently is every single message in every single channel would be sent to every single listener and then every single listener has to filter out the 99% they dont need individually so assuming 12 listeners each message will be filtered 12 times and ignored 11 times, instead of being filtered once and only sent to the one listener that is interested in it.

The routing system is already designed and filters slash commands once to send them to the relevant command handler instead of just letting every command manually register its own listener and personally filter out the unneeded commands .... the routing should be expanded and re-used to reduce overhead in message listeners in the same way

@Zabuzard
Copy link
Member Author

Zabuzard commented Jan 19, 2022

The fact is that few of our message listeners will want / need global message listening. for example the free system will only need to listen on the help channels. Illum's suggestion of a dad bot would only need to listen on lobby and geek speak. if the bot gets functionality that blocks ppl from talking in snippets (by deleting any posts after the first by the same user in a 24hr period) it would only need to listen to messages in the snippets channel.

Okay, I think I get your point now. You are suggessting to not only add a general MessageReceiver but a more specific way instead where people will also tell you which channels they are interested in, to only receive some messages instead.
So basically you are saying that majority (if not all) message receivers are only interested in a small portion of channels (or maybe even other filters, such as by author) and we should take care of that filtering to make it easier for users and also get rid of the code duplication that occurs when everyone has to filter themselves.

And if someone really has an interest in all channels, they could always also still fall back to EventReceiver which is seen as the unfiltered fallback option.

I like that idea and will consider it, thanks.

- Renamed Commands to Features
- Renamed CommandSystem to BotCore
- Integrated new MessageReceiver and EventReceiver capabilities
- Migrated RejoinMuteListener to new EventReceiver
nobody except /free needed it. /free now uses `EventReceiver` which also propagates `ReadyEvent` already.
@Zabuzard
Copy link
Member Author

MessageReceiver now subscribe to channels they are interested in by their name and BotCore takes care of the forwarding (as requested by @borgrel ).

In a future PR, /free could now be simplified to basically implementing MessageReceiver and using that channel-name-subscription system, probably simplifying its own logic quite a bit (and getting rid of the EventReceiver part).

Tais993
Tais993 previously approved these changes Jan 23, 2022
Copy link
Member

@Tais993 Tais993 left a comment

Choose a reason for hiding this comment

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

It's good enough for now, it's blocking some other PR's (including #350)

We can change the subscription system for the Message events later one if required.

* Empty lines for readability
* Slightly rephrased a user facing string
@sonarcloud
Copy link

sonarcloud bot commented Jan 24, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@java-coding-prodigy java-coding-prodigy left a comment

Choose a reason for hiding this comment

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

I have put my suggestions for the refactoring of our MessageReciever interface. However, you still have to change the imports, variable names, and documentation.

@Zabuzard
Copy link
Member Author

7 days without real code changes + approval -> merging

@Zabuzard Zabuzard merged commit cdc00ba into develop Jan 27, 2022
@Zabuzard Zabuzard deleted the feature/add_core_features branch January 27, 2022 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority: major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add arbitrary JDA event support Add Message Listener support
5 participants