From b16d0bce8b998c3dc4c8c2177b8467dea95069f3 Mon Sep 17 00:00:00 2001
From: Shaowen Yin <cosven@users.noreply.github.com>
Date: Sun, 14 Apr 2024 23:37:02 +0800
Subject: [PATCH] gui: fix memory leak in SongMinicardListModel (#822)

---
 feeluown/gui/components/player_playlist.py   | 12 ++++++----
 feeluown/gui/widgets/song_minicard_list.py   | 12 ++++++++--
 tests/gui/widgets/test_song_minicard_list.py | 24 ++++++++++++++++++++
 3 files changed, 42 insertions(+), 6 deletions(-)
 create mode 100644 tests/gui/widgets/test_song_minicard_list.py

diff --git a/feeluown/gui/components/player_playlist.py b/feeluown/gui/components/player_playlist.py
index 87b1507813..ab3698dad9 100644
--- a/feeluown/gui/components/player_playlist.py
+++ b/feeluown/gui/components/player_playlist.py
@@ -11,7 +11,9 @@
 
 
 class PlayerPlaylistModel(SongMiniCardListModel):
-    # FIXME: make this singleton
+    """
+    this is a singleton class (ensured by PlayerPlaylistView)
+    """
 
     def __init__(self, playlist, *args, **kwargs):
         reader = create_reader(playlist.list())
@@ -47,17 +49,19 @@ def on_songs_removed(self, index, count):
 
 class PlayerPlaylistView(SongMiniCardListView):
 
+    _model = None
+
     def __init__(self, app, *args, **kwargs):
         super().__init__(*args, **kwargs)
         self._app = app
 
         self.play_song_needed.connect(self._app.playlist.play_model)
-        self.setModel(
-            PlayerPlaylistModel(
+        if PlayerPlaylistView._model is None:
+            PlayerPlaylistView._model = PlayerPlaylistModel(
                 self._app.playlist,
                 fetch_cover_wrapper(self._app),
             )
-        )
+        self.setModel(PlayerPlaylistView._model)
 
     def contextMenuEvent(self, e):
         index = self.indexAt(e.pos())
diff --git a/feeluown/gui/widgets/song_minicard_list.py b/feeluown/gui/widgets/song_minicard_list.py
index edab0e64e0..df36ea8439 100644
--- a/feeluown/gui/widgets/song_minicard_list.py
+++ b/feeluown/gui/widgets/song_minicard_list.py
@@ -31,6 +31,7 @@ def __init__(self, fetch_image, parent=None):
         self._items = []
         self.fetch_image = fetch_image
         self.pixmaps = {}  # {uri: (Option<pixmap>, Option<color>)}
+        self.rowsAboutToBeRemoved.connect(self.on_rows_about_to_be_removed)
 
     def rowCount(self, _=QModelIndex()):
         return len(self._items)
@@ -45,7 +46,7 @@ def _fetch_image_callback(self, item):
         # TODO: duplicate code with ImgListModel
         def cb(content):
             uri = reverse(item)
-            if content is None:
+            if content is None and uri in self.pixmaps:
                 self.pixmaps[uri] = (self.pixmaps[uri][1], None)
                 return
 
@@ -87,8 +88,15 @@ def data(self, index, role=Qt.DisplayRole):
             return (song, pixmap)
         return None
 
+    def on_rows_about_to_be_removed(self, _, first, last):
+        for i in range(first, last+1):
+            item = self._items[i]
+            uri = reverse(item)
+            # clear pixmap cache
+            self.pixmaps.pop(uri, None)
+
 
-class SongMiniCardListModel(BaseSongMiniCardListModel, ReaderFetchMoreMixin):
+class SongMiniCardListModel(ReaderFetchMoreMixin, BaseSongMiniCardListModel):
     def __init__(self, reader, fetch_image, parent=None):
         super().__init__(fetch_image, parent)
 
diff --git a/tests/gui/widgets/test_song_minicard_list.py b/tests/gui/widgets/test_song_minicard_list.py
new file mode 100644
index 0000000000..18c29e2302
--- /dev/null
+++ b/tests/gui/widgets/test_song_minicard_list.py
@@ -0,0 +1,24 @@
+import pytest
+from PyQt5.QtCore import QModelIndex
+
+from feeluown.utils import aio
+from feeluown.utils.reader import create_reader
+from feeluown.gui.widgets.song_minicard_list import SongMiniCardListModel
+
+
+@pytest.mark.asyncio
+async def test_song_mini_card_list_model_remove_pixmap(qtbot, song):
+    async def fetch_cover(*_): return b'image content'
+    model = SongMiniCardListModel(create_reader([song]), fetch_cover)
+    assert model.rowCount() == 0
+    model.fetchMore(QModelIndex())
+    await aio.sleep(0.1)
+    assert model.rowCount() == 1
+    model.get_pixmap_unblocking(song)
+    await aio.sleep(0.1)
+    assert len(model.pixmaps) == 1
+    with qtbot.waitSignal(model.rowsAboutToBeRemoved):
+        model.beginRemoveRows(QModelIndex(), 0, 0)
+        model._items.pop(0)
+        model.endRemoveRows()
+    assert len(model.pixmaps) == 0