Skip to content

Conversation

ZepsiZola
Copy link
Contributor

Implements this #2864

Specifically, this functionality:

afk-timeout-command: "server afk {USERNAME}"
Sends the player to the AFK server on the server-network.

afk-timeout-command: "litebans:kick {USERNAME} You were kicked after {KICK_TIME} minutes of inactivity."
Kicks the player using litebans so that it shows up in their litebans history.

afk-timeout-command: "none"
Default behaviour of essentials kicking the player on afk-timeout.

Copy link
Member

@JRoy JRoy left a comment

Choose a reason for hiding this comment

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

have a few issues with this, would also like @mdcfe's input on this

Copy link
Member

@JRoy JRoy left a comment

Choose a reason for hiding this comment

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

have a few issues with this, would also like @mdcfe's input on this

@JRoy JRoy requested a review from mdcfe June 13, 2025 03:11
@ZepsiZola
Copy link
Contributor Author

no reason to add this

I added it because this is what I got after doing ./gradlew and then ./gradlew shadowJar. This was directly after cloning the repo for the first time.
unknown
You can reject this change if you want, but I added it because there were unneeded files created on my repo that I did not want to push.

why would we replaceFormat here? you cannot send section signs thru commands

Fair enough. I wasn't familiar with the EssentialsX codebase and thought that that was needed if I wanted to parse placeholders in the config option. I can change that line to not use FormatUtil.

is there a reason we need this variable, wouldn't the time the player has been afk always be the same (the auto-afk-kick above).

The variable is there so that if a server owner changes the AFK timeout, they don't also have to change the afk-timeout-command. And no, the auto-afk-kick doesn't always stay the same. As a server owner myself I've changed it around a fair few times. It would be an inconvenience to anyone messing with their afk configuration to have to change a number in two places because they changed the auto-afk-kick config. A minor inconvenience, but a completely unneeded one as well.

maybe we rename this option to auto-afk-timeout and explain that it will run the command below if set, otherwise kick the player?

@JRoy auto-afk-timeout is bad naming. From the name, it would seem like it should define the timeout for a player's AFK. I don't think it should be named that just because the other config options have "auto-afk" in them. We shouldn't give something a bad name on purpose just so that it looks similar to the other config options. It should be called afk-timeout-command because it is the command that is run upon the players AFK-time running out. Isn't the fact that the config option is right below auto-afk-kick enough for it to be understood that it closely relates to that config option?

@SrBedrock
Copy link
Contributor

SrBedrock commented Jun 13, 2025

I believe this could be a list of commands and instead of none, check if the list is empty, just like other settings already do

@JasonHorkles
Copy link
Contributor

I added it because there were unneeded files created on my repo that I did not want to push.

You can deselect items that you don't want to commit 😉

It should be called afk-timeout-command because it is the command that is run upon the players AFK-time running out.

