Skip to content

Honor baseunit when deriving SVG dimensions from viewBox - #255

Open
snowyukitty wants to merge 1 commit into
mathandy:masterfrom
snowyukitty:fix/respect-svg-viewbox-baseunit
Open

snowyukitty wants to merge 1 commit into
mathandy:masterfrom
snowyukitty:fix/respect-svg-viewbox-baseunit

Conversation

@snowyukitty

@snowyukitty snowyukitty commented Oct 23, 2026 •

Copy link
Copy Markdown

Fixes #215.

When viewbox supplies the SVG dimensions, disvg currently ignores baseunit. A viewBox of (0, 0, 2.5, 3.75) with baseunit='cm' produces unitless width='2.5' and height='3.75', rather than the requested centimeter dimensions.

Append non-pixel unit strings to inferred dimensions. Keep default/explicit pixel output and legacy non-string handling unchanged; explicit dimensions and svg_attributes width/height still take precedence.

The native regressions cover tuple/string viewboxes, cm/mm/in/% units, pixel/empty/None controls, explicit dimensions, partial SVG attribute overrides, Drawing serialization and separate files written through disvg and wsvg. They also check preserved viewBox/path data and automatic sizing.

Validation from the repository root on Linux, Python 3.12:

  • python -m unittest discover test: 115 tests, OK.
  • Restoring the original export module produces 13 expected assertion failures in the new tests.
  • python -m build --no-isolation: sdist and wheel built.

Other OS/Python versions were not run locally. The existing slow area test remains disabled by upstream's RUN_SLOW_TESTS setting.

PR #252 adds nearby type annotations but leaves this dimension inference unchanged; a mechanical rebase may be needed.

Summary by CodeRabbit

  • New Features
    • SVGs with a supplied viewBox and no explicit dimensions now use the selected baseunit for inferred display dimensions when it is a string other than px. ViewBox coordinates remain unitless.
    • Explicit dimensions and SVG attributes continue to take precedence over inferred dimensions.
  • Documentation
    • Clarified how baseunit, explicit dimensions, and SVG attributes affect inferred display dimensions.

@coderabbitai

coderabbitai Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 11d5c97c-4f0e-4fd0-8e3a-c2a1fdf8689c
📥 Commits

Reviewing files that changed from the base of the PR and between 15b500e and 79f79c5.

📒 Files selected for processing: 2
  • svgpathtools/paths2svg.py
  • test/test_paths2svg.py

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Walkthrough

When viewbox is provided without dimensions, inferred SVG dimensions now append baseunit when it is a string other than 'px'. Tests cover supported base units, saved output, and precedence for explicit dimensions and SVG attributes.

Changes: SVG dimensions and base units

Layer / File(s) Summary
Infer dimensions with base units
svgpathtools/paths2svg.py, test/test_paths2svg.py
When viewbox is provided without dimensions, inferred width and height now receive the baseunit suffix for string values other than 'px'. The documentation describes this behavior and the precedence of explicit dimensions and SVG attributes. Tests cover inferred dimensions, saved output from disvg and wsvg, and those precedence rules.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 79f79

The change appears ready to merge after normal checks; no unresolved issue was identified with viewBox-derived dimensions or their overrides.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: applying baseunit when deriving SVG dimensions from viewBox.
Linked Issues check Passed Issue [#215] requires disvg to include baseunit when it derives SVG dimensions from viewbox. In svgpathtools/paths2svg.py, the viewbox and omitted dimensions path appends a string `baseuni…
Out of Scope Changes check Passed The changes are limited to the [#215] dimension inference behavior, its public documentation, and regression tests. The tests cover disvg, wsvg, Drawing serialization, viewBox and path preservat…
Full details: Linked Issues check

Explanation

Issue [#215] requires disvg to include baseunit when it derives SVG dimensions from viewbox. In svgpathtools/paths2svg.py, the viewbox and omitted dimensions path appends a string baseunit other than 'px' to both inferred dimensions. Pixel, empty, and None controls retain unitless output. Explicit dimensions and svg_attributes width/height retain precedence. The added tests cover tuple and string viewboxes, requested units, file output, and precedence behavior.

Full details: Out of Scope Changes check

Explanation

The changes are limited to the [#215] dimension inference behavior, its public documentation, and regression tests. The tests cover disvg, wsvg, Drawing serialization, viewBox and path preservation, and precedence cases that support the objective. No unrelated production behavior is identified.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

baseunit ignored in disvg when viewbox parameter is provided

1 participant