Automatically leave voice channel after some time spent idle #42

SriRamanujam · 2021-11-04T05:52:42Z

This PR enables the bot to leave a voice channel after some time spent idle. Upon joining a channel, a scheduled task will fire every minute that checks to see if the player is idle. If it is, the task will increment an internal timer. After some configurable amount of time (idleTimeMinutes in ukulele.yml, defaults to 10 minutes) the task will have the bot automatically leave the voice channel.

First time working with Kotlin and Spring, so please do point out any stylistic foibles. If there's a linter I should run, I can do that no problem.

Fixes #26

freyacodes

You've changed the bot so that it will leave once the queue is empty after approximately 10 minutes without explanation

This is probably not what all Ukulele users want. This feature should default to be disabled.

src/main/kotlin/dev/arbjerg/ukulele/features/LeaveOnIdle.kt

freyacodes · 2021-11-04T12:28:39Z

src/main/kotlin/dev/arbjerg/ukulele/config/BotProps.kt

@@ -9,5 +9,6 @@ class BotProps(
        var prefix: String = "::",
        var database: String = "./database",
        var game: String = "",
-        var trackDurationLimit: Int = 0
+        var trackDurationLimit: Int = 0,
+        var idleTimeMinutes: Int = 10


I think this should be disabled by default

Suggested change

var idleTimeMinutes: Int = 10

var idleTimeMinutes: Int = 0

SriRamanujam · 2021-11-06T21:44:37Z

Okay, so in the process of reworking the PR with code review feedback I discovered a race condition of sorts. Basically, oftentimes the onVoiceJoin handler of the service would fire before the onTrackStart handler in Player, so the logic would determine see an empty queue and no playing track, which meant remainingDuration was 0, and so it would start the idle timeout task. If the video was under one minute long, that meant that the bot would leave early. Not the end of the world, but incorrect behavior nonetheless.

I tried a couple things to resolve the issue. I first tried implementing onTrackStart and onTrackEnd on LeaveOnIdleService itself, which didn't work for the same reason (couldn't guarantee the new handlers would be called after Player's handlers). The approach that does seem to work is what I've reworked this PR into: extending the event handlers on Player to call into LeaveOnIdleService. This neatly resolves the ordering issues, and so far has worked for me in my testing.

freyacodes · 2021-11-08T22:54:41Z

src/main/kotlin/dev/arbjerg/ukulele/audio/Player.kt

+class Player(
+        private val beans: Beans,
+        guildProperties: GuildProperties,
+        private val leaveOnIdleService: LeaveOnIdleService,


You're actually supposed to put this value in the Beans class and not change PlayerRegistry at all

freyacodes · 2021-11-08T22:57:41Z

src/main/kotlin/dev/arbjerg/ukulele/features/LeaveOnIdleService.kt

+     * Create a new timer which will start counting from the current moment and cancel any existing timers
+     * associated with this guild.
+     */
+    fun maybeCreateTimer(guild: Guild) {


A more appropriate name is onQueueEmpty. The player doesn't control this service

freyacodes · 2021-11-08T22:59:43Z

src/main/kotlin/dev/arbjerg/ukulele/features/LeaveOnIdleService.kt

+    /**
+     * If a timer exists for this guild, cancel it.
+     */
+    fun maybeDestroyTimer(guild: Guild) {


destroyTimer sounds better. We don't need to communicate that there might not be a timer to destroy

freyacodes · 2021-11-08T23:01:54Z

src/main/kotlin/dev/arbjerg/ukulele/features/LeaveOnIdleService.kt

+        // don't do anything if idleTimeMinutes is not set
+        if (botProps.idleTimeMinutes <= 0) return
+
+        timers[guild.idLong]?.let {


You can do timers.remove(guild.idLong)?.let { ... } and delete the other remove call

freyacodes · 2021-11-08T23:05:01Z

src/main/kotlin/dev/arbjerg/ukulele/features/LeaveOnIdleService.kt

+     * a player in a guild has spent idling.
+     */
+    class IdleTask(val guild: Guild, private val idleTimeMinutes: Int) : Runnable {
+        private var numberOfMinutesIdleElapsed = -1


It's a bit odd to me that you're using a counter and not a unix timestamp, but I suppose this works too.

On second thought, could you not just schedule the voice leave after idleTimeMinutes minutes?

freyacodes · 2021-11-08T23:05:36Z

src/main/kotlin/dev/arbjerg/ukulele/features/LeaveOnIdleService.kt

+     * Runnable that gets passed to the Spring TaskScheduler. Used to keep track of how many minutes
+     * a player in a guild has spent idling.
+     */
+    class IdleTask(val guild: Guild, private val idleTimeMinutes: Int) : Runnable {


If you make this an inner class you don't need to pass idleTimeMinutes

src/main/kotlin/dev/arbjerg/ukulele/features/LeaveOnIdleService.kt

SriRamanujam · 2021-12-26T05:51:35Z

Hi, anything else I can do to move this along? I believed I've addressed all the comments from the last round of review.

Abi45x · 2021-12-26T14:25:04Z

Hi SriRamanujam, Freya is currently focusing her time on her bachelor project until the 4th of January, I would probably expect replies to be sparse until then

freyacodes

Sorry for the wait! There's one small thing I want you to change still

src/main/kotlin/dev/arbjerg/ukulele/features/LeaveOnIdleService.kt

Unfortunately, trying to model the task as a duration-based timeout saw race conditions where the callback to determine whether to start a timer was called before the callback to add a new track to the player had run. This caused an issue where the code believed that no track was playing when in fact a track would be queued mere milliseconds later. When the video in question was under a minute long, this led to a state where the bot would leave the channel a minute early. I've re-implemented the functionality such that it is invoked as part of the onTrackStart and onTrackEnd callbacks of Player. This guarantees that the idle timer kickoff logic runs _after_ all other Player state updates have completed, which neatly avoids race/ordering bugs.

* put leaveOnIdleService in Player.Beans * better names for leaveOnIdleService's callbacks * Directly call timers.remove() * Schedule VC leave task directly, rather than periodically calling timer * Clean up redundant comments

freyacodes requested changes Nov 4, 2021

View reviewed changes

SriRamanujam requested a review from freyacodes November 6, 2021 21:45

freyacodes requested changes Nov 8, 2021

View reviewed changes

SriRamanujam requested a review from freyacodes November 9, 2021 02:44

freyacodes requested changes Feb 4, 2022

View reviewed changes

src/main/kotlin/dev/arbjerg/ukulele/features/LeaveOnIdleService.kt Outdated Show resolved Hide resolved

SriRamanujam added 4 commits February 19, 2022 11:41

Automatically leave voice channel after 4 minutes spent idle

b29c0bb

Changes from code review

6ab6f87

* put leaveOnIdleService in Player.Beans * better names for leaveOnIdleService's callbacks * Directly call timers.remove() * Schedule VC leave task directly, rather than periodically calling timer * Clean up redundant comments

Use inline lambda for one-liner runnable

b623ad2

SriRamanujam force-pushed the auto-part branch from a54c36c to b623ad2 Compare February 19, 2022 17:00

SriRamanujam requested a review from freyacodes February 19, 2022 17:01

zeyus mentioned this pull request Mar 29, 2023

music bot stopped working(Fixed) #65

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically leave voice channel after some time spent idle #42

Automatically leave voice channel after some time spent idle #42

SriRamanujam commented Nov 4, 2021 •

edited

Loading

freyacodes left a comment

freyacodes Nov 4, 2021

SriRamanujam commented Nov 6, 2021

freyacodes Nov 8, 2021

freyacodes Nov 8, 2021

freyacodes Nov 8, 2021

freyacodes Nov 8, 2021

freyacodes Nov 8, 2021

freyacodes Nov 8, 2021

freyacodes Nov 8, 2021

SriRamanujam commented Dec 26, 2021

Abi45x commented Dec 26, 2021

freyacodes left a comment

Automatically leave voice channel after some time spent idle #42

Are you sure you want to change the base?

Automatically leave voice channel after some time spent idle #42

Conversation

SriRamanujam commented Nov 4, 2021 • edited Loading

freyacodes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SriRamanujam commented Nov 6, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SriRamanujam commented Dec 26, 2021

Abi45x commented Dec 26, 2021

freyacodes left a comment

Choose a reason for hiding this comment

SriRamanujam commented Nov 4, 2021 •

edited

Loading