Josh was referring to the older config option auto-afk-kick (not the one you're adding)


I believe this could be a list of commands and instead of none, check if the list is empty, just like other settings already do

This is something that the team seems to agree with as well, although it's still being debated

@ZepsiZola
Copy link
Contributor Author

I added it because there were unneeded files created on my repo that I did not want to push.

You can deselect items that you don't want to commit 😉

I know that. But isn't the point of .gitignore to ignore files that will never be comitted? Unless this repo does require files in bin/ directories being pushed.

It should be called afk-timeout-command because it is the command that is run upon the players AFK-time running out.

Josh was referring to the older config option auto-afk-kick (not the one you're adding)

Ahh he was suggesting changing the existing config option name. Fair enough. I'm impartial to that.

I believe this could be a list of commands and instead of none, check if the list is empty, just like other settings already do

This is something that the team seems to agree with as well, although it's still being debated

I can implement this if you want. I don't necessarily care about that functionality, but it won't interfere with my own functionality, plus I just want this feature implemented so I can use it in my server as soon as I can.

Renamed config option to afk-timeout-commands and made it accept a list of strings.

Removed unnecessary FormatUtil.

Reverted .gitignore
@ZepsiZola
Copy link
Contributor Author

@SrBedrock @JasonHorkles @JRoy
Btw I made these changes:

  • Reverted the .gitignore change. So it remains unchanged from before the PR.
  • Removed use of FormatUtil where not needed.
  • Made it so you can specify a list of commands that run in succession as opposed to just one command. Config option is now called afk-timeout-commands:

Now the config.yml looks like this...

# Define a set of commands the server runs when a player's AFK time elapses.
# Set to [] to use Essentials' default AFK timeout kick behavior.
#
# Available placeholders:
# {USERNAME} - The player's username.
# {KICKTIME} - The time, in minutes, the player has been AFK for.
afk-timeout-commands: []

and you can set commands like this

afk-timeout-commands:
- "msg {USERNAME} You have been AFK for {KICKTIME}. Jailing you and moving you to AFK server..."
- "jail {USERNAME} jail-1"
- "server afk {USERNAME}"

@JasonHorkles
Copy link
Contributor

Seems fine to me, although I do have a concern about your possible intended use for this - the /server command is created by the proxy and not the backend servers, so unless you have a custom server switching solution, the /server command won't work

@ZepsiZola
Copy link
Contributor Author

Seems fine to me, although I do have a concern about your possible intended use for this - the /server command is created by the proxy and not the backend servers, so unless you have a custom server switching solution, the /server command won't work

I have a plugin that creates /server commands on the backend servers. Don't worry.

@ZepsiZola
Copy link
Contributor Author

Do I need to "Mark as resolved" on requested changes or can I just leave this?

@JasonHorkles
Copy link
Contributor

Can't hurt to resolve ones you implemented

@ZepsiZola
Copy link
Contributor Author

Can't hurt to resolve ones you implemented

I've done that, but what about the ones I haven't implemented?

#6169 (comment)
#6169 (comment)

@JasonHorkles
Copy link
Contributor

Leave them open for now

@JRoy
Copy link
Member

JRoy commented Jun 22, 2025

./gradlew and then ./gradlew shadowJar

run ./gradlew build instead, jars will appear in the jars folder in the root directory. this shouldn't create any bin folders.

Copy link
Member

@JRoy JRoy left a comment

Choose a reason for hiding this comment

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

few things

@ZepsiZola
Copy link
Contributor Author

#6169 (comment)

Can I just rename the config option? Wouldn't that affect people with older configs? Or is there more I have to do when renaming a config option?
I'd rename it to auto-afk-timeout.

Also when you say to update the docs, which docs do you mean and can I even update them within this repo? I thought the docs were hosted on https://essentialsx.net/wiki

@JRoy
Copy link
Member

JRoy commented Jun 25, 2025

Can I just rename the config option? Wouldn't that affect people with older configs? Or is there more I have to do when renaming a config option?

Look at this example to see how to do it, you would look for the new name, if that isn't set look for the old name, if that isn't set, use the default value

private int _getChatRadius() {
return config.getInt("chat.radius", config.getInt("chat-radius", 0));
}

Also when you say to update the docs, which docs do you mean

Just the comments above the option in the config.yml

Copy link
Member

@JRoy JRoy left a comment

Choose a reason for hiding this comment

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

lgtm, will merge after our next release

@JRoy JRoy added this to the 2.22.0 milestone Jun 28, 2025
@forsakenliquid
Copy link

forsakenliquid commented Jul 29, 2025

when will be the rollout? (release)

@JasonHorkles
Copy link
Contributor

when will be the rollout? (release)

It'll be released when it gets released. We don't have any ETAs

@JRoy JRoy added this pull request to the merge queue Aug 3, 2025
Merged via the queue into EssentialsX:2.x with commit 334c5a4 Aug 3, 2025
1 check passed
@ZepsiZola
Copy link
Contributor Author

Hell yeah.

Euphillya added a commit to Euphillya/Essentials-Folia that referenced this pull request Aug 6, 2025
EssentialsX@c01ba4e Bump adventure dependencies (EssentialsX#6243)
EssentialsX@16f643a New Crowdin updates (EssentialsX#6225)
EssentialsX@eb781a1 Release 2.21.2
EssentialsX@b36d433 Prepare for 2.22.0 Dev Builds (EssentialsX#6209)
EssentialsX@74337ff Add wind charge explosion protect config (EssentialsX#6183)
EssentialsX@636bb09 Add tipped arrow support to /potion (EssentialsX#6191)
EssentialsX@d402f17 Reset oxygen with /heal (EssentialsX#6201)
EssentialsX@334c5a4 New config.yml option: afk-timeout-command: (EssentialsX#6169)
EssentialsX@790644b Ignore cancelled PlayerDeathEvents (EssentialsX#6179)
EssentialsX@03d8938 feat: Add /powertoollist (EssentialsX#6096)
EssentialsX@689c35f Add bypass permission node for the whitelist. (EssentialsX#6232)
EssentialsX@8bcf03d Discord: Add PAPI support to avatar-url (EssentialsX#6189)
EssentialsX@025ca48 Discord: Respect game rule for advancement message type (EssentialsX#6195)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants