fix: PropertyMap remove() correctly removes internal listeners, closes #1229

AlmasB · AlmasB · commit e0d7015e9ed1 · 2023-03-14T20:22:28.000Z
diff --git a/fxgl-core/src/main/kotlin/com/almasb/fxgl/core/collection/PropertyMap.kt b/fxgl-core/src/main/kotlin/com/almasb/fxgl/core/collection/PropertyMap.kt
@@ -136,6 +136,16 @@ class PropertyMap {
             mapChangeListeners.forEach { it.onRemoved(propertyName, value) }
         }
 
+        listeners.filter { it.key.propertyName == propertyName }.forEach { (key, listener) ->
+            // clean up all non-removed JavaFX listeners for given [propertyName]
+            (get(key.propertyName) as ObservableValue<Any>).removeListener(listener as ChangeListener<Any>)
+        }
+
+        // remove all FXGL listeners for given [propertyName]
+        listeners.keys
+                .filter { it.propertyName == propertyName }
+                .forEach { listeners.remove(it) }
+
         properties.remove(propertyName)
     }
 
diff --git a/fxgl-core/src/test/kotlin/com/almasb/fxgl/core/collection/PropertyMapTest.kt b/fxgl-core/src/test/kotlin/com/almasb/fxgl/core/collection/PropertyMapTest.kt
@@ -11,9 +11,7 @@ import javafx.beans.property.SimpleIntegerProperty
 import javafx.beans.property.StringProperty
 import org.hamcrest.MatcherAssert.assertThat
 import org.hamcrest.Matchers.*
-import org.junit.jupiter.api.Assertions.assertEquals
-import org.junit.jupiter.api.Assertions.assertFalse
-import org.junit.jupiter.api.Assertions.assertTrue
+import org.junit.jupiter.api.Assertions.*
 import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertThrows
@@ -240,6 +238,13 @@ class PropertyMapTest {
         assertThat(count, `is`(2))
     }
 
+    @Test
+    fun `Removing a property also removes its listeners`() {
+        map.setValue("test", 0)
+        map.remove("test")
+        assertDoesNotThrow { map.clear() }
+    }
+
     @Test
     fun `Copy returns a shallow copy`() {
         map.setValue("testInt", 3)
