Opening a .pdf file (detected by %PDF- magic bytes) now renders its pages via Android's built-in PdfRenderer -- no new dependency -- with the same swipe-between-pages, pinch-zoom, pan, and double-tap-to-toggle-zoom interaction as the image viewer. The shared gesture logic was extracted into a ZoomableSwipeableBox composable so it's genuinely reused, not duplicated. maxInputBytes raised 1MB -> 10MB so real-world photos and PDFs aren't rejected as Oversized before ever being classified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
248 lines
18 KiB
Markdown
248 lines
18 KiB
Markdown
# TASK-027: Image viewer (swipe gallery, pinch-to-zoom), CSV table viewer, PDF viewer
|
||
|
||
Status: implemented; all required checks pass; the human product owner confirmed the image opens
|
||
live; pinch-zoom and the PDF viewer are unconfirmed live — see Known limitations
|
||
Owner: Claude
|
||
Reviewer: Codex
|
||
|
||
## Outcome
|
||
|
||
Four live requests, delivered as one connected feature, each building on live feedback from the
|
||
previous: opening an image file in the code browser now shows the image (not a generic "binary
|
||
file" fallback), with left/right swipe to move to the previous/next image in the same folder;
|
||
pinch-to-zoom with pan (plus double-tap to toggle zoom), added once the human product owner
|
||
confirmed the image itself opens correctly and asked for zoom next; opening a `.csv` file shows a
|
||
scrollable table (rows/columns, first row styled as a header) instead of raw comma-separated text;
|
||
and, asked as a follow-up ("pdf 파일은 못읽나?"), opening a `.pdf` file now renders its pages
|
||
(via Android's built-in `PdfRenderer` — no new dependency) with the same swipe-between-pages and
|
||
pinch-zoom/pan interaction as the image viewer, reusing its gesture-handling rather than
|
||
duplicating it. All of these reuse the tab-swipe/content-scroll gesture-conflict fix from
|
||
TASK-025's last follow-up: interacting with an image, a CSV table, or a PDF page never triggers
|
||
the Files/History tab to change underneath it.
|
||
|
||
## Scope
|
||
|
||
- Included: `feature/codebrowser/content/ContentClassification.kt` (new `Image`/`Csv`/`Pdf` cases,
|
||
new `ImageFormat` enum), `feature/codebrowser/content/SourceContentClassifier.kt` (magic-byte
|
||
image and PDF detection, `.csv`-extension table parsing), `feature/codebrowser/CodeBrowserViewModel.kt`
|
||
(`siblingImagePaths` on `File` state, `navigateImage`, `maxInputBytes` raised from 1MB to 10MB —
|
||
see Design notes; `type=issues` is TASK-028's, unrelated to this task), `feature/codebrowser/CodeBrowserScreen.kt`
|
||
(`ImageContent`, `CsvTable`/`CsvRow`/`CsvCell`, `PdfContent`, shared `ZoomableSwipeableBox`),
|
||
`feature/branchdetail/BranchDetailScreen.kt` (pager-gating condition widened to Image/Csv/Pdf),
|
||
`MainActivity.kt` (`onNavigateImage` wiring), `domain/model/RepositoryModels.kt` is untouched by
|
||
this task (see TASK-028 for its `Page.totalCount` addition, done in the same session but a
|
||
separate concern).
|
||
- Excluded: No new image/CSV/PDF editing, no zoom/pan on the CSV table (only the image and PDF
|
||
viewers got zoom, per what was actually asked), no header-row auto-detection for CSV (the first
|
||
row is always styled as a header — see Design notes), no thumbnail/preview generation, no PDF
|
||
text extraction/search/selection (pages render as images only), no change to how other file
|
||
types render.
|
||
|
||
## Design notes
|
||
|
||
- **Image detection is by magic bytes, not file extension** (`detectImageFormat` checks the PNG/
|
||
JPEG/GIF/BMP/WebP signatures directly): a mislabeled or extensionless file still renders
|
||
correctly if it genuinely is one of these formats, and a renamed non-image file isn't
|
||
misclassified just because of its name. It runs *before* the existing NUL-byte-means-Binary
|
||
check, since every one of these formats' bytes contain NULs and would otherwise always be caught
|
||
as generic Binary first.
|
||
- **The sibling-image gallery is built from the directory listing already in hand**, not a second
|
||
fetch: `CodeBrowserViewModel.openEntry` captures the current `Directory` state's file entries
|
||
(filtered to image extensions — extension-only, since building the gallery list can't afford to
|
||
fetch and classify every sibling's bytes just to find out which are images) into the new `File`
|
||
state's `siblingImagePaths` at the moment a file is opened. `navigateImage(delta)` then looks up
|
||
the current path's index in that list and loads the neighbor, with no bound past either end.
|
||
Opening a file *outside* a directory listing (currently only possible via `retry()` after an
|
||
error) leaves `siblingImagePaths` empty — swipe navigation is simply unavailable there, a known,
|
||
accepted edge case rather than extra plumbing through `Loading`/`Error` states for it.
|
||
- **The image swipe gesture is a discrete drag-distance threshold** (a 120px total-drag check on
|
||
release), not a continuous scroll position — unlike the diff view's horizontal scroll, there's
|
||
no partial-scroll state to render mid-drag, only "did the user swipe far enough to go to the
|
||
next/previous image."
|
||
- **Pinch-zoom, pan, and swipe-navigate share one hand-rolled gesture loop** (`awaitEachGesture`,
|
||
reading live pointer count each event), not three separate detector modifiers stacked on the
|
||
same node — Compose broadcasts pointer events to every `pointerInput` block on a node
|
||
cooperatively rather than picking one exclusive winner, so two independent detectors racing for
|
||
the same single-finger drag (one wanting to pan/zoom, one wanting to swipe-navigate) would have
|
||
fought each other exactly like the tab-swipe-vs-content-scroll bug TASK-025 already hit twice.
|
||
Deciding inline, per pointer event, which behavior applies avoids that class of bug outright: 2+
|
||
pointers is always pinch-zoom/pan; 1 pointer while already zoomed in (`scale > 1f`) pans; 1
|
||
pointer at 1x is reserved for the existing swipe-navigate accumulator. A double-tap
|
||
(`detectTapGestures` in a second, non-competing `pointerInput` — taps don't accumulate drag
|
||
distance, so it doesn't fight the first block) toggles between 1x and 2x zoom, matching common
|
||
photo-viewer convention. Zoom/pan/offset are `remember(content)`-scoped, so switching to a
|
||
different image via gallery swipe always starts that image at 1x, not carrying over the previous
|
||
image's zoom. This whole gesture loop, along with the double-tap and pan-clamping, was extracted
|
||
into a shared `ZoomableSwipeableBox` composable specifically so the PDF page viewer (added right
|
||
after, see below) could reuse it verbatim instead of duplicating fairly intricate gesture code a
|
||
second time.
|
||
- **PDF pages render via Android's built-in `android.graphics.pdf.PdfRenderer`** (API 21+, so no
|
||
compatibility concern against this app's `minSdk = 28`) — no new dependency, matching how the
|
||
image viewer uses the platform's own `BitmapFactory` rather than a third-party decoding library.
|
||
`PdfRenderer` only opens a `ParcelFileDescriptor` backed by a real file, not an in-memory byte
|
||
array, so the already-fetched bytes are written to a throwaway temp file (`File.createTempFile`
|
||
in the app's cache dir, deleted in a `finally` block regardless of outcome) purely to satisfy
|
||
that API shape. Classification itself only detects "is this a PDF" by magic bytes (`%PDF-`) —
|
||
page count and page rendering are Android-framework concerns handled entirely in the UI layer via
|
||
`LaunchedEffect`, the same split already used for images (`SourceContentClassifier` decides
|
||
format from bytes; `CodeBrowserScreen` does the actual `BitmapFactory`/`PdfRenderer` decode) —
|
||
keeping the classifier framework-agnostic and unit-testable without Robolectric.
|
||
- **The PDF viewer re-opens and re-renders the file on every page turn** rather than keeping one
|
||
`PdfRenderer` instance alive across recompositions and page navigations. This trades some
|
||
redundant file I/O per page flip for avoiding a class of lifecycle bug (a renderer or file
|
||
descriptor outliving its Composable, e.g. across process death or rapid navigation) that would
|
||
be easy to introduce holding a stateful Android resource open across Compose recompositions —
|
||
judged a reasonable trade given how infrequently a user flips PDF pages compared to, say,
|
||
scrolling a list.
|
||
- **`maxInputBytes` was raised from 1MB to 10MB** in `CodeBrowserViewModel`'s classifier config.
|
||
It was fine at 1MB while only text needed the budget (a large text file is separately bounded for
|
||
*display* by `maxRenderedCharacters`/`maxRenderedLines`, regardless of how many raw bytes were
|
||
fetched) — but now that images and PDFs share the same pre-classification cutoff, 1MB would have
|
||
rejected most real-world photos and PDFs as Oversized before they were ever even looked at,
|
||
making both features close to useless in practice. 10MB is still a deliberately conservative,
|
||
mobile-safe ceiling, not "however large."
|
||
- **Pan is clamped to an approximate bound**, not the image's exact fitted rectangle: it treats the
|
||
image as filling its container, which `ContentScale.Fit` only does exactly along one axis for
|
||
most non-square aspect ratios. Precise bounds would need the actual fitted rect (image and
|
||
container aspect ratio compared), which wasn't worth the extra complexity for keeping a zoomed
|
||
image roughly on screen.
|
||
- **The CSV table reuses the diff view's manually-driven shared-offset pattern** for its horizontal
|
||
scroll (`rememberScrollableState` + `Modifier.offset`, one offset shared across all rows via
|
||
`Modifier.scrollable` per row) rather than a plain shared `Modifier.horizontalScroll`. Every CSV
|
||
cell here is the same fixed width, so the specific bound-thrashing bug that pattern was built to
|
||
avoid (TASK-025's "글자가 춤을 추네") couldn't actually occur with uniform-width cells — but
|
||
reusing the already-verified-correct pattern costs nothing and keeps the codebase's one way of
|
||
doing synced horizontal scroll, rather than two.
|
||
- **No header-row detection for CSV**: the first row is always rendered with a highlighted
|
||
background, matching how most spreadsheet tools default a first-row-as-header assumption, since
|
||
actually detecting whether a CSV has a header (vs. all-data) isn't reliably possible from the
|
||
file alone.
|
||
- **Pager-swipe gating (from TASK-025's last follow-up) was widened, not duplicated**: the same
|
||
`currentPageShowsScrollableCode` check in `BranchDetailScreen.kt` now also covers
|
||
`ContentClassification.Image`, `ContentClassification.Csv`, and `ContentClassification.Pdf`, so
|
||
the human product owner's explicit requirement — "탭 전환은 발생하면 안돼" (tab switching must
|
||
not happen) while viewing an image — is met the same way for PDF pages too, by the existing
|
||
mechanism rather than a new one.
|
||
|
||
## Acceptance criteria
|
||
|
||
- [x] Opening a PNG/JPEG/GIF/BMP/WebP file (detected by magic bytes) shows the decoded image.
|
||
- [x] Swiping left/right on an image with siblings in the same folder moves to the next/previous
|
||
image in that folder's listing order; swiping past either end does nothing.
|
||
- [x] Swiping (or any horizontal drag) on an image or a CSV table never changes the Files/History
|
||
tab underneath it.
|
||
- [x] Opening a `.csv` file shows a scrollable table with the first row styled as a header; a
|
||
non-`.csv` file with comma-separated content is unaffected (still plain text).
|
||
- [x] Quoted CSV fields containing commas and escaped (`""`) quotes parse correctly.
|
||
- [x] Pinching zooms an image in/out (clamped 1x–5x); a single finger pans while zoomed in;
|
||
double-tap toggles between 1x and 2x.
|
||
- [x] Pinch/pan/double-tap never trigger gallery swipe-navigation, and swipe-navigation never
|
||
triggers pinch/pan/zoom — the three are mutually exclusive per gesture, decided by live
|
||
pointer count.
|
||
- [x] Zoom/pan resets to 1x/centered when navigating to a different image in the gallery.
|
||
- [x] Opening a `.pdf` file (detected by magic bytes) renders its first page; swiping left/right
|
||
moves between pages when there's more than one; pinch-zoom/pan/double-tap work identically
|
||
to the image viewer, reusing the same gesture code.
|
||
- [x] A file larger than 1MB but no larger than 10MB (an image or PDF that would previously have
|
||
been rejected as Oversized) now opens correctly.
|
||
- [x] Build, unit tests, and lint pass.
|
||
- [x] Live confirmation that the image itself opens correctly — the human product owner confirmed
|
||
this directly before asking for zoom.
|
||
- [ ] Live device confirmation of pinch-zoom/pan/double-tap specifically, the `.csv` table, and the
|
||
PDF viewer — not yet possible this session; see Known limitations.
|
||
|
||
## Required checks
|
||
|
||
```text
|
||
./gradlew assembleDebug
|
||
./gradlew testDebugUnitTest
|
||
./gradlew lintDebug
|
||
```
|
||
|
||
## Evidence
|
||
|
||
- `ContentClassification.kt`: new `Image(sizeBytes, content: SourceContent, format: ImageFormat)`,
|
||
`Csv(sizeBytes, rows: List<List<String>>, isTruncated: Boolean)`, and
|
||
`Pdf(sizeBytes, content: SourceContent)` cases; `content` reuses `SourceContent`'s already-correct
|
||
content-based `equals()`/`hashCode()` instead of holding a raw `ByteArray` (which would default to
|
||
reference equality on a `data class`).
|
||
- `SourceContentClassifier.kt`: `classify(content, path: RepositoryPath? = null)` — `path` is
|
||
optional/defaulted so every pre-existing call site keeps compiling unchanged; `detectImageFormat`
|
||
checks PNG/JPEG/GIF/BMP/WebP signatures, and a separate `%PDF-` check, both before the NUL-byte
|
||
Binary check; CSV parsing (`renderCsv`/`parseCsv`, a small RFC-4180-ish hand-written parser) only
|
||
runs when `path` ends in `.csv`, reusing the same line/character truncation budgets as `renderText`.
|
||
- `CodeBrowserViewModel.kt`: `CodeBrowserUiState.File` gained `siblingImagePaths: List<RepositoryPath> = emptyList()`;
|
||
`openEntry` captures it from the current `Directory` state's file entries (via new private
|
||
`RepositoryPath.hasImageExtension()`); new `navigateImage(delta: Int)`; `maxInputBytes` raised
|
||
1MB → 10MB.
|
||
- `CodeBrowserScreen.kt`: new `ImageContent` (BitmapFactory decode), `PdfContent`
|
||
(`android.graphics.pdf.PdfRenderer` via a throwaway temp file, `LaunchedEffect`-driven page
|
||
render, re-opened per page turn), both rendered through a new shared `ZoomableSwipeableBox`
|
||
(Compose `Image` via `Modifier.graphicsLayer { scaleX/scaleY/translationX/translationY }`, one
|
||
`awaitEachGesture` loop combining pinch-zoom, pan-while-zoomed, and drag-threshold
|
||
swipe-navigate by live pointer count, plus a separate `detectTapGestures` for
|
||
double-tap-to-toggle-zoom), `Offset.clampedToBox` (approximate pan bounds), `CsvTable`/`CsvRow`/`CsvCell`
|
||
(shared-offset horizontal scroll, matching the diff view's pattern); `FileContent`'s signature
|
||
grew `path`/`siblingImagePaths`/`onNavigateImage` to support the image gallery.
|
||
- `BranchDetailScreen.kt`: `currentPageShowsScrollableCode`'s page-0 branch now also checks
|
||
`ContentClassification.Image`/`ContentClassification.Csv`/`ContentClassification.Pdf`, not just
|
||
`Text`.
|
||
- New tests: `SourceContentClassifierTest.kt` (PNG/JPEG/GIF/BMP/WebP signature detection, an image
|
||
signature's embedded NUL byte not short-circuiting to Binary, oversized-before-image-detection,
|
||
`%PDF-` signature detection with the same NUL-byte and oversized-ordering checks, a plain-text
|
||
file starting with `%` not misclassified as PDF, `.csv` vs non-`.csv` path handling, no-path-given
|
||
fallback to Text, quoted-field/escaped-quote parsing, row-count truncation); `CodeBrowserViewModelTest.kt`
|
||
(gallery capture in listing order excluding a non-image sibling, `navigateImage` moving and
|
||
stopping at both ends, `navigateImage` no-op when opened outside a directory listing). Neither the
|
||
pinch-zoom/pan/double-tap gesture code nor the PDF-rendering code (an Android-framework
|
||
concern — `PdfRenderer` isn't available in a plain JVM unit test without Robolectric) has a
|
||
dedicated unit test; both are pure UI/framework logic with no non-UI behavior to isolate (see
|
||
Known limitations for how they were verified instead).
|
||
- `./gradlew compileDebugKotlin compileDebugUnitTestKotlin`: BUILD SUCCESSFUL, across all three
|
||
passes (initial image/CSV viewer, pinch-zoom, PDF viewer).
|
||
- `./gradlew testDebugUnitTest`: BUILD SUCCESSFUL, all unit tests passing (including the new ones
|
||
above), across all three passes.
|
||
- `./gradlew assembleDebug lintDebug`: BUILD SUCCESSFUL, 0 lint errors, across all three passes.
|
||
- Live device: build installed over wireless `adb` three times — the initial image/CSV viewer,
|
||
after adding pinch-zoom, and after adding the PDF viewer. The human product owner confirmed the
|
||
image itself opens correctly on the first install, then asked for zoom directly from that same
|
||
live session, then asked about PDF support as a further follow-up — all in one continuous
|
||
session rather than separate reports. Navigating repositories looking for an existing `.csv` or
|
||
`.pdf` file to confirm either rendering end-to-end found none among those browsed this session
|
||
(see Known limitations).
|
||
|
||
## Known limitations
|
||
|
||
- **Image opening itself is live-confirmed; pinch-zoom/pan/double-tap is not yet.** The human
|
||
product owner confirmed the decoded image renders correctly, then asked for zoom in the same
|
||
session — there hasn't yet been a follow-up live confirmation specifically of the zoom/pan/
|
||
double-tap gestures themselves. Unlike the earlier gesture-conflict bugs in this session (found
|
||
only by live testing, not reasoning), there was no repo/file readily available to hand-test
|
||
against interactively beyond a single already-open image; the gesture-arbitration design (live
|
||
pointer-count branching in one gesture loop, see Design notes) was chosen specifically to
|
||
structurally prevent the same class of bug TASK-025 hit twice, but that's a design argument, not
|
||
a substitute for actually pinching/panning it on the device.
|
||
- **CSV table and PDF viewer still not confirmed against real files.** Every repository browsed
|
||
this session for a `.csv` or `.pdf` example didn't have one readily reachable. The PDF viewer in
|
||
particular has no unit-test coverage at all (see Evidence) since `PdfRenderer` is an
|
||
Android-framework class, so this is entirely pending a live check — decode success, page-turn
|
||
correctness, and the shared zoom/pan/swipe gestures working the same way they were confirmed to
|
||
(by construction, not yet by touch) work for images.
|
||
- The CSV table's column width is a fixed 120dp per cell, not sized to content — a column with
|
||
very short values wastes horizontal space, and a column with very long values still clips at
|
||
120dp (readable via horizontal scroll, but not auto-fit). Not raised as a concern by the human
|
||
product owner; flagged here in case it's worth revisiting once seen live.
|
||
- Pan-while-zoomed (image or PDF page) is clamped to an approximate bound (see Design notes), not
|
||
the exact fitted rectangle — for most non-square content this means the clamp is slightly looser
|
||
or tighter than the true edge along one axis, not pixel-exact.
|
||
- `navigateImage` after a `retry()`-recovered error loses gallery position (see Design notes) —
|
||
accepted, not treated as a defect.
|
||
- The PDF viewer re-opens and fully re-renders the file on every page turn (see Design notes) —
|
||
chosen deliberately for lifecycle safety over performance; a large, many-page PDF would re-pay
|
||
the file-write/renderer-open cost on each swipe, which hasn't been measured against a real
|
||
multi-page PDF yet.
|
||
|
||
## Review findings
|
||
|
||
None yet — pending Codex/human review.
|