diff --git a/AGENTS.md b/AGENTS.md index 3fe4477..b9cb4a9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,6 +55,7 @@ These structural constraints must hold at all times: | Device state (persisted pairs) | `~/.local/state/kcd/devices.json` (`$XDG_STATE_HOME/kcd/devices.json`) | | TLS cert / key | `~/.config/kcd/cert.pem`, `~/.config/kcd/key.pem` | | IPC Unix socket | `/run/user//kcd/kcd.sock` (`$XDG_RUNTIME_DIR/kcd/kcd.sock`) | +| Album art cache | `~/.cache/kcd/art/` (`$XDG_CACHE_HOME/kcd/art`) — resolved `kdeconnect://` art URIs, keyed by `kdeArtHash` | | Downloaded files | `~/Downloads/kcd/` (overridable via `download_dir` in config) | | systemd user unit | `~/.config/systemd/user/kcd.service` | diff --git a/cmd/kcd/cli_watch.go b/cmd/kcd/cli_watch.go index 84f77af..cadbd1c 100644 --- a/cmd/kcd/cli_watch.go +++ b/cmd/kcd/cli_watch.go @@ -128,7 +128,7 @@ var watchCmd = &cli.Command{ err := cl.Watch(ctx, c.StringSlice("events"), ch) // If err != nil, the connection failed or disconnected - if !isJSON && err != nil { + if err != nil { if err == context.Canceled { return nil } diff --git a/docs/CLI.md b/docs/CLI.md index 9dc45c8..b219266 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -424,7 +424,14 @@ kcd watch --events=mpris.update # bindsym XF86AudioNext exec kcd mpris next ``` -> **Note:** The `mpris.update` event fires whenever the phone sends a now-playing state change (track change, play/pause toggle). Subscribe with `kcd watch --events=mpris.update`. +> **Note:** The `mpris.update` event fires whenever the phone sends a now-playing state change (track change, play/pause toggle). Subscribe with `kcd watch --events=mpris.update`. The daemon also re-requests now-playing every 5 seconds from devices with an **actively-playing** player, so state stays fresh for pure-push clients without polling — but events are deduplicated, so `mpris.update` only fires on real changes. Stopped/paused players are not polled (a stopped-but-alive track stays listed so it can be resumed), and when the phone removes a player from its `playerList` (session destroyed) the cached state is dropped and an empty `mpris.update` is emitted so the widget falls back to "no media playing". +> +> **Note:** Phone album art URIs (`kdeconnect:/artUri?...`) are resolved by the +> daemon: it fetches the art bytes from the phone, caches them to +> `$XDG_CACHE_HOME/kcd/art/.`, and emits a loadable +> `file://` URL in `albumArtUrl` (both in `kcd mpris status --json` and in +> `mpris.update` events). A second `mpris.update` is published when the art +> arrives. --- diff --git a/docs/CLIENT_GUIDE.md b/docs/CLIENT_GUIDE.md index 347fd18..2a52fca 100644 --- a/docs/CLIENT_GUIDE.md +++ b/docs/CLIENT_GUIDE.md @@ -245,7 +245,6 @@ def on_mpris(device_id, payload): print(f'Now playing: {payload["title"]} by {payload["artist"]}') else: print("Paused/stopped") - w.start() # Keep main thread alive @@ -267,11 +266,28 @@ except KeyboardInterrupt: | `notification` | Push notification from device | | `share.progress` | File transfer progress update | | `share.complete` | File transfer finished | -| `mpris.update` | Now-playing state changed | +| `mpris.update` | Now-playing state changed (deduplicated — only on real changes) | | `sms.incoming` | SMS/MMS received | | `pair.requested` | Remote device wants to pair | | `ping.received` | Ping from device | +> **Freshness:** the daemon re-requests now-playing from devices with an +> actively-playing player every 5 seconds, so a pure-push client (a widget watching the +> event stream, with no polling) receives the current track within one poll +> interval of subscribing — including mid-track mount, thanks to the initial +> event dump. Events are deduplicated: `mpris.update` only fires when the +> state actually changed, so the stream stays quiet between track changes. +> Stopped/paused players are not polled, and when the phone removes a player +> from its `playerList` (session destroyed) the cached state is dropped with +> an empty `mpris.update` — showing "no media playing". + +> **Album art:** `mpris.update` payloads (and `kcd mpris status --json`) +> expose `albumArtUrl` as a loadable `file://` path once the daemon has +> fetched the art from the phone into `$XDG_CACHE_HOME/kcd/art/`. If the +> art is still being fetched or fails, the raw `kdeconnect:/artUri?...` +> URI is emitted instead — treat anything that isn't `http(s)://` or +> `file://` as "no art available" and show a placeholder. + See [`IPC_PROTOCOL.md §5`](IPC_PROTOCOL.md#5-event-types) for the full list. --- diff --git a/docs/IPC_PROTOCOL.md b/docs/IPC_PROTOCOL.md index 50f8fe0..4250b19 100644 --- a/docs/IPC_PROTOCOL.md +++ b/docs/IPC_PROTOCOL.md @@ -666,6 +666,14 @@ are delivered. {"type":"mpris.update","deviceId":"...","timestamp":"...","payload":{...NowPlaying...}} ``` + The daemon keeps now-playing state fresh by re-requesting it every 5 + seconds from devices with an **actively-playing** player (see the + `mpris.update` section below), so this initial dump fires reliably for + mid-track state — a pure-push client can mount and see the current track + without polling. Stopped/paused players are deliberately not polled, so + they age past the 10-second gate and no ghost track is shown after a + reconnect. + 3. **Live stream:** All matching events are streamed as they occur, one per line, until the client disconnects or the daemon shuts down. @@ -822,6 +830,25 @@ A notification was received from a device. | `requestReplyId` | string | Present if the notification supports inline replies | | `id` | string | Notification identifier | +> **Desktop popups:** the daemon shows each phone notification via `notify-send`. +> Popups render **without an icon by default** (`show_icons = false`); set +> `show_icons = true` to display the phone's app icon (downloaded via +> `fetch_icons` and reused across re-posts). Because Android re-posts a +> notification on every update with a stable `id` (e.g. a scrobbler's +> now-playing notification), the daemon replaces the existing desktop popup +> in place (`--replace-id`) so repeated updates collapse to one popup instead +> of flooding the screen — mirroring the reference desktop's +> `Notification::update()`. Disable with `replace_notifications = false`. +> +> When the phone **cancels** a notification (e.g. a scrobbler's now-playing +> popup torn down on pause), the daemon defers closing the desktop popup by +> `cancel_grace_ms` (default `1500`). If the same notification id is re-posted +> within that window (rapid play/pause toggling), the popup is updated in +> place instead of flickering closed and open. A real dismissal — no re-post — +> closes the popup after the grace window. Set `cancel_grace_ms = 0` to close +> immediately on cancel. The `notification.canceled` event is always emitted +> immediately on the cancel packet, regardless of the grace window. + #### `notification.canceled` A notification was dismissed by the device. @@ -1057,6 +1084,32 @@ this daemon to ring). Now-playing state from a device's media player. +> **Album art:** when the phone advertises a `kdeconnect:/artUri?...` URI, +> the daemon requests the art bytes over a side channel and caches them to +> `$XDG_CACHE_HOME/kcd/art/.`. Once fetched, `albumArtUrl` +> is emitted as a loadable `file://` path. A second `mpris.update` event is +> published when the art arrives. If the fetch is still in flight or fails, +> the original URI is left untouched — clients should fall back to a +> placeholder. + +> **Freshness:** the daemon re-requests now-playing from every connected +> device with an **actively-playing** player every 5 seconds +> (`kdeconnect.mpris.request` with `requestNowPlaying: true`). Responses are +> deduplicated — an event is only emitted when the state actually changes. +> This keeps `pos`/state current for pure-push clients (widgets, Waybar) +> that never poll the CLI. Devices that haven't reported a player yet, or +> whose player is stopped/paused, are not polled — a stopped-but-alive +> player keeps its last pushed track (so it can be resumed), but stops being +> refreshed and ages out of the 10-second initial-dump gate. + +> **Session teardown:** when the phone reports a `playerList` that no longer +> contains the device's currently-tracked player (the media session was +> destroyed, e.g. the app was swiped away), the daemon drops the cached +> state and publishes an empty `mpris.update` (`{...NowPlaying...}` with no +> title) so watchers fall back to "no media playing". An empty `playerList` +> (all sessions destroyed) has the same effect. A stopped-but-alive session +> is **not** cleared — only sessions removed from `playerList` are. + **Payload:** ```json @@ -1065,7 +1118,7 @@ Now-playing state from a device's media player. "title": "Song Title", "artist": "Artist Name", "album": "Album Name", - "albumArtUrl": "https://i.scdn.co/image/...", + "albumArtUrl": "file:///home/user/.cache/kcd/art/1141556203.jpg", "url": "spotify:track:...", "length": 240000, "pos": 45000, @@ -1083,6 +1136,10 @@ Now-playing state from a device's media player. } ``` +The extension is sniffed from the received bytes (JPEG/PNG/GIF/WebP), +defaulting to `.jpg`. The cache directory holds at most 500 files and is +cleared when it overflows. + --- ## 6. Outbound Packet Reference @@ -1102,7 +1159,7 @@ who may want to implement a full network-level implementation. | `kdeconnect.clipboard.connect` | Clipboard | Push clipboard with timestamp on connect | | `kdeconnect.mousepad.keyboardstate` | Mousepad | Advertise keyboard capability on connect | | `kdeconnect.mpris` | MPRIS | Player list, NowPlaying state, seek positions, album art (broadcast + request-reply) | -| `kdeconnect.mpris.request` | MPRIS | Request player list, now-playing, volume; send control actions | +| `kdeconnect.mpris.request` | MPRIS | Request player list, now-playing, volume, album art; send control actions | | `kdeconnect.notification.reply` | Notification | Reply to a notification with inline reply support | | `kdeconnect.notification` | RunCommand | Command output notification pushed to phone | | `kdeconnect.runcommand` | RunCommand | Send command list to phone | @@ -1148,8 +1205,8 @@ plugin processes it and a link to the body struct definition. | `kdeconnect.clipboard.file` | Clipboard | `ClipboardFileBody{Filename string}` | | `kdeconnect.lock` | LockDevice | `LockBody{RequestLocked, SetLocked, IsLocked}` | | `kdeconnect.lock.request` | LockDevice | `LockBody{}` (triggers lock/unlock) | -| `kdeconnect.mpris` | MPRIS | `MPRISRequest{RequestPlayerList, RequestNowPlaying, RequestVolume, Player, Action, ...}` | -| `kdeconnect.mpris.request` | MPRIS | `MPRISRequest{}` (same struct, different semantics) | +| `kdeconnect.mpris` | MPRIS | `MPRISRequest{RequestPlayerList, RequestNowPlaying, RequestVolume, Player, Action, AlbumArtUrl, TransferringAlbumArt, ...}` — inbound packets with `transferringAlbumArt: true` + `payloadTransferInfo` carry album art bytes (side channel) that the daemon caches to `$XDG_CACHE_HOME/kcd/art/` | +| `kdeconnect.mpris.request` | MPRIS | `MPRISRequest{}` (same struct, different semantics) — an outbound `kdeconnect.mpris.request` with `player` + `albumArtUrl` asks the phone to stream art back | | `kdeconnect.runcommand.request` | RunCommand | `RequestBody{RequestCommandList bool, Key string}` | | `kdeconnect.presenter` | Presenter | `PresenterBody{Dx, Dy *float64, Stop *bool}` | | `kdeconnect.systemvolume` | RemoteSystemVolume | `VolumeBody{SinkList, Name, Volume, Muted}` | diff --git a/internal/config/plugins.go b/internal/config/plugins.go index 2ec05a5..2323e8d 100644 --- a/internal/config/plugins.go +++ b/internal/config/plugins.go @@ -43,6 +43,19 @@ type NotificationPluginConfig struct { MaxBodyLength int `toml:"max_body_length"` ExpireMS int `toml:"expire_ms"` SkipNonClearable bool `toml:"skip_non_clearable"` + // ReplaceNotifications replaces the previous desktop popup when the phone + // re-posts a notification with the same id (e.g. media/scrobble updates), + // instead of creating a new popup each time. On by default. + ReplaceNotifications bool `toml:"replace_notifications"` + // ShowIcons shows the app icon on desktop popups. Off by default — + // kcd notifications render without an icon. When enabled, the phone's + // icon is downloaded (subject to FetchIcons) and reused across re-posts. + ShowIcons bool `toml:"show_icons"` + // CancelGraceMS defers closing the desktop popup after the phone cancels + // a notification. If the same notification is re-posted within the window + // (e.g. media now-playing toggling play/pause), the popup is updated in + // place instead of closing and re-opening. 0 closes immediately. + CancelGraceMS int `toml:"cancel_grace_ms"` } type ShareConfig struct { @@ -118,6 +131,8 @@ func (c *NotificationPluginConfig) Defaults() { c.FetchIcons = true c.MaxBodyLength = 0 c.ExpireMS = -1 + c.ReplaceNotifications = true + c.CancelGraceMS = 1500 } type ClipboardConfig struct { diff --git a/internal/daemon/transport.go b/internal/daemon/transport.go index 0ce77c5..f36c80b 100644 --- a/internal/daemon/transport.go +++ b/internal/daemon/transport.go @@ -17,6 +17,12 @@ import ( "go.uber.org/zap" ) +// reconnectFlapThreshold is the minimum connection lifetime for it to count +// as genuinely stable. A connection that drops sooner is treated as a flap +// (e.g. a peer that keeps dying), so the auto-reconnect backoff continues +// escalating instead of resetting to the 2s floor on every drop. +const reconnectFlapThreshold = 15 * time.Second + // DialDevice manually connects to a device at the given IP and port. func DialDevice(ctx context.Context, targetIP net.IP, targetPort int, targetID string, targetProto int, identity *protocol.Packet, cfg *tls.Config, devices *device.Registry, plugins *plugin.Registry, localDeviceID string, logger *zap.Logger) { addr := fmt.Sprintf("%s:%d", targetIP, targetPort) @@ -227,6 +233,14 @@ func handleNewConnection(ctx context.Context, conn *transport.Conn, identity *pr if lastIP == nil { return } + // A connection that lasted long enough was genuinely stable, so the + // next drop should start the backoff over. A flap (connection dies + // shortly after a successful dial, e.g. a dying peer) keeps the + // counter so the backoff keeps escalating instead of hammering at the + // 2s floor forever. + if sender.ConnectionAge() >= reconnectFlapThreshold { + sender.ResetReconnectAttempt() + } // Prevent multiple concurrent reconnect goroutines for the same device. if !sender.TryReconnect() { logger.Debug("auto-reconnect: already reconnecting, skipping", @@ -260,7 +274,7 @@ func reconnectWithBackoff( logger *zap.Logger, ) { const maxBackoff = 5 * time.Minute - attempt := 0 + attempt := dev.ReconnectAttempt() defer dev.ReconnectDone() @@ -321,6 +335,11 @@ func reconnectWithBackoff( zap.String("device_id", dev.ID()), zap.Int("attempts", attempt+1), ) + // Persist the counter before returning: if this connection flaps, + // the next reconnect cycle continues backing off rather than + // resetting to the 2s floor. onDisconnect resets it if the + // connection proved stable. + dev.SetReconnectAttempt(attempt + 1) return } diff --git a/internal/device/device.go b/internal/device/device.go index a027eac..9afdac7 100644 --- a/internal/device/device.go +++ b/internal/device/device.go @@ -43,6 +43,17 @@ type Device struct { // auto-reconnect goroutines for this device. reconnecting atomic.Bool + // reconnectAttempt persists the auto-reconnect backoff counter across + // disconnect cycles. A connection that flaps (drops shortly after a + // successful dial) keeps the counter so the backoff escalates instead of + // resetting to the 2s floor; a stable connection resets it on drop. + reconnectAttempt int + + // connectStarted is when the most recent connection was established, + // used to detect flaps (connections that die too quickly to count as + // genuinely stable). + connectStarted time.Time + // pluginDispatch routes incoming packets to registered plugins pluginDispatch func(ctx context.Context, dev *Device, pkt *protocol.Packet) bool onConnect func(dev *Device) @@ -109,6 +120,12 @@ func (d *Device) Connect(ctx context.Context, conn *transport.Conn, dispatch fun d.mu.Unlock() } + // Record when the connection was established so a quick drop can be + // distinguished from a genuinely stable connection. + d.mu.Lock() + d.connectStarted = time.Now() + d.mu.Unlock() + go d.readLoop(ctx, conn) go d.writerLoop(ctx, conn) } @@ -314,3 +331,38 @@ func (d *Device) TryReconnect() bool { func (d *Device) ReconnectDone() { d.reconnecting.Store(false) } + +// ReconnectAttempt returns the persisted auto-reconnect backoff counter. +func (d *Device) ReconnectAttempt() int { + d.mu.RLock() + defer d.mu.RUnlock() + return d.reconnectAttempt +} + +// SetReconnectAttempt stores the auto-reconnect backoff counter so the next +// reconnect cycle (spawned when this connection drops) continues backing off +// instead of resetting to the initial floor. +func (d *Device) SetReconnectAttempt(n int) { + d.mu.Lock() + d.reconnectAttempt = n + d.mu.Unlock() +} + +// ResetReconnectAttempt clears the backoff counter after a stable connection +// (one that stayed up long enough to count as genuinely healthy) drops. +func (d *Device) ResetReconnectAttempt() { + d.mu.Lock() + d.reconnectAttempt = 0 + d.mu.Unlock() +} + +// ConnectionAge returns how long the current connection has been established, +// or 0 if the device has never connected in this process. +func (d *Device) ConnectionAge() time.Duration { + d.mu.RLock() + defer d.mu.RUnlock() + if d.connectStarted.IsZero() { + return 0 + } + return time.Since(d.connectStarted) +} diff --git a/internal/device/device_test.go b/internal/device/device_test.go index d61ab91..5c3b2df 100644 --- a/internal/device/device_test.go +++ b/internal/device/device_test.go @@ -1,9 +1,13 @@ package device import ( + "context" + "crypto/tls" + "net" "testing" "time" + "github.com/bethropolis/kcd/internal/transport" "go.uber.org/zap/zaptest" ) @@ -48,3 +52,57 @@ func TestReconnectBackoff(t *testing.T) { } } } + +func TestDevice_ReconnectAttempt(t *testing.T) { + logger := zaptest.NewLogger(t) + d := NewDevice("123", "Phone", "phone", logger) + + if got := d.ReconnectAttempt(); got != 0 { + t.Fatalf("expected initial attempt 0, got %d", got) + } + + d.SetReconnectAttempt(3) + if got := d.ReconnectAttempt(); got != 3 { + t.Fatalf("expected attempt 3, got %d", got) + } + + d.ResetReconnectAttempt() + if got := d.ReconnectAttempt(); got != 0 { + t.Fatalf("expected attempt 0 after reset, got %d", got) + } +} + +func TestDevice_ConnectionAge(t *testing.T) { + logger := zaptest.NewLogger(t) + d := NewDevice("123", "Phone", "phone", logger) + + if got := d.ConnectionAge(); got != 0 { + t.Fatalf("expected 0 for never-connected device, got %v", got) + } + + // Connect with a pipe-based TLS connection (no handshake required). + left, right := net.Pipe() + conn := transport.NewConn(tls.Client(left, &tls.Config{InsecureSkipVerify: true})) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + d.Connect(ctx, conn, nil, nil, nil) + + if got := d.ConnectionAge(); got <= 0 { + t.Fatalf("expected positive age after connect, got %v", got) + } + + time.Sleep(20 * time.Millisecond) + if got := d.ConnectionAge(); got < 20*time.Millisecond { + t.Errorf("expected age to grow past 20ms, got %v", got) + } + + // Tear down: closing the pipe unblocks readLoop, which clears the + // connection. Wait for it so no goroutine logs after the test returns. + _ = right.Close() + deadline := time.Now().Add(2 * time.Second) + for d.IsConnected() && time.Now().Before(deadline) { + time.Sleep(time.Millisecond) + } + d.Disconnect() +} diff --git a/internal/plugins/clipboard/clipboard.go b/internal/plugins/clipboard/clipboard.go index eed77b0..4ab36eb 100644 --- a/internal/plugins/clipboard/clipboard.go +++ b/internal/plugins/clipboard/clipboard.go @@ -1,6 +1,7 @@ package clipboard import ( + "bytes" "context" "crypto/tls" "encoding/json" @@ -35,7 +36,8 @@ type ClipboardPlugin struct { tlsConfig *tls.Config logger *zap.Logger backend clipboardBackend - backendOnce sync.Once + wlDisplay string // WAYLAND_DISPLAY value for spawned subprocesses + probe func() (clipboardBackend, string) mu sync.Mutex lastContent string // last content received from phone (inbound) lastPushedContent string // last content sent to phone (outbound) @@ -43,71 +45,171 @@ type ClipboardPlugin struct { // NewClipboardPlugin creates a clipboard plugin. func NewClipboardPlugin(tlsConfig *tls.Config, logger *zap.Logger, pushOnConnect bool) *ClipboardPlugin { + if logger == nil { + logger = zap.NewNop() + } return &ClipboardPlugin{ tlsConfig: tlsConfig, pushOnConnect: pushOnConnect, logger: logger.With(zap.String("plugin", "clipboard")), + probe: probeBackend, } } -// detectBackend probes for a working clipboard tool (wl-paste → xclip) -// and caches the result so the probe runs at most once. +// probeBackend determines the usable clipboard backend (wl-paste/xclip) by +// inspecting the environment and $XDG_RUNTIME_DIR. It is side-effect free and +// unit-testable. The returned string is the WAYLAND_DISPLAY value to inject +// into spawned subprocesses (empty for X11/unknown). // -// Under systemd user services neither WAYLAND_DISPLAY nor DISPLAY -// is typically set. We probe by checking the display variable first, -// then falling back to scanning $XDG_RUNTIME_DIR for a Wayland socket. -func (p *ClipboardPlugin) detectBackend() clipboardBackend { - p.backendOnce.Do(func() { - wlDisplay := os.Getenv("WAYLAND_DISPLAY") - xDisplay := os.Getenv("DISPLAY") - - switch { - case wlDisplay != "": +// A Wayland socket is preferred over DISPLAY: under a systemd user service +// WAYLAND_DISPLAY is often unset at startup, and treating DISPLAY as the +// backend on a Wayland session silently copies to the X clipboard where +// Wayland-native apps never see it. +func probeBackend() (clipboardBackend, string) { + rtDir := os.Getenv("XDG_RUNTIME_DIR") + + // Wayland: trust WAYLAND_DISPLAY only if its socket actually exists, + // otherwise scan the runtime dir for any live wayland-* socket. + if disp := os.Getenv("WAYLAND_DISPLAY"); disp != "" && rtDir != "" { + if _, err := os.Stat(filepath.Join(rtDir, disp)); err == nil { if _, err := exec.LookPath("wl-paste"); err == nil { - p.backend = backendWayland - p.logger.Debug("clipboard: backend=wayland (WAYLAND_DISPLAY set)") - } - case xDisplay != "": - if _, err := exec.LookPath("xclip"); err == nil { - p.backend = backendX11 - p.logger.Debug("clipboard: backend=x11 (DISPLAY set)") + return backendWayland, disp } - default: - // Systemd user service — neither variable is set. - // Probe $XDG_RUNTIME_DIR for any Wayland socket. - rtDir := os.Getenv("XDG_RUNTIME_DIR") - hasWaylandSock := false - if rtDir != "" { - if entries, err := os.ReadDir(rtDir); err == nil { - for _, e := range entries { - if strings.HasPrefix(e.Name(), "wayland-") && !e.IsDir() { - hasWaylandSock = true - break - } + } + } + if rtDir != "" { + if entries, err := os.ReadDir(rtDir); err == nil { + for _, e := range entries { + name := e.Name() + if !e.IsDir() && strings.HasPrefix(name, "wayland-") { + if _, err := exec.LookPath("wl-paste"); err == nil { + return backendWayland, name } } } - if hasWaylandSock { - if _, err := exec.LookPath("wl-paste"); err == nil { - p.backend = backendWayland - p.logger.Debug("clipboard: backend=wayland (socket probe)") - } - } - if p.backend == backendUnknown { - if _, err := exec.LookPath("xclip"); err == nil { - p.backend = backendX11 - p.logger.Debug("clipboard: backend=x11 (fallback)") - } - } } + } - if p.backend == backendUnknown { - p.logger.Warn("clipboard: no clipboard tool found (install wl-clipboard or xclip)") + // X11 fallback. + if os.Getenv("DISPLAY") != "" { + if _, err := exec.LookPath("xclip"); err == nil { + return backendX11, "" } - }) - return p.backend + } + return backendUnknown, "" +} + +// getBackend returns the cached backend, re-probing while it is unknown so an +// early failed probe (compositor not up yet, env not imported) does not stick +// for the lifetime of the process. Only non-unknown results are cached. +func (p *ClipboardPlugin) getBackend() (clipboardBackend, string) { + p.mu.Lock() + defer p.mu.Unlock() + if p.backend != backendUnknown { + return p.backend, p.wlDisplay + } + backend, disp := p.probe() + if backend != backendUnknown { + p.backend = backend + p.wlDisplay = disp + p.logger.Debug("clipboard: backend detected", + zap.Int("backend", int(backend)), zap.String("wl_display", disp)) + } + return backend, disp +} + +// clipboardCmd builds an exec.Cmd for a clipboard tool with WAYLAND_DISPLAY +// injected into the subprocess environment from the probed socket, so the +// tool works even when the daemon's own environment lacks the variable. +// The command itself carries no deadline; runClipboard applies the timeout +// around execution so a hung wl-paste can never stall the daemon. +func (p *ClipboardPlugin) clipboardCmd(name string, args ...string) *exec.Cmd { + // A background context: the deadline lives in runClipboard, which binds + // a bounded context around execution so a hung tool is killed there. + cmd := exec.CommandContext(context.Background(), name, args...) + cmd.WaitDelay = time.Second + if _, disp := p.getBackend(); disp != "" { + cmd.Env = append(os.Environ(), "WAYLAND_DISPLAY="+disp) + } + return cmd +} + +// runClipboard runs a clipboard subprocess bounded by the clipboard timeout +// and captures any stderr the tool prints. On failure the stderr is wrapped +// into the returned error so the real reason (e.g. "No selection", a compositor +// error) is visible instead of a bare "exit status N". +func (p *ClipboardPlugin) runClipboard(ctx context.Context, cmd *exec.Cmd) ([]byte, error) { + tctx, cancel := context.WithTimeout(ctx, clipboardTimeout) + defer cancel() + + timed := exec.CommandContext(tctx, cmd.Path, cmd.Args[1:]...) + timed.Env = cmd.Env + timed.WaitDelay = capKillDelay(cmd.WaitDelay) + timed.Stdin = nil + + var stderr bytes.Buffer + timed.Stderr = &stderr + + out, err := timed.Output() + if err != nil { + if msg := strings.TrimSpace(stderr.String()); msg != "" { + err = fmt.Errorf("%w: %s", err, msg) + } + } + return out, err } +// runCopy writes clipboard data via wl-copy/xclip -i. Unlike runClipboard it +// must NOT capture stdout/stderr through os.Pipe: wl-copy forks a persistent +// background manager that inherits the pipe fds, so the pipe never EOFs and +// cmd.Output()/cmd.Wait() would block until the manager dies. Pointing the +// child's fds at the null device instead makes Run() return as soon as the +// forking wl-copy process exits. wl-paste never forks, which is why the read +// path above can still use pipes. +func (p *ClipboardPlugin) runCopy(ctx context.Context, cmd *exec.Cmd, stdin io.Reader) error { + tctx, cancel := context.WithTimeout(ctx, clipboardTimeout) + defer cancel() + + timed := exec.CommandContext(tctx, cmd.Path, cmd.Args[1:]...) + timed.Env = cmd.Env + timed.WaitDelay = capKillDelay(cmd.WaitDelay) + timed.Stdin = stdin + timed.Stdout = nil + timed.Stderr = nil + return timed.Run() +} + +// capKillDelay returns nonzero bound on how long Wait may block on the +// stdout/stderr pipes after the timeout kills the child. Callers built via +// clipboardCmd start with WaitDelay=1s, but a bare exec.CommandContext +// (tests, or any future caller) defaults to 0, which means Wait blocks until +// the pipes EOF — and a killed child that forked a grandchild holding those +// fds would pin runClipboard open until the grandchild exits (e.g. a whole +// 30s sleep). Ensure it always returns promptly after the deadline. +func capKillDelay(d time.Duration) time.Duration { + if d <= 0 { + return time.Second + } + return d +} + +// isNoSelection reports whether a clipboard tool failure actually means the +// clipboard is empty (nothing to push) rather than a real problem with the +// tool or compositor. Matches wl-paste's and xclip's "nothing here" messages. +func isNoSelection(err error) bool { + if err == nil { + return false + } + s := strings.ToLower(err.Error()) + return strings.Contains(s, "no selection") || + strings.Contains(s, "nothing is copied") || + strings.Contains(s, "no data") +} + +// clipboardTimeout bounds every wl-copy/wl-paste/xclip subprocess so a hung +// clipboard tool cannot block clipboard sync indefinitely. +const clipboardTimeout = 2 * time.Second + // ClipboardBody represents the content of a clipboard packet. type ClipboardBody struct { Content string `json:"content"` @@ -167,19 +269,21 @@ func (p *ClipboardPlugin) Handle(ctx context.Context, dev device.Sender, pkt *pr // Spawning goroutine as Handlers must not block. go func() { - var cmd *exec.Cmd - switch p.detectBackend() { + switch backend, _ := p.getBackend(); backend { case backendWayland: - cmd = exec.CommandContext(context.Background(), "wl-copy") + // -n: wl-copy appends a trailing newline by default. Without it the + // local selection becomes content+"\n", which differs from the + // inbound lastContent guard and makes --watch echo the phone's own + // clipboard straight back to it. + if err := p.runCopy(context.Background(), p.clipboardCmd("wl-copy", "-n"), strings.NewReader(body.Content)); err != nil { + p.logger.Warn("clipboard: failed to set clipboard", zap.Error(err)) + } case backendX11: - cmd = exec.CommandContext(context.Background(), "xclip", "-selection", "clipboard") + if err := p.runCopy(context.Background(), p.clipboardCmd("xclip", "-selection", "clipboard"), strings.NewReader(body.Content)); err != nil { + p.logger.Warn("clipboard: failed to set clipboard", zap.Error(err)) + } default: - return - } - - cmd.Stdin = strings.NewReader(body.Content) - if err := cmd.Run(); err != nil { - p.logger.Warn("clipboard: failed to set clipboard", zap.Error(err)) + p.logger.Debug("clipboard: no backend available, dropping inbound copy") } }() @@ -248,17 +352,16 @@ func (p *ClipboardPlugin) handleClipboardFile(ctx context.Context, dev device.Se defer t.Close() var cmd *exec.Cmd - switch p.detectBackend() { + switch backend, _ := p.getBackend(); backend { case backendWayland: - cmd = exec.CommandContext(context.Background(), "wl-copy", "--type", mimeType) + cmd = p.clipboardCmd("wl-copy", "-n", "--type", mimeType) case backendX11: - cmd = exec.CommandContext(context.Background(), "xclip", "-selection", "clipboard", "-t", mimeType, "-i") + cmd = p.clipboardCmd("xclip", "-selection", "clipboard", "-t", mimeType, "-i") default: return } - cmd.Stdin = t - if out, err := cmd.CombinedOutput(); err != nil { - p.logger.Warn("clipboard file: failed to set clipboard", zap.Error(err), zap.String("output", string(out))) + if err := p.runCopy(context.Background(), cmd, t); err != nil { + p.logger.Warn("clipboard file: failed to set clipboard", zap.Error(err)) } }() @@ -297,21 +400,31 @@ func downloadToFile(ctx context.Context, ip net.IP, port int, size int64, dest s // Push copies the local clipboard to the remote device using wl-paste or xclip -o. func Push(ctx context.Context, dev device.Sender, p *ClipboardPlugin) error { var cmd *exec.Cmd - switch p.detectBackend() { + switch backend, _ := p.getBackend(); backend { case backendWayland: - cmd = exec.CommandContext(ctx, "wl-paste", "-n") + cmd = p.clipboardCmd("wl-paste", "-n") case backendX11: - cmd = exec.CommandContext(ctx, "xclip", "-selection", "clipboard", "-o") + cmd = p.clipboardCmd("xclip", "-selection", "clipboard", "-o") default: return fmt.Errorf("clipboard: no clipboard tool available") } - out, err := cmd.Output() + out, err := p.runClipboard(ctx, cmd) if err != nil { + // An empty clipboard (fresh session, nothing copied yet) is not an + // error worth failing a push for — the tool exits non-zero with a + // "no selection"-style message. Treat it as nothing to push so the + // CLI and --watch don't spam errors until something is copied. + if isNoSelection(err) { + return nil + } return err } content := string(out) + if content == "" { + return nil + } p.mu.Lock() // Skip if content matches what we last received from the phone (lastContent) @@ -320,7 +433,11 @@ func Push(ctx context.Context, dev device.Sender, p *ClipboardPlugin) error { // lastContent guard: prevents sending the phone's own content back. // lastPushedContent guard: prevents duplicate pushes when the local // clipboard hasn't changed between two Push calls. - if content == p.lastContent || content == p.lastPushedContent { + // + // Comparisons are normalized of trailing newlines so a stray \n appended + // by some clipboard tooling cannot silently disable the guard and cause + // an echo of the phone's own content back to it. + if normClip(content) == normClip(p.lastContent) || normClip(content) == normClip(p.lastPushedContent) { p.mu.Unlock() return nil } @@ -338,17 +455,24 @@ func Push(ctx context.Context, dev device.Sender, p *ClipboardPlugin) error { return dev.Send(pkt) } +// normClip strips trailing newlines so content read back from wl-copy/xclip +// compares equal to the raw content we stored, regardless of which tool +// appended (or not) a trailing newline. +func normClip(s string) string { + return strings.TrimRight(s, "\n") +} + func (p *ClipboardPlugin) readClipboard() string { var cmd *exec.Cmd - switch p.detectBackend() { + switch backend, _ := p.getBackend(); backend { case backendWayland: - cmd = exec.CommandContext(context.Background(), "wl-paste", "-n") + cmd = p.clipboardCmd("wl-paste", "-n") case backendX11: - cmd = exec.CommandContext(context.Background(), "xclip", "-selection", "clipboard", "-o") + cmd = p.clipboardCmd("xclip", "-selection", "clipboard", "-o") default: return "" } - out, err := cmd.Output() + out, err := p.runClipboard(context.Background(), cmd) if err != nil { p.logger.Debug("clipboard: read failed", zap.Error(err)) return "" diff --git a/internal/plugins/clipboard/clipboard_test.go b/internal/plugins/clipboard/clipboard_test.go index 47db2ce..568071e 100644 --- a/internal/plugins/clipboard/clipboard_test.go +++ b/internal/plugins/clipboard/clipboard_test.go @@ -2,17 +2,373 @@ package clipboard import ( "context" + "crypto/x509" + "encoding/json" + "fmt" + "net" + "os" + "os/exec" + "path/filepath" + "strings" "testing" + "time" "github.com/bethropolis/kcd/internal/device" "github.com/bethropolis/kcd/internal/protocol" - "go.uber.org/zap/zaptest" + "go.uber.org/zap" ) +// setProbe points the plugin's probe at fn for the lifetime of the plugin. +// It is safe to call before any async goroutine spawns (tests set it, then +// never write it again, so the fire-and-forget Handle goroutine only reads). +func setProbe(t *testing.T, p *ClipboardPlugin, fn func() (clipboardBackend, string)) { + t.Helper() + p.probe = fn +} + +// shimBin puts executable stand-ins for the given tools at the front of PATH, +// so probeBackend does not depend on what's installed on the host. +func shimBin(t *testing.T, names ...string) { + t.Helper() + dir := t.TempDir() + for _, n := range names { + p := filepath.Join(dir, n) + if err := os.WriteFile(p, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Fatal(err) + } + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) +} + +func TestProbeBackend_WaylandFromSocket(t *testing.T) { + // No WAYLAND_DISPLAY in the env, but a live socket in the runtime dir — + // the systemd-user-service boot case. Must resolve to Wayland. + t.Setenv("WAYLAND_DISPLAY", "") + t.Setenv("DISPLAY", ":0") + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + shimBin(t, "wl-paste") + sock := filepath.Join(os.Getenv("XDG_RUNTIME_DIR"), "wayland-1") + if err := os.WriteFile(sock, nil, 0o600); err != nil { + t.Fatal(err) + } + + backend, disp := probeBackend() + if backend != backendWayland { + t.Fatalf("expected wayland via socket probe, got %v", backend) + } + if disp != "wayland-1" { + t.Fatalf("expected wayland-1, got %q", disp) + } +} + +func TestProbeBackend_WaylandFromEnv(t *testing.T) { + t.Setenv("WAYLAND_DISPLAY", "wayland-0") + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + shimBin(t, "wl-paste") + sock := filepath.Join(os.Getenv("XDG_RUNTIME_DIR"), "wayland-0") + if err := os.WriteFile(sock, nil, 0o600); err != nil { + t.Fatal(err) + } + + backend, disp := probeBackend() + if backend != backendWayland || disp != "wayland-0" { + t.Fatalf("expected wayland-0, got backend=%v disp=%q", backend, disp) + } +} + +func TestProbeBackend_StaleEnvFallsBackToSocket(t *testing.T) { + // WAYLAND_DISPLAY points at a socket that no longer exists, but another + // live socket is present — the probe must not trust the stale var. + t.Setenv("WAYLAND_DISPLAY", "wayland-99") + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + shimBin(t, "wl-paste") + sock := filepath.Join(os.Getenv("XDG_RUNTIME_DIR"), "wayland-3") + if err := os.WriteFile(sock, nil, 0o600); err != nil { + t.Fatal(err) + } + + backend, disp := probeBackend() + if backend != backendWayland || disp != "wayland-3" { + t.Fatalf("expected fallback to wayland-3, got backend=%v disp=%q", backend, disp) + } +} + +func TestProbeBackend_X11Fallback(t *testing.T) { + t.Setenv("WAYLAND_DISPLAY", "") + t.Setenv("DISPLAY", ":0") + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + shimBin(t, "xclip") + + backend, disp := probeBackend() + if backend != backendX11 { + t.Fatalf("expected x11 fallback, got %v", backend) + } + if disp != "" { + t.Fatalf("expected empty display for x11, got %q", disp) + } +} + +func TestProbeBackend_None(t *testing.T) { + t.Setenv("WAYLAND_DISPLAY", "") + t.Setenv("DISPLAY", "") + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + shimBin(t, "wl-paste", "xclip") + + backend, _ := probeBackend() + if backend != backendUnknown { + t.Fatalf("expected unknown backend, got %v", backend) + } +} + +func TestClipboardPlugin_ReProbesAfterFailure(t *testing.T) { + p := NewClipboardPlugin(nil, zap.NewNop(), false) + + // First probe fails (compositor not up at boot) — nothing cached. + setProbe(t, p, func() (clipboardBackend, string) { return backendUnknown, "" }) + if b, _ := p.getBackend(); b != backendUnknown { + t.Fatalf("expected unknown on first probe, got %v", b) + } + + // Wayland becomes available later — the next call must re-probe and cache it. + setProbe(t, p, func() (clipboardBackend, string) { return backendWayland, "wayland-1" }) + if b, d := p.getBackend(); b != backendWayland || d != "wayland-1" { + t.Fatalf("expected wayland-1 after re-probe, got backend=%v disp=%q", b, d) + } + + // Now cached — a broken probe afterwards must not clobber it. + setProbe(t, p, func() (clipboardBackend, string) { return backendUnknown, "" }) + if b, d := p.getBackend(); b != backendWayland || d != "wayland-1" { + t.Fatalf("expected cached wayland-1, got backend=%v disp=%q", b, d) + } +} + +func TestClipboardPlugin_CmdInjectsWaylandEnv(t *testing.T) { + p := NewClipboardPlugin(nil, zap.NewNop(), false) + setProbe(t, p, func() (clipboardBackend, string) { return backendWayland, "wayland-9" }) + + cmd := p.clipboardCmd("wl-copy") + found := false + for _, kv := range cmd.Env { + if kv == "WAYLAND_DISPLAY=wayland-9" { + found = true + break + } + } + if !found { + t.Fatalf("expected WAYLAND_DISPLAY=wayland-9 in subprocess env, got %v", cmd.Env) + } +} + +func TestRunClipboard_Success(t *testing.T) { + p := NewClipboardPlugin(nil, zap.NewNop(), false) + + out, err := p.runClipboard(context.Background(), exec.CommandContext(context.Background(), "/bin/sh", "-c", "printf hello")) + if err != nil { + t.Fatalf("expected success, got %v", err) + } + if string(out) != "hello" { + t.Fatalf("expected 'hello', got %q", out) + } +} + +func TestRunClipboard_WrapsStderr(t *testing.T) { + p := NewClipboardPlugin(nil, zap.NewNop(), false) + + _, err := p.runClipboard(context.Background(), exec.CommandContext(context.Background(), "/bin/sh", "-c", "echo boom >&2; exit 2")) + if err == nil { + t.Fatal("expected an error") + } + // The real stderr reason must surface instead of a bare "exit status 2". + if !strings.Contains(err.Error(), "boom") { + t.Fatalf("expected stderr in error, got %v", err) + } +} + +func TestRunClipboard_TimesOutHungSubprocess(t *testing.T) { + p := NewClipboardPlugin(nil, zap.NewNop(), false) + + // A hung subprocess must be killed by the 2s timeout — and not instantly + // (that would regress to the old premature-cancel bug). + // + // Two shapes of "hung" are covered: a plain child (sh -c 'sleep 30') and + // a child that forks a grandchild still holding the stdout pipe + // (sh -c 'sleep 30 & wait'). The grandchild survives the parent's SIGKILL, + // so without a WaitDelay bound Output() would block on the pipe until the + // grandchild exits — pinning runClipboard open for the whole 30s. + for _, cmd := range []*exec.Cmd{ + exec.CommandContext(context.Background(), "/bin/sh", "-c", "sleep 30"), + exec.CommandContext(context.Background(), "/bin/sh", "-c", "sleep 30 & wait"), + } { + start := time.Now() + _, err := p.runClipboard(context.Background(), cmd) + if err == nil { + t.Fatal("expected a timeout error from a hung clipboard subprocess") + } + if elapsed := time.Since(start); elapsed < time.Second { + t.Fatalf("subprocess killed too early (instant cancel regression?), elapsed=%v", elapsed) + } else if elapsed > 3*clipboardTimeout { + t.Fatalf("expected subprocess killed within ~2s, took %v", elapsed) + } + } +} + +func TestIsNoSelection(t *testing.T) { + for _, msg := range []string{ + "exit status 1: wl-paste: No selection", + "exit status 1: Nothing is copied", + "exit status 1: Error: There is no data to be read", + } { + if !isNoSelection(fmt.Errorf("%s", msg)) { + t.Errorf("expected %q to be treated as no-selection", msg) + } + } + if isNoSelection(fmt.Errorf("exit status 1: failed to connect to compositor")) { + t.Error("compositor errors must remain hard failures") + } +} + +// fakeSender records packets like a device without touching the network. +type fakeSender struct { + sent []*protocol.Packet +} + +func (f *fakeSender) ID() string { return "fake" } +func (f *fakeSender) Name() string { return "fake" } +func (f *fakeSender) SetName(string) {} +func (f *fakeSender) State() device.PairingState { return device.StatePaired } +func (f *fakeSender) SetState(device.PairingState) { +} + +func (f *fakeSender) Send(p *protocol.Packet) error { + f.sent = append(f.sent, p) + return nil +} +func (f *fakeSender) IsConnected() bool { return true } +func (f *fakeSender) RemoteIP() net.IP { return net.ParseIP("127.0.0.1") } +func (f *fakeSender) PeerCert() *x509.Certificate { return nil } +func (f *fakeSender) HasCapability(string) bool { return false } +func (f *fakeSender) UpdateBattery(int, bool) {} +func (f *fakeSender) GetBattery() (int, bool) { return 0, false } + +// shimWith replaces PATH with a single scripted binary so Push exercises the +// real command execution without depending on the host's clipboard tools. +func shimWith(t *testing.T, name, script string) { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, name), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir) +} + +func TestPush_EmptyClipboardIsSoftFail(t *testing.T) { + p := NewClipboardPlugin(nil, zap.NewNop(), false) + setProbe(t, p, func() (clipboardBackend, string) { return backendWayland, "wayland-1" }) + + // Empty clipboard with nothing copied: wl-paste exits non-zero with a + // "no selection" diagnostic — push must not fail the CLI for this. + shimWith(t, "wl-paste", "#!/bin/sh\necho 'No selection' >&2\nexit 1\n") + dev := &fakeSender{} + + if err := Push(context.Background(), dev, p); err != nil { + t.Fatalf("expected soft pass on empty clipboard, got %v", err) + } + if len(dev.sent) != 0 { + t.Fatalf("expected no packet for empty clipboard, got %d", len(dev.sent)) + } +} + +func TestPush_EmptyOutputSendsNothing(t *testing.T) { + p := NewClipboardPlugin(nil, zap.NewNop(), false) + setProbe(t, p, func() (clipboardBackend, string) { return backendWayland, "wayland-1" }) + + // Tool reports success but no content — still nothing to push. + shimWith(t, "wl-paste", "#!/bin/sh\nexit 0\n") + dev := &fakeSender{} + + if err := Push(context.Background(), dev, p); err != nil { + t.Fatalf("expected success, got %v", err) + } + if len(dev.sent) != 0 { + t.Fatalf("expected no packet for empty output, got %d", len(dev.sent)) + } +} + +func TestPush_SendsContent(t *testing.T) { + p := NewClipboardPlugin(nil, zap.NewNop(), false) + setProbe(t, p, func() (clipboardBackend, string) { return backendWayland, "wayland-1" }) + + shimWith(t, "wl-paste", "#!/bin/sh\nprintf 'hello world'\n") + dev := &fakeSender{} + + if err := Push(context.Background(), dev, p); err != nil { + t.Fatalf("expected success, got %v", err) + } + if len(dev.sent) != 1 { + t.Fatalf("expected 1 packet, got %d", len(dev.sent)) + } + var body ClipboardBody + if err := json.Unmarshal(dev.sent[0].Body, &body); err != nil { + t.Fatalf("bad body: %v", err) + } + if body.Content != "hello world" { + t.Fatalf("expected 'hello world', got %q", body.Content) + } +} + +// TestPush_DoesNotEchoReceivedClipboard guards the echo bug: after the phone +// pushes content, the local clipboard holds it (possibly with a trailing +// newline appended by wl-copy). A --watch-triggered Push must NOT send that +// same content straight back to the phone. +func TestPush_DoesNotEchoReceivedClipboard(t *testing.T) { + p := NewClipboardPlugin(nil, zap.NewNop(), false) + setProbe(t, p, func() (clipboardBackend, string) { return backendWayland, "wayland-1" }) + + // Phone sent "hello"; wl-copy stored "hello\n" (no -n); wl-paste reads it + // back with the trailing newline still attached. + p.mu.Lock() + p.lastContent = "hello" + p.mu.Unlock() + shimWith(t, "wl-paste", "#!/bin/sh\nprintf 'hello\\n'\n") + dev := &fakeSender{} + + if err := Push(context.Background(), dev, p); err != nil { + t.Fatalf("expected success, got %v", err) + } + if len(dev.sent) != 0 { + t.Fatalf("expected no echo packet, got %d", len(dev.sent)) + } + + // Exact round-trip (what wl-copy -n produces) must also be suppressed. + p.mu.Lock() + p.lastContent = "new clip" + p.mu.Unlock() + shimWith(t, "wl-paste", "#!/bin/sh\nprintf 'new clip'\n") + dev = &fakeSender{} + + if err := Push(context.Background(), dev, p); err != nil { + t.Fatalf("expected success, got %v", err) + } + if len(dev.sent) != 0 { + t.Fatalf("expected no echo packet, got %d", len(dev.sent)) + } + + // Genuinely different content must still be pushed. + shimWith(t, "wl-paste", "#!/bin/sh\nprintf 'something else'\n") + if err := Push(context.Background(), dev, p); err != nil { + t.Fatalf("expected success, got %v", err) + } + if len(dev.sent) != 1 { + t.Fatalf("expected 1 packet, got %d", len(dev.sent)) + } +} + func TestClipboardPlugin_Handle(t *testing.T) { - logger := zaptest.NewLogger(t) + logger := zap.NewNop() dev := device.NewDevice("dev1", "Test", "phone", logger) - p := &ClipboardPlugin{} + p := NewClipboardPlugin(nil, logger, false) + // Force the no-backend path so no real clipboard tool is invoked. + setProbe(t, p, func() (clipboardBackend, string) { return backendUnknown, "" }) body := ClipboardBody{ Content: "Hello world!", diff --git a/internal/plugins/mpris/artcache.go b/internal/plugins/mpris/artcache.go new file mode 100644 index 0000000..e0d157d --- /dev/null +++ b/internal/plugins/mpris/artcache.go @@ -0,0 +1,155 @@ +package mpris + +import ( + "hash/fnv" + "io" + "net/url" + "os" + "path/filepath" + "strconv" + "strings" + "sync" + + "go.uber.org/zap" +) + +// maxAlbumArtBytes caps inbound album art payloads (matches the 5 MiB +// limit in the KDE Connect reference AlbumArtCache). +const maxAlbumArtBytes = 5 * 1024 * 1024 + +// maxArtCacheFiles bounds the on-disk cache. Exceeding it clears the +// directory, mirroring the reference implementation's startup purge. +const maxArtCacheFiles = 500 + +// ArtCache resolves kdeconnect:/artUri album art URIs to local file:// +// URLs. Bytes fetched from the phone are streamed to +// $XDG_CACHE_HOME/kcd/art/.. +type ArtCache struct { + dir string + mu sync.RWMutex + resolved map[string]string // raw albumArtUrl -> file:// path +} + +// NewArtCache creates the cache directory and returns an empty cache. +func NewArtCache(logger *zap.Logger) *ArtCache { + base, err := os.UserCacheDir() + if err != nil || base == "" { + base = filepath.Join(os.TempDir(), "kcd-cache") + } + dir := filepath.Join(base, "kcd", "art") + if err := os.MkdirAll(dir, 0700); err != nil { + logger.Warn("mpris: failed to create album art cache dir", + zap.String("path", dir), zap.Error(err)) + } + purgeArtCacheDir(dir, logger) + return &ArtCache{ + dir: dir, + resolved: make(map[string]string), + } +} + +// purgeArtCacheDir clears the cache when it grows past maxArtCacheFiles. +func purgeArtCacheDir(dir string, logger *zap.Logger) { + entries, err := os.ReadDir(dir) + if err != nil { + return + } + if len(entries) <= maxArtCacheFiles { + return + } + logger.Debug("mpris: clearing oversized album art cache", + zap.Int("files", len(entries))) + for _, e := range entries { + if !e.IsDir() { + _ = os.Remove(filepath.Join(dir, e.Name())) + } + } +} + +// Dir returns the on-disk cache directory. +func (c *ArtCache) Dir() string { + return c.dir +} + +// Key derives the cache filename stem for an album art URI: the +// kdeArtHash query parameter when present, otherwise an FNV-1a hash of +// the full URI (matching the reference qHash-based naming). +func (c *ArtCache) Key(rawURL string) string { + if u, err := url.Parse(rawURL); err == nil { + if h := u.Query().Get("kdeArtHash"); h != "" { + return h + } + } + h := fnv.New32a() + _, _ = h.Write([]byte(rawURL)) + return strconv.FormatUint(uint64(h.Sum32()), 10) +} + +// Resolve returns a file:// URL for a cached album art URI, or "" when +// the art has not been downloaded (or is still in flight). +func (c *ArtCache) Resolve(rawURL string) string { + if !strings.HasPrefix(rawURL, "kdeconnect:/") { + return "" + } + c.mu.RLock() + path, ok := c.resolved[rawURL] + c.mu.RUnlock() + if ok { + return path + } + + // Disk fallback: a previously cached file survives a daemon restart. + matches, err := filepath.Glob(filepath.Join(c.dir, c.Key(rawURL)+".*")) + if err != nil || len(matches) == 0 { + return "" + } + path = "file://" + matches[0] + c.mu.Lock() + c.resolved[rawURL] = path + c.mu.Unlock() + return path +} + +// Commit moves a streamed temp file into the cache, sniffing its type +// from the first bytes, and returns the file:// URL for rawURL. +func (c *ArtCache) Commit(rawURL, tmpPath string) (string, error) { + f, err := os.Open(tmpPath) + if err != nil { + return "", err + } + buf := make([]byte, 16) + n, _ := io.ReadFull(f, buf) + f.Close() + + ext := sniffImageExt(buf[:n]) + dst := filepath.Join(c.dir, c.Key(rawURL)+ext) + if err := os.Rename(tmpPath, dst); err != nil { + _ = os.Remove(dst) + if err := os.Rename(tmpPath, dst); err != nil { + return "", err + } + } + + fileURL := "file://" + dst + c.mu.Lock() + c.resolved[rawURL] = fileURL + c.mu.Unlock() + return fileURL, nil +} + +// sniffImageExt guesses the file extension from image magic bytes. +// Falls back to .jpg, matching the reference implementation. +func sniffImageExt(data []byte) string { + switch { + case len(data) >= 3 && data[0] == 0xFF && data[1] == 0xD8 && data[2] == 0xFF: + return ".jpg" + case len(data) >= 8 && data[0] == 0x89 && data[1] == 'P' && data[2] == 'N' && data[3] == 'G': + return ".png" + case len(data) >= 6 && string(data[:4]) == "GIF8": + return ".gif" + case len(data) >= 12 && string(data[8:12]) == "WEBP": + return ".webp" + default: + return ".jpg" + } +} diff --git a/internal/plugins/mpris/artcache_test.go b/internal/plugins/mpris/artcache_test.go new file mode 100644 index 0000000..2434b4d --- /dev/null +++ b/internal/plugins/mpris/artcache_test.go @@ -0,0 +1,113 @@ +package mpris + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestSniffImageExt(t *testing.T) { + cases := []struct { + name string + data []byte + want string + }{ + {"jpeg", []byte{0xFF, 0xD8, 0xFF, 0xE0, 0x00}, ".jpg"}, + {"png", []byte{0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A}, ".png"}, + {"gif", []byte("GIF89a..."), ".gif"}, + {"webp", []byte("RIFF\x00\x00\x00\x00WEBPVP8 "), ".webp"}, + {"unknown-falls-back-jpg", []byte("not an image"), ".jpg"}, + {"empty", nil, ".jpg"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := sniffImageExt(tc.data); got != tc.want { + t.Fatalf("sniffImageExt(%v) = %q, want %q", tc.data, got, tc.want) + } + }) + } +} + +func TestArtCacheKey(t *testing.T) { + c := &ArtCache{} + // kdeArtHash query param wins. + got := c.Key("kdeconnect:/artUri?title=Test&kdeArtHash=1141556203") + if got != "1141556203" { + t.Fatalf("Key with kdeArtHash = %q, want 1141556203", got) + } + // No kdeArtHash -> stable FNV fallback, same for identical URLs. + a := c.Key("kdeconnect:/artUri?title=OnlyTitle") + b := c.Key("kdeconnect:/artUri?title=OnlyTitle") + if a == "" || a != b { + t.Fatalf("expected stable non-empty key, got %q / %q", a, b) + } + // Different URLs -> different keys (extremely unlikely to collide). + c2 := c.Key("kdeconnect:/artUri?title=Other") + if a == c2 { + t.Fatalf("expected different keys, both %q", a) + } +} + +func TestArtCacheResolveAndCommit(t *testing.T) { + dir := t.TempDir() + c := &ArtCache{dir: dir, resolved: make(map[string]string)} + + rawURL := "kdeconnect:/artUri?title=Test&kdeArtHash=42" + if got := c.Resolve(rawURL); got != "" { + t.Fatalf("expected miss before commit, got %q", got) + } + + // Stream a tiny JPEG into a temp file, then commit. + tmp := filepath.Join(dir, "tmp-art") + png := []byte{0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A, 0x00} + if err := os.WriteFile(tmp, png, 0600); err != nil { + t.Fatal(err) + } + + fileURL, err := c.Commit(rawURL, tmp) + if err != nil { + t.Fatal(err) + } + if !strings.HasPrefix(fileURL, "file://") { + t.Fatalf("expected file:// prefix, got %q", fileURL) + } + path := strings.TrimPrefix(fileURL, "file://") + if !strings.HasSuffix(path, "42.png") { + t.Fatalf("expected extension sniffed to .png with key 42, got %q", path) + } + if _, err := os.Stat(path); err != nil { + t.Fatalf("cached file missing: %v", err) + } + + if got := c.Resolve(rawURL); got != fileURL { + t.Fatalf("expected resolve to return %q, got %q", fileURL, got) + } + + // Disk fallback: a fresh cache (restart) finds the file again. + c2 := &ArtCache{dir: dir, resolved: make(map[string]string)} + if got := c2.Resolve(rawURL); got != fileURL { + t.Fatalf("expected disk fallback resolve %q, got %q", fileURL, got) + } +} + +func TestArtCacheCommitRenamesTemp(t *testing.T) { + dir := t.TempDir() + c := &ArtCache{dir: dir, resolved: make(map[string]string)} + + tmp := filepath.Join(dir, "tmp-art") + jpeg := bytes.Repeat([]byte{0xFF, 0xD8, 0xFF}, 32) + if err := os.WriteFile(tmp, jpeg, 0600); err != nil { + t.Fatal(err) + } + if _, err := c.Commit("kdeconnect:/artUri?kdeArtHash=7", tmp); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(tmp); !os.IsNotExist(err) { + t.Fatalf("expected temp file removed after commit, err=%v", err) + } + if _, err := os.Stat(filepath.Join(dir, "7.jpg")); err != nil { + t.Fatalf("expected cached 7.jpg: %v", err) + } +} diff --git a/internal/plugins/mpris/mpris.go b/internal/plugins/mpris/mpris.go index 52d26f7..327d93a 100644 --- a/internal/plugins/mpris/mpris.go +++ b/internal/plugins/mpris/mpris.go @@ -48,6 +48,8 @@ type MPRISPlugin struct { pauseMusic bool callPausedPlayers []string // names of local players paused during a call + + artCache *ArtCache } type trackIdentity struct { @@ -83,6 +85,7 @@ func NewMPRISPlugin(tlsConfig *tls.Config, bus *events.Bus, pauseMusic bool, log remoteStateTimes: make(map[string]time.Time), positionTrackers: make(map[string]*remotePositionTracker), callPausedPlayers: make([]string, 0), + artCache: NewArtCache(logger), } // Start the watcher immediately (like C++ does in constructor). @@ -91,6 +94,7 @@ func NewMPRISPlugin(tlsConfig *tls.Config, bus *events.Bus, pauseMusic bool, log p.watchCancel = cancel p.watching = true p.startWatcher(watchCtx) + p.startRemoteStatePoller(watchCtx) // Subscribe to telephony events for pause-music-on-call. if p.pauseMusic && p.dbus != nil { @@ -129,6 +133,9 @@ type MPRISRequest struct { // Album art AlbumArtUrl string `json:"albumArtUrl,omitempty"` + // Set when this packet carries album art bytes over a side channel. + TransferringAlbumArt bool `json:"transferringAlbumArt,omitempty"` + // Player list from remote device PlayerList []string `json:"playerList,omitempty"` @@ -206,13 +213,48 @@ func (p *MPRISPlugin) Handle(ctx context.Context, dev device.Sender, pkt *protoc return nil } + // Inbound album art payload — the phone responds to requestAlbumArt + // with a side-channel transfer carrying the art bytes. + if body.TransferringAlbumArt && pkt.PayloadSize > 0 && pkt.PayloadTransferInfo != nil { + go p.receiveAlbumArt(ctx, dev, body.Player, body.AlbumArtUrl, + pkt.PayloadSize, pkt.PayloadTransferInfo.Port) + return nil + } + if body.RequestPlayerList { return p.sendPlayerList(dev) } - // Incoming playerList from remote device — request status for each player - if len(body.PlayerList) > 0 { + // Incoming playerList from remote device — prune players that no longer + // exist (their media session was destroyed) and request fresh status for + // the ones still around. + if body.PlayerList != nil { p.logger.Debug("mpris: received player list from remote", zap.Strings("players", body.PlayerList)) + pruned := false + p.mu.Lock() + if prev := p.remoteStates[dev.ID()]; prev != nil { + inList := false + for _, name := range body.PlayerList { + if name == prev.Player { + inList = true + break + } + } + if !inList { + delete(p.remoteStates, dev.ID()) + delete(p.remoteStateTimes, dev.ID()) + delete(p.positionTrackers, dev.ID()) + pruned = true + } + } + p.mu.Unlock() + + if pruned && p.bus != nil { + // The tracked player's session is gone — emit an empty update so + // watchers fall back to "no media playing" for this device. + p.bus.Publish(events.TypeMprisUpdate, dev.ID(), &NowPlaying{}) + } + for _, player := range body.PlayerList { player := player go p.requestPlayerStatus(dev, player) @@ -274,8 +316,21 @@ func (p *MPRISPlugin) Handle(ctx context.Context, dev device.Sender, pkt *protoc p.remoteStates[dev.ID()] = state p.remoteStateTimes[dev.ID()] = time.Now() p.mu.Unlock() + + // Request art bytes from the phone when the advertsed album art is + // a kdeconnect:// URI we have not cached yet. Resolve any already + // cached art before publishing so watch clients get a loadable URL. + if p.artCache != nil && p.artCache.Resolve(state.AlbumArtUrl) == "" { + go p.requestAlbumArt(dev, state.Player, state.AlbumArtUrl) + } if shouldPublish && p.bus != nil { - p.bus.Publish(events.TypeMprisUpdate, dev.ID(), state) + pub := state.DeepCopy() + if p.artCache != nil { + if resolved := p.artCache.Resolve(pub.AlbumArtUrl); resolved != "" { + pub.AlbumArtUrl = resolved + } + } + p.bus.Publish(events.TypeMprisUpdate, dev.ID(), pub) } return nil } @@ -474,6 +529,87 @@ func (p *MPRISPlugin) sendAlbumArt(ctx context.Context, dev device.Sender, playe } } +// requestAlbumArt asks the remote device to stream the album art bytes +// referenced by a kdeconnect:/artUri URI over a side channel. +func (p *MPRISPlugin) requestAlbumArt(dev device.Sender, player, artUrl string) { + if player == "" || artUrl == "" { + return + } + p.mu.Lock() + reqKey := "req|" + dev.ID() + "|" + artUrl + if lastReq, exists := p.artRequests[reqKey]; exists && time.Since(lastReq) < 10*time.Second { + p.mu.Unlock() + return + } + p.artRequests[reqKey] = time.Now() + p.mu.Unlock() + + body := MPRISRequest{ + Player: player, + AlbumArtUrl: artUrl, + } + pkt, err := protocol.NewPacket("kdeconnect.mpris.request", body) + if err != nil { + return + } + if err := dev.Send(pkt); err != nil { + p.logger.Debug("mpris: album art request failed", zap.Error(err)) + } +} + +// receiveAlbumArt streams an inbound album art payload into the cache +// and re-publishes the device state with a resolved file:// URL so watch +// clients and kcd mpris status surface the loadable location. +func (p *MPRISPlugin) receiveAlbumArt(_ context.Context, dev device.Sender, player, artUrl string, size int64, port int) { + remoteIP := dev.RemoteIP() + if remoteIP == nil { + return + } + if p.artCache == nil || size <= 0 || size > maxAlbumArtBytes { + return + } + + p.logger.Debug("mpris: receiving album art from remote", + zap.String("player", player), + zap.String("device_id", dev.ID()), + zap.Int64("size", size)) + + tmp, err := os.CreateTemp(p.artCache.Dir(), ".art-*") + if err != nil { + p.logger.Warn("mpris: failed to create temp file for album art", zap.Error(err)) + return + } + tmpPath := tmp.Name() + tmp.Close() + defer os.Remove(tmpPath) + + // The Handle ctx is canceled as soon as Handle returns; use an + // independent context so the side-channel dial isn't aborted. + dlCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + if err := share.ReceiveSideChannel(dlCtx, remoteIP, port, size, tmpPath, p.tlsConfig, nil, p.logger); err != nil { + p.logger.Warn("mpris: album art transfer failed", zap.Error(err)) + return + } + + fileURL, err := p.artCache.Commit(artUrl, tmpPath) + if err != nil { + p.logger.Warn("mpris: failed to cache album art", zap.Error(err)) + return + } + + p.mu.Lock() + state := p.remoteStates[dev.ID()] + if state != nil { + state = state.DeepCopy() + state.AlbumArtUrl = fileURL + } + p.mu.Unlock() + if state != nil && p.bus != nil { + p.bus.Publish(events.TypeMprisUpdate, dev.ID(), state) + } +} + func (p *MPRISPlugin) OnConnect(dev device.Sender) { p.logger.Info("mpris: device connected, requesting player list", zap.String("device_id", dev.ID())) go p.requestPlayerListPeriodic(dev) @@ -498,6 +634,64 @@ func (p *MPRISPlugin) requestPlayerListPeriodic(dev device.Sender) { p.requestPlayerList(dev) } +// remoteStatePollInterval is how often the daemon re-requests now-playing +// from devices with an active remote player. Clients are then pure-push: +// fresh state arrives within one interval of connect, and position stays +// current without any client-side polling. +const remoteStatePollInterval = 5 * time.Second + +// startRemoteStatePoller periodically re-requests now-playing from every +// connected device that has a known active player. The responses flow back +// through Handle, where shouldPublishRemoteState dedupes them, so an +// mpris.update is only republished when the state actually changes — not +// on every poll. This closes the "watch client misses mid-track state" +// gap from the initial dump's 10s freshness gate. +func (p *MPRISPlugin) startRemoteStatePoller(ctx context.Context) { + go func() { + ticker := time.NewTicker(remoteStatePollInterval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + p.pollRemoteStates() + } + } + }() +} + +// pollRemoteStates requests a now-playing refresh from devices that have a +// cached, actively-playing player. Devices without a cached state (never +// reported a player) or whose player is stopped/paused are skipped — stopped +// players are intentionally left to go stale instead of keeping a ghost track +// perpetually fresh. +func (p *MPRISPlugin) pollRemoteStates() { + p.mu.RLock() + type target struct { + dev device.Sender + player string + } + var targets []target + for id, dev := range p.devices { + if !dev.IsConnected() { + continue + } + state := p.remoteStates[id] + if state == nil || state.Player == "" || !state.IsPlaying { + continue + } + targets = append(targets, target{dev: dev, player: state.Player}) + } + p.mu.RUnlock() + + for _, t := range targets { + if err := p.requestPlayerStatus(t.dev, t.player); err != nil { + p.logger.Debug("mpris: state poll request failed", zap.Error(err)) + } + } +} + func (p *MPRISPlugin) OnDisconnect(dev device.Sender) { p.mu.Lock() defer p.mu.Unlock() @@ -657,6 +851,7 @@ func (p *MPRISPlugin) RemoteState(deviceID string) *NowPlaying { return nil } copy := state.DeepCopy() + copy.AlbumArtUrl = p.resolveArtURL(copy.AlbumArtUrl) if tracker, ok := p.positionTrackers[deviceID]; ok && tracker.playing { elapsed := time.Since(tracker.lastPositionAt).Milliseconds() copy.Pos = tracker.lastPosition + elapsed @@ -687,6 +882,7 @@ func (p *MPRISPlugin) RemoteStates() map[string]*NowPlaying { continue } copy := state.DeepCopy() + copy.AlbumArtUrl = p.resolveArtURL(copy.AlbumArtUrl) if tracker, ok := p.positionTrackers[id]; ok && tracker.playing { elapsed := time.Since(tracker.lastPositionAt).Milliseconds() copy.Pos = tracker.lastPosition + elapsed @@ -696,6 +892,18 @@ func (p *MPRISPlugin) RemoteStates() map[string]*NowPlaying { return result } +// resolveArtURL maps a cached kdeconnect:// album art URI to a loadable +// file:// URL. Non-kdeconnect URIs and not-yet-cached art pass through. +func (p *MPRISPlugin) resolveArtURL(raw string) string { + if raw == "" || p.artCache == nil { + return raw + } + if resolved := p.artCache.Resolve(raw); resolved != "" { + return resolved + } + return raw +} + // watchTelephony subscribes to telephony events and pauses/resumes // local MPRIS players when calls start/end. func (p *MPRISPlugin) watchTelephony(ctx context.Context) { diff --git a/internal/plugins/mpris/mpris_test.go b/internal/plugins/mpris/mpris_test.go index d1e2b6e..0c00d1c 100644 --- a/internal/plugins/mpris/mpris_test.go +++ b/internal/plugins/mpris/mpris_test.go @@ -3,7 +3,9 @@ package mpris import ( "context" "crypto/x509" + "encoding/json" "net" + "sync" "testing" "time" @@ -106,3 +108,269 @@ func expectNoEvent(t *testing.T, sub *events.Subscriber) { case <-time.After(50 * time.Millisecond): } } + +type recordingSender struct { + id string + mu sync.Mutex + packets []*protocol.Packet +} + +func (s *recordingSender) ID() string { return s.id } +func (s *recordingSender) Name() string { return "" } +func (s *recordingSender) SetName(string) {} +func (s *recordingSender) State() device.PairingState { return device.StateUnpaired } +func (s *recordingSender) SetState(device.PairingState) {} +func (s *recordingSender) Send(p *protocol.Packet) error { + s.mu.Lock() + defer s.mu.Unlock() + s.packets = append(s.packets, p) + return nil +} +func (s *recordingSender) IsConnected() bool { return true } +func (s *recordingSender) RemoteIP() net.IP { return nil } +func (s *recordingSender) PeerCert() *x509.Certificate { return nil } +func (s *recordingSender) HasCapability(string) bool { return false } +func (s *recordingSender) UpdateBattery(charge int, charging bool) {} +func (s *recordingSender) GetBattery() (int, bool) { return 0, false } + +func (s *recordingSender) sent() []*protocol.Packet { + s.mu.Lock() + defer s.mu.Unlock() + out := make([]*protocol.Packet, len(s.packets)) + copy(out, s.packets) + return out +} + +func TestHandleRequestsAlbumArtForKdeconnectURI(t *testing.T) { + plugin := NewMPRISPlugin(nil, events.NewBus(zap.NewNop()), false, zap.NewNop()) + if plugin.watchCancel != nil { + defer plugin.watchCancel() + } + dev := &recordingSender{id: "device-art"} + + state := MPRISRequest{ + Player: "Spotify", + Title: "Some Track", + Artist: "Some Artist", + IsPlaying: true, + AlbumArtUrl: "kdeconnect:/artUri?title=Some+Track&kdeArtHash=12345", + } + if err := plugin.Handle(context.Background(), dev, newMPRISPacket(t, state)); err != nil { + t.Fatal(err) + } + + // requestAlbumArt runs async — wait for the packet to be sent. + deadline := time.Now().Add(time.Second) + for time.Now().Before(deadline) && len(dev.sent()) == 0 { + time.Sleep(5 * time.Millisecond) + } + plugin.mu.Lock() + _, requested := plugin.artRequests["req|device-art|"+state.AlbumArtUrl] + plugin.mu.Unlock() + if !requested { + t.Fatal("expected an album art request to be recorded") + } + + if len(dev.sent()) == 0 { + t.Fatal("expected at least one packet sent to the device") + } + pkt := dev.sent()[0] + if pkt.Type != "kdeconnect.mpris.request" { + t.Fatalf("expected kdeconnect.mpris.request, got %s", pkt.Type) + } + var body MPRISRequest + if err := json.Unmarshal(pkt.Body, &body); err != nil { + t.Fatal(err) + } + if body.Player != "Spotify" || body.AlbumArtUrl != state.AlbumArtUrl { + t.Fatalf("unexpected request body: %+v", body) + } +} + +func TestHandleIgnoresEmptyAlbumArtPayload(t *testing.T) { + plugin := NewMPRISPlugin(nil, events.NewBus(zap.NewNop()), false, zap.NewNop()) + if plugin.watchCancel != nil { + defer plugin.watchCancel() + } + dev := &recordingSender{id: "device-art-empty"} + + // A transferringAlbumArt packet with no payload must not crash and + // must not attempt a download (RemoteIP() is nil in the test sender). + pkt := newMPRISPacket(t, MPRISRequest{ + Player: "Spotify", + TransferringAlbumArt: true, + AlbumArtUrl: "kdeconnect:/artUri?kdeArtHash=1", + }) + if err := plugin.Handle(context.Background(), dev, pkt); err != nil { + t.Fatal(err) + } + time.Sleep(50 * time.Millisecond) // let any goroutine finish +} + +func TestPollRemoteStatesOnlyTargetsKnownPlayers(t *testing.T) { + plugin := NewMPRISPlugin(nil, events.NewBus(zap.NewNop()), false, zap.NewNop()) + if plugin.watchCancel != nil { + defer plugin.watchCancel() + } + + withPlayer := &recordingSender{id: "device-with-player"} + stoppedPlayer := &recordingSender{id: "device-stopped-player"} + noPlayer := &recordingSender{id: "device-no-player"} + + // Seed cached remote state for the device with a known active player. + state := MPRISRequest{Player: "Spotify", Title: "T", Artist: "A", IsPlaying: true} + if err := plugin.Handle(context.Background(), withPlayer, newMPRISPacket(t, state)); err != nil { + t.Fatal(err) + } + + // Seed cached state for a device whose player is stopped/paused — the + // poller must not keep refreshing it. + stopped := MPRISRequest{Player: "Namida", Title: "S", Artist: "B", IsPlaying: false} + if err := plugin.Handle(context.Background(), stoppedPlayer, newMPRISPacket(t, stopped)); err != nil { + t.Fatal(err) + } + + plugin.mu.Lock() + plugin.devices[withPlayer.id] = withPlayer + plugin.devices[stoppedPlayer.id] = stoppedPlayer + plugin.devices[noPlayer.id] = noPlayer + plugin.mu.Unlock() + + // Clear the recorded art-request packets from Handle so we can assert + // exactly what the poller sends. + withPlayer.sent() + stoppedPlayer.sent() + + plugin.pollRemoteStates() + + sent := withPlayer.sent() + if len(sent) == 0 { + t.Fatal("expected poller to request state from device with known active player") + } + var body MPRISRequest + if err := json.Unmarshal(sent[0].Body, &body); err != nil { + t.Fatal(err) + } + if !body.RequestNowPlaying { + t.Fatalf("expected requestNowPlaying=true in poll request, got %+v", body) + } + if body.Player != "Spotify" { + t.Fatalf("expected poll request for player Spotify, got %q", body.Player) + } + if got := stoppedPlayer.sent(); len(got) != 0 { + t.Fatalf("expected no poll request to device with stopped player, got %d packets", len(got)) + } + if got := noPlayer.sent(); len(got) != 0 { + t.Fatalf("expected no poll request to device without cached player, got %d packets", len(got)) + } +} + +func newMPRISRawPacket(t *testing.T, body map[string]interface{}) *protocol.Packet { + t.Helper() + pkt, err := protocol.NewPacket("kdeconnect.mpris", body) + if err != nil { + t.Fatal(err) + } + return pkt +} + +func TestHandlePrunesRemovedPlayer(t *testing.T) { + bus := events.NewBus(zap.NewNop()) + sub := bus.Subscribe(4, events.TypeMprisUpdate) + defer sub.Close() + + plugin := NewMPRISPlugin(nil, bus, false, zap.NewNop()) + if plugin.watchCancel != nil { + defer plugin.watchCancel() + } + dev := &recordingSender{id: "device-prune"} + + state := MPRISRequest{Player: "Namida", Title: "Otonoke", Artist: "Creepy Nuts", IsPlaying: true} + if err := plugin.Handle(context.Background(), dev, newMPRISPacket(t, state)); err != nil { + t.Fatal(err) + } + expectEvent(t, sub) // initial state event + + // The phone destroys the session: playerList no longer contains Namida. + list := newMPRISPacket(t, MPRISRequest{PlayerList: []string{"Spotify"}}) + if err := plugin.Handle(context.Background(), dev, list); err != nil { + t.Fatal(err) + } + + if got := plugin.RemoteState(dev.ID()); got != nil { + t.Fatalf("expected remote state to be pruned, got %+v", got) + } + plugin.mu.Lock() + _, hasTracker := plugin.positionTrackers[dev.ID()] + _, hasTime := plugin.remoteStateTimes[dev.ID()] + plugin.mu.Unlock() + if hasTracker { + t.Fatal("expected position tracker to be pruned") + } + if hasTime { + t.Fatal("expected remote state time to be pruned") + } + + // Empty update published so watchers fall back to "no media playing". + expectEvent(t, sub) +} + +func TestHandlePrunesPlayerOnEmptyList(t *testing.T) { + bus := events.NewBus(zap.NewNop()) + sub := bus.Subscribe(4, events.TypeMprisUpdate) + defer sub.Close() + + plugin := NewMPRISPlugin(nil, bus, false, zap.NewNop()) + if plugin.watchCancel != nil { + defer plugin.watchCancel() + } + dev := &recordingSender{id: "device-empty-list"} + + state := MPRISRequest{Player: "Namida", Title: "Otonoke", IsPlaying: true} + if err := plugin.Handle(context.Background(), dev, newMPRISPacket(t, state)); err != nil { + t.Fatal(err) + } + expectEvent(t, sub) // initial state event + + // All sessions destroyed — Android sends an explicit empty playerList. + // (MPRISRequest marshals []string{} to "playerList":[]; the omitempty tag + // would drop the key, so inject a raw body to exercise the presence check.) + empty := newMPRISRawPacket(t, map[string]interface{}{"playerList": []string{}}) + if err := plugin.Handle(context.Background(), dev, empty); err != nil { + t.Fatal(err) + } + + if got := plugin.RemoteState(dev.ID()); got != nil { + t.Fatalf("expected remote state to be pruned, got %+v", got) + } + expectEvent(t, sub) // empty update published +} + +func TestHandleKeepsListedPlayer(t *testing.T) { + bus := events.NewBus(zap.NewNop()) + sub := bus.Subscribe(4, events.TypeMprisUpdate) + defer sub.Close() + + plugin := NewMPRISPlugin(nil, bus, false, zap.NewNop()) + if plugin.watchCancel != nil { + defer plugin.watchCancel() + } + dev := &recordingSender{id: "device-keep"} + + state := MPRISRequest{Player: "Spotify", Title: "T", IsPlaying: true} + if err := plugin.Handle(context.Background(), dev, newMPRISPacket(t, state)); err != nil { + t.Fatal(err) + } + expectEvent(t, sub) // initial state event + + // playerList still contains the tracked player — state must survive. + list := newMPRISPacket(t, MPRISRequest{PlayerList: []string{"Spotify", "Namida"}}) + if err := plugin.Handle(context.Background(), dev, list); err != nil { + t.Fatal(err) + } + + if got := plugin.RemoteState(dev.ID()); got == nil || got.Player != "Spotify" { + t.Fatalf("expected state to survive for listed player, got %+v", got) + } + expectNoEvent(t, sub) // no spurious empty update +} diff --git a/internal/plugins/notification/notification.go b/internal/plugins/notification/notification.go index 7c286ad..991481f 100644 --- a/internal/plugins/notification/notification.go +++ b/internal/plugins/notification/notification.go @@ -28,12 +28,14 @@ type NotificationPlugin struct { bus *events.Bus tlsConfig *tls.Config logger *zap.Logger - notifIDs sync.Map // maps body.ID (string) -> desktop notify-send ID (string) + notifIDs sync.Map // maps deviceID|body.ID -> desktop notify-send ID (string) + pendingCloses sync.Map // maps deviceID|body.ID -> *time.Timer (deferred close for cancel-grace) iconDir string // temp dir for cached notification icons cfg config.NotificationPluginConfig canCloseNotifs bool // whether notify-send supports --print-id mu sync.RWMutex filters config.NotificationConfig + newExec func(ctx context.Context, name string, args ...string) *exec.Cmd } // NewNotificationPlugin creates a NotificationPlugin. @@ -45,6 +47,7 @@ func NewNotificationPlugin(cfg config.NotificationPluginConfig, bus *events.Bus, bus: bus, tlsConfig: tlsConfig, logger: logger.With(zap.String("plugin", "notification")), + newExec: exec.CommandContext, } // Probe --print-id support by checking --help output. @@ -68,6 +71,12 @@ func (p *NotificationPlugin) Close() { if p.iconDir != "" { _ = os.RemoveAll(p.iconDir) } + p.pendingCloses.Range(func(k, v any) bool { + if t, ok := v.(*time.Timer); ok { + t.Stop() + } + return true + }) } // SetFilters atomically replaces the per-app notification filter map. @@ -128,15 +137,34 @@ func (p *NotificationPlugin) Handle(ctx context.Context, dev device.Sender, pkt // Handle cancellation — close the corresponding desktop notification. if body.IsCancel { if body.ID != "" { - if desktopID, ok := p.notifIDs.LoadAndDelete(body.ID); ok { - go func() { - _ = exec.CommandContext(context.Background(), "gdbus", "call", "--session", - "--dest", "org.freedesktop.Notifications", - "--object-path", "/org/freedesktop/Notifications", - "--method", "org.freedesktop.Notifications.CloseNotification", - desktopID.(string), - ).Run() - }() + key := p.notifKey(dev.ID(), body.ID) + if desktopID, ok := p.notifIDs.Load(key); ok { + if p.cfg.CancelGraceMS > 0 { + // Cancel-grace: hold the popup open so a same-id re-post + // (media now-playing toggling play/pause) updates it in + // place instead of closing and re-opening. If no re-post + // arrives, close after the grace window. + if t, ok := p.pendingCloses.Load(key); ok { + t.(*time.Timer).Stop() + } + want := desktopID.(string) + p.pendingCloses.Store(key, time.AfterFunc( + time.Duration(p.cfg.CancelGraceMS)*time.Millisecond, + func() { + if cur, ok := p.notifIDs.LoadAndDelete(key); ok { + // Only close the popup we scheduled — if a + // re-post replaced it, leave the new one. + if cur.(string) == want { + p.closeNotification(want) + } + } + p.pendingCloses.Delete(key) + }, + )) + } else { + p.notifIDs.Delete(key) + p.closeNotification(desktopID.(string)) + } } } if p.bus != nil { @@ -212,13 +240,22 @@ func (p *NotificationPlugin) Handle(ctx context.Context, dev device.Sender, pkt // Handlers must not block — all I/O in a goroutine. go func() { - iconPath := p.fetchIcon(ctx, appName, body.ID, remoteIP, payloadPort, payloadSize, hasIcon) - p.sendDesktopNotification(appName, body.ID, body.Title, text, iconPath) + var iconPath string + if p.cfg.ShowIcons { + iconPath = p.fetchIcon(ctx, appName, body.ID, remoteIP, payloadPort, payloadSize, hasIcon) + } + p.sendDesktopNotification(dev.ID(), appName, body.ID, body.Title, text, iconPath) }() return nil } +// notifKey scopes a notification id to its device so two paired phones with +// colliding Android notification keys don't replace each other's popups. +func (p *NotificationPlugin) notifKey(devID, id string) string { + return devID + "|" + id +} + // fetchIcon downloads the notification icon payload and returns the path to the // saved file, or an empty string if unavailable. func (p *NotificationPlugin) fetchIcon( @@ -229,7 +266,7 @@ func (p *NotificationPlugin) fetchIcon( size int64, hasIcon bool, ) string { - if !hasIcon || !p.cfg.FetchIcons || p.tlsConfig == nil || p.iconDir == "" { + if !p.cfg.FetchIcons || p.tlsConfig == nil || p.iconDir == "" { // Fall back to icon name derived from app name. return "" } @@ -239,11 +276,18 @@ func (p *NotificationPlugin) fetchIcon( safeName := nonAlphaNumeric.ReplaceAllString(appName, "_") iconPath := filepath.Join(p.iconDir, fmt.Sprintf("%s-%s.png", safeName, notifID)) - // Already cached from a previous notification from this app. + // Reuse the cached icon even when the phone re-posts the notification + // without an icon payload (Android only sends the bytes when the icon + // hash changes). Without this, every re-post would fall back to a theme + // icon name that doesn't exist, showing a placeholder image. if _, err := os.Stat(iconPath); err == nil { return iconPath } + if !hasIcon { + return "" + } + addr := fmt.Sprintf("%s:%d", remoteIP, port) dialer := &tls.Dialer{ NetDialer: &net.Dialer{Timeout: 10 * time.Second}, @@ -271,19 +315,23 @@ func (p *NotificationPlugin) fetchIcon( return iconPath } -// sendDesktopNotification calls notify-send with the collected parameters. -func (p *NotificationPlugin) sendDesktopNotification(appName, id, title, text, iconPath string) { - // Derive a fallback icon name from the app name when no payload icon is available. - iconArg := strings.ToLower(strings.ReplaceAll(appName, " ", "-")) - if iconPath != "" { - iconArg = iconPath - } - if iconArg == "" { - iconArg = "smartphone" - } +// closeNotification closes a previously-shown desktop popup by id. +func (p *NotificationPlugin) closeNotification(desktopID string) { + go func() { + _ = p.newExec(context.Background(), "gdbus", "call", "--session", + "--dest", "org.freedesktop.Notifications", + "--object-path", "/org/freedesktop/Notifications", + "--method", "org.freedesktop.Notifications.CloseNotification", + desktopID, + ).Run() + }() +} +// sendDesktopNotification calls notify-send with the collected parameters. +func (p *NotificationPlugin) sendDesktopNotification(devID, appName, id, title, text, iconPath string) { // Dunst / mako / swaync: stack notifications from the same app so they - // replace each other instead of flooding the screen. + // replace each other instead of flooding the screen. Daemons that ignore + // this hint (e.g. Quickshell) are covered by --replace-id below. groupHint := "string:x-dunst-stack-tag:kcd-" + appName args := []string{"-a", appName} @@ -293,18 +341,51 @@ func (p *NotificationPlugin) sendDesktopNotification(appName, id, title, text, i if p.cfg.ExpireMS >= 0 { args = append(args, "-t", strconv.Itoa(p.cfg.ExpireMS)) } + + // No icon by default. Pass an explicit empty icon so daemons (e.g. + // Quickshell) don't fall back to deriving an icon name from the app name + // and render a placeholder. When show_icons is enabled, pass the phone's + // downloaded icon, falling back to a name derived from the app. + iconArg := "" + if p.cfg.ShowIcons { + iconArg = strings.ToLower(strings.ReplaceAll(appName, " ", "-")) + if iconPath != "" { + iconArg = iconPath + } + if iconArg == "" { + iconArg = "smartphone" + } + } args = append(args, "-i", iconArg, "-h", groupHint) if p.canCloseNotifs && id != "" { + // Android re-posts notifications on every update with a stable id + // (e.g. media/scrobble now-playing). Replace the existing desktop + // popup via D-Bus replaces_id so repeated updates collapse to one, + // mirroring the reference desktop's Notification::update(). Disable + // via `replace_notifications = false`. + if p.cfg.ReplaceNotifications { + key := p.notifKey(devID, id) + // Cancel-grace: if a cancel was deferred for this key, drop it so + // the popup is updated in place rather than closed and re-opened. + if t, ok := p.pendingCloses.LoadAndDelete(key); ok { + t.(*time.Timer).Stop() + } + if prevID, ok := p.notifIDs.Load(key); ok { + if s, ok := prevID.(string); ok && s != "" { + args = append(args, "-r", s) + } + } + } args = append(args, "--print-id", title, text) } else { args = append(args, title, text) } - out, err := exec.CommandContext(context.Background(), "notify-send", args...).Output() + out, err := p.newExec(context.Background(), "notify-send", args...).Output() if err == nil && p.canCloseNotifs && id != "" { if desktopID := strings.TrimSpace(string(out)); desktopID != "" { - p.notifIDs.Store(id, desktopID) + p.notifIDs.Store(p.notifKey(devID, id), desktopID) } } } diff --git a/internal/plugins/notification/notification_test.go b/internal/plugins/notification/notification_test.go index 641420a..bcf81e9 100644 --- a/internal/plugins/notification/notification_test.go +++ b/internal/plugins/notification/notification_test.go @@ -2,7 +2,15 @@ package notification import ( "context" + "crypto/tls" + "fmt" + "os" + "os/exec" + "path/filepath" + "strconv" + "sync" "testing" + "time" "github.com/bethropolis/kcd/internal/config" "github.com/bethropolis/kcd/internal/device" @@ -20,6 +28,10 @@ func newPlugin(t *testing.T) *NotificationPlugin { // tlsConfig is nil — icon fetching is skipped in unit tests. p := NewNotificationPlugin(cfg, bus, nil, logger) t.Cleanup(p.Close) + // Never touch the real notification daemon from tests. + p.newExec = func(_ context.Context, name string, args ...string) *exec.Cmd { + return exec.CommandContext(context.Background(), "true") + } return p } @@ -41,11 +53,13 @@ func TestNotificationPlugin_Handle_Normal(t *testing.T) { func TestNotificationPlugin_Handle_Cancel(t *testing.T) { p := newPlugin(t) + // Immediate close — no grace debounce for this test. + p.cfg.CancelGraceMS = 0 logger := zaptest.NewLogger(t) dev := device.NewDevice("dev1", "Test", "phone", logger) // Store a fake desktop ID so the cancel path can look it up. - p.notifIDs.Store("notif-abc", "42") + p.notifIDs.Store(p.notifKey(dev.ID(), "notif-abc"), "42") body := NotificationBody{ ID: "notif-abc", @@ -57,7 +71,7 @@ func TestNotificationPlugin_Handle_Cancel(t *testing.T) { } // Entry should have been removed. - if _, ok := p.notifIDs.Load("notif-abc"); ok { + if _, ok := p.notifIDs.Load(p.notifKey(dev.ID(), "notif-abc")); ok { t.Error("expected notifIDs entry to be removed after cancel") } } @@ -78,3 +92,267 @@ func TestNotificationPlugin_Handle_Silent(t *testing.T) { t.Fatalf("Handle returned error: %v", err) } } + +// fakeNotifier records the notify-send invocations and fakes a print-id. +type fakeNotifier struct { + mu sync.Mutex + calls [][]string + nextID int +} + +func (f *fakeNotifier) command(name string, args ...string) *exec.Cmd { + f.mu.Lock() + defer f.mu.Unlock() + f.calls = append(f.calls, append([]string{name}, args...)) + f.nextID++ + id := strconv.Itoa(f.nextID) + // Simulate notify-send --print-id printing the new desktop id. + return exec.CommandContext(context.Background(), "sh", "-c", "printf '%s' '"+id+"'") +} + +func (f *fakeNotifier) argFor(call int, flag string) string { + for i, a := range f.calls[call] { + if a == flag { + if i+1 < len(f.calls[call]) { + return f.calls[call][i+1] + } + } + } + return "" +} + +func newFakePlugin(t *testing.T, replace bool) (*NotificationPlugin, *fakeNotifier) { + t.Helper() + logger := zaptest.NewLogger(t) + bus := events.NewBus(logger) + cfg := config.NotificationPluginConfig{} + cfg.Defaults() + cfg.ReplaceNotifications = replace + p := NewNotificationPlugin(cfg, bus, nil, logger) + t.Cleanup(p.Close) + p.canCloseNotifs = true + f := &fakeNotifier{} + p.newExec = func(_ context.Context, name string, args ...string) *exec.Cmd { + return f.command(name, args...) + } + return p, f +} + +func TestNotificationPlugin_ReplaceByID(t *testing.T) { + p, f := newFakePlugin(t, true) + dev := device.NewDevice("dev1", "Test", "phone", zaptest.NewLogger(t)) + id := "0|com.arn.scrobble|0|com.msob7y.namida|10247" + + // First post — no replace, stores the printed desktop id. + p.sendDesktopNotification(dev.ID(), "Pano Scrobbler", id, "Song", "Artist", "") + if r := f.argFor(0, "-r"); r != "" { + t.Fatalf("expected no replace on first post, got -r %q", r) + } + key := p.notifKey(dev.ID(), id) + if v, ok := p.notifIDs.Load(key); !ok || v != "1" { + t.Fatalf("expected stored desktop id 1 under %q, got %v/%v", key, ok, v) + } + + // Second post, same id — must replace the previous popup. + p.sendDesktopNotification(dev.ID(), "Pano Scrobbler", id, "Song", "Artist", "") + if r := f.argFor(1, "-r"); r != "1" { + t.Fatalf("expected -r 1 on update, got %q", r) + } + if v, ok := p.notifIDs.Load(key); !ok || v != "2" { + t.Fatalf("expected stored desktop id 2, got %v/%v", ok, v) + } + + // Same id from a different device must not replace this device's popup. + other := device.NewDevice("dev2", "Other", "phone", zaptest.NewLogger(t)) + p.sendDesktopNotification(other.ID(), "Pano Scrobbler", id, "Song", "Artist", "") + if r := f.argFor(2, "-r"); r != "" { + t.Fatalf("expected no replace for a different device, got -r %q", r) + } + if v, ok := p.notifIDs.Load(p.notifKey(other.ID(), id)); !ok || v != "3" { + t.Fatalf("expected stored desktop id 3 under dev2 key, got %v/%v", ok, v) + } + + // After a cancel, the entry is gone and the next post is fresh again. + p.notifIDs.Delete(key) + p.sendDesktopNotification(dev.ID(), "Pano Scrobbler", id, "Song", "Artist", "") + if r := f.argFor(3, "-r"); r != "" { + t.Fatalf("expected no replace after cancel, got -r %q", r) + } +} + +func TestNotificationPlugin_ReplaceByIDDisabled(t *testing.T) { + p, f := newFakePlugin(t, false) + dev := device.NewDevice("dev1", "Test", "phone", zaptest.NewLogger(t)) + id := "0|com.arn.scrobble|0|com.msob7y.namida|10247" + + // Seed a stored id — with replacement disabled it must be ignored. + p.notifIDs.Store(p.notifKey(dev.ID(), id), "7") + + p.sendDesktopNotification(dev.ID(), "Pano Scrobbler", id, "Song", "Artist", "") + if r := f.argFor(0, "-r"); r != "" { + t.Fatalf("expected no replace when config disabled, got -r %q", r) + } + // The print-id is still captured so cancel/close keeps working. + if v, ok := p.notifIDs.Load(p.notifKey(dev.ID(), id)); !ok || v != "1" { + t.Fatalf("expected stored desktop id 1, got %v/%v", ok, v) + } +} + +func TestNotificationPlugin_FetchIconReusesCacheOnRepost(t *testing.T) { + p := newPlugin(t) + p.tlsConfig = &tls.Config{} + p.iconDir = t.TempDir() + app := "Pano Scrobbler" + id := "0|com.arn.scrobble|0|com.msob7y.namida|10247" + + // Seed the cached icon for this (app, id) — simulates the first post + // having downloaded the phone's icon. + cachedPath := filepath.Join(p.iconDir, fmt.Sprintf("%s-%s.png", + nonAlphaNumeric.ReplaceAllString(app, "_"), id)) + if err := os.WriteFile(cachedPath, []byte("png"), 0o600); err != nil { + t.Fatal(err) + } + + // Re-post without an icon payload must reuse the cached file so the + // popup keeps the real app icon instead of a placeholder. + got := p.fetchIcon(context.Background(), app, id, nil, 0, 0, false) + if got != cachedPath { + t.Fatalf("expected cached icon reuse %q, got %q", cachedPath, got) + } + + // Unknown id, no payload, no cache → empty (theme fallback downstream). + if got := p.fetchIcon(context.Background(), app, "unknown-id", nil, 0, 0, false); got != "" { + t.Fatalf("expected empty icon for uncached payload-less repost, got %q", got) + } +} + +func TestNotificationPlugin_ShowIconsGating(t *testing.T) { + dev := device.NewDevice("dev1", "Test", "phone", zaptest.NewLogger(t)) + + // Default (show_icons = false): an explicit empty icon — popups render + // icon-less and daemons won't derive a placeholder from the app name. + p, f := newFakePlugin(t, true) + p.sendDesktopNotification(dev.ID(), "Pano Scrobbler", "id-1", "Song", "Artist", "") + if got := f.argFor(0, "-i"); got != "" { + t.Fatalf("expected empty -i by default, got -i %q", got) + } + + // show_icons = true: -i carries the icon path/name. + p2, f2 := newFakePlugin(t, true) + p2.cfg.ShowIcons = true + p2.sendDesktopNotification(dev.ID(), "Pano Scrobbler", "id-2", "Song", "Artist", "/tmp/icon.png") + if got := f2.argFor(0, "-i"); got != "/tmp/icon.png" { + t.Fatalf("expected -i /tmp/icon.png with icons enabled, got %q", got) + } +} + +// gdbusCalls returns the recorded CloseNotification invocations. +func (f *fakeNotifier) gdbusCalls() [][]string { + f.mu.Lock() + defer f.mu.Unlock() + var out [][]string + for _, c := range f.calls { + if len(c) > 0 && c[0] == "gdbus" { + out = append(out, c) + } + } + return out +} + +func waitFor(t *testing.T, what string, cond func() bool) { + t.Helper() + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + if cond() { + return + } + time.Sleep(10 * time.Millisecond) + } + t.Fatalf("timed out waiting for %s", what) +} + +func postAndCancel(t *testing.T, p *NotificationPlugin, dev device.Sender, id string) { + t.Helper() + p.sendDesktopNotification(dev.ID(), "Pano Scrobbler", id, "Song", "Artist", "") + body := NotificationBody{ID: id, IsCancel: true} + pkt, _ := protocol.NewPacket("kdeconnect.notification", body) + if err := p.Handle(context.Background(), dev, pkt); err != nil { + t.Fatalf("Handle(cancel) returned error: %v", err) + } +} + +func TestNotificationPlugin_CancelGrace_RePostWithinWindow(t *testing.T) { + p, f := newFakePlugin(t, true) + p.cfg.CancelGraceMS = 200 + dev := device.NewDevice("dev1", "Test", "phone", zaptest.NewLogger(t)) + id := "0|com.arn.scrobble|0|com.msob7y.namida|10247" + key := p.notifKey(dev.ID(), id) + + // Post then cancel — the close is deferred, the popup stays open. + postAndCancel(t, p, dev, id) + if len(f.gdbusCalls()) != 0 { + t.Fatalf("expected no immediate close under cancel-grace, got %d gdbus calls", len(f.gdbusCalls())) + } + if v, ok := p.notifIDs.Load(key); !ok || v != "1" { + t.Fatalf("expected desktop id 1 retained during grace, got %v/%v", ok, v) + } + + // Re-post within the window — must update in place, not close+reopen. + p.sendDesktopNotification(dev.ID(), "Pano Scrobbler", id, "Song", "Artist", "") + if r := f.argFor(1, "-r"); r != "1" { + t.Fatalf("expected -r 1 on re-post during grace, got %q", r) + } + + // Wait past the grace window: the deferred close must have been cancelled. + time.Sleep(250 * time.Millisecond) + if len(f.gdbusCalls()) != 0 { + t.Fatalf("expected deferred close cancelled on re-post, got %d gdbus calls", len(f.gdbusCalls())) + } + if v, ok := p.notifIDs.Load(key); !ok || v != "2" { + t.Fatalf("expected desktop id 2 after in-place update, got %v/%v", ok, v) + } +} + +func TestNotificationPlugin_CancelGrace_NoRePost(t *testing.T) { + p, f := newFakePlugin(t, true) + p.cfg.CancelGraceMS = 50 + dev := device.NewDevice("dev1", "Test", "phone", zaptest.NewLogger(t)) + id := "0|com.arn.scrobble|0|com.msob7y.namida|10247" + key := p.notifKey(dev.ID(), id) + + postAndCancel(t, p, dev, id) + if len(f.gdbusCalls()) != 0 { + t.Fatalf("expected no immediate close under cancel-grace, got %d gdbus calls", len(f.gdbusCalls())) + } + + // No re-post: after the grace window the popup is closed and the entry dropped. + waitFor(t, "deferred close", func() bool { + if len(f.gdbusCalls()) != 1 { + return false + } + _, ok := p.notifIDs.Load(key) + return !ok + }) + if got := f.gdbusCalls()[0][len(f.gdbusCalls()[0])-1]; got != "1" { + t.Fatalf("expected gdbus close of desktop id 1, got %q", got) + } +} + +func TestNotificationPlugin_CancelGraceDisabled(t *testing.T) { + p, f := newFakePlugin(t, true) + p.cfg.CancelGraceMS = 0 + dev := device.NewDevice("dev1", "Test", "phone", zaptest.NewLogger(t)) + id := "0|com.arn.scrobble|0|com.msob7y.namida|10247" + key := p.notifKey(dev.ID(), id) + + postAndCancel(t, p, dev, id) + waitFor(t, "immediate gdbus close", func() bool { + if _, ok := p.notifIDs.Load(key); ok { + return false + } + return len(f.gdbusCalls()) == 1 + }) + if got := f.gdbusCalls()[0][len(f.gdbusCalls()[0])-1]; got != "1" { + t.Fatalf("expected gdbus close of desktop id 1, got %q", got) + } +} diff --git a/packaging/kcd-user.service b/packaging/kcd-user.service index 9a6f608..7a69bd2 100644 --- a/packaging/kcd-user.service +++ b/packaging/kcd-user.service @@ -1,9 +1,13 @@ [Unit] Description=kcd — Headless KDE Connect Daemon Documentation=https://github.com/bethropolis/kcd -# Wait for a network interface to be up before starting. -After=network-online.target +# Wait for the graphical session: the compositor exports WAYLAND_DISPLAY into +# the systemd user manager at startup, and kcd's clipboard plugin probes the +# Wayland socket to spawn wl-copy/wl-paste. Starting at default.target means +# kcd boots before the compositor and misses the display environment. +After=network-online.target graphical-session.target Wants=network-online.target +PartOf=graphical-session.target [Service] # sd_notify: systemd waits for the daemon to send READY=1 before marking @@ -43,4 +47,4 @@ TasksMax=64 PrivateNetwork=false [Install] -WantedBy=default.target +WantedBy=graphical-session.target diff --git a/packaging/kcd.example.toml b/packaging/kcd.example.toml index 3d21c45..c6f7334 100644 --- a/packaging/kcd.example.toml +++ b/packaging/kcd.example.toml @@ -209,6 +209,9 @@ suspend = "systemctl suspend" # max_body_length = 0 # 0 = no limit (truncates with … if set) # expire_ms = -1 # notification timeout in milliseconds, -1 = default # skip_non_clearable = false # skip non-dismissible notifications (media playback, etc.) +# replace_notifications = true # replace the same notification in place on re-posts (e.g. scrobble now-playing) instead of flooding new popups +# show_icons = false # show app icons on desktop popups (downloaded from the phone); off by default +# cancel_grace_ms = 1500 # delay closing a popup after a phone cancel; a same-id re-post within this window updates in place instead of flickering (0 = close immediately) # ─── Share: file transfer settings ────────────────────────────────────────────