Refactor key point implementation and tests

6 files changed, 230 insertions, 172 deletions

diff --git a/solo-tool-project/src/solo_tool/notifier.py b/solo-tool-project/src/solo_tool/notifier.py
index ac1c736..5b3539c 100644
--- a/solo-tool-project/src/solo_tool/notifier.py
+++ b/solo-tool-project/src/solo_tool/notifier.py
@@ -5,7 +5,7 @@ class Notifier:
     CURRENT_SONG_EVENT = 3
     SONG_LIST_EVENT = 4
     CURRENT_KEY_POINT_EVENT = 5
-    KEY_POINTS_EVENT = 6
+    KEY_POINT_LIST_EVENT = 6
 
     def __init__(self, player):
         self._callbacks = dict()
diff --git a/solo-tool-project/src/solo_tool/solo_tool.py b/solo-tool-project/src/solo_tool/solo_tool.py
index bc20013..0489517 100644
--- a/solo-tool-project/src/solo_tool/solo_tool.py
+++ b/solo-tool-project/src/solo_tool/solo_tool.py
@@ -13,12 +13,18 @@ class SoloTool:
         self._keyPoint = None
 
     def _updateSong(self, index):
+        previousSong = self._song
         self._song = index
-        path = self._songs[index]
-        self._player.setCurrentSong(path)
-        self._keyPoint = 0.0
+        self._player.setCurrentSong(self._songs[index])
         self._notifier.notify(Notifier.CURRENT_SONG_EVENT, index)
-        self._notifier.notify(Notifier.CURRENT_KEY_POINT_EVENT, index)
+
+        previousKp = self._keyPoint
+        self._keyPoint = self.keyPoints[0] if len(self.keyPoints) > 0 else 0.0
+        if previousKp != self._keyPoint:
+            self._notifier.notify(Notifier.CURRENT_KEY_POINT_EVENT, self._keyPoint)
+
+        if previousSong is None or self._keyPoints[previousSong] != self._keyPoints[index]:
+            self._notifier.notify(Notifier.KEY_POINT_LIST_EVENT, self.keyPoints)
 
     @staticmethod
     def _keyPointValid(kp: float) -> bool:
@@ -61,8 +67,8 @@ class SoloTool:
     def keyPoints(self, new: list[float]) -> None:
         if new is not None and self._song is not None:
             sanitized = sorted(list(set([p for p in new if SoloTool._keyPointValid(p)])))
-            self._keyPoints[self._song] = sanitized
-            self._notifier.notify(Notifier.KEY_POINTS_EVENT, sanitized.copy())
+            self._keyPoints[self._song] = sanitized.copy()
+            self._notifier.notify(Notifier.KEY_POINT_LIST_EVENT, self.keyPoints)
 
     @property
     def keyPoint(self) -> float:
@@ -125,6 +131,12 @@ class SoloTool:
     def registerSongListCallback(self, callback):
         self._notifier.registerCallback(Notifier.SONG_LIST_EVENT, callback)
 
+    def registerKeyPointSelectionCallback(self, callback):
+        self._notifier.registerCallback(Notifier.CURRENT_KEY_POINT_EVENT, callback)
+
+    def registerKeyPointListCallback(self, callback):
+        self._notifier.registerCallback(Notifier.KEY_POINT_LIST_EVENT, callback)
+
     def registerPlayingStateCallback(self, callback):
         self._notifier.registerCallback(Notifier.PLAYING_STATE_EVENT, callback)
 
@@ -134,8 +146,3 @@ class SoloTool:
     def registerRateCallback(self, callback):
         self._notifier.registerCallback(Notifier.PLAYBACK_RATE_EVENT, callback)
 
-    def registerCurrentKeyPointCallback(self, callback):
-        self._notifier.registerCallback(Notifier.CURRENT_KEY_POINT_EVENT, callback)
-
-    def registerKeyPointsCallback(self, callback):
-        self._notifier.registerCallback(Notifier.KEY_POINTS_EVENT, callback)
diff --git a/solo-tool-project/test/notifier_unittest.py b/solo-tool-project/test/notifier_unittest.py
index 51d3e48..4ab6096 100644
--- a/solo-tool-project/test/notifier_unittest.py
+++ b/solo-tool-project/test/notifier_unittest.py
@@ -38,7 +38,7 @@ def test_allEvents(uut):
     checkEvent(uut, Notifier.PLAYBACK_RATE_EVENT)
     checkEvent(uut, Notifier.CURRENT_SONG_EVENT)
     checkEvent(uut, Notifier.CURRENT_KEY_POINT_EVENT)
