feat: OCR language via config file / env var, and tessdata-dir selection - #9
Conversation
FR-006 completion plus the tessdata half of Phase 7's accuracy options:
- New quickview-core config module: ~/.config/quickview/config.toml
([ocr] lang / tessdata_dir / max_dimension), loaded once in the invoking
process. Precedence lang: --lang > QUICKVIEW_LANG > config > eng;
tessdata: --tessdata-dir > config. Blank values are unset; unknown keys
are treated as typos (whole file warned about and ignored, never fatal —
NFR-004). max_dimension is accepted but only consumed by the upcoming
downscale guardrail.
- OcrOptions {lang, tessdata_dir} replaces the bare lang string end to
end (LaunchOptions, ipc argv codec, ViewerController, cache key,
tesseract runner). Arg construction extracted into a pure, tested
tesseract_args(); --tessdata-dir must precede the tsv/quiet config
names.
- Cache key gains the tessdata dir with a presence marker per ADR-0009
(None != Some("")); existing entries become a one-time miss and
repopulate. ProjectDirs triple now lives in one place
(config::project_dirs), shared with cache_dir().
- templates/config.example.toml documents the file, including pointing
tessdata_dir at a tessdata_fast/tessdata_best checkout.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds a config.toml-based OCR configuration system with CLI/env/config precedence resolution, extends OcrOptions with a tessdata_dir field, and threads these options through tesseract invocation, cache key generation, IPC argv encoding, UI controller state, and CLI resolution, alongside documentation and example config updates. ChangesOCR configuration and options plumbing
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as main.rs
participant ConfigModule as config.rs
participant LaunchOptions
participant ViewerController
participant Cache as cache.rs
participant Tesseract as tesseract.rs
CLI->>ConfigModule: load(config_path)
ConfigModule-->>CLI: Config
CLI->>CLI: resolve_ocr_options(cli, env, config)
CLI->>LaunchOptions: ocr: OcrOptions{lang, tessdata_dir}
LaunchOptions->>ViewerController: set_ocr_options(ocr)
ViewerController->>Cache: ocr_cache_path(file, &ocr_opts)
ViewerController->>Tesseract: run_tesseract_tsv(&path, &ocr_opts)
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18d936c36d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let env_lang = std::env::var("QUICKVIEW_LANG").ok(); | ||
| quickview_core::ocr::tesseract::OcrOptions { | ||
| lang: config::resolve_lang(cli_lang.as_deref(), env_lang.as_deref(), &cfg), | ||
| tessdata_dir: cli_tessdata_dir.or_else(|| cfg.ocr.tessdata_dir.clone()), |
There was a problem hiding this comment.
When --tessdata-dir or the config value is relative and a QuickView primary instance is already running, this stores and forwards the relative path unchanged; OCR later runs in the primary process, so the path is interpreted relative to that process's cwd rather than the invoking process's cwd. The image path is canonicalized at the app boundary for this same single-instance issue, but the tessdata path is not, so invocations from a different directory can make Tesseract look in the wrong tessdata directory and fail OCR despite a valid CLI/config path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — same single-instance cwd trap the image path was canonicalized for in PR #4, and it also made the cache key spelling-dependent for one directory. Fixed in b70f343: the tessdata dir is canonicalized (or made absolute when missing, via std::path::absolute) in the invoking process before it crosses the instance boundary, with a unit test covering existing/missing/already-absolute cases.
A relative --tessdata-dir (or config value) forwarded to a running primary instance resolved against the primary's cwd, not the invoking process's — same single-instance cwd trap the image path is canonicalized for. Canonicalize (or absolutize when missing) before the value crosses the instance boundary; also makes the cache key stable across spellings of the same directory. Found by Codex on PR #9.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/quickview-core/src/config.rs`:
- Around line 40-49: The OcrSection config currently treats blank tessdata_dir
values as set, so empty strings become Some(PathBuf::new()) instead of unset.
Update the config normalization in OcrSection to apply the same blank-as-unset
handling already used for lang, so an empty [ocr] tessdata_dir falls back to
None rather than being passed through to the OCR CLI. Locate the fix in the
OcrSection parsing/normalization logic in config.rs and ensure tessdata_dir is
cleared when the input string is blank.
In `@crates/quickview/src/main.rs`:
- Around line 72-92: The `resolve_ocr_options` function treats `tessdata_dir`
differently from `lang`, so blank CLI/config values are not normalized to unset.
Update the `tessdata_dir` resolution in `resolve_ocr_options` to trim and ignore
empty values before falling back to the next source, matching the behavior of
`config::resolve_lang`. Keep the change localized to the `OcrOptions`
construction path so empty `--tessdata-dir` or empty config values do not become
`Some(PathBuf::new())`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c27feac4-0b58-4db5-9b06-65ea1e41facc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.claude/CLAUDE.mdadrs/ADR-0009-Caching.mdcrates/quickview-core/Cargo.tomlcrates/quickview-core/src/cache.rscrates/quickview-core/src/config.rscrates/quickview-core/src/lib.rscrates/quickview-core/src/ocr/tesseract.rscrates/quickview-ui/src/ipc.rscrates/quickview-ui/src/lib.rscrates/quickview-ui/src/windows/full_viewer.rscrates/quickview-ui/src/windows/quick_preview.rscrates/quickview-ui/src/windows/shared.rscrates/quickview/Cargo.tomlcrates/quickview/src/main.rsdocs/PHASED_PLAN.mdtemplates/config.example.toml
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: build-test
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use rustfmt and clippy for code formatting and linting, as tracked in rust-toolchain.toml
Run clippy with all targets and features enabled with -D warnings flag to treat warnings as errors
Files:
crates/quickview-core/src/ocr/tesseract.rscrates/quickview-ui/src/windows/quick_preview.rscrates/quickview-core/src/lib.rscrates/quickview-ui/src/windows/full_viewer.rscrates/quickview-ui/src/lib.rscrates/quickview/src/main.rscrates/quickview-core/src/config.rscrates/quickview-ui/src/ipc.rscrates/quickview-ui/src/windows/shared.rscrates/quickview-core/src/cache.rs
🔇 Additional comments (16)
crates/quickview-core/src/lib.rs (1)
6-6: LGTM!crates/quickview-core/src/config.rs (1)
1-37: LGTM!Also applies to: 51-64, 80-151
crates/quickview-core/src/ocr/tesseract.rs (1)
1-92: LGTM!crates/quickview-core/src/cache.rs (1)
14-21: LGTM!Also applies to: 23-59, 146-231, 232-286
crates/quickview-core/Cargo.toml (1)
17-17: 🚀 Performance & ScalabilityNo action needed for
toml0.8. The crate is only used fortoml::from_stron owned config types, so the current pin is fine unless you want to take on the 1.x API changes.crates/quickview-ui/src/ipc.rs (1)
9-10: LGTM!Also applies to: 29-30, 41-56, 72-73, 83-86, 99-102, 135-148
crates/quickview-ui/src/lib.rs (1)
17-17: LGTM!Also applies to: 30-32, 105-106
crates/quickview-ui/src/windows/shared.rs (1)
12-12: LGTM!Also applies to: 42-42, 53-73, 87-93, 223-223, 244-249
crates/quickview-ui/src/windows/full_viewer.rs (1)
23-23: LGTM!crates/quickview-ui/src/windows/quick_preview.rs (1)
30-30: LGTM!crates/quickview/Cargo.toml (1)
19-19: LGTM!crates/quickview/src/main.rs (1)
15-32: LGTM!.claude/CLAUDE.md (1)
60-63: LGTM!adrs/ADR-0009-Caching.md (1)
55-59: LGTM!docs/PHASED_PLAN.md (1)
175-181: LGTM!templates/config.example.toml (1)
1-24: LGTM!
An empty --tessdata-dir or config tessdata_dir = "" flowed through as Some(PathBuf::new()), reaching tesseract as --tessdata-dir "" and hashing as a distinct cache key — inconsistent with lang's blank-as-unset rule. Filter blanks at each precedence level before absolutizing. Found by CodeRabbit on PR #9.
1 participant
Completes FR-006 and the tessdata half of Phase 7's OCR accuracy options.
Config module (
quickview-core/src/config.rs)~/.config/quickview/config.toml, schema[ocr] lang / tessdata_dir / max_dimension(max_dimensionaccepted now, consumed by the upcoming downscale guardrail so configs don't error).--lang>QUICKVIEW_LANG> config >eng; tessdata:--tessdata-dir> config. Blank values are unset at each level.ipccodec grew an optional--tessdata-dir=token — same single-token framing as before).OcrOptions
OcrOptions { lang, tessdata_dir }replaces the bare lang string end-to-end (LaunchOptions → ipc → ViewerController → cache key → tesseract runner). Arg construction is now a pure, unit-testedtesseract_args();--tessdata-diris emitted before thetsv quietconfig names, which tesseract requires. Tessdata is a plain directory path rather than a fast/best enum — those sets have no conventional install location, users clone them anywhere (documented intemplates/config.example.toml).Cache key (ADR-0009)
The tessdata dir joins the blake3 key with a presence marker (
None≠Some("")). One-time full cache invalidation: existing entries miss once and repopulate (no-eviction v1, nothing to clean).ProjectDirstriple deduplicated intoconfig::project_dirs(), shared withcache_dir().Testing
tesseract_args±tessdata, cache-key tessdata variants, ipc round-trip with/without tessdata-dir); clippy-D warningsclean.QUICKVIEW_LANG=deureaches tesseract (fails cleanly withouttesseract-data-deu, image still displays);--tessdata-dir /nonexistentditto (/nonexistent/eng.traineddatain the error proves the flag flows); second open after the key-format change is a cache hit again.