fix(entry): reuse local icons if present, before attempting downloads

Signed-off-by: Luca Di Maio <luca.dimaio1@gmail.com>

pkg/commands/generate_entry.go

@@ -5,9 +5,13 @@ import (
 	_ "embed"
 	"fmt"
 	"html/template"
+	"io"
+	"net/http"
 	"os"
+	"path"
 	"path/filepath"
 	"strings"
+	"time"
 
 	"github.com/89luca89/distrobox/internal/userenv"
 	pkgconfig "github.com/89luca89/distrobox/pkg/config"
@@ -18,9 +22,15 @@ var desktopEntryTmpl string
 
 const (
 	defaultContainerName = "my-distrobox"
-	defaultEntryIcon     = "https://raw.githubusercontent.com/89luca89/distrobox/main/icons/terminal-distrobox-icon.svg"
+	// fallbackIconName is a freedesktop theme icon installed by distrobox
+	// (icons/hicolor/**/terminal-distrobox-icon). Used when no distro is
+	// detected or its logo can't be downloaded.
+	fallbackIconName = "terminal-distrobox-icon"
 )
 
+// iconDownloadTimeout bounds the per-icon HTTP download.
+const iconDownloadTimeout = 10 * time.Second
+
 type GenerateEntryOptions struct {
 	Verbose             bool
 	Delete              bool
@@ -35,12 +45,16 @@ type GenerateEntryOptions struct {
 type GenerateEntryCommand struct {
 	cfg         *pkgconfig.Values
 	listCommand *ListCommand
+	// fetchIcon downloads url into destPath. It is a field so tests can inject
+	// a fake that writes bytes without touching the network.
+	fetchIcon func(ctx context.Context, url, destPath string) error
 }
 
 func NewGenerateEntryCommand(cfg *pkgconfig.Values, listCommand *ListCommand) *GenerateEntryCommand {
 	return &GenerateEntryCommand{
 		cfg:         cfg,
 		listCommand: listCommand,
+		fetchIcon:   downloadIconFile,
 	}
 }
 
@@ -94,7 +108,7 @@ func (c *GenerateEntryCommand) Execute(
 		if image, ok := containerImages[containerName]; ok && image != "" {
 			distroHint = image
 		}
-		if err := c.createEntry(containerName, icon, distroHint, desktopEntryBaseDir, distroboxPath, opts.Root); err != nil {
+		if err := c.createEntry(ctx, containerName, icon, distroHint, desktopEntryBaseDir, distroboxPath, opts.Root); err != nil {
 			return fmt.Errorf("failed to create desktop entry for container %s: %w", containerName, err)
 		}
 	}
@@ -158,6 +172,7 @@ func (c *GenerateEntryCommand) deleteEntry(containerName string, desktopEntryBas
 }
 
 func (c *GenerateEntryCommand) createEntry(
+	ctx context.Context,
 	containerName string,
 	icon string,
 	distroHint string,
@@ -165,13 +180,13 @@ func (c *GenerateEntryCommand) createEntry(
 	distroboxPath string,
 	root bool,
 ) error {
-	desktopEntryAppsDir, _, err := c.ensureDesktopEntryDirExists(desktopEntryBaseDir)
+	desktopEntryAppsDir, desktopEntryIconsDir, err := c.ensureDesktopEntryDirExists(desktopEntryBaseDir)
 	if err != nil {
 		return fmt.Errorf("failed to ensure desktop entry directories exist: %w", err)
 	}
 
 	entryFilePath := c.getEntryFilePath(desktopEntryAppsDir, containerName)
-	data := c.composeDesktopEntryData(containerName, icon, distroHint, distroboxPath, root)
+	data := c.composeDesktopEntryData(ctx, containerName, icon, distroHint, distroboxPath, desktopEntryIconsDir, root)
 	if err := c.writeDesktopEntryFile(entryFilePath, data); err != nil {
 		return fmt.Errorf("failed to write desktop entry file for container %s: %w", containerName, err)
 	}
@@ -197,10 +212,12 @@ func (c *GenerateEntryCommand) ensureDesktopEntryDirExists(desktopEntryBaseDir s
 // logo; pass the container's image name when available, otherwise the
 // container name itself.
 func (c *GenerateEntryCommand) composeDesktopEntryData(
+	ctx context.Context,
 	containerName string,
 	icon string,
 	distroHint string,
 	distroboxPath string,
+	iconsDir string,
 	root bool,
 ) map[string]string {
 	extraFlags := ""
@@ -212,7 +229,7 @@ func (c *GenerateEntryCommand) composeDesktopEntryData(
 		"entry_name":     getEntryName(containerName),
 		"container_name": containerName,
 		"distrobox_path": distroboxPath,
-		"icon":           c.getDesktopIcon(icon, distroHint),
+		"icon":           c.resolveIcon(ctx, icon, distroHint, iconsDir),
 		"extra_flags":    extraFlags,
 	}
 }
@@ -257,19 +274,78 @@ func (c *GenerateEntryCommand) getEntryFilePath(desktopEntryDir, containerName s
 	return filepath.Join(desktopEntryDir, containerName+".desktop")
 }
 
-// getDesktopIcon resolves the icon URL written to the desktop entry.
+// resolveIcon resolves the Icon= value written to the desktop entry.
 //
-// When the caller passes a concrete icon it is returned unchanged. When the
-// special value "auto" is used, distroHint (typically the container's image
-// name, falling back to its name) is matched against the known distro icon
-// map. The generic terminal icon is returned if no distro can be detected.
-func (c *GenerateEntryCommand) getDesktopIcon(icon, distroHint string) string {
-	if icon != "auto" {
+// An explicit icon (anything other than ""/"auto") is returned unchanged. For
+// "auto", the distro is detected from distroHint and its bundled logo is cached
+// under <iconsDir>/distrobox and downloaded only once: a non-empty cache file is
+// reused, a miss is fetched, and a fetch failure falls back to the generic
+// terminal theme icon WITHOUT clobbering any existing cache file.
+func (c *GenerateEntryCommand) resolveIcon(ctx context.Context, icon, distroHint, iconsDir string) string {
+	if icon != "" && icon != "auto" {
 		return icon
 	}
 
-	if url := lookupDistroIcon(distroHint); url != "" {
-		return url
+	url := lookupDistroIcon(distroHint)
+	if url == "" {
+		return fallbackIconName
 	}
-	return defaultEntryIcon
+
+	localPath := filepath.Join(iconsDir, "distrobox", path.Base(url))
+
+	// Cache hit: reuse a previously downloaded icon, so we neither re-download
+	// nor risk truncating it (works offline once cached).
+	if info, err := os.Stat(localPath); err == nil && info.Size() > 0 {
+		return localPath
+	}
+
+	if err := c.fetchIcon(ctx, url, localPath); err != nil {
+		return fallbackIconName
+	}
+	return localPath
+}
+
+// downloadIconFile downloads url into destPath, creating the parent directory.
+// It writes to a temp file and renames atomically, so a partial or failed
+// download never leaves a truncated icon in place.
+func downloadIconFile(ctx context.Context, url, destPath string) error {
+	if err := os.MkdirAll(filepath.Dir(destPath), 0o750); err != nil {
+		return fmt.Errorf("failed to create icon directory: %w", err)
+	}
+
+	ctx, cancel := context.WithTimeout(ctx, iconDownloadTimeout)
+	defer cancel()
+
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
+	if err != nil {
+		return fmt.Errorf("failed to build icon request: %w", err)
+	}
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		return fmt.Errorf("failed to download icon: %w", err)
+	}
+	defer resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		return fmt.Errorf("failed to download icon: status %d", resp.StatusCode)
+	}
+
+	tmp, err := os.CreateTemp(filepath.Dir(destPath), ".icon-*")
+	if err != nil {
+		return fmt.Errorf("failed to create temp icon file: %w", err)
+	}
+	tmpName := tmp.Name()
+	defer os.Remove(tmpName) // no-op once renamed
+
+	if _, err := io.Copy(tmp, resp.Body); err != nil {
+		tmp.Close()
+		return fmt.Errorf("failed to write icon: %w", err)
+	}
+	if err := tmp.Close(); err != nil {
+		return fmt.Errorf("failed to close icon file: %w", err)
+	}
+
+	if err := os.Rename(tmpName, destPath); err != nil {
+		return fmt.Errorf("failed to finalize icon: %w", err)
+	}
+	return nil
 }

pkg/commands/generate_entry_internal_test.go

@@ -1,78 +1,106 @@
 package commands
 
 import (
+	"context"
+	"errors"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
-func TestGetDesktopIcon_AutoDetectsFedora(t *testing.T) {
-	c := &GenerateEntryCommand{}
-
-	got := c.getDesktopIcon("auto", "fedora-39")
-
-	want := "https://raw.githubusercontent.com/89luca89/distrobox/main/docs/assets/png/distros/fedora-distrobox.png"
-	assert.Equal(t, want, got)
+func newIconTestCmd(fetch func(context.Context, string, string) error) *GenerateEntryCommand {
+	c := NewGenerateEntryCommand(nil, nil)
+	if fetch != nil {
+		c.fetchIcon = fetch
+	}
+	return c
 }
 
-func TestGetDesktopIcon_AutoDetectsUbuntu(t *testing.T) {
-	c := &GenerateEntryCommand{}
-
-	got := c.getDesktopIcon("auto", "ubuntu-22")
-
-	want := "https://raw.githubusercontent.com/89luca89/distrobox/main/docs/assets/png/distros/ubuntu-distrobox.png"
-	assert.Equal(t, want, got)
+// fakeIconWriter simulates a successful download by writing bytes to dest.
+func fakeIconWriter(t *testing.T) func(context.Context, string, string) error {
+	t.Helper()
+	return func(_ context.Context, _ string, dest string) error {
+		if err := os.MkdirAll(filepath.Dir(dest), 0o750); err != nil {
+			return err
+		}
+		return os.WriteFile(dest, []byte("png"), 0o644)
+	}
 }
 
-func TestGetDesktopIcon_AutoFallsBackToTerminalOnUnknownDistro(t *testing.T) {
-	c := &GenerateEntryCommand{}
-
-	got := c.getDesktopIcon("auto", "unknown-distro")
-
-	assert.Equal(t, defaultEntryIcon, got)
-}
-
-func TestGetDesktopIcon_CustomIconIsPassedThroughUnchanged(t *testing.T) {
-	c := &GenerateEntryCommand{}
-
-	got := c.getDesktopIcon("custom-url", "fedora")
-
+func TestResolveIcon_ExplicitIconPassthrough(t *testing.T) {
+	c := newIconTestCmd(nil)
+	got := c.resolveIcon(context.Background(), "custom-url", "fedora", t.TempDir())
 	assert.Equal(t, "custom-url", got)
 }
 
-func TestGetDesktopIcon_AutoDetectsOpenSUSETumbleweedBeforeGenericOpenSUSE(t *testing.T) {
-	// Regression: opensuse-tumbleweed must be matched as a specific variant,
-	// not collapsed to a non-existent generic "opensuse" entry.
-	c := &GenerateEntryCommand{}
+func TestResolveIcon_AutoDownloadsDistroIconOnce(t *testing.T) {
+	dir := t.TempDir()
+	calls := 0
+	c := newIconTestCmd(func(_ context.Context, _ string, dest string) error {
+		calls++
+		require.NoError(t, os.MkdirAll(filepath.Dir(dest), 0o750))
+		return os.WriteFile(dest, []byte("png"), 0o644)
+	})
 
-	got := c.getDesktopIcon("auto", "registry.opensuse.org/opensuse/tumbleweed:latest")
+	want := filepath.Join(dir, "distrobox", "fedora-distrobox.png")
 
-	want := "https://raw.githubusercontent.com/89luca89/distrobox/main/docs/assets/png/distros/opensuse-distrobox.png"
+	got := c.resolveIcon(context.Background(), "auto", "fedora-39", dir)
 	assert.Equal(t, want, got)
+	assert.FileExists(t, want)
+	assert.Equal(t, 1, calls)
+
+	// Second resolution is a cache hit: no second download.
+	got2 := c.resolveIcon(context.Background(), "auto", "fedora-39", dir)
+	assert.Equal(t, want, got2)
+	assert.Equal(t, 1, calls, "cached icon must not be re-downloaded")
 }
 
-func TestGetDesktopIcon_AutoDetectsArchFromImageRegistry(t *testing.T) {
-	c := &GenerateEntryCommand{}
+func TestResolveIcon_DownloadFailureFallsBackWithoutClobber(t *testing.T) {
+	dir := t.TempDir()
+	c := newIconTestCmd(func(_ context.Context, _ string, _ string) error {
+		return errors.New("offline")
+	})
 
-	got := c.getDesktopIcon("auto", "docker.io/library/archlinux:latest")
-
-	want := "https://raw.githubusercontent.com/89luca89/distrobox/main/docs/assets/png/distros/arch-distrobox.png"
-	assert.Equal(t, want, got)
+	got := c.resolveIcon(context.Background(), "auto", "ubuntu-22", dir)
+	assert.Equal(t, fallbackIconName, got)
+	assert.NoFileExists(t, filepath.Join(dir, "distrobox", "ubuntu-distrobox.png"))
 }
 
-func TestGetDesktopIcon_AutoIsCaseInsensitive(t *testing.T) {
-	c := &GenerateEntryCommand{}
+func TestResolveIcon_CacheHitReusedOffline(t *testing.T) {
+	dir := t.TempDir()
+	cached := filepath.Join(dir, "distrobox", "arch-distrobox.png")
+	require.NoError(t, os.MkdirAll(filepath.Dir(cached), 0o750))
+	require.NoError(t, os.WriteFile(cached, []byte("png"), 0o644))
 
-	got := c.getDesktopIcon("auto", "Fedora-Toolbox")
+	c := newIconTestCmd(func(_ context.Context, _ string, _ string) error {
+		t.Fatal("must not download when a cached icon exists")
+		return nil
+	})
 
-	want := "https://raw.githubusercontent.com/89luca89/distrobox/main/docs/assets/png/distros/fedora-distrobox.png"
-	assert.Equal(t, want, got)
+	got := c.resolveIcon(context.Background(), "auto", "docker.io/library/archlinux:latest", dir)
+	assert.Equal(t, cached, got)
 }
 
-func TestGetDesktopIcon_AutoWithEmptyHintFallsBackToDefault(t *testing.T) {
-	c := &GenerateEntryCommand{}
+func TestResolveIcon_UnknownDistroUsesFallback(t *testing.T) {
+	c := newIconTestCmd(func(_ context.Context, _ string, _ string) error {
+		t.Fatal("must not download for an unknown distro")
+		return nil
+	})
 
-	got := c.getDesktopIcon("auto", "")
-
-	assert.Equal(t, defaultEntryIcon, got)
+	assert.Equal(t, fallbackIconName, c.resolveIcon(context.Background(), "auto", "unknown-distro", t.TempDir()))
+	assert.Equal(t, fallbackIconName, c.resolveIcon(context.Background(), "auto", "", t.TempDir()))
+}
+
+func TestResolveIcon_AutoDetectsCaseInsensitiveAndSpecific(t *testing.T) {
+	dir := t.TempDir()
+	c := newIconTestCmd(fakeIconWriter(t))
+
+	assert.Equal(t, filepath.Join(dir, "distrobox", "fedora-distrobox.png"),
+		c.resolveIcon(context.Background(), "auto", "Fedora-Toolbox", dir))
+	// opensuse-tumbleweed must match the specific variant, not a generic miss.
+	assert.Equal(t, filepath.Join(dir, "distrobox", "opensuse-distrobox.png"),
+		c.resolveIcon(context.Background(), "auto", "registry.opensuse.org/opensuse/tumbleweed:latest", dir))
 }

pkg/commands/generate_entry_test.go

@@ -206,6 +206,12 @@ func TestGenerateEntryCommand_SingleModeUsesImageHint(t *testing.T) {
 	listCmd := commands.NewListCommand(&config.Values{}, mock)
 	cmd := commands.NewGenerateEntryCommand(&config.Values{}, listCmd)
 
+	// Pre-seed the icon cache so resolution reuses it (no network) while still
+	// proving the distro was detected from the image, not the box name.
+	iconPath := filepath.Join(tempDir, "icons", "distrobox", "ubuntu-distrobox.png")
+	require.NoError(t, os.MkdirAll(filepath.Dir(iconPath), 0o750))
+	require.NoError(t, os.WriteFile(iconPath, []byte("png"), 0o644))
+
 	err := cmd.Execute(context.Background(), &commands.GenerateEntryOptions{
 		ContainerName:       "dev",
 		Icon:                "auto",
@@ -217,5 +223,5 @@ func TestGenerateEntryCommand_SingleModeUsesImageHint(t *testing.T) {
 	content, err := os.ReadFile(filepath.Join(tempDir, "applications", "dev.desktop"))
 	require.NoError(t, err)
 	assert.Contains(t, string(content), "ubuntu-distrobox.png",
-		"single-mode must key auto-detection off the image, not the box name 'dev'")
+		"single-mode must key auto-detection off the image (cached icon path), not the box name 'dev'")
 }