Add sharded listener provider #595

schnapster · 2018-01-09T19:06:07Z

Pull Request Etiquette

There are several guidelines you should follow in order for your
Pull Request to be merged.

I have checked the PRs for upcoming features/bug fixes.
I have read the contributing guidelines a long time ago lol.

It is sometimes better to include more changes in a single commit.
If you find yourself having an overwhelming amount of commits, you
can rebase your branch.

Description

This PR adds a feature to the DefaultShardManager.
This will allow the ShardManager and ShardManagerBuilder to be provided with a function which provides listeners based on shard id.
This is helpful when the user wishes for an approach where each shard gets it's own listener object of a certain kind, instead of having a single central listener object of a certain kind for all shards.

ToDos

Figure out removal of listener provider including any potentially created listeners with it from built shard manager.

Input is welcome on how to write a convenience method on how to handle removing existing listeners created by a provider when the provider is removed from a shard manager.

MinnDevelopment · 2018-01-27T10:45:58Z

src/main/java/net/dv8tion/jda/bot/sharding/DefaultShardManagerBuilder.java

+     *
+     * @return The {@link net.dv8tion.jda.bot.sharding.DefaultShardManagerBuilder DefaultShardManagerBuilder} instance. Useful for chaining.
+     */
+    public DefaultShardManagerBuilder addShardedEventListenerProvider(final Function<Integer, Object> shardedListenerProvider)


Calling something on ShardManager a ShardedEventListener is pretty unnecessary.
Everything you add to a ShardManager is "sharded" so it should just be addEventListenerProvider

MinnDevelopment · 2018-01-27T10:47:56Z

src/main/java/net/dv8tion/jda/bot/sharding/ShardManager.java

+     * @throws java.lang.IllegalArgumentException
+     *         If the provided listener provider {@code null}.
+     */
+    default void addShardedEventListeners(final Function<Integer, Object> shardedEventListenerProvider)


This is missing a removeEventListeners(IntPredicate)

Could we use IntFunction for these?

This is missing a removeEventListeners(IntPredicate)

Which removes all listeners from a specific shard, or other shards? I like the idea, and I'll gladly add that if you confirm. Just making sure, because we're also missing a method that removes the listeners provided by a listener provider from all shards.

MinnDevelopment · 2018-02-02T10:45:08Z

Requesting update on this

MinnDevelopment · 2018-02-04T12:45:53Z

src/main/java/net/dv8tion/jda/bot/sharding/ShardManager.java

+     * @throws java.lang.IllegalArgumentException
+     *         If the provided listener provider {@code null}.
+     */
+    default void addEventListeners(final IntFunction<Object> eventListenerProvider)


Can we also get the same as removeEventListeners(IntFunction<Collection<Object>> predicate)

schnapster · 2018-02-09T15:49:18Z

default void removeEventListeners(IntFunction<Collection<Object>> predicate) clashes with default void removeEventListeners(IntFunction<Object> predicate), both methods have the same erasure. Do we want both? How to name them in that case? Or is there another possibility?

MinnDevelopment · 2018-02-09T15:51:49Z

removeEventListeners(IntFunction<Object> predicate) is to remove the provider from the manager, isn't it?

MinnDevelopment · 2018-02-09T15:53:01Z

src/main/java/net/dv8tion/jda/bot/sharding/ShardManager.java

+     * @throws java.lang.IllegalArgumentException
+     *         If the provided shard filter is {@code null}.
+     */
+    default void removeEventListeners(final IntPredicate shardFilter)


I don't like this method, I suggested something different on discord removeEventListeners(IntFunction<Collection<Object>>)

…ners by shard ids

schnapster · 2018-02-09T17:37:01Z

Sorry, I derped about what we talked over in Discord. This should look now more like it.

Almighty-Alpaca · 2018-02-09T19:41:08Z

src/main/java/net/dv8tion/jda/bot/sharding/ShardManager.java

