diff --git a/internal/imageinput/clipboard.go b/internal/imageinput/clipboard.go index eb092eb62..a0127b0fa 100644 --- a/internal/imageinput/clipboard.go +++ b/internal/imageinput/clipboard.go @@ -2,6 +2,7 @@ package imageinput import ( "bytes" + "errors" "fmt" "net/http" "os" @@ -16,6 +17,13 @@ import ( // clipboard, or (nil, "", nil) when the clipboard has no image. Called when // text clipboard is empty (the user pasted a screenshot). The media type is // sniffed from the bytes, not trusted from the clipboard. +// ErrClipboardImageUnreadable reports that the clipboard holds an image the host +// has no way to extract. It is deliberately distinct from "no image on the +// clipboard", which is an ordinary no-op: this one is actionable, and silence is +// the wrong response to it. +var ErrClipboardImageUnreadable = errors.New( + "the clipboard holds an image but no helper on this host can read it; on macOS install pngpaste (brew install pngpaste) or a Python with pyobjc") + func ReadClipboardImage() ([]byte, string, error) { data, err := readClipboardImageBytes() if err != nil { @@ -95,35 +103,71 @@ func readClipboardImageWindows() ([]byte, error) { // readClipboardImageDarwin uses osascript to check for and read a clipboard // image. Returns (nil, nil) when no image is present. -func readClipboardImageDarwin() ([]byte, error) { - // Check clipboard info for image classes. - check := `osascript -e 'clipboard info'` - out, err := exec.Command("sh", "-c", check).Output() - if err != nil { - return nil, nil - } - info := string(out) - if !strings.Contains(info, "PNG") && !strings.Contains(info, "JPEG") && !strings.Contains(info, "TIFF") && !strings.Contains(info, "GIF") { - return nil, nil - } - // Write clipboard image to a temp file via AppleScript, then read it. - // Using pngpaste if available, falling back to a Python one-liner. - cmd := exec.Command("sh", "-c", `pngpaste - 2>/dev/null || python3 -c " +// darwinClipboardInfo reports what the pasteboard is currently offering, and +// darwinClipboardExtract pulls the PNG bytes out of it. +// +// Both are vars so the two-step logic below can be driven in a test. The step +// that matters, "the clipboard holds an image and nothing here can extract it", +// only happens when the first succeeds and the second fails, which cannot be +// arranged by stubbing the caller: a test that substitutes ReadClipboardImage +// asserts the plumbing around this function rather than the decision inside it, +// and stays green with that decision reverted. +var ( + darwinClipboardInfo = func() (string, error) { + out, err := exec.Command("sh", "-c", `osascript -e 'clipboard info'`).Output() + return string(out), err + } + darwinClipboardExtract = func() ([]byte, error) { + // pngpaste if available, falling back to a Python one-liner. Neither + // ships with macOS: pngpaste is Homebrew-only and Apple dropped the + // bundled PyObjC, so a stock Mac fails here every time. + cmd := exec.Command("sh", "-c", `pngpaste - 2>/dev/null || python3 -c " import AppKit, sys pb = AppKit.NSPasteboard.generalPasteboard() data = pb.dataForType_(AppKit.NSPasteboardTypePNG) if data: sys.stdout.buffer.write(data.bytes()) "`) - var stdout bytes.Buffer - cmd.Stdout = &stdout - if err := cmd.Run(); err != nil { + var stdout bytes.Buffer + cmd.Stdout = &stdout + if err := cmd.Run(); err != nil { + return nil, err + } + return stdout.Bytes(), nil + } +) + +// darwinClipboardOffersImage reports whether the pasteboard info line names an +// image class. +func darwinClipboardOffersImage(info string) bool { + for _, class := range []string{"PNG", "JPEG", "TIFF", "GIF"} { + if strings.Contains(info, class) { + return true + } + } + return false +} + +func readClipboardImageDarwin() ([]byte, error) { + info, err := darwinClipboardInfo() + if err != nil { return nil, nil } - if stdout.Len() == 0 { + if !darwinClipboardOffersImage(info) { return nil, nil } - return stdout.Bytes(), nil + // Past this point the clipboard is KNOWN to hold an image, because the probe + // above said so. A failure here is therefore not "no image", it is "there is + // an image and nothing on this host can extract it". Reporting that as + // absence is what left users staring at a paste that silently did nothing. + data, err := darwinClipboardExtract() + if err != nil { + return nil, ErrClipboardImageUnreadable + } + if len(data) == 0 { + return nil, ErrClipboardImageUnreadable + } + return data, nil } // readClipboardImageLinux tries wl-paste (Wayland) then xclip (X11) to read diff --git a/internal/imageinput/clipboard_darwin_logic_test.go b/internal/imageinput/clipboard_darwin_logic_test.go new file mode 100644 index 000000000..50aae9c13 --- /dev/null +++ b/internal/imageinput/clipboard_darwin_logic_test.go @@ -0,0 +1,84 @@ +package imageinput + +import ( + "errors" + "testing" +) + +// Drives readClipboardImageDarwin's own decision rather than the plumbing around +// it. Both shell steps are substituted, so this runs on any platform and, more +// importantly, actually reaches the branch under test: a test that stubs +// ReadClipboardImage instead stays green with the production change reverted, +// which is how the first version of this coverage slipped through. +func TestReadClipboardImageDarwinDistinguishesUnreadableFromAbsent(t *testing.T) { + restore := func() func() { + info, extract := darwinClipboardInfo, darwinClipboardExtract + return func() { darwinClipboardInfo, darwinClipboardExtract = info, extract } + }() + t.Cleanup(restore) + + const pngInfo = `«class PNGf», «class 8BPS»` + + t.Run("image present but no extractor is actionable", func(t *testing.T) { + darwinClipboardInfo = func() (string, error) { return pngInfo, nil } + darwinClipboardExtract = func() ([]byte, error) { return nil, errors.New("exit status 127") } + + data, err := readClipboardImageDarwin() + if !errors.Is(err, ErrClipboardImageUnreadable) { + t.Fatalf("err = %v, want ErrClipboardImageUnreadable; a stock Mac gets silence instead of a remedy", err) + } + if data != nil { + t.Fatalf("data = %q, want none", data) + } + }) + + t.Run("extractor producing nothing is also actionable", func(t *testing.T) { + darwinClipboardInfo = func() (string, error) { return pngInfo, nil } + darwinClipboardExtract = func() ([]byte, error) { return nil, nil } + + if _, err := readClipboardImageDarwin(); !errors.Is(err, ErrClipboardImageUnreadable) { + t.Fatalf("err = %v, want ErrClipboardImageUnreadable for an extractor that exits 0 with no bytes", err) + } + }) + + // The other half of the distinction, and the reason this cannot simply always + // report an error: an empty clipboard is an ordinary no-op, and turning that + // into a notice would make every stray Ctrl+V nag the user. + t.Run("no image on the clipboard stays silent", func(t *testing.T) { + darwinClipboardInfo = func() (string, error) { return `«class utf8»`, nil } + darwinClipboardExtract = func() ([]byte, error) { + t.Fatal("extractor ran despite the clipboard offering no image") + return nil, nil + } + + data, err := readClipboardImageDarwin() + if err != nil || data != nil { + t.Fatalf("data=%q err=%v, want a silent no-op", data, err) + } + }) + + t.Run("probe failure stays silent", func(t *testing.T) { + darwinClipboardInfo = func() (string, error) { return "", errors.New("osascript missing") } + darwinClipboardExtract = func() ([]byte, error) { + t.Fatal("extractor ran despite the info probe failing") + return nil, nil + } + + if data, err := readClipboardImageDarwin(); err != nil || data != nil { + t.Fatalf("data=%q err=%v, want a silent no-op", data, err) + } + }) + + t.Run("working extractor returns the bytes", func(t *testing.T) { + darwinClipboardInfo = func() (string, error) { return pngInfo, nil } + darwinClipboardExtract = func() ([]byte, error) { return []byte("png-bytes"), nil } + + data, err := readClipboardImageDarwin() + if err != nil { + t.Fatalf("err = %v, want none", err) + } + if string(data) != "png-bytes" { + t.Fatalf("data = %q, want the extractor's bytes", data) + } + }) +} diff --git a/internal/tui/clipboard.go b/internal/tui/clipboard.go index b171307b6..34b03b456 100644 --- a/internal/tui/clipboard.go +++ b/internal/tui/clipboard.go @@ -23,12 +23,16 @@ type clipboardImageMsg struct { err error } +// readClipboardImage is the OS clipboard image reader. A var so tests can +// substitute a stub instead of depending on the developer's real clipboard. +var readClipboardImage = imageinput.ReadClipboardImage + // readClipboardImageCmd reads the OS clipboard for image content off the // Update goroutine. Returns a clipboardImageMsg with the bytes, or nil (no // command) if there is no image — the caller treats nil as a silent no-op. func readClipboardImageCmd() tea.Cmd { return func() tea.Msg { - data, mediaType, err := imageinput.ReadClipboardImage() + data, mediaType, err := readClipboardImage() if err != nil { return clipboardImageMsg{err: err} } diff --git a/internal/tui/composer_test.go b/internal/tui/composer_test.go index f1adfd919..4305d22ce 100644 --- a/internal/tui/composer_test.go +++ b/internal/tui/composer_test.go @@ -6,6 +6,8 @@ import ( "testing" tea "charm.land/bubbletea/v2" + + "github.com/Gitlawb/zero/internal/imageinput" ) func TestComposerInsertNewlineAtCursor(t *testing.T) { @@ -129,22 +131,77 @@ func TestSanitizeComposerPastePreservesNewlines(t *testing.T) { } } -func TestCtrlVDoesNotPasteIntoComposer(t *testing.T) { +// Ctrl+V must never insert TEXT: bracketed paste is the only text path, so +// letting Bubble's own Ctrl+V binding also read the clipboard would double-paste. +// It may return a command, but that command is the image probe below, never a +// text paste, so the composer contents are unchanged either way. +func TestCtrlVDoesNotPasteTextIntoComposer(t *testing.T) { m := newModel(context.Background(), Options{}) m.input.SetValue("hello") m.input.CursorEnd() - updated, cmd := m.Update(testKeyCtrl('v')) + updated, _ := m.Update(testKeyCtrl('v')) next := updated.(model) - if cmd != nil { - t.Fatal("ctrl+v should not run the textinput clipboard paste command") - } if got := next.composerValue(); got != "hello" { t.Fatalf("composer value after ctrl+v = %q, want unchanged", got) } } +// Ctrl+V probes the clipboard for an image (#534). A clipboard holding a +// screenshot produces no bracketed paste at all, so without this the empty-paste +// image probe in routePaste never runs and Ctrl+V does nothing, which is exactly +// what users reported: right-click paste attached the image, Ctrl+V did not. +func TestCtrlVProbesClipboardForImage(t *testing.T) { + // Stubbed BEFORE Update, so the command Update returns is the one that runs. + // Asserting only that some command came back, then running a freshly built + // probe, would pass even if the key never reached the image route at all. + original := readClipboardImage + t.Cleanup(func() { readClipboardImage = original }) + readClipboardImage = func() ([]byte, string, error) { + return []byte("png-bytes"), "image/png", nil + } + + // Both modifiers: macOS reports Command as ModSuper, so a handler matching + // only ModCtrl leaves Cmd+V doing nothing on the platform where screenshots + // are most often on the clipboard. + for name, key := range map[string]tea.KeyPressMsg{ + "ctrl+v": testKeyCtrl('v'), + "cmd+v": testKeyPressMod('v', tea.ModSuper), + } { + t.Run(name, func(t *testing.T) { + m := newModel(context.Background(), Options{}) + _, cmd := m.Update(key) + if cmd == nil { + t.Fatal("no command issued; the clipboard image probe never ran") + } + image, ok := execCmd(cmd).(clipboardImageMsg) + if !ok { + t.Fatal("the command issued was not the clipboard image probe") + } + if string(image.data) != "png-bytes" || image.mediaType != "image/png" { + t.Fatalf("unexpected image message: %+v", image) + } + }) + } +} + +// With no image on the clipboard the probe stays silent: Ctrl+V while copying +// text must not emit a notice or disturb the composer. Driven through Update +// rather than by calling the probe directly, so this covers the real key route +// and not just the command in isolation. +func TestClipboardImageProbeSilentWithoutImage(t *testing.T) { + original := readClipboardImage + t.Cleanup(func() { readClipboardImage = original }) + readClipboardImage = func() ([]byte, string, error) { return nil, "", nil } + + m := newModel(context.Background(), Options{}) + _, cmd := m.Update(testKeyCtrl('v')) + if msg := execCmd(cmd); msg != nil { + t.Fatalf("probe emitted %T with no image on the clipboard, want no message", msg) + } +} + func TestPastedMultilineComposerContentRendersAsPreview(t *testing.T) { paste := strings.Join([]string{ "Create a book library dashboard page with the Bootstrap theme.", @@ -465,3 +522,53 @@ func TestComposerTerminalWordKeybindings(t *testing.T) { }) } } + +// A clipboard that holds an image the host cannot extract must say so. This is +// the macOS case: neither pngpaste nor PyObjC ships with the OS, so the reader +// fails on a stock Mac, and reporting that as "no image" meant Ctrl+V did +// nothing at all with no explanation. Silence is the wrong answer to a +// condition the user can act on. +func TestClipboardImageUnreadableSurfacesToUser(t *testing.T) { + original := readClipboardImage + t.Cleanup(func() { readClipboardImage = original }) + readClipboardImage = func() ([]byte, string, error) { + return nil, "", imageinput.ErrClipboardImageUnreadable + } + + m := newModel(context.Background(), Options{}) + _, cmd := m.Update(testKeyCtrl('v')) + if cmd == nil { + t.Fatal("no command issued; the clipboard image probe never ran") + } + msg := execCmd(cmd) + image, ok := msg.(clipboardImageMsg) + if !ok { + t.Fatalf("probe returned %T, want clipboardImageMsg carrying the failure", msg) + } + if image.err == nil { + t.Fatal("the unreadable-clipboard failure was swallowed; the user is told nothing") + } + // The message has to name the remedy, since the whole point is that the user + // can fix this by installing something. + if !strings.Contains(image.err.Error(), "pngpaste") { + t.Fatalf("error = %q, want it to name what to install", image.err.Error()) + } + + // And it must reach the transcript as an error row rather than stopping at + // the message, which is where the old silent no-op ended. + updated, _ := m.Update(image) + next, ok := updated.(model) + if !ok { + t.Fatalf("Update returned %T, want model", updated) + } + found := false + for _, row := range next.transcript { + if row.kind == rowError && strings.Contains(row.text, "Clipboard image read failed") { + found = true + break + } + } + if !found { + t.Fatal("the failure never reached the transcript; the user still sees nothing") + } +} diff --git a/internal/tui/input_compat.go b/internal/tui/input_compat.go index 547d9c119..06f239d49 100644 --- a/internal/tui/input_compat.go +++ b/internal/tui/input_compat.go @@ -34,6 +34,14 @@ func keyCtrl(msg tea.KeyMsg, code rune) bool { return keyCode(msg) == code && keyHasMod(msg, tea.ModCtrl) } +// keySuper matches the Command key on macOS, which the rest of the TUI models as +// ModSuper (see keybindings.go). Kept separate from keyCtrl because a binding +// that means Ctrl on Linux and Windows usually means Command on macOS, and the +// two arrive as different modifiers. +func keySuper(msg tea.KeyMsg, code rune) bool { + return keyCode(msg) == code && keyHasMod(msg, tea.ModSuper) +} + func keyCtrlArrow(msg tea.KeyMsg, code rune) bool { return keyIs(msg, code) && keyHasMod(msg, tea.ModCtrl) } diff --git a/internal/tui/model.go b/internal/tui/model.go index ffbc62c27..525f37f25 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -1699,6 +1699,27 @@ func (m model) updateModel(msg tea.Msg) (tea.Model, tea.Cmd) { m.input.SetWidth(maxInt(20, m.chatColumnWidth()-14)) return m, nil } + case keyCtrl(msg, 'v'), keySuper(msg, 'v'): + // Ctrl+V probes the clipboard for an IMAGE only. Text pasting stays + // exclusively on the terminal's bracketed-paste path (Bubble's own + // Ctrl+V binding is disabled in newModel for exactly that reason), so + // this cannot double-insert text. It is needed because a clipboard + // holding a screenshot produces no bracketed paste at all: the terminal + // has no text to send, so routePaste never runs and its empty-content + // image probe never fires. Right-click paste reached that probe only + // because it always delivers a clipboardReadMsg, empty or not. + // readClipboardImageCmd yields no message when the clipboard holds no + // image, so Ctrl+V with text on the clipboard stays a no-op here and is + // handled by the bracketed paste exactly as before. + // + // Cmd+V is matched too, since macOS reports Command as ModSuper rather + // than ModCtrl. That only helps on terminals that deliver the key to the + // application: one that handles Cmd+V itself pastes the clipboard TEXT + // and sends no key event, so an image-only clipboard still produces + // nothing for this to react to. + if m.noBlockingModal() { + return m, readClipboardImageCmd() + } case keyCtrl(msg, 'f'): if m.picker != nil && m.picker.kind == pickerModel { if m.modelPickerIsLoading() {