Feature: API for AFK and excluded players

[aakatz3]

I've been maintaining a fork of Harbor to work with a couple of plugins on a server I develop for. Rather than maintain a fork, I thought it might be better to introduce an API that allows external plugins to provide logic to Harbor on excluding players, or determining if a player is AFK. In addition, I added a way to unregister Harbor's AFK listeners, which would be useful if an external plugin is being used.

I am open to comment and suggestion. One thought I had is moving the essentials code into an example implementation of AFKProvider, and including that by default in Harbor. I was also thinking of adding support for StaffFacilities, as an example of how to use the ExclusionProvider.

[aakatz3]

I have done a bit of a rewrite that should address all of the earlier concerns

[aakatz3]

Biggest question is if the following ternary (in PlayerManager) is acceptable:

    /**
     * Checks if a player is considered "AFK" for Harbor's player checks.
     *
     * @param player The player to check.
     *
     * @return Whether the player is considered AFK.
     */
    public boolean isAfk(@NotNull Player player) {
        return !(andedProviders.isEmpty() && oredProviders.isEmpty()) ?
                (!andedProviders.isEmpty() && andedProviders.stream().allMatch(provider -> provider.isAFK(player)))
                        && (oredProviders.stream().anyMatch(provider -> provider.isAFK(player)))
                : defaultProvider.isAFK(player);
    }

[thatgoofydev]

Biggest question is if the following ternary (in PlayerManager) is acceptable:

    /**
     * Checks if a player is considered "AFK" for Harbor's player checks.
     *
     * @param player The player to check.
     *
     * @return Whether the player is considered AFK.
     */
    public boolean isAfk(@NotNull Player player) {
        return !(andedProviders.isEmpty() && oredProviders.isEmpty()) ?
                (!andedProviders.isEmpty() && andedProviders.stream().allMatch(provider -> provider.isAFK(player)))
                        && (oredProviders.stream().anyMatch(provider -> provider.isAFK(player)))
                : defaultProvider.isAFK(player);
    }

No it isn't 😅 It has very bad readability. Use a regular if statement, reversing the statement would be even cleaner. ex:

   public boolean isAfk(@NotNull Player player) {
        if (andedProviders.isEmpty() && oredProviders.isEmpty())
             return defaultProvider.isAFK(player);

        return ...
    }

[aakatz3]

Biggest question is if the following ternary (in PlayerManager) is acceptable:

    /**
     * Checks if a player is considered "AFK" for Harbor's player checks.
     *
     * @param player The player to check.
     *
     * @return Whether the player is considered AFK.
     */
    public boolean isAfk(@NotNull Player player) {
        return !(andedProviders.isEmpty() && oredProviders.isEmpty()) ?
                (!andedProviders.isEmpty() && andedProviders.stream().allMatch(provider -> provider.isAFK(player)))
                        && (oredProviders.stream().anyMatch(provider -> provider.isAFK(player)))
                : defaultProvider.isAFK(player);
    }

No it isn't 😅 It has very bad readability. Use a regular if statement, reversing the statement would be even cleaner. ex:

   public boolean isAfk(@NotNull Player player) {
        if (andedProviders.isEmpty() && oredProviders.isEmpty())
             return defaultProvider.isAFK(player);

        return ...
    }

Would this be acceptible?

    /**
     * Checks if a player is considered "AFK" for Harbor's player checks.
     *
     * @param player The player to check.
     *
     * @return Whether the player is considered AFK.
     */
    public boolean isAfk(@NotNull Player player) {
        // If there are no providers registered, we go to the default provider
        if(oredProviders.isEmpty() && andedProviders.isEmpty()){
            return defaultProvider.isAFK(player);
        }
        return
                // Is true if at least one of oredProviders returns true; is false if there are no oredProviders
                oredProviders.stream().anyMatch(provider -> provider.isAFK(player)) ||
                /*
                 * If all of anded providers are true are true, then this statement is true. If there are no anded providers
                 * then allMatch would return true, therefore since we don't want this to be true in that case (where there
                 * are no anded providers), we want this to be false, therefor we need to make sure it's not empty
                 */
                (!andedProviders.isEmpty() && andedProviders.stream().allMatch(provider -> provider.isAFK(player)));
    }

[thatgoofydev]

...

Would this be acceptible?

    /**
     * Checks if a player is considered "AFK" for Harbor's player checks.
     *
     * @param player The player to check.
     *
     * @return Whether the player is considered AFK.
     */
    public boolean isAfk(@NotNull Player player) {
        // If there are no providers registered, we go to the default provider
        if(oredProviders.isEmpty() && andedProviders.isEmpty()){
            return defaultProvider.isAFK(player);
        }
        return
                // Is true if at least one of oredProviders returns true; is false if there are no oredProviders
                oredProviders.stream().anyMatch(provider -> provider.isAFK(player)) ||
                /*
                 * If all of anded providers are true are true, then this statement is true. If there are no anded providers
                 * then allMatch would return true, therefore since we don't want this to be true in that case (where there
                 * are no anded providers), we want this to be false, therefor we need to make sure it's not empty
                 */
                (!andedProviders.isEmpty() && andedProviders.stream().allMatch(provider -> provider.isAFK(player)));
    }

Yes, if you remove the comments in the middle of the code 😅. The reason that the previous code was "unreadable" was because of the formatting not the actual logic. Ternary operators are never a good idea when handling long lines of code.

Also note that comments do not necessarily add to code readability or clarity. Sometimes comments do make sense, but I think the logic in this method is easy enough to understand without them 😄

[aakatz3]

Understood, will remove before commit!

On Wed, Mar 31, 2021, 12:25 PM Jeroen Verwimp @.***> wrote:

...

Would this be acceptible?

/**
 * Checks if a player is considered "AFK" for Harbor's player checks.
 *
 * @param player The player to check.
 *
 * @return Whether the player is considered AFK.
 */

public boolean isAfk(@NotNull Player player) {

    // If there are no providers registered, we go to the default provider

    if(oredProviders.isEmpty() && andedProviders.isEmpty()){

        return defaultProvider.isAFK(player);

    }

    return

            // Is true if at least one of oredProviders returns true; is false if there are no oredProviders

            oredProviders.stream().anyMatch(provider -> provider.isAFK(player)) ||

            /*
             * If all of anded providers are true are true, then this statement is true. If there are no anded providers
             * then allMatch would return true, therefore since we don't want this to be true in that case (where there
             * are no anded providers), we want this to be false, therefor we need to make sure it's not empty
             */

            (!andedProviders.isEmpty() && andedProviders.stream().allMatch(provider -> provider.isAFK(player)));

}

Yes, if you remove the comments in the middle of the code 😅. The reason that the previous code was "unreadable" was because of the formatting not the actual logic. Ternary operators are never a good idea when handling long lines of code.

Also note that comments do not necessarily add to code readability or clarity. Sometimes comments do make sense, but I think the logic in this method is easy enough to understand without them 😄

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nkomarn/Harbor/pull/88#issuecomment-811309983, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABFUCWUZFVE373Q46SMRYGTTGNSKXANCNFSM4ZVXZRBQ .

[aakatz3]

All fixed

[aakatz3]

@RainbowDashLabs I replaced my atrocious code as per your suggestion; this is why I shouldn't try to code on minimal sleep for public projects!

[rainbowdashlabs]

@aakatz3 // This might cause lag if there are a TON of players on the server, but by keeping things minimalistic, this wouldnt happen with my code tbh xD Its requires way less computation resources per tick :D My could will check 1/20 of the players per tick which means that every player will have been checked every second. thats why I had this queue mechanic and the players per tick double. You code checks every player every tick. Tbh I dont even know why you replaced my code since this is the nearly optimal implementation in case of runtime and performance. Also streams look fancy, but arent by fast the fastest things and will nearly always lose agains a normal for loop.

[aakatz3]

@RainbowDashLabs

@aakatz3 // This might cause lag if there are a TON of players on the server, but by keeping things minimalistic, this wouldnt happen with my code tbh xD Its requires way less computation resources per tick :D My could will check 1/20 of the players per tick which means that every player will have been checked every second. thats why I had this queue mechanic and the players per tick double. You code checks every player every tick. Tbh I dont even know why you replaced my code since this is the nearly optimal implementation in case of runtime and performance. Also streams look fancy, but arent by fast the fastest things and will nearly always lose agains a normal for loop.

I misunderstood what you were doing, in the checking 1/20th players per tick, I misread as 20 ticks in a second.

My only concern is the double rather than an int being used in the loop. Now that I actually understand your code I will put it back, I misunderstood what you were saying before.

I was under the impression that filtered streams would be faster, and I apologize for that misunderstanding.

I want to really apologize for being incredibly annoying, and I'd like this to be taken as a formal apology as I try to re-merge your code, in a way that doesnt break things.

[aakatz3]

@RainbowDashLabs I really hope I got it in there properly this time; I'm not quite sure what I'm going to do if I somehow did it wrong yet again.

[aakatz3]

@thatgoofydev Don't mean to bother you but can you give a final review?

[aakatz3]

Alright, I'll try to put these in!

[aakatz3]

@thatgoofydev All done!

[thatgoofydev]

@aakatz3 So during testing I came across two bad broken parts.

1. changing the exclusion configuration and using /harbor reload doesn't enable/disable the exclusion providers.
2. AFK system doesn't work? All players are counted as AFK for me when config is set to exclude-afk: true (the first message was with exclude-afk: true after the reload is exclude-afk: false) (Not only the message is incorrect the night actually skips for the first one)

Environment:

• No essentials or other integration plugins active
• Default config, except the changed to exclude-afk and percentage: 100 (So I could test with two accounts)

A other small thing: I think excluding vanished players should be enabled by default? The default configuration should be what fits best for most people. And I think most people would want to keep vanished people hidden by not including them in the player count. 😅

[aakatz3]

@aakatz3 So during testing I came across two bad broken parts.

1. changing the exclusion configuration and using `/harbor reload` doesn't enable/disable the exclusion providers.

2. AFK system  doesn't work? All players are counted as AFK for me when config is set to `exclude-afk: true`
   ![image](https://user-images.githubusercontent.com/10576071/115071330-6c3ccf00-9ef6-11eb-902d-5126342a2400.png)
   (the first message was with `exclude-afk: true` after the reload is `exclude-afk: false`) (Not only the message is incorrect the night actually skips for the first one)

Environment:

* No essentials or other integration plugins active

* Default config, except the changed to `exclude-afk` and `percentage: 100` (So I could test with two accounts)

A other small thing: I think excluding vanished players should be enabled by default? The default configuration should be what fits best for most people. And I think most people would want to keep vanished people hidden by not including them in the player count. 😅

I agree on the vanished, it was set as false in the original. Harbor reload should be easy to fix, but AFK I need to take a look in a little bit deeper. I'll keep you posted!

[aakatz3]

Found the issue with AFK, the config value was renamed and I didn't take it into account, because i'm dumb

[aakatz3]

It's slightly more expensive, but I also made each exclusion provider check if it should be enabled via the config, as otherwise I'd need to add callbacks on reloads of the config.

[aakatz3]

Bumping this one more time, trying not to be annoying. I believe I've addressed everything