-    checkEvent(uut, Notifier.KEY_POINTS_EVENT)
+    checkEvent(uut, Notifier.KEY_POINT_LIST_EVENT)
 
 def test_eventWithoutRegisteredCallbacks(uut):
     uut.notify(Notifier.PLAYING_STATE_EVENT, 0)
diff --git a/solo-tool-project/test/solo_tool_integrationtest.py b/solo-tool-project/test/solo_tool_integrationtest.py
index e63ea3f..fb759b0 100644
--- a/solo-tool-project/test/solo_tool_integrationtest.py
+++ b/solo-tool-project/test/solo_tool_integrationtest.py
@@ -107,95 +107,6 @@ def test_sanitizePlaybackVolume(uut):
     uut.volume = 150.0
     assert uut.volume == 150.0
 
-def test_addAndJumpToKeyPoints(uut, mockPlayer):
-    def checkJump(before, expectedAfter):
-        mockPlayer.position = before
-        uut.jump()
-        assert mockPlayer.position == expectedAfter
-
-    # Key points are None as long as no song is selected
-    uut.keyPoints = [0.1, 0.2]
-    uut.keyPoint = 0.5
-    assert uut.keyPoints is None
-    assert uut.keyPoint is None
-
-    uut.addSong("test.flac")
-    uut.addSong("test.mp3")
-
-    # Once a song is selected, jump to start by default
-    assert uut.keyPoint == 0.0
-    checkJump(0.5, 0.0)
-
-    # By default songs have an empty list of key points
-    assert uut.keyPoints == []
-
-    uut.keyPoints = [0.2, 0.4, 0.1, 0.2]
-
-    # Added key points are not automatically selected
-    assert uut.keyPoint == 0.0
-    checkJump(0.1, 0.0)
-
-    # Any key point can be selected
-    uut.keyPoint = uut.keyPoints[0]
-    checkJump(0.0, uut.keyPoints[0])
-
-    uut.keyPoint = 0.5
-    checkJump(0.0, 0.5)
-
-def test_sanitizeKeyPoint(uut):
-    song = "test.flac"
-    uut.addSong(song)
-    uut.song = 0
-    uut.keyPoints = [0.2, 0.4, 0.1, 0.2, None, -0.5, 1.0, 1.5]
-
-    # Added key points are automatically de-duplicated, sanitized and sorted to ascending order
-    assert uut.keyPoints == [0.1, 0.2, 0.4]
-
-    # Key point and key point list cannot be none
-    uut.keyPoint = 0.5
-
-    uut.keyPoint = None
-    assert uut.keyPoint == 0.5
-
-    uut.keyPoints = None
-    assert uut.keyPoints == [0.1, 0.2, 0.4]
-
-    # Valid key points are in [0, 1)
-    uut.keyPoint = -0.1
-    assert uut.keyPoint == 0.5
-
-    uut.keyPoint = 1.0
-    assert uut.keyPoint == 0.5
-
-    uut.keyPoint = 0.999
-    assert uut.keyPoint == 0.999
-
-def test_keyPointsPerSong(uut, mockPlayer):
-    songs = [
-        ("test.flac", [0.0, 0.5]),
-        ("test.mp3", [0.1])
-    ]
-    
-    # Key points list is set for the selected song
-    for i, (song, keyPoints) in enumerate(songs):
-        uut.addSong(song)
-        uut.song = i
-        uut.keyPoints = keyPoints
-
-    # Key points list is automatically loaded when the song selection changes
-    # Active key point is always reset to 0 when song selection changes
-    for i, (song, keyPoints) in enumerate(songs):
-        uut.keyPoint = 0.5
-        uut.song = i
-        assert uut.keyPoints == keyPoints
-        assert uut.keyPoint == 0.0
-
-    # Key points are copied, not stored by reference
-    for i, (song, keyPoints) in enumerate(songs):
-        uut.song = i
-        keyPoints.append(1.0)
-        assert 1.0 not in uut.keyPoints
-
 def test_playingStateNotification(uut, mockPlayer):
     song = "test.flac"
     uut.addSong(song)
@@ -288,71 +199,3 @@ def test_playbackRateNotification(uut, mockPlayer):
     assert not called
 
 
