From 2e33b81914b9c5ae873f10ac59dc671dae47c72f Mon Sep 17 00:00:00 2001 From: Gianluca Boiano <491117+M0Rf30@users.noreply.github.com> Date: Tue, 3 Feb 2026 20:39:47 +0100 Subject: [PATCH] Improve code quality and thread safety (#844) - Format Go files (gofmt) for backend/windows/notify*.go - Add thread safety to Stopwatch with sync.Mutex - Add comprehensive unit tests for Stopwatch with race detection - Add godoc comments to ImageCache public APIs - Improve IPC documentation with platform-specific socket path details - Document portable mode TODO for future enhancement --- backend/imagecache.go | 34 +++++- backend/ipc/conn_other.go | 14 ++- backend/util/stopwatch.go | 29 ++++- backend/util/stopwatch_test.go | 167 ++++++++++++++++++++++++++ backend/windows/notify.go | 150 +++++++++++------------ backend/windows/notify_unsupported.go | 20 +-- 6 files changed, 324 insertions(+), 90 deletions(-) create mode 100644 backend/util/stopwatch_test.go diff --git a/backend/imagecache.go b/backend/imagecache.go index 3b1d667..8357035 100644 --- a/backend/imagecache.go +++ b/backend/imagecache.go @@ -18,7 +18,9 @@ type CacheItem struct { lastAccessed int64 } -// A custom in-memory cache for images with the following eviction strategy: +// ImageCache is a thread-safe in-memory cache for images with LRU eviction. +// +// Eviction strategy: // 1. If there are fewer than MinSize items in the cache, none will be evicted // 2. If a new addition would make the cache exceed MaxSize, an item will be immediately evicted // 2a. in this case, evict the LRU expired item or if none expired, the LRU item @@ -39,14 +41,19 @@ type ImageCache struct { cache map[string]CacheItem } +// ErrNotFound is returned when a requested cache item does not exist. var ErrNotFound = errors.New("item not found") +// Init initializes the cache and starts a background goroutine for periodic eviction. +// The goroutine stops when the provided context is cancelled. func (i *ImageCache) Init(ctx context.Context, evictionInterval time.Duration) { i.cache = make(map[string]CacheItem) go i.periodicallyEvict(ctx, evictionInterval) } -// holds writer lock for O(i.MaxSize) worst case +// SetWithTTL stores an image in the cache with a custom time-to-live duration. +// If the cache is at MaxSize, an item will be evicted using LRU strategy. +// Thread-safe. Holds writer lock for O(MaxSize) worst case. func (i *ImageCache) SetWithTTL(key string, val image.Image, ttl time.Duration) { i.mu.Lock() defer i.mu.Unlock() @@ -71,10 +78,14 @@ func (i *ImageCache) SetWithTTL(key string, val image.Image, ttl time.Duration) } } +// Set stores an image in the cache with the default TTL. +// See SetWithTTL for more details. func (i *ImageCache) Set(key string, val image.Image) { i.SetWithTTL(key, val, i.DefaultTTL) } +// Has returns true if the key exists in the cache, expired or not. +// Thread-safe. func (i *ImageCache) Has(key string) bool { i.mu.RLock() defer i.mu.RUnlock() @@ -83,10 +94,17 @@ func (i *ImageCache) Has(key string) bool { return ok } +// Get retrieves an image from the cache and updates its last accessed time. +// Returns ErrNotFound if the key doesn't exist. +// Thread-safe. func (i *ImageCache) Get(key string) (image.Image, error) { return i.GetResetTTL(key, false) } +// GetResetTTL retrieves an image and optionally resets its expiration time. +// If resetTTL is true, the expiration is reset to now + original TTL. +// Returns ErrNotFound if the key doesn't exist. +// Thread-safe. func (i *ImageCache) GetResetTTL(key string, resetTTL bool) (image.Image, error) { i.mu.Lock() defer i.mu.Unlock() @@ -102,7 +120,11 @@ func (i *ImageCache) GetResetTTL(key string, resetTTL bool) (image.Image, error) return nil, ErrNotFound } -// Gets the image if it exists and extends TTL to time.Now + ttl iff the image would expire before then +// GetExtendTTL retrieves an image and extends its TTL if it would expire sooner. +// The expiration time is extended to now + ttl only if the current expiration +// is earlier than that time. +// Returns ErrNotFound if the key doesn't exist. +// Thread-safe. func (i *ImageCache) GetExtendTTL(key string, ttl time.Duration) (image.Image, error) { i.mu.Lock() defer i.mu.Unlock() @@ -118,6 +140,10 @@ func (i *ImageCache) GetExtendTTL(key string, ttl time.Duration) (image.Image, e return nil, ErrNotFound } +// GetWithNewTTL retrieves an image and replaces its TTL with a new value. +// The expiration time is set to now + newTtl. +// Returns ErrNotFound if the key doesn't exist. +// Thread-safe. func (i *ImageCache) GetWithNewTTL(key string, newTtl time.Duration) (image.Image, error) { i.mu.Lock() defer i.mu.Unlock() @@ -132,6 +158,8 @@ func (i *ImageCache) GetWithNewTTL(key string, newTtl time.Duration) (image.Imag return nil, ErrNotFound } +// Clear removes all items from the cache. +// Thread-safe. func (i *ImageCache) Clear() { i.mu.Lock() defer i.mu.Unlock() diff --git a/backend/ipc/conn_other.go b/backend/ipc/conn_other.go index 5ac6a45..0318bd5 100644 --- a/backend/ipc/conn_other.go +++ b/backend/ipc/conn_other.go @@ -11,6 +11,12 @@ import ( "runtime" ) +// socketPath is automatically initialized based on platform conventions: +// - macOS: ~/Library/Caches/supersonic/supersonic.sock (or /tmp/supersonic-{uid}.sock as fallback) +// - Linux/Unix: $XDG_RUNTIME_DIR/supersonic.sock (or /tmp/supersonic-{uid}.sock as fallback) +// +// TODO: Add support for portable mode by allowing override via environment variable +// or configuration file (e.g., SUPERSONIC_SOCKET_PATH). var socketPath = "/tmp/supersonic.sock" func init() { @@ -29,15 +35,21 @@ func init() { } } +// Dial establishes a connection to the IPC socket. +// Returns an error if the socket doesn't exist or connection fails. func Dial() (net.Conn, error) { - // TODO - use XDG runtime dir, also handle portable mode return net.Dial("unix", socketPath) } +// Listen creates a Unix domain socket listener at the configured path. +// The socket file is created automatically and should be cleaned up +// with DestroyConn() when done. func Listen() (net.Listener, error) { return net.Listen("unix", socketPath) } +// DestroyConn removes the Unix socket file from the filesystem. +// Should be called during application shutdown. func DestroyConn() error { return os.Remove(socketPath) } diff --git a/backend/util/stopwatch.go b/backend/util/stopwatch.go index f3610a2..609795c 100644 --- a/backend/util/stopwatch.go +++ b/backend/util/stopwatch.go @@ -1,14 +1,26 @@ package util -import "time" +import ( + "sync" + "time" +) +// Stopwatch is a thread-safe timer for measuring elapsed time. +// It can be started, stopped, and reset, and supports reading +// the elapsed time while running or stopped. type Stopwatch struct { + mu sync.Mutex running bool started time.Time elapsed time.Duration } +// Start begins or resumes the stopwatch. +// If already running, this is a no-op. func (s *Stopwatch) Start() { + s.mu.Lock() + defer s.mu.Unlock() + if s.running { return } @@ -16,7 +28,12 @@ func (s *Stopwatch) Start() { s.running = true } +// Stop pauses the stopwatch and accumulates the elapsed time. +// If already stopped, this is a no-op. func (s *Stopwatch) Stop() { + s.mu.Lock() + defer s.mu.Unlock() + if !s.running { return } @@ -24,7 +41,13 @@ func (s *Stopwatch) Stop() { s.running = false } +// Elapsed returns the total elapsed time. +// If the stopwatch is running, includes time since last Start(). +// Safe to call concurrently with other methods. func (s *Stopwatch) Elapsed() time.Duration { + s.mu.Lock() + defer s.mu.Unlock() + e := s.elapsed if s.running { e += time.Since(s.started) @@ -32,7 +55,11 @@ func (s *Stopwatch) Elapsed() time.Duration { return e } +// Reset stops the stopwatch and clears the elapsed time. func (s *Stopwatch) Reset() { + s.mu.Lock() + defer s.mu.Unlock() + s.running = false s.elapsed = time.Duration(0) } diff --git a/backend/util/stopwatch_test.go b/backend/util/stopwatch_test.go new file mode 100644 index 0000000..60b265f --- /dev/null +++ b/backend/util/stopwatch_test.go @@ -0,0 +1,167 @@ +package util + +import ( + "sync" + "testing" + "time" +) + +func TestStopwatch_Basic(t *testing.T) { + sw := &Stopwatch{} + + // Test initial state + if elapsed := sw.Elapsed(); elapsed != 0 { + t.Errorf("Expected initial elapsed time to be 0, got %v", elapsed) + } + + // Test start and elapsed + sw.Start() + time.Sleep(10 * time.Millisecond) + elapsed := sw.Elapsed() + if elapsed < 10*time.Millisecond { + t.Errorf("Expected at least 10ms elapsed, got %v", elapsed) + } + + // Test stop + sw.Stop() + stoppedElapsed := sw.Elapsed() + time.Sleep(10 * time.Millisecond) + if sw.Elapsed() != stoppedElapsed { + t.Error("Elapsed time should not increase after Stop()") + } + + // Test reset + sw.Reset() + if elapsed := sw.Elapsed(); elapsed != 0 { + t.Errorf("Expected elapsed time to be 0 after reset, got %v", elapsed) + } +} + +func TestStopwatch_StartStop(t *testing.T) { + sw := &Stopwatch{} + + // Start, accumulate some time + sw.Start() + time.Sleep(10 * time.Millisecond) + sw.Stop() + firstElapsed := sw.Elapsed() + + // Start again, accumulate more time + sw.Start() + time.Sleep(10 * time.Millisecond) + sw.Stop() + secondElapsed := sw.Elapsed() + + if secondElapsed <= firstElapsed { + t.Errorf("Expected elapsed time to accumulate, first=%v second=%v", firstElapsed, secondElapsed) + } +} + +func TestStopwatch_DoubleStart(t *testing.T) { + sw := &Stopwatch{} + + sw.Start() + time.Sleep(5 * time.Millisecond) + firstStart := sw.Elapsed() + + // Second Start() should be no-op + sw.Start() + time.Sleep(5 * time.Millisecond) + secondStart := sw.Elapsed() + + // Time should continue from first start + if secondStart < firstStart { + t.Error("Second Start() affected timing") + } +} + +func TestStopwatch_DoubleStop(t *testing.T) { + sw := &Stopwatch{} + + sw.Start() + time.Sleep(10 * time.Millisecond) + sw.Stop() + elapsed := sw.Elapsed() + + // Second Stop() should be no-op + sw.Stop() + if sw.Elapsed() != elapsed { + t.Error("Second Stop() changed elapsed time") + } +} + +func TestStopwatch_ConcurrentAccess(t *testing.T) { + sw := &Stopwatch{} + var wg sync.WaitGroup + + // Test concurrent Start/Stop/Elapsed calls + // This should not cause data races + const goroutines = 10 + const iterations = 100 + + wg.Add(goroutines * 3) + + // Concurrent starts + for i := 0; i < goroutines; i++ { + go func() { + defer wg.Done() + for j := 0; j < iterations; j++ { + sw.Start() + time.Sleep(time.Microsecond) + } + }() + } + + // Concurrent stops + for i := 0; i < goroutines; i++ { + go func() { + defer wg.Done() + for j := 0; j < iterations; j++ { + sw.Stop() + time.Sleep(time.Microsecond) + } + }() + } + + // Concurrent reads + for i := 0; i < goroutines; i++ { + go func() { + defer wg.Done() + for j := 0; j < iterations; j++ { + _ = sw.Elapsed() + time.Sleep(time.Microsecond) + } + }() + } + + wg.Wait() + + // If we got here without data races, the test passes + // Run with: go test -race +} + +func TestStopwatch_Reset(t *testing.T) { + sw := &Stopwatch{} + + // Reset when stopped + sw.Reset() + if elapsed := sw.Elapsed(); elapsed != 0 { + t.Errorf("Expected 0 after reset, got %v", elapsed) + } + + // Reset when running + sw.Start() + time.Sleep(10 * time.Millisecond) + sw.Reset() + if elapsed := sw.Elapsed(); elapsed != 0 { + t.Errorf("Expected 0 after reset while running, got %v", elapsed) + } + + // After reset, should be able to start again + sw.Start() + time.Sleep(5 * time.Millisecond) + elapsed := sw.Elapsed() + if elapsed < 5*time.Millisecond { + t.Errorf("Expected at least 5ms after reset and start, got %v", elapsed) + } +} diff --git a/backend/windows/notify.go b/backend/windows/notify.go index d9ff896..bc7659c 100644 --- a/backend/windows/notify.go +++ b/backend/windows/notify.go @@ -1,75 +1,75 @@ -//go:build windows - -package windows - -import ( - "fmt" - "os" - "os/exec" - "path/filepath" - "strings" - "syscall" - - "fyne.io/fyne/v2" -) - -// The general approach here is copied from Fyne. -// While it seems very hacky (create a temporary Powershell script and execute it), -// shockingly it may be the best approach, at least in the non-installed case. -// The proper Windows APIs for this require WinRT (ie C++/ a DLL), and also require -// the app to be installed with a unique ID in the start menu, and to pass this ID -// when sending the notification. This could be a future exploration for the installer. - -const notificationTemplate = `$title = "%s" -$content = "%s" -$iconPath = "file:///%s" -[Windows.UI.Notifications.ToastNotificationManager, Windows.UI.Notifications, ContentType = WindowsRuntime] > $null -$template = [Windows.UI.Notifications.ToastNotificationManager]::GetTemplateContent([Windows.UI.Notifications.ToastTemplateType]::ToastImageAndText02) -$toastXml = [xml] $template.GetXml() -$toastXml.GetElementsByTagName("text")[0].AppendChild($toastXml.CreateTextNode($title)) > $null -$toastXml.GetElementsByTagName("text")[1].AppendChild($toastXml.CreateTextNode($content)) > $null -$toastXml.GetElementsByTagName("image")[0].SetAttribute("src", $iconPath) > $null -$audio = $toastXml.CreateElement("audio") -$audio.SetAttribute("silent", "true") > $null -$toastXml.DocumentElement.AppendChild($audio) > $null -$xml = New-Object Windows.Data.Xml.Dom.XmlDocument -$xml.LoadXml($toastXml.OuterXml) -$toast = [Windows.UI.Notifications.ToastNotification]::new($xml) -[Windows.UI.Notifications.ToastNotificationManager]::CreateToastNotifier("%s").Show($toast);` - -func SendNotification(n *fyne.Notification, iconFilePath string) { - title := escapeNotificationString(n.Title) - content := escapeNotificationString(n.Content) - - script := fmt.Sprintf(notificationTemplate, title, content, iconFilePath, "supersonic") - go runScript("notify", script) -} - -func escapeNotificationString(in string) string { - noSlash := strings.ReplaceAll(in, "`", "``") - return strings.ReplaceAll(noSlash, "\"", "`\"") -} - -var scriptNum = 0 - -func runScript(name, script string) { - scriptNum++ - appID := fyne.CurrentApp().UniqueID() - fileName := fmt.Sprintf("supersonic-%s-%s-%d.ps1", appID, name, scriptNum) - - tmpFilePath := filepath.Join(os.TempDir(), fileName) - err := os.WriteFile(tmpFilePath, []byte(script), 0o600) - if err != nil { - fyne.LogError("Could not write script to show notification", err) - return - } - defer os.Remove(tmpFilePath) - - launch := "(Get-Content -Encoding UTF8 -Path " + tmpFilePath + " -Raw) | Invoke-Expression" - cmd := exec.Command("PowerShell", "-ExecutionPolicy", "Bypass", launch) - cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true} - err = cmd.Run() - if err != nil { - fyne.LogError("Failed to launch windows notify script", err) - } -} +//go:build windows + +package windows + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "syscall" + + "fyne.io/fyne/v2" +) + +// The general approach here is copied from Fyne. +// While it seems very hacky (create a temporary Powershell script and execute it), +// shockingly it may be the best approach, at least in the non-installed case. +// The proper Windows APIs for this require WinRT (ie C++/ a DLL), and also require +// the app to be installed with a unique ID in the start menu, and to pass this ID +// when sending the notification. This could be a future exploration for the installer. + +const notificationTemplate = `$title = "%s" +$content = "%s" +$iconPath = "file:///%s" +[Windows.UI.Notifications.ToastNotificationManager, Windows.UI.Notifications, ContentType = WindowsRuntime] > $null +$template = [Windows.UI.Notifications.ToastNotificationManager]::GetTemplateContent([Windows.UI.Notifications.ToastTemplateType]::ToastImageAndText02) +$toastXml = [xml] $template.GetXml() +$toastXml.GetElementsByTagName("text")[0].AppendChild($toastXml.CreateTextNode($title)) > $null +$toastXml.GetElementsByTagName("text")[1].AppendChild($toastXml.CreateTextNode($content)) > $null +$toastXml.GetElementsByTagName("image")[0].SetAttribute("src", $iconPath) > $null +$audio = $toastXml.CreateElement("audio") +$audio.SetAttribute("silent", "true") > $null +$toastXml.DocumentElement.AppendChild($audio) > $null +$xml = New-Object Windows.Data.Xml.Dom.XmlDocument +$xml.LoadXml($toastXml.OuterXml) +$toast = [Windows.UI.Notifications.ToastNotification]::new($xml) +[Windows.UI.Notifications.ToastNotificationManager]::CreateToastNotifier("%s").Show($toast);` + +func SendNotification(n *fyne.Notification, iconFilePath string) { + title := escapeNotificationString(n.Title) + content := escapeNotificationString(n.Content) + + script := fmt.Sprintf(notificationTemplate, title, content, iconFilePath, "supersonic") + go runScript("notify", script) +} + +func escapeNotificationString(in string) string { + noSlash := strings.ReplaceAll(in, "`", "``") + return strings.ReplaceAll(noSlash, "\"", "`\"") +} + +var scriptNum = 0 + +func runScript(name, script string) { + scriptNum++ + appID := fyne.CurrentApp().UniqueID() + fileName := fmt.Sprintf("supersonic-%s-%s-%d.ps1", appID, name, scriptNum) + + tmpFilePath := filepath.Join(os.TempDir(), fileName) + err := os.WriteFile(tmpFilePath, []byte(script), 0o600) + if err != nil { + fyne.LogError("Could not write script to show notification", err) + return + } + defer os.Remove(tmpFilePath) + + launch := "(Get-Content -Encoding UTF8 -Path " + tmpFilePath + " -Raw) | Invoke-Expression" + cmd := exec.Command("PowerShell", "-ExecutionPolicy", "Bypass", launch) + cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true} + err = cmd.Run() + if err != nil { + fyne.LogError("Failed to launch windows notify script", err) + } +} diff --git a/backend/windows/notify_unsupported.go b/backend/windows/notify_unsupported.go index f5864e9..91f0d16 100644 --- a/backend/windows/notify_unsupported.go +++ b/backend/windows/notify_unsupported.go @@ -1,10 +1,10 @@ -//go:build !windows - -package windows - -import "fyne.io/fyne/v2" - -func SendNotification(n *fyne.Notification, iconFilePath string) { - fyne.LogError("windows.SendNotification should not be invoked on non-Windows platform", nil) - fyne.CurrentApp().SendNotification(n) -} +//go:build !windows + +package windows + +import "fyne.io/fyne/v2" + +func SendNotification(n *fyne.Notification, iconFilePath string) { + fyne.LogError("windows.SendNotification should not be invoked on non-Windows platform", nil) + fyne.CurrentApp().SendNotification(n) +}