feat: add transparent halfcell - #311
Merged
Merged
Conversation
Reviewer's GuideAdds a monkey-patched transparent-aware halfcell image implementation and tests to ensure transparent pixels render correctly in the terminal. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (13)
📝 WalkthroughWalkthroughChangesTransparent half-cell rendering
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Monkey-patching
halfcell.Image.__init__and__rich_console__directly could make this fragile against upstream changes; consider delegating to the original implementations where possible or wrapping/conditionally extending behavior instead of fully replacing it. - The custom
__rich_console__implementation always emits a trailing newlineSegment("\n")after the last row; if this differs from the originalhalfcell.Imagebehavior, you may want to align it or make the extra newline configurable to avoid subtle layout issues when composing renderables.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Monkey-patching `halfcell.Image.__init__` and `__rich_console__` directly could make this fragile against upstream changes; consider delegating to the original implementations where possible or wrapping/conditionally extending behavior instead of fully replacing it.
- The custom `__rich_console__` implementation always emits a trailing newline `Segment("\n")` after the last row; if this differs from the original `halfcell.Image` behavior, you may want to align it or make the extra newline configurable to avoid subtle layout issues when composing renderables.
## Individual Comments
### Comment 1
<location path="tests/test_transparent_halfcell_image.py" line_range="31-36" />
<code_context>
+ assert segments[0].style is None
+
+
+def test_render_single_transparent_halfcell_without_background() -> None:
+ segments = _render_pixels([(0, 0, 0, 0), (255, 0, 0, 255)])
+
+ assert segments[0].text == "▄"
+ assert segments[0].style is not None
+ assert segments[0].style.bgcolor is None
</code_context>
<issue_to_address>
**suggestion (testing):** Add a complementary test for the case where the upper pixel is opaque and the lower pixel is fully transparent.
We already cover the `(upper alpha == 0, lower alpha > 0)` case via `test_render_single_transparent_halfcell_without_background`, which hits the "▄" branch. Please add the complementary `(upper alpha > 0, lower alpha == 0)` test to cover the "▀" branch without background—for example with pixel data `[(255, 0, 0, 255), (0, 0, 0, 0)]`, asserting the character is "▀", `style` is not `None`, and `bgcolor` is `None`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+31
to
+36
| def test_render_single_transparent_halfcell_without_background() -> None: | ||
| segments = _render_pixels([(0, 0, 0, 0), (255, 0, 0, 255)]) | ||
|
|
||
| assert segments[0].text == "▄" | ||
| assert segments[0].style is not None | ||
| assert segments[0].style.bgcolor is None |
There was a problem hiding this comment.
suggestion (testing): Add a complementary test for the case where the upper pixel is opaque and the lower pixel is fully transparent.
We already cover the (upper alpha == 0, lower alpha > 0) case via test_render_single_transparent_halfcell_without_background, which hits the "▄" branch. Please add the complementary (upper alpha > 0, lower alpha == 0) test to cover the "▀" branch without background—for example with pixel data [(255, 0, 0, 255), (0, 0, 0, 0)], asserting the character is "▀", style is not None, and bgcolor is None.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_transparent_halfcell_image.py (1)
31-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the opaque-upper branch too.
This test only exercises
upper[3] == 0. Add the inverse pixel order and assert"▀"with no background colour, so the separate branch at Line 61 remains protected.Proposed test
+def test_render_opaque_upper_halfcell_without_background() -> None: + segments = _render_pixels([(255, 0, 0, 255), (0, 0, 0, 0)]) + + assert segments[0].text == "▀" + assert segments[0].style is not None + assert segments[0].style.bgcolor is None🤖 Prompt for 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. In `@tests/test_transparent_halfcell_image.py` around lines 31 - 36, Extend test_render_single_transparent_halfcell_without_background with the inverse pixel order, using an opaque upper pixel and transparent lower pixel, and assert the rendered text is "▀" with a non-None style whose bgcolor remains None, covering the opaque-upper branch.
🤖 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.
Nitpick comments:
In `@tests/test_transparent_halfcell_image.py`:
- Around line 31-36: Extend
test_render_single_transparent_halfcell_without_background with the inverse
pixel order, using an opaque upper pixel and transparent lower pixel, and assert
the rendered text is "▀" with a non-None style whose bgcolor remains None,
covering the opaque-upper branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 62c93537-337f-4070-8dc3-3c8a79d1ccc1
📒 Files selected for processing (2)
src/rovr/monkey_patches/_classes.pytests/test_transparent_halfcell_image.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Sourcery review
- GitHub Check: format_and_alert
- GitHub Check: Test on (windows-arm, Python 3.13)
- GitHub Check: Test on (windows-arm, Python 3.14)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python 3.12 or newer is required.
Use double quotes, four-space indentation, and Ruff as the formatter; there is no strict line-length limit.
Use absolute imports only; do not use relative imports.
All functions must have type annotations; use ty for type checking rather than mypy.
Do not useif TYPE_CHECKINGblocks.
UseCamelCasefor classes,snake_casefor functions and variables,UPPER_CASEfor constants, and a leading underscore for private names.
Use specific exceptions instead of bareexcept:; define custom exceptions inrovr/classes/exceptions.py. Usecontextlib.suppressfor expected errors.
Files:
tests/test_transparent_halfcell_image.pysrc/rovr/monkey_patches/_classes.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Tests use pytest; test files follow the
test_*.pynaming convention and shared fixtures belong inconftest.py.
Files:
tests/test_transparent_halfcell_image.py
src/rovr/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/rovr/**/*.py: For custom Textual keybindings, inherit from widgets withinherit_bindings=False.
Use@on(EventType)decorators oron_eventmethods for Textual event handling.
Use@workfor asynchronous background work, addingthread=Truefor blocking tasks.
When modifying widgets from a thread, usecall_from_thread,call_next,call_later, orcall_after_refresh; reactive variables and properties are exempt.
Do not define functions in the same file asmultiprocessing.ProcessPoolExecutorormultiprocessing.Process; place worker functions in a separate module to avoid configuration reload and stdout corruption.
Textual event handlers do not need to callsuper(); callprevent_default()when an event should not bubble.
Usingsubprocess.run(shell=True)is allowed because command execution is controlled by the user through configuration.
Files:
src/rovr/monkey_patches/_classes.py
🧠 Learnings (8)
📚 Learning: 2025-12-11T03:10:54.344Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 167
File: src/rovr/footer/process_container.py:346-346
Timestamp: 2025-12-11T03:10:54.344Z
Learning: In Python/Textual code, when calling checker_wrapper() or any other method that starts workers from within a work(thread=True) decorated method, do not wrap the call in call_from_thread. Starting workers is thread-safe in Textual and the work decorator handles threading. Only UI updates (e.g., DOM modifications) from worker threads require call_from_thread. Apply this guideline to Python files across the codebase where worker-start calls occur; do not wrap such calls with call_from_thread.
Applied to files:
tests/test_transparent_halfcell_image.pysrc/rovr/monkey_patches/_classes.py
📚 Learning: 2026-01-24T02:10:57.940Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 203
File: src/rovr/first_launch.py:273-295
Timestamp: 2026-01-24T02:10:57.940Z
Learning: In the rovr codebase (Python/Textual), App.notify() is thread-safe and may be called directly from worker threads decorated with work(thread=True) without wrapping in call_from_thread(). Only DOM modifications require call_from_thread from worker threads. Apply this guideline to Python files across the codebase (not just a single file) and avoid unnecessary call_from_thread() wrappers when invoking App.notify() from worker threads.
Applied to files:
tests/test_transparent_halfcell_image.pysrc/rovr/monkey_patches/_classes.py
📚 Learning: 2026-01-30T13:54:15.110Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 207
File: src/rovr/first_launch.py:96-107
Timestamp: 2026-01-30T13:54:15.110Z
Learning: In Python projects using the Textual framework, async workers decorated with work are automatically cancelled when the parent screen/widget is dismissed or unmounted. Do not add manual cancellation in cleanup/dismiss handlers for these workers; rely on Textual to cancel them. Verify this behavior against the project’s Textual version before relying on it.
Applied to files:
tests/test_transparent_halfcell_image.pysrc/rovr/monkey_patches/_classes.py
📚 Learning: 2026-02-27T09:15:49.243Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 229
File: src/rovr/functions/system_clipboard.py:143-143
Timestamp: 2026-02-27T09:15:49.243Z
Learning: In Python files that display error messages in toast/notification UI (e.g., ClipboardError and similar exceptions), escape unescaped square brackets in the message so the toast system does not interpret them as markup. Specifically render the content with double backslashes before brackets, e.g. "\\[{type(exc).__name__}]" in the toast description. This guideline applies to all Python files that format and show exception details in toasts across the codebase.
Applied to files:
tests/test_transparent_halfcell_image.pysrc/rovr/monkey_patches/_classes.py
📚 Learning: 2026-03-01T10:35:17.682Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 233
File: tests/test_functions_path.py:21-21
Timestamp: 2026-03-01T10:35:17.682Z
Learning: In Python test files (using the Textual framework), use an assertion like Static(id=val, classes=val) to verify that the value contains only characters valid for Textual IDs and classes. The test should confirm initialization succeeds without raising BadIdentifier (do not test attribute storage). This pattern is applicable to tests that validate Textual-compatible identifier strings returned by utilities (e.g., compress/decompress). Apply this guideline to test files under the tests/ directory (e.g., tests/**/*.py) where relevant.
Applied to files:
tests/test_transparent_halfcell_image.py
📚 Learning: 2025-12-11T03:11:24.387Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 167
File: src/rovr/screens/fd_search.py:205-209
Timestamp: 2025-12-11T03:11:24.387Z
Learning: In src/rovr/screens/fd_search.py, FileSearch.FILTER_TYPES should be implemented as a class attribute (not an instance attribute) to persist filter state across all modal instances within the same session. This enables remember/restore of filter settings when the modal is dismissed and reopened. Ensure this behavior is intentional, document the session lifecycle implications, and include tests verifying persistence across modal reopenings, as well as proper reset or isolation when a new session starts. If applicable, consider thread-safety and potential memory growth due to long-lived state.
Applied to files:
src/rovr/monkey_patches/_classes.py
📚 Learning: 2025-12-11T03:11:27.833Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 167
File: src/rovr/screens/fd_search.py:205-209
Timestamp: 2025-12-11T03:11:27.833Z
Learning: In src/rovr/screens/fd_search.py, document that the FileSearch class uses a class-level FILTER_TYPES dict to persist filter settings across multiple modal invocations within a single application session. Ensure this pattern is intentional by clarifying the lifecycle of the shared state (lifetime of the process/session) and the implications of mutable shared state (thread-safety, potential memory retention). Consider alternatives (per-instance state, dedicated session storage, explicit reset method) and add tests to verify persistence, correctness after modal reopenings, and safe reset behavior. Also mention any framework or runtime constraints that affect shared-class state.
Applied to files:
src/rovr/monkey_patches/_classes.py
📚 Learning: 2026-01-03T14:17:18.132Z
Learnt from: lazysegtree
Repo: NSPC911/rovr PR: 184
File: src/rovr/core/preview_container.py:280-283
Timestamp: 2026-01-03T14:17:18.132Z
Learning: In src/rovr/core/preview_container.py, mypy may not recognize that pdfinfo_from_path's poppler_path parameter accepts None even though the library signature is Optional[Union[str, pathlib.PurePath]]. If you encounter a type error, explicitly cast the argument to Optional[Union[str, pathlib.PurePath]] before passing it (e.g., from typing import cast; pdfinfo_from_path(path, poppler_path=cast(Optional[Union[str, pathlib.PurePath]], None)) or cast the argument to the expected type). This documents the typing limitation and provides a repeatable workaround for similar cases.
Applied to files:
src/rovr/monkey_patches/_classes.py
🔇 Additional comments (2)
src/rovr/monkey_patches/_classes.py (1)
1-15: LGTM!Also applies to: 28-42, 45-71
tests/test_transparent_halfcell_image.py (1)
1-29: LGTM!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
monkey patching with reference to my other pr lnqs/textual-image#101
by submitting this pull request, i agree that
poe checkto check for any style issues and fixed thempoe testif applicable) to make sure my changes do not break anythingSummary by Sourcery
Add monkey patches to support transparent halfcell image rendering and cover the behavior with tests.
New Features:
Tests:
Summary by CodeRabbit