fix(slides): enhance text overflow and occlusion detection in xml lint - #2152
fix(slides): enhance text overflow and occlusion detection in xml lint#2152ethan-zhx wants to merge 0 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe slide XML overlap linter adds text measurement, wrapping, overflow, occlusion, container, rotation, stacking-order, and issue-deduplication logic. Regression tests cover Unicode sizing, spacing, auto-fit growth, geometry, z-order, and error severity. ChangesSlide overlap and overflow linting
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@5a1214760e8537da8a8aa300f6196a0f4b04c488🧩 Skill updatenpx skills add larksuite/cli#fix/text_over_flow -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/lark-slides/scripts/xml_text_overlap_lint.py (1)
1954-1972: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAnchor the unclamped auto-fit box to the authored top.
shape-auto-fitnow keepsraw_heighteven when it exceedscontent_height. The vertical anchoring below still runs. With the defaultverticalAlignof"middle"(set inextract_elements),(content_height - visual_height) / 2is negative, so the box is shifted up by half the excess. With"bottom"it is shifted up by the full excess.The renderer grows a
shape-auto-fitbox downward from the authored top, as the docstring ondetect_auto_fit_growth_collisionsstates. The current geometry therefore places the grown region too high:
detect_auto_fit_growth_collisionsmeasuresglyph["y"] + glyph["height"] - authored_bottom, which reports about half of the real downward growth for middle-aligned runs.- The box also extends above the authored top, which can create overlap reports against content sitting above the run.
Skip the vertical re-anchoring when the estimated height exceeds the content box.
🐛 Suggested fix
y = element["y"] + padding_top - if element.get("verticalAlign") == "middle": - y += (content_height - visual_height) / 2 - elif element.get("verticalAlign") == "bottom": - y += content_height - visual_height + # A grown shape-auto-fit box extends downward from the authored top, so alignment + # offsets only apply while the estimated block still fits the content box. + if visual_height <= content_height: + if element.get("verticalAlign") == "middle": + y += (content_height - visual_height) / 2 + elif element.get("verticalAlign") == "bottom": + y += content_height - visual_height🤖 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 `@skills/lark-slides/scripts/xml_text_overlap_lint.py` around lines 1954 - 1972, Update the vertical alignment logic in the text geometry calculation to skip middle/bottom re-anchoring when shape-auto-fit uses a visual_height larger than content_height. Keep the authored top as the y origin for the grown box, while preserving existing vertical alignment behavior for non-grown boxes and auto-fit cases that do not exceed the content height.
🧹 Nitpick comments (3)
skills/lark-slides/scripts/xml_text_overlap_lint.py (3)
1204-1213: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrecompute the glyph boxes once per text element.
estimate_text_visual_bboxruns per-character width estimation and wrapped line counting. This loop calls it once for every (shape, text) pair, anddetect_auto_fit_growth_collisionsrepeats the same pattern forother_bbox. Build the glyph boxes once before the shape loop.♻️ Suggested change
+ glyph_boxes = { + element["id"]: estimate_text_visual_bbox(element) for element in text_elements + } for shape in covering_shapes: for text_element in text_elements: if not is_drawn_in_front_of(shape, text_element): continue - glyph = estimate_text_visual_bbox(text_element) + glyph = glyph_boxes[text_element["id"]] if glyph is None: continue🤖 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 `@skills/lark-slides/scripts/xml_text_overlap_lint.py` around lines 1204 - 1213, Precompute each text element’s glyph bounding box once before iterating through covering_shapes, then reuse the cached result when checking shape overlap. Apply the same caching approach to the repeated other_bbox estimation in detect_auto_fit_growth_collisions, while preserving the existing handling for None boxes and overlap thresholds.
1102-1102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an area constant for the area gate.
Line 1089 compares
growth(pixels) againstCONTAINER_OVERFLOW_MIN_PX, which is correct. Line 1102 comparesintersection_area(...)(square pixels) against the same constant. The units differ. Both constants are 4.0 today, so behavior is unchanged, but a future tuning of the linear slack would silently move the area gate.♻️ Suggested change
- if intersection_area(grown_region, other_bbox) <= CONTAINER_OVERFLOW_MIN_PX: + if intersection_area(grown_region, other_bbox) <= SHAPE_TEXT_OCCLUSION_MIN_AREA: continue🤖 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 `@skills/lark-slides/scripts/xml_text_overlap_lint.py` at line 1102, Update the area comparison in the overlap-checking logic around intersection_area to use a dedicated area-threshold constant rather than CONTAINER_OVERFLOW_MIN_PX. Keep CONTAINER_OVERFLOW_MIN_PX for the growth pixel comparison and initialize the new area constant to preserve the current behavior.
1016-1050: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared occluder-scan helper.
detect_chart_text_occlusionsduplicatesdetect_table_text_occlusionsexcept for the element kind, issue code, message, and hint. A single parameterized helper keeps the two detectors in sync when the text filter changes.♻️ Suggested consolidation
def detect_element_text_occlusions( elements: list[dict[str, Any]], kind: str, code: str, noun: str, hint: str ) -> list[dict[str, Any]]: issues: list[dict[str, Any]] = [] text_elements = [ element for element in elements if is_text_element(element) and has_text_content(element) and not is_ghost_text(element) ] occluders = [element for element in elements if element["kind"] == kind and element["alpha"] > 0] for text_element in text_elements: if is_decorative_text(text_element): continue glyph_bbox = estimate_text_visual_bbox(text_element) if glyph_bbox is None: continue for occluder in occluders: if not intersects(occluder, glyph_bbox): continue issues.append({ "level": "error", "code": code, "elements": [occluder["id"], text_element["id"]], "message": f'text shape {text_element["id"]} overlaps {noun} {occluder["id"]}', "hint": hint, }) return issues🤖 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 `@skills/lark-slides/scripts/xml_text_overlap_lint.py` around lines 1016 - 1050, Extract the shared scanning logic from detect_table_text_occlusions and detect_chart_text_occlusions into a parameterized detect_element_text_occlusions helper. Pass the occluder kind, issue code, noun, and hint for each detector, while preserving the existing filtering, intersection checks, and issue output.
🤖 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 `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 1152-1163: Update the container-overflow handling around the
overflow gate to align with the documented scope of CONTAINER_OVERFLOW_MIN_PX:
either apply that tolerance to this authored text-frame check, or revise the
constant’s comment to explicitly state that it only gates
detect_auto_fit_growth_collisions. Preserve the existing tests’ behavior that
reports a 4px frame overhang.
- Around line 2602-2611: Introduce one element-id-keyed glyph-bbox cache scoped
to lint_slide and pass it through the detectors, so each element’s
estimate_text_visual_bbox result is reused. In
skills/lark-slides/scripts/xml_text_overlap_lint.py#L2602-L2611, resolve left
and right glyph boxes from the shared cache before should_flag_overlap instead
of re-estimating per pair. In
skills/lark-slides/scripts/xml_text_overlap_lint.py#L1204-L1213, update
detect_shape_text_occlusions and detect_auto_fit_growth_collisions to read
cached boxes for each shape/text pair and other_bbox, preserving existing
detector behavior.
- Around line 1836-1850: Update the width-wrap filtering loop around the
existing single-line and short-label checks to skip any element recognized by
is_vertical_text, including all accepted vertical-run values. Keep vertical text
out of the width-based overflow calculation while preserving existing filtering
for horizontal text.
- Around line 2516-2525: Update the crossing-box adjustment around the
glyph_bbox handling to skip the vertical re-anchoring when the text run is
rotated, preserving the rotated bounds returned by estimate_text_visual_bbox.
Keep the existing unrotated vertical-align calculations for non-rotated text,
and ensure rotated runs do not mix rotated x/width with recomputed unrotated
y/height.
---
Outside diff comments:
In `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 1954-1972: Update the vertical alignment logic in the text
geometry calculation to skip middle/bottom re-anchoring when shape-auto-fit uses
a visual_height larger than content_height. Keep the authored top as the y
origin for the grown box, while preserving existing vertical alignment behavior
for non-grown boxes and auto-fit cases that do not exceed the content height.
---
Nitpick comments:
In `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 1204-1213: Precompute each text element’s glyph bounding box once
before iterating through covering_shapes, then reuse the cached result when
checking shape overlap. Apply the same caching approach to the repeated
other_bbox estimation in detect_auto_fit_growth_collisions, while preserving the
existing handling for None boxes and overlap thresholds.
- Line 1102: Update the area comparison in the overlap-checking logic around
intersection_area to use a dedicated area-threshold constant rather than
CONTAINER_OVERFLOW_MIN_PX. Keep CONTAINER_OVERFLOW_MIN_PX for the growth pixel
comparison and initialize the new area constant to preserve the current
behavior.
- Around line 1016-1050: Extract the shared scanning logic from
detect_table_text_occlusions and detect_chart_text_occlusions into a
parameterized detect_element_text_occlusions helper. Pass the occluder kind,
issue code, noun, and hint for each detector, while preserving the existing
filtering, intersection checks, and issue output.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 239a59f1-92ef-45f9-980b-00a67f3d8699
📒 Files selected for processing (2)
skills/lark-slides/scripts/xml_text_overlap_lint.pyskills/lark-slides/scripts/xml_text_overlap_lint_test.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2152 +/- ##
==========================================
+ Coverage 75.70% 75.72% +0.01%
==========================================
Files 944 944
Lines 100288 100355 +67
==========================================
+ Hits 75926 75994 +68
Misses 18565 18565
+ Partials 5797 5796 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fec80e5 to
ad0651a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
skills/lark-slides/scripts/xml_text_overlap_lint.py (2)
980-1050: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract a shared occluder-versus-text detector.
detect_table_text_occlusionsanddetect_chart_text_occlusionsdiffer only in the element kind, the issue code, the message, and the hint. The filtering, glyph-box estimation, and intersection logic are identical. A future change to the shared logic must be applied twice.♻️ Proposed consolidation
+def detect_element_text_occlusions( + elements: list[dict[str, Any]], kind: str, code: str, hint: str +) -> list[dict[str, Any]]: + issues: list[dict[str, Any]] = [] + text_elements = [ + element + for element in elements + if is_text_element(element) and has_text_content(element) and not is_ghost_text(element) + ] + occluders = [element for element in elements if element["kind"] == kind and element["alpha"] > 0] + for text_element in text_elements: + if is_decorative_text(text_element): + continue + glyph_bbox = estimate_text_visual_bbox(text_element) + if glyph_bbox is None: + continue + for occluder in occluders: + if not intersects(occluder, glyph_bbox): + continue + issues.append({ + "level": "error", + "code": code, + "elements": [occluder["id"], text_element["id"]], + "message": f'text shape {text_element["id"]} overlaps {kind} {occluder["id"]}', + "hint": hint, + }) + return issues
detect_table_text_occlusionsanddetect_chart_text_occlusionsthen become thin wrappers that keep their docstrings and pass the kind, code, and hint.🤖 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 `@skills/lark-slides/scripts/xml_text_overlap_lint.py` around lines 980 - 1050, Extract the duplicated filtering, glyph-bounding-box estimation, and intersection logic from detect_table_text_occlusions and detect_chart_text_occlusions into a shared helper that accepts the occluder kind, issue code, message context, and hint. Convert both existing functions into thin wrappers that preserve their docstrings and pass their table- or chart-specific values while retaining the current issue structure and behavior.
1102-1103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an area constant for the area gate.
Line 1102 compares an intersection area in px² against
CONTAINER_OVERFLOW_MIN_PX, which lines 76-79 document as linear pixels of slack. The neighbouring detector already has an area-scoped constant with the same value,SHAPE_TEXT_OCCLUSION_MIN_AREA. Reuse it here so the units match the comparison.♻️ Proposed change
- if intersection_area(grown_region, other_bbox) <= CONTAINER_OVERFLOW_MIN_PX: + if intersection_area(grown_region, other_bbox) <= SHAPE_TEXT_OCCLUSION_MIN_AREA: continue🤖 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 `@skills/lark-slides/scripts/xml_text_overlap_lint.py` around lines 1102 - 1103, Update the area threshold in the intersection check around intersection_area(grown_region, other_bbox) to use SHAPE_TEXT_OCCLUSION_MIN_AREA instead of CONTAINER_OVERFLOW_MIN_PX, preserving the existing comparison and control flow.
🤖 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 `@skills/lark-slides/scripts/xml_text_overlap_lint_test.py`:
- Around line 1177-1179: Update the comment above the assertions in
test_lint_xml_reports_wrap_false_text_wider_than_box to remove the claim that
wrap="false" opts a run out; state instead that no-wrap-label is not flagged
because its estimated width exceeds the heuristic risk band but remains within
the exact available-width tolerance, while comfortable fits normally.
---
Nitpick comments:
In `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 980-1050: Extract the duplicated filtering, glyph-bounding-box
estimation, and intersection logic from detect_table_text_occlusions and
detect_chart_text_occlusions into a shared helper that accepts the occluder
kind, issue code, message context, and hint. Convert both existing functions
into thin wrappers that preserve their docstrings and pass their table- or
chart-specific values while retaining the current issue structure and behavior.
- Around line 1102-1103: Update the area threshold in the intersection check
around intersection_area(grown_region, other_bbox) to use
SHAPE_TEXT_OCCLUSION_MIN_AREA instead of CONTAINER_OVERFLOW_MIN_PX, preserving
the existing comparison and control flow.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d41aff56-d416-4ade-a24b-28ac38aa7fc5
📒 Files selected for processing (2)
skills/lark-slides/scripts/xml_text_overlap_lint.pyskills/lark-slides/scripts/xml_text_overlap_lint_test.py
ethan-zhx
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 1033-1041: Update the innerRadius handling in the visible
chart-radius calculation to enforce the documented 0..1 fraction range: reject
or clamp values above 1 before multiplying by pie_radius, while preserving the
existing behavior for missing or non-positive values. Ensure the resulting hole
radius never exceeds the pie radius used by bbox_within_circle.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6329e495-306f-4fe3-991f-b8eef8c3ba2d
📒 Files selected for processing (2)
skills/lark-slides/scripts/xml_text_overlap_lint.pyskills/lark-slides/scripts/xml_text_overlap_lint_test.py
54ca7d3 to
5d603d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
skills/lark-slides/scripts/xml_text_overlap_lint.py (2)
1159-1172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompare the overlap area against an area constant.
Line 1159 compares a length (
growth, px) againstCONTAINER_OVERFLOW_MIN_PX. Line 1172 compares an area (px²) against the same constant. The two gates now share one number with two different units. If someone retunesCONTAINER_OVERFLOW_MIN_PXfor the growth threshold, the area gate changes silently.Use a dedicated area threshold for line 1172, for example the existing
SHAPE_TEXT_OCCLUSION_MIN_AREA.♻️ Proposed change
- if intersection_area(grown_region, other_bbox) <= CONTAINER_OVERFLOW_MIN_PX: + if intersection_area(grown_region, other_bbox) <= SHAPE_TEXT_OCCLUSION_MIN_AREA: continue🤖 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 `@skills/lark-slides/scripts/xml_text_overlap_lint.py` around lines 1159 - 1172, Use a dedicated area threshold for the intersection check in the overflow-detection loop around grown_region and intersection_area, replacing CONTAINER_OVERFLOW_MIN_PX with the existing SHAPE_TEXT_OCCLUSION_MIN_AREA while leaving the growth-length threshold unchanged.
1391-1407: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
is_full_canvas_background_imageduplicatesis_canvas_sized_background_shape.Both functions compute the same value: the intersection with the canvas divided by the canvas area, compared against
FULL_CANVAS_BACKGROUND_COVERAGE_RATIO(lines 1186-1194). Only the docstrings and parameter names differ. Keep one predicate, for examplecovers_full_canvas(element, slide_width, slide_height), and call it from both sites so the two rules cannot drift apart.🤖 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 `@skills/lark-slides/scripts/xml_text_overlap_lint.py` around lines 1391 - 1407, Consolidate the duplicated full-canvas coverage predicate by introducing or reusing a shared helper such as covers_full_canvas(element, slide_width, slide_height). Replace the logic in both is_full_canvas_background_image and is_canvas_sized_background_shape with calls to that helper, preserving the existing canvas-area validation and FULL_CANVAS_BACKGROUND_COVERAGE_RATIO threshold.
🤖 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 `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 1876-1887: Update short_line_passive_wrap_width to return None for
elements where is_vertical_text is true, matching the existing guard in
detect_text_may_wrap_shapes. Keep vertical runs out of passive-wrap
reclassification so they are not assigned overflow_axis "width" or a shape-width
remediation hint.
---
Nitpick comments:
In `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 1159-1172: Use a dedicated area threshold for the intersection
check in the overflow-detection loop around grown_region and intersection_area,
replacing CONTAINER_OVERFLOW_MIN_PX with the existing
SHAPE_TEXT_OCCLUSION_MIN_AREA while leaving the growth-length threshold
unchanged.
- Around line 1391-1407: Consolidate the duplicated full-canvas coverage
predicate by introducing or reusing a shared helper such as
covers_full_canvas(element, slide_width, slide_height). Replace the logic in
both is_full_canvas_background_image and is_canvas_sized_background_shape with
calls to that helper, preserving the existing canvas-area validation and
FULL_CANVAS_BACKGROUND_COVERAGE_RATIO threshold.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 92243a19-56cd-4294-8942-5d28ef3ae9b6
📒 Files selected for processing (2)
skills/lark-slides/scripts/xml_text_overlap_lint.pyskills/lark-slides/scripts/xml_text_overlap_lint_test.py
5d603d1 to
5a12147
Compare
2c3518d to
3b66d47
Compare
3 participants
No description provided.