+    {
+        Checks.notNull(eventListenersProvider, "event listeners provider");
+        this.getShardCache().forEach(jda ->
+            jda.removeEventListener(eventListenersProvider.apply(jda.getShardInfo().getShardId()))


This does not work when an EventListenersProvider doesn't always return the same listener instance for the a shard id (unless a proper equals() method has been implemented)

Also the method names are not consistent: There's removeEventListenersProvider, removeEventListenerProvider and removeEventListeners

removeEventListenersProvider looks like a typo and should be removeEventListenerProvider

As for ShardManager#removeEventListeners, can you do a suggestion how it should be called?

This does not work when an EventListenersProvider doesn't always return the same listener instance for the a shard id (unless a proper equals() method has been implemented)

Getting mixed signals here.

MinnDevelopment · 2018-02-09T20:52:26Z

src/main/java/net/dv8tion/jda/bot/sharding/DefaultShardManager.java

@@ -342,6 +342,11 @@ public void addEventListeners(IntFunction<Object> eventListenerProvider) {
        this.listenerProviders.add(eventListenerProvider);
    }

+    @Override
+    public void removeEventListenersProvider(IntFunction<Object> eventListenerProvider) {


bracket on new line

MinnDevelopment · 2018-02-10T16:16:13Z

src/main/java/net/dv8tion/jda/bot/sharding/ShardManager.java

+     * @throws java.lang.IllegalArgumentException
+     *         If the provided listener provider is {@code null}.
+     */
+    void removeEventListenerProvider(IntFunction<Object> eventListenerProvider);


Make this a nop default for backward compatibility

MinnDevelopment · 2018-02-10T16:17:58Z

src/main/java/net/dv8tion/jda/bot/sharding/DefaultShardManagerBuilder.java

+     *
+     * @return The {@link net.dv8tion.jda.bot.sharding.DefaultShardManagerBuilder DefaultShardManagerBuilder} instance. Useful for chaining.
+     */
+    public DefaultShardManagerBuilder removeEventListenerProvider(final IntFunction<Object> listenerProvider)


~~Make this vararg, same for add~~ Decided this is not needed

MinnDevelopment · 2018-03-05T13:44:03Z

Sorry for all the delay on this, I'll try to get to this PR this week.

schnapster · 2018-03-06T04:09:00Z

Can't blame you, forgot to update the title :)

MinnDevelopment · 2018-03-07T10:27:46Z

src/main/java/net/dv8tion/jda/bot/sharding/ShardManager.java

+        this.getShardCache().forEach(jda ->
+        {
+            Object listener = eventListenerProvider.apply(jda.getShardInfo().getShardId());
+            Checks.notNull(listener, "listener");


Could we skip on null?
like if (listener != null) jda.addEventListener(listener);

MinnDevelopment

lgtm

Add sharded listener provider

ce3ed74

MinnDevelopment added type: enhancement version: revision labels Jan 9, 2018

MinnDevelopment requested review from MinnDevelopment and Almighty-Alpaca January 9, 2018 19:10

MinnDevelopment reviewed Jan 27, 2018

View reviewed changes

MinnDevelopment added this to the Release 3.6 milestone Feb 2, 2018

schnapster added 2 commits February 3, 2018 16:05

Use IntFunction<Object> instead of Function<Integer, Object>

c365a17

More concise naming of event listener providers

13cf4e0

MinnDevelopment reviewed Feb 4, 2018

View reviewed changes

Add provider and shard id predicate based listener removal methods

5cd56df

MinnDevelopment reviewed Feb 9, 2018

View reviewed changes

Adjust methods to remove a listener provider and collections of liste…

deda5c1

…ners by shard ids

Almighty-Alpaca requested changes Feb 9, 2018

View reviewed changes

Uniform naming of eventListenerProvider

b87b4a9

MinnDevelopment reviewed Feb 9, 2018

View reviewed changes

Allman master race

1a6bbb2

MinnDevelopment requested a review from Almighty-Alpaca February 10, 2018 16:12

MinnDevelopment reviewed Feb 10, 2018

View reviewed changes

schnapster added 2 commits February 10, 2018 20:27

Add noop for newly introduced interface method

13d1318

Use correct call to remove listener provider

d10bc77

MinnDevelopment added the status: completed has been completed but is not yet released label Mar 5, 2018

schnapster changed the title ~~[WIP] Add sharded listener provider~~ Add sharded listener provider Mar 6, 2018

MinnDevelopment reviewed Mar 7, 2018

View reviewed changes

Adjust null check when adding provided listener

81aa7f7

MinnDevelopment approved these changes Mar 7, 2018

View reviewed changes

MinnDevelopment merged commit 0befe74 into discord-jda:development Mar 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sharded listener provider #595

Add sharded listener provider #595

schnapster commented Jan 9, 2018 •

edited

Loading

MinnDevelopment Jan 27, 2018

MinnDevelopment Jan 27, 2018

MinnDevelopment Jan 27, 2018

schnapster Feb 3, 2018

MinnDevelopment commented Feb 2, 2018

MinnDevelopment Feb 4, 2018 •

edited

Loading

schnapster commented Feb 9, 2018

MinnDevelopment commented Feb 9, 2018

MinnDevelopment Feb 9, 2018

schnapster commented Feb 9, 2018

Almighty-Alpaca Feb 9, 2018

Almighty-Alpaca Feb 9, 2018

schnapster Feb 9, 2018

MinnDevelopment Feb 9, 2018

MinnDevelopment Feb 10, 2018

MinnDevelopment Feb 10, 2018 •

edited

Loading

MinnDevelopment commented Mar 5, 2018

schnapster commented Mar 6, 2018

MinnDevelopment Mar 7, 2018

schnapster Mar 7, 2018

MinnDevelopment left a comment

Add sharded listener provider #595

Add sharded listener provider #595

Conversation

schnapster commented Jan 9, 2018 • edited Loading

Pull Request Etiquette

Description

ToDos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MinnDevelopment commented Feb 2, 2018

MinnDevelopment Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

schnapster commented Feb 9, 2018

MinnDevelopment commented Feb 9, 2018

Choose a reason for hiding this comment

schnapster commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MinnDevelopment Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

MinnDevelopment commented Mar 5, 2018

schnapster commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MinnDevelopment left a comment

Choose a reason for hiding this comment

schnapster commented Jan 9, 2018 •

edited

Loading

MinnDevelopment Feb 4, 2018 •

edited

Loading

MinnDevelopment Feb 10, 2018 •

edited

Loading