Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/book/epub_render.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ func (p epubTextPage) open(ctx context.Context) (io.ReadCloser, error) {
}
}
var data bytes.Buffer
if err := png.Encode(contextWriter{ctx, &data}, canvas); err != nil {
if err := (&png.Encoder{CompressionLevel: png.BestSpeed}).Encode(contextWriter{ctx, &data}, canvas); err != nil {
return nil, err
}
return io.NopCloser(bytes.NewReader(data.Bytes())), nil
Expand Down
45 changes: 24 additions & 21 deletions internal/render/transform.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,34 +70,37 @@ func Highlight(src image.Image, rects []image.Rectangle) *image.RGBA {
return dst
}

// rotate turns src by q quarter turns clockwise. Pixels move through an
// RGBA copy; At and Set would box a color per pixel.
func rotate(src image.Image, q int) image.Image {
if q == 0 {
return src
}
b := src.Bounds()
w, h := b.Dx(), b.Dy()
var dst *image.RGBA
switch q {
case 1: // 90 cw
dst = image.NewRGBA(image.Rect(0, 0, h, w))
for y := 0; y < h; y++ {
for x := 0; x < w; x++ {
dst.Set(h-1-y, x, src.At(b.Min.X+x, b.Min.Y+y))
}
}
case 2:
dst = image.NewRGBA(image.Rect(0, 0, w, h))
for y := 0; y < h; y++ {
for x := 0; x < w; x++ {
dst.Set(w-1-x, h-1-y, src.At(b.Min.X+x, b.Min.Y+y))
}
}
default: // 270 cw
dst = image.NewRGBA(image.Rect(0, 0, h, w))
for y := 0; y < h; y++ {
for x := 0; x < w; x++ {
dst.Set(y, w-1-x, src.At(b.Min.X+x, b.Min.Y+y))
rgba, ok := src.(*image.RGBA)
if !ok {
rgba = image.NewRGBA(b)
draw.Draw(rgba, b, src, b.Min, draw.Src)
}
dw, dh := h, w
if q == 2 {
dw, dh = w, h
}
dst := image.NewRGBA(image.Rect(0, 0, dw, dh))
for y := 0; y < h; y++ {
row := rgba.Pix[rgba.PixOffset(b.Min.X, b.Min.Y+y):]
for x := 0; x < w; x++ {
var i int
switch q {
case 1: // 90 cw
i = dst.PixOffset(h-1-y, x)
case 2:
i = dst.PixOffset(w-1-x, h-1-y)
default: // 270 cw
i = dst.PixOffset(y, w-1-x)
}
*(*[4]byte)(dst.Pix[i:]) = [4]byte(row[4*x:])
}
}
return dst
Expand Down
41 changes: 41 additions & 0 deletions internal/render/transform_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,47 @@ func TestInvertPreservesAlphaBoundsAndSource(t *testing.T) {
}
}

func TestRotateQuarterTurnsClockwise(t *testing.T) {
src := image.NewNRGBA(image.Rect(1, 1, 3, 4)) // 2 wide, 3 tall, offset bounds
for y := range 3 {
for x := range 2 {
src.SetNRGBA(1+x, 1+y, color.NRGBA{uint8(x), uint8(y), 0, 255})
}
}
at := func(img image.Image, x, y int) color.RGBA { return color.RGBAModel.Convert(img.At(x, y)).(color.RGBA) }
cases := []struct {
q, w, h int
pos func(x, y int) (int, int) // where source pixel (x, y) lands
}{
{1, 3, 2, func(x, y int) (int, int) { return 2 - y, x }},
{2, 2, 3, func(x, y int) (int, int) { return 1 - x, 2 - y }},
{3, 3, 2, func(x, y int) (int, int) { return y, 1 - x }},
}
for _, c := range cases {
got := rotate(src, c.q)
if got.Bounds() != image.Rect(0, 0, c.w, c.h) {
t.Fatalf("q=%d bounds = %v", c.q, got.Bounds())
}
for y := range 3 {
for x := range 2 {
dx, dy := c.pos(x, y)
if want := (color.RGBA{uint8(x), uint8(y), 0, 255}); at(got, dx, dy) != want {
t.Fatalf("q=%d pixel (%d,%d) = %v, want %v", c.q, dx, dy, at(got, dx, dy), want)
}
}
}
}
// RGBA input skips the conversion copy, including a sub-image with offset bounds.
rgba := rotate(src, 2).(*image.RGBA)
sub := rgba.SubImage(image.Rect(1, 1, 2, 3))
if got := rotate(sub, 1); at(got, 1, 0) != at(sub, 1, 1) || at(got, 0, 0) != at(sub, 1, 2) {
t.Fatalf("sub-image rotation: %v %v", at(got, 1, 0), at(got, 0, 0))
}
if back := rotate(rotate(rgba, 1), 1); at(back, 0, 0) != at(src, 1, 1) || at(back, 1, 2) != at(src, 2, 3) {
t.Fatal("four quarter turns did not restore the image")
}
}

func TestHighlightTintsRectanglesOnly(t *testing.T) {
src := image.NewRGBA(image.Rect(0, 0, 4, 4))
for y := 0; y < 4; y++ {
Expand Down
50 changes: 29 additions & 21 deletions internal/ui/ui.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,8 @@ func New(r render.Renderer, srv *server.Server, marks *bookmarks.Store, position
}
}
}
if m.panes[1].book != nil {
m.split = true
if len(paths) > 1 {
m.split = true // a second book that failed to open still shows its error
} else {
m.webtoon = resumeWebtoon
}
Expand Down Expand Up @@ -1366,28 +1366,36 @@ func (m Model) updateOCR(msg ocrMsg) (tea.Model, tea.Cmd) {
m.find.running = false
return m, nil
}
if msg.err == nil {
key := p.book.Path + "\x00" + strconv.Itoa(msg.page)
m.ocrText[key] = msg.text
if strings.Contains(strings.ToLower(msg.text), m.find.term) {
m.find.hits = append(m.find.hits, msg.page)
}
} else {
m.find.skipped++
}
m.find.scanned = msg.page + 1
if m.find.scanned >= m.find.total {
m.find.running = false
m.status = fmt.Sprintf("search done: %d hit(s) for %q · n/p to jump", len(m.find.hits), m.find.term)
if m.find.skipped > 0 {
m.status += fmt.Sprintf(" · %d page(s) not searched", m.find.skipped)
for batch := 1; ; batch++ { // pages handled in this update
if msg.err == nil {
key := p.book.Path + "\x00" + strconv.Itoa(msg.page)
m.ocrText[key] = msg.text
if strings.Contains(strings.ToLower(msg.text), m.find.term) {
m.find.hits = append(m.find.hits, msg.page)
}
} else {
m.find.skipped++
}
m.find.scanned = msg.page + 1
if m.find.scanned >= m.find.total {
m.find.running = false
m.status = fmt.Sprintf("search done: %d hit(s) for %q · n/p to jump", len(m.find.hits), m.find.term)
if m.find.skipped > 0 {
m.status += fmt.Sprintf(" · %d page(s) not searched", m.find.skipped)
}
if len(m.find.hits) > 0 && m.active == m.find.pane {
return m.gotoHit(1)
}
return m, nil
}
if len(m.find.hits) > 0 && m.active == m.find.pane {
return m.gotoHit(1)
// Book text needs no OCR: read it here in batches instead of one message
// per page, and yield between batches so input and progress stay live.
next := msg.page + 1
if batch == 64 || !p.book.IsTextPage(next) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium ui/ui.go:1394

updateOCR processes 65 contiguous text pages before yielding, so a 65th large page can keep input and progress blocked beyond the intended 64-page responsiveness bound. Because the current message is processed before checking batch == 64, change the threshold to 63.

Suggested change
if batch == 64 || !p.book.IsTextPage(next) {
if batch == 63 || !p.book.IsTextPage(next) {
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @internal/ui/ui.go around line 1394:

`updateOCR` processes 65 contiguous text pages before yielding, so a 65th large page can keep input and progress blocked beyond the intended 64-page responsiveness bound. Because the current message is processed before checking `batch == 64`, change the threshold to `63`.

Evidence trail:
Reviewed commit 582df77: `internal/ui/ui.go:1369-1398`, especially lines 1391-1397, shows the zero-based batch loop, current-message processing before the `batch == 64` check, and synchronous text-page loading. `internal/ui/ui.go:1329-1357` shows `ocrPage` returning text-page messages and the OCR command chain.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3f0b979: the counter now starts at 1 and counts pages handled, so an update yields after the 64th page.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Search batches can delay input A text-heavy book can make one update extract and search 65 pages before the reader handles another key or redraws progress. The fixed page count does not limit how long that work takes when pages contain substantial text. Consider yielding after a time or work budget so a dense batch does not stall the interface.

Knowledge Base Used:

Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/ui/ui.go
Line: 1394

Comment:
**Search batches can delay input** A text-heavy book can make one update extract and search 65 pages before the reader handles another key or redraws progress. The fixed page count does not limit how long that work takes when pages contain substantial text. Consider yielding after a time or work budget so a dense batch does not stall the interface.

**Knowledge Base Used:**
- [Terminal UI navigation](https://app.greptile.com/looneytunes/-/custom-context/knowledge-base/hawkff/cbzr/-/docs/terminal-ui-navigation.md)
- [OCR integration](https://app.greptile.com/looneytunes/-/custom-context/knowledge-base/hawkff/cbzr/-/docs/ocr-integration.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Page geometry bounds the work here. A laid-out text page is 900×1200 px at 32 px, about 25 lines of 60 characters, so 64 pages is roughly 150 KB of string scanning per update, well under a millisecond. A clock read per page would add cost without a measurable gain. 3f0b979 corrects the count to 64.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s a fair correction. With batch starting at 1 and yielding when it reaches 64, each update handles at most 64 pages (not 65). Given the fixed 900×1200 layout and the resulting bounded text volume, the batch is small enough that a per-page clock check would add overhead without addressing a practical stall. I’m withdrawing the “Search batches can delay input” finding; no time-based yield is needed here.

return m, m.ocrPage(msg.gen, next)
}
return m, nil
msg = ocrMsg{gen: msg.gen, page: next, book: p.book, text: strings.Join(p.book.PageText(next), "\n")}
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Resolved
}
return m, m.ocrPage(msg.gen, msg.page+1)
}

func (m Model) gotoHit(dir int) (tea.Model, tea.Cmd) {
Expand Down
54 changes: 52 additions & 2 deletions internal/ui/ui_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"os"
"os/exec"
"path/filepath"
"slices"
"strconv"
"strings"
"testing"
Expand Down Expand Up @@ -416,6 +417,49 @@ func TestReplacedBookRejectsRenderAndOCR(t *testing.T) {
}
}

func TestTextSearchScansPagesInBatches(t *testing.T) {
// Each heading starts a page: 70 text pages, the image, then the same 70 again.
var doc strings.Builder
for i := range 70 {
doc.WriteString("<h1>Page " + strconv.Itoa(i) + "</h1>")
if i == 63 || i == 64 {
doc.WriteString("<p>needle</p>")
}
}
m := New(render.NewHalfBlock(), new(server.Server), &bookmarks.Store{}, &progress.Store{}, nil, false, []string{testEPUBPath(t, doc.String())})
if m.panes[0].book == nil {
t.Fatalf("open EPUB: %v", m.panes[0].err)
}
defer m.panes[0].book.Close()
if n := m.panes[0].book.Len(); n != 141 {
t.Fatalf("pages = %d, want 141", n)
}
updated, cmd := m.startSearch("needle")
m = updated.(Model)
step := func(msg tea.Msg, scanned int, hits ...int) tea.Cmd {
t.Helper()
updated, cmd := m.Update(msg)
m = updated.(Model)
if m.find.scanned != scanned || !slices.Equal(m.find.hits, hits) {
t.Fatalf("scanned %d hits %v, want %d %v", m.find.scanned, m.find.hits, scanned, hits)
}
return cmd
}
// Page 0 arrives by message; the same update scans through page 63, then yields.
cmd = step(cmd(), 64, 63)
// Pages 64-69 scan in one update; the image page gets its own OCR command.
cmd = step(cmd(), 70, 63, 64)
if cmd == nil || !m.find.running {
t.Fatal("search stopped at the image page")
}
// The failed image page counts as skipped; the text after it scans in batches again.
cmd = step(ocrMsg{gen: m.find.gen, page: 70, book: m.panes[0].book, err: errors.New("no OCR")}, 134, 63, 64)
step(cmd(), 141, 63, 64, 134, 135)
if m.find.running || m.find.skipped != 1 || m.panes[0].page != 63 {
t.Fatalf("search end: running=%v skipped=%d page=%d", m.find.running, m.find.skipped, m.panes[0].page)
}
}

func TestPaneRemapRejectsOldRender(t *testing.T) {
m := testModel()
m.split = true
Expand Down Expand Up @@ -545,8 +589,14 @@ func TestOpenSplitRendersOriginalPaneAndCancelsSearch(t *testing.T) {
}
}

func testEPUBPath(t *testing.T) string {
// testEPUBPath writes an EPUB whose spine holds a text document, an image
// and the text document again. A body replaces the default text content.
func testEPUBPath(t *testing.T, body ...string) string {
t.Helper()
xhtml := `<p><a href="https://example.com/read?utm_source=book&amp;id=7">Readable text.</a></p><p>` + strings.Repeat("scroll ", 30) + `</p>`
if len(body) > 0 {
xhtml = strings.Join(body, "")
}
path := filepath.Join(t.TempDir(), "text.epub")
f, err := os.Create(path)
if err != nil {
Expand All @@ -556,7 +606,7 @@ func testEPUBPath(t *testing.T) string {
for name, text := range map[string]string{
"META-INF/container.xml": `<container xmlns="urn:oasis:names:tc:opendocument:xmlns:container"><rootfiles><rootfile full-path="book.opf" media-type="application/oebps-package+xml"/></rootfiles></container>`,
"book.opf": `<package xmlns="http://www.idpf.org/2007/opf"><manifest><item id="text" href="text.xhtml" media-type="application/xhtml+xml"/><item id="image" href="page.png" media-type="image/png"/></manifest><spine><itemref idref="text"/><itemref idref="image"/><itemref idref="text"/></spine></package>`,
"text.xhtml": `<html xmlns="http://www.w3.org/1999/xhtml"><body><p><a href="https://example.com/read?utm_source=book&amp;id=7">Readable text.</a></p><p>` + strings.Repeat("scroll ", 30) + `</p></body></html>`,
"text.xhtml": `<html xmlns="http://www.w3.org/1999/xhtml"><body>` + xhtml + `</body></html>`,
} {
w, err := z.Create(name)
if err != nil {
Expand Down
Loading