-def test_currentKeyPointNotification(uut):
-    called = False
-    receivedValue = None
-    def callback(value):
-        nonlocal called, receivedValue
-        called = True
-        receivedValue = value
-
-    uut.registerCurrentKeyPointCallback(callback)
-    assert not called
-
-    song = "test.flac"
-    uut.addSong(song)
-    uut.song = 0
-
-    # Selecting a song for the first time sets the key point to 0.0
-    assert called
-    assert receivedValue == 0.0
-    called = False
-
-    # Changing the key point triggers a notification
-    uut.keyPoint = 0.5
-    assert called
-    assert receivedValue == 0.5
-    called = False
-
-    # Adding list of key points does not trigger a notification
-    uut.keyPoints = [0.2, 0.4]
-    assert not called
-
-    # Assigning the same key point again does not trigger a notification
-    uut.keyPoint = 0.5
-    assert not called
-
-def test_keyPointsNotification(uut):
-    called = False
-    receivedValue = None
-    def callback(value):
-        nonlocal called, receivedValue
-        called = True
-        receivedValue = value
-
-    uut.registerKeyPointsCallback(callback)
-    assert not called
-
-    song = "test.flac"
-    uut.addSong(song)
-    uut.song = 0
-    assert not called
-
-    # Adding list of key points triggers a notification
-    uut.keyPoints = [0.2, 0.4]
-    assert called
-    assert receivedValue == [0.2, 0.4]
-    called = False
-
-    # Same list does trigger a notification again
-    uut.keyPoints = [0.2, 0.4]
-    assert called
-    assert receivedValue == [0.2, 0.4]
-    called = False
-
-    # Incrementing list of key points triggers a notification after sanitization
-    uut.keyPoints += [0.2, None, 0.1]
-    assert called
-    assert receivedValue == [0.1, 0.2, 0.4]
-    called = False
-
diff --git a/solo-tool-project/test/solo_tool_keypoints_integrationtest.py b/solo-tool-project/test/solo_tool_keypoints_integrationtest.py
new file mode 100644
index 0000000..a0a8663
--- /dev/null
+++ b/solo-tool-project/test/solo_tool_keypoints_integrationtest.py
@@ -0,0 +1,210 @@
+import pytest
+
+from solo_tool.solo_tool import SoloTool
+from player_mock import Player as MockPlayer
+
+@pytest.fixture
+def mockPlayer():
+    return MockPlayer()
+
+@pytest.fixture
+def uut(mockPlayer):
+    return SoloTool(mockPlayer)
+
+@pytest.fixture
+def testSongs():
+    return [
+        "test.flac",
+        "test.mp3"
+    ]
+
+def test_keyPointAndSongSelection(uut, mockPlayer, testSongs):
+    def checkJump(before, expectedAfter):
+        mockPlayer.position = before
+        uut.jump()
+        assert mockPlayer.position == expectedAfter
+
+    # Key point is initially unset
+    assert uut.keyPoint is None
+
+    # If no song is selected, setting the key point has no effect
+    assert uut.song is None
+    uut.keyPoint = 0.5
+    assert uut.keyPoint is None
+
+    # With a song selected, key point can be set and jumping works
+    uut.addSong(testSongs[0])
+    uut.keyPoints = [0.3, 0.5]
+
+    uut.keyPoint = 0.6
+    assert uut.keyPoint == 0.6
+    checkJump(0.8, 0.6)
+
+    # When another song is selected, the key point is set to 0.0
+    uut.addSong(testSongs[1])
+    uut.song = 1
+    assert uut.keyPoint == 0.0
+    checkJump(0.5, 0.0)
+
+    # If the selected song has stored key points, the key point is set to the first one instead
+    uut.song = 0
+    assert uut.keyPoint == 0.3
+    checkJump(0.5, 0.3)
+
+def test_keyPointListAndSongSelection(uut, testSongs):
+    # Key point list is initially unset, since no song is selected
+    assert uut.keyPoint is None
+
+    # If no song is selected, setting the key point list has no effect
+    assert uut.song is None
+    uut.keyPoints = [0.5]
+    assert uut.keyPoints is None
+
+    # When a song is added, key point list is initialized to empty
+    uut.addSong(testSongs[0])
+    assert uut.keyPoints == []
+    
+    # A new list can be assigned to the song, but it does not affect the current key point
+    uut.keyPoints = [0.1, 0.3]
+    assert uut.keyPoints == [0.1, 0.3]
+    assert uut.keyPoint == 0.0
+
+    # Each song has its own list of key points 
+    uut.addSong(testSongs[1])
+    uut.song = 1
+    uut.keyPoints = [0.4]
+
+    uut.song = 0
+    assert uut.keyPoints == [0.1, 0.3]
+    uut.song = 1
+    assert uut.keyPoints == [0.4]
+
+def test_keyPointEdgeCases(uut, testSongs):
+    uut.addSong(testSongs[0])
+
+    # Key point cannot be unset
+    uut.keyPoint = None
+    assert uut.keyPoint == 0.0
+
+    # Valid key points are in [0, 1)
+    uut.keyPoint = -0.1
+    assert uut.keyPoint == 0.0
+
+    uut.keyPoint = 1.0
+    assert uut.keyPoint == 0.0
+
+    uut.keyPoint = 0.999
+    assert uut.keyPoint == 0.999
+
+def test_keyPointListEdgeCases(uut, testSongs):
+    uut.addSong(testSongs[0])
+    
+    # Key point list cannot be unset
+    uut.keyPoints = None
+    assert uut.keyPoints == []
+
+    # Appending to the list has no effect
+    uut.keyPoints.append(0.5)
+    assert uut.keyPoints == []
+
+    # Added key points are automatically de-duplicated, sanitized and sorted to ascending order
+    uut.keyPoints = [0.2, 0.4, 0.1, 0.2, None, -0.5, 1.0, 1.5]
+    assert uut.keyPoints == [0.1, 0.2, 0.4]
+
+def test_keyPointSelectionNotification(uut, testSongs):
+    called = False
+    receivedValue = None
+    def callback(value):
+        nonlocal called, receivedValue
+        called = True
+        receivedValue = value
+
+    uut.registerKeyPointSelectionCallback(callback)
+    assert not called
+
+    # Selecting a song for the first time sets the key point to 0.0
+    uut.addSong(testSongs[0])
+    assert called
+    assert receivedValue == 0.0
+    called = False
+
+    # Changing the key point triggers a notification
+    uut.keyPoint = 0.5
+    assert called
+    assert receivedValue == 0.5
+    called = False
+
+    # Adding list of key points does not trigger a notification
+    uut.keyPoints = [0.2, 0.4]
+    assert not called
+
+    # Assigning the same key point again does not trigger a notification
+    uut.keyPoint = 0.5
+    assert not called
+
+    # Changing song triggers the notification
+    uut.addSong(testSongs[1])
+    uut.song = 1
+    assert called
+    assert receivedValue == 0.0
+    called = False
+
+    # But only if the key point really changes
+    uut.keyPoint = 0.2
+    assert called
+    assert receivedValue == 0.2
+    called = False
+
+    uut.song = 0
+    assert not called
+
+def test_keyPointListNotification(uut, testSongs):
+    called = False
+    receivedValue = None
+    def callback(value):
+        nonlocal called, receivedValue
+        called = True
+        receivedValue = value
+
+    uut.registerKeyPointListCallback(callback)
+    assert not called
+
+    # Adding the first song triggers since the list is now not None
+    uut.addSong(testSongs[0])
+    assert called
+    assert receivedValue == []
+    called = False
+
+    # Adding list of key points triggers
+    uut.keyPoints = [0.2, 0.4]
+    assert called
+    assert receivedValue == [0.2, 0.4]
+    called = False
+
+    # Same list does not trigger
+    uut.keyPoints = [0.2, 0.4]
+    assert called
+    assert receivedValue == [0.2, 0.4]
+    called = False
+
+    # Incrementing list of key points triggers after sanitization
+    uut.keyPoints += [0.2, None, 0.1]
+    assert called
+    assert receivedValue == [0.1, 0.2, 0.4]
+    called = False
+
+    # Changing song triggers
+    uut.addSong(testSongs[1])
+    uut.song = 1
+    assert called
+    assert receivedValue == []
+    called = False
+
+    # But only if the list really changed
+    uut.keyPoints = [0.1, 0.2, 0.4]
+    assert called
+    assert receivedValue == [0.1, 0.2, 0.4]
+    called = False
+
+    uut.song = 0
+    assert not called
diff --git a/solo-tool-project/test/solo_tool_songs_integrationtest.py b/solo-tool-project/test/solo_tool_songs_integrationtest.py
index 3d9368b..092ea93 100644
--- a/solo-tool-project/test/solo_tool_songs_integrationtest.py
+++ b/solo-tool-project/test/solo_tool_songs_integrationtest.py
@@ -1,5 +1,3 @@
-import pathlib
-import shutil
 import pytest
 
 from solo_tool.solo_tool import SoloTool