fix: resolve memory leaks by improving resource cleanup and management
This commit is contained in:
tr1x_em committed
1 parent
b86edfc213
commit
75236973f0
1 file changed
+127
-84
+127
-84
@@ -4,6 +4,8 @@ import tempfile
|
||||
import urllib.parse
|
||||
import urllib.request
|
||||
from typing import List
|
||||
import threading
|
||||
import weakref
|
||||
|
||||
from fabric.utils import (
|
||||
bulk_connect,
|
||||
@@ -69,6 +71,9 @@ class PlayerBoxStack(Box):
|
||||
|
||||
# List to store player buttons
|
||||
self.player_buttons: list[Button] = []
|
||||
|
||||
# Track signal connections for cleanup
|
||||
self._signal_connections = []
|
||||
|
||||
# Create a "No media playing" placeholder
|
||||
self.no_media_box = self._create_no_media_box()
|
||||
@@ -81,13 +86,15 @@ class PlayerBoxStack(Box):
|
||||
|
||||
self.mpris_manager = mpris_manager
|
||||
|
||||
bulk_connect(
|
||||
# Track connections for cleanup
|
||||
connections = bulk_connect(
|
||||
self.mpris_manager,
|
||||
{
|
||||
"player-appeared": self.on_new_player,
|
||||
"player-vanished": self.on_lost_player,
|
||||
},
|
||||
)
|
||||
self._signal_connections.extend(connections)
|
||||
|
||||
for player in self.mpris_manager.players: # type: ignore
|
||||
logger.info(
|
||||
@@ -95,6 +102,34 @@ class PlayerBoxStack(Box):
|
||||
)
|
||||
self.on_new_player(self.mpris_manager, player)
|
||||
|
||||
def destroy(self):
|
||||
"""Clean up resources when the widget is destroyed."""
|
||||
# Disconnect all signal connections
|
||||
for connection in self._signal_connections:
|
||||
try:
|
||||
connection.disconnect()
|
||||
except Exception as e:
|
||||
logger.warning(f"Failed to disconnect signal: {e}")
|
||||
self._signal_connections.clear()
|
||||
|
||||
# Clean up player buttons
|
||||
for button in self.player_buttons:
|
||||
try:
|
||||
button.destroy()
|
||||
except Exception:
|
||||
pass
|
||||
self.player_buttons.clear()
|
||||
|
||||
# Clean up player boxes
|
||||
for child in self.player_stack.get_children():
|
||||
if hasattr(child, 'destroy') and child != self.no_media_box:
|
||||
try:
|
||||
child.destroy()
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
super().destroy()
|
||||
|
||||
def _create_no_media_box(self):
|
||||
"""Create a placeholder box for when no media is playing."""
|
||||
fallback_cover_path = f"{data.HOME_DIR}/.current.wall"
|
||||
@@ -303,8 +338,22 @@ class PlayerBoxStack(Box):
|
||||
logger.info(f"[PLAYER_MANAGER] Player Removed {player_name}")
|
||||
players: List[PlayerBox] = self.player_stack.get_children()
|
||||
|
||||
# Find and properly destroy the player box
|
||||
player_box_to_remove = None
|
||||
for player_box in players:
|
||||
if hasattr(player_box, 'player') and player_box.player.player_name == player_name:
|
||||
player_box_to_remove = player_box
|
||||
break
|
||||
|
||||
if player_box_to_remove:
|
||||
try:
|
||||
player_box_to_remove.destroy()
|
||||
except Exception as e:
|
||||
logger.warning(f"Failed to destroy player box: {e}")
|
||||
|
||||
# Check if this was the last player
|
||||
if len(players) == 1 and player_name == players[0].player.player_name:
|
||||
remaining_players = [p for p in self.player_stack.get_children() if p != player_box_to_remove]
|
||||
if len(remaining_players) == 0:
|
||||
# Show the no media box instead of hiding
|
||||
self.player_stack.children = [self.no_media_box]
|
||||
self.current_stack_pos = 0
|
||||
@@ -314,6 +363,7 @@ class PlayerBoxStack(Box):
|
||||
if (
|
||||
players
|
||||
and self.current_stack_pos < len(players)
|
||||
and hasattr(players[self.current_stack_pos], 'player')
|
||||
and players[self.current_stack_pos].player.player_name == player_name
|
||||
):
|
||||
self.current_stack_pos = max(0, self.current_stack_pos - 1)
|
||||
@@ -347,13 +397,15 @@ class PlayerBoxStack(Box):
|
||||
self.player_buttons.append(new_button)
|
||||
|
||||
# This will automatically destroy our used button
|
||||
player_box.connect(
|
||||
"destroy",
|
||||
lambda *_: [
|
||||
new_button.destroy(), # type: ignore
|
||||
self.player_buttons.pop(self.player_buttons.index(new_button)),
|
||||
],
|
||||
)
|
||||
def cleanup_button(*_):
|
||||
try:
|
||||
if new_button in self.player_buttons:
|
||||
self.player_buttons.remove(new_button)
|
||||
new_button.destroy()
|
||||
except Exception as e:
|
||||
logger.warning(f"Failed to cleanup button: {e}")
|
||||
|
||||
player_box.connect("destroy", cleanup_button)
|
||||
|
||||
def _update_all_player_buttons(self):
|
||||
"""Update all player boxes with the current button state"""
|
||||
@@ -405,6 +457,8 @@ class PlayerBox(Box):
|
||||
# Memory management
|
||||
self.temp_artwork_files = [] # Track temp files for cleanup
|
||||
self.current_download_thread = None # Track current download thread
|
||||
self._download_cancelled = False # Flag to cancel downloads
|
||||
self._signal_connections = [] # Track signal connections
|
||||
|
||||
self.album_cover = Box(style_classes="album-image-c")
|
||||
self.album_cover.set_style(
|
||||
@@ -605,28 +659,14 @@ class PlayerBox(Box):
|
||||
# self.stack_buttons_box,
|
||||
],
|
||||
)
|
||||
# self.overlay_box = Overlay(
|
||||
# child=self.outer_box,
|
||||
# overlays=[
|
||||
# # self.inner_box,
|
||||
# self.player_info_box,
|
||||
# self.image_stack,
|
||||
# Box(
|
||||
# children=Image(icon_name=self.player.player_name, icon_size=18),
|
||||
# h_align="end",
|
||||
# v_align="start",
|
||||
# style="margin-top: 5px; margin-right: 10px;",
|
||||
# tooltip_text=self.player.player_name, # type: ignore
|
||||
# ),
|
||||
# ],
|
||||
# )
|
||||
|
||||
self.children = [
|
||||
*self.children,
|
||||
self.box,
|
||||
]
|
||||
|
||||
bulk_connect(
|
||||
# Track signal connections for cleanup
|
||||
connections = bulk_connect(
|
||||
self.player,
|
||||
{
|
||||
"exit": self._on_player_exit,
|
||||
@@ -635,6 +675,34 @@ class PlayerBox(Box):
|
||||
"notify::metadata": self._on_metadata,
|
||||
},
|
||||
)
|
||||
self._signal_connections.extend(connections)
|
||||
|
||||
def destroy(self):
|
||||
"""Clean up all resources when the widget is destroyed."""
|
||||
# Cancel any ongoing downloads
|
||||
self._download_cancelled = True
|
||||
|
||||
# Disconnect all signal connections
|
||||
for connection in self._signal_connections:
|
||||
try:
|
||||
connection.disconnect()
|
||||
except Exception as e:
|
||||
logger.warning(f"Failed to disconnect signal: {e}")
|
||||
self._signal_connections.clear()
|
||||
|
||||
# Clean up temp files
|
||||
self._cleanup_temp_files()
|
||||
|
||||
# Clean up UI components
|
||||
try:
|
||||
if hasattr(self, 'ex_mousecapture'):
|
||||
self.ex_mousecapture.destroy()
|
||||
if hasattr(self, 'expanded_player'):
|
||||
self.expanded_player.destroy()
|
||||
except Exception as e:
|
||||
logger.warning(f"Failed to cleanup UI components: {e}")
|
||||
|
||||
super().destroy()
|
||||
|
||||
def __del__(self):
|
||||
"""Ensure cleanup happens even if player exits unexpectedly."""
|
||||
@@ -656,55 +724,6 @@ class PlayerBox(Box):
|
||||
def update_buttons(self, player_buttons, show_buttons):
|
||||
# """Update the stack switcher buttons in this player box"""
|
||||
pass
|
||||
# logger.info(
|
||||
# f"[PlayerBox] update_buttons called: show_buttons={
|
||||
# show_buttons
|
||||
# }, num_buttons={len(player_buttons)}"
|
||||
# )
|
||||
#
|
||||
# # Clear existing buttons
|
||||
# for child in self.stack_buttons_box.get_children():
|
||||
# child.destroy()
|
||||
#
|
||||
# if show_buttons and len(player_buttons) > 1:
|
||||
# logger.info(f"[PlayerBox] Creating {len(player_buttons)} stack buttons")
|
||||
# # Create simple dot indicators for each player
|
||||
# for i, button in enumerate(player_buttons):
|
||||
# # Create a more visible dot button
|
||||
# dot_button = Button(
|
||||
# name="player-stack-button",
|
||||
# style="min-width: 16px; min-height: 16px; border-radius: 8px; margin: 4px; background-color: #666; border: 1px solid #999;",
|
||||
# )
|
||||
#
|
||||
# # Set active state based on original button
|
||||
# if button.get_style_context().has_class("active"):
|
||||
# dot_button.add_style_class("active")
|
||||
# dot_button.set_style(
|
||||
# "min-width: 16px; min-height: 16px; border-radius: 8px; margin: 4px; background-color: #fff; border: 1px solid #ccc;"
|
||||
# )
|
||||
#
|
||||
# # Connect click handler to switch to this player
|
||||
# def make_click_handler(index):
|
||||
# return lambda *_: self.player_stack.on_player_clicked_by_index(
|
||||
# index
|
||||
# )
|
||||
#
|
||||
# dot_button.connect("clicked", make_click_handler(i))
|
||||
# self.stack_buttons_box.children = [
|
||||
# *self.stack_buttons_box.children,
|
||||
# dot_button,
|
||||
# ]
|
||||
# logger.info(
|
||||
# f"[PlayerBox] Added dot button {i}, total children: {
|
||||
# len(self.stack_buttons_box.get_children())
|
||||
# }"
|
||||
# )
|
||||
#
|
||||
# self.stack_buttons_box.show_all()
|
||||
# logger.info("[PlayerBox] Stack buttons box shown")
|
||||
# else:
|
||||
# self.stack_buttons_box.hide()
|
||||
# logger.info("[PlayerBox] Stack buttons box hidden")
|
||||
|
||||
def _on_metadata(self, *_):
|
||||
self._set_image()
|
||||
@@ -784,14 +803,21 @@ class PlayerBox(Box):
|
||||
local_arturl = urllib.parse.unquote(parsed.path)
|
||||
self._update_image(local_arturl)
|
||||
elif parsed.scheme in ("http", "https"):
|
||||
# Cancel any existing download thread to prevent memory buildup
|
||||
if self.current_download_thread is not None:
|
||||
# Note: GLib.Thread doesn't have a direct cancel method,
|
||||
# but we can track and ignore old downloads
|
||||
# Cancel any existing download to prevent memory buildup
|
||||
self._download_cancelled = True
|
||||
|
||||
# Use threading.Thread instead of GLib.Thread for better control
|
||||
if self.current_download_thread and self.current_download_thread.is_alive():
|
||||
# Thread will check _download_cancelled flag and exit early
|
||||
pass
|
||||
self.current_download_thread = GLib.Thread.new(
|
||||
"download-artwork", self._download_and_set_artwork, art_url
|
||||
|
||||
self._download_cancelled = False
|
||||
self.current_download_thread = threading.Thread(
|
||||
target=self._download_and_set_artwork,
|
||||
args=(art_url,),
|
||||
daemon=True # Dies with main thread
|
||||
)
|
||||
self.current_download_thread.start()
|
||||
else:
|
||||
self._update_image(art_url)
|
||||
|
||||
@@ -804,6 +830,10 @@ class PlayerBox(Box):
|
||||
temp_file_path = None
|
||||
|
||||
try:
|
||||
# Check if download was cancelled
|
||||
if self._download_cancelled:
|
||||
return
|
||||
|
||||
# Clean up old temp files first (keep only last 2 to avoid excessive cleanup)
|
||||
if len(self.temp_artwork_files) > 2:
|
||||
old_files = self.temp_artwork_files[:-2]
|
||||
@@ -815,13 +845,23 @@ class PlayerBox(Box):
|
||||
pass
|
||||
self.temp_artwork_files = self.temp_artwork_files[-2:]
|
||||
|
||||
# Check again if cancelled
|
||||
if self._download_cancelled:
|
||||
return
|
||||
|
||||
# Download artwork
|
||||
parsed = urllib.parse.urlparse(arturl)
|
||||
suffix = os.path.splitext(parsed.path)[1] or ".png"
|
||||
|
||||
with urllib.request.urlopen(arturl, timeout=10) as response: # Add timeout
|
||||
if self._download_cancelled:
|
||||
return
|
||||
data = response.read()
|
||||
|
||||
# Check one more time if cancelled
|
||||
if self._download_cancelled:
|
||||
return
|
||||
|
||||
# Create temp file in cache directory instead of system temp
|
||||
os.makedirs(CACHE_DIR, exist_ok=True)
|
||||
with tempfile.NamedTemporaryFile(
|
||||
@@ -832,20 +872,23 @@ class PlayerBox(Box):
|
||||
local_arturl = temp_file_path
|
||||
|
||||
# Track temp file for cleanup
|
||||
if temp_file_path:
|
||||
if temp_file_path and not self._download_cancelled:
|
||||
self.temp_artwork_files.append(temp_file_path)
|
||||
|
||||
except Exception as e:
|
||||
logger.warning(f"Failed to download artwork from {arturl}: {e}")
|
||||
if not self._download_cancelled:
|
||||
logger.warning(f"Failed to download artwork from {arturl}: {e}")
|
||||
# Clean up failed temp file
|
||||
if temp_file_path and os.path.exists(temp_file_path):
|
||||
try:
|
||||
os.unlink(temp_file_path)
|
||||
except Exception:
|
||||
pass
|
||||
return
|
||||
|
||||
GLib.idle_add(self._update_image, local_arturl)
|
||||
return None
|
||||
# Only update UI if not cancelled
|
||||
if not self._download_cancelled:
|
||||
GLib.idle_add(self._update_image, local_arturl)
|
||||
|
||||
def close_bluetooth(self, *args):
|
||||
"""Close Bluetooth control center"""
|
||||
|
||||
Reference in new issue
Block a user