Conversation
|
@copilot give a code review for the pull request.Whether or not can add line spacing for text/link/toc/quote/code block/form/footer note ui unit |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new lineSpacing configuration option intended to let consumers control the main text’s line spacing, updates documentation changelogs to announce the feature, and bumps the CocoaPods version to 1.7.0.
Changes:
- Add
lineSpacingtoMarkdownConfiguration(with a default value inMarkdownConfiguration.default). - Apply
configuration.lineSpacingwhen creating views for.attributedTextrender elements. - Update README/README_zh changelogs and bump
MarkdownDisplayKit.podspecto1.7.0.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Adds a 1.7.0 changelog entry mentioning lineSpacing. |
| README_zh.md | Adds a 1.7.0 changelog entry mentioning lineSpacing (Chinese). |
| MarkdownDisplayView/Sources/MarkdownDisplayView/MarkdownRenderElement.swift | Adds lineSpacing to MarkdownConfiguration and sets a default value in .default. |
| MarkdownDisplayView/Sources/MarkdownDisplayView/MarkdownDisplayView.swift | Applies lineSpacing by injecting a paragraph style before creating the text view for .attributedText. |
| MarkdownDisplayKit.podspec | Bumps podspec version to 1.7.0. |
Comments suppressed due to low confidence (1)
MarkdownDisplayView/Sources/MarkdownDisplayView/MarkdownDisplayView.swift:2133
lineSpacingis only applied in the.attributedTextrendering path; other element types (e.g..heading,.blockquotecontent, etc.) will keep whatever line spacing the parser hard-codes. If the intent is to makeMarkdownConfiguration.lineSpacingthe single source of truth for text line spacing, consider pushing it into attribute generation (parser/defaultTextAttributes) or applying the same adjustment in the other text-producing cases as well.
case .attributedText(let attributedString):
if attributedString.length > 0 {
let isInlineSegment = attributedString.attribute(inlineSegmentAttributeKey, at: 0, effectiveRange: nil) != nil
let topSpacing = suppressTopSpacing ? 0 : (isInlineSegment ? 0 : configuration.paragraphTopSpacing)
let bottomSpacing = suppressBottomSpacing ? 0 : (isInlineSegment ? 0 : configuration.paragraphBottomSpacing)
let mutableAttri = NSMutableAttributedString(attributedString: attributedString)
let paragraphStyle = NSMutableParagraphStyle()
paragraphStyle.lineSpacing = configuration.lineSpacing
mutableAttri.addAttribute(.paragraphStyle, value: paragraphStyle, range: NSRange(location: 0, length: mutableAttri.length))
return createTextView(
with: mutableAttri,
width: containerWidth,
insets: UIEdgeInsets(top: topSpacing, left: 0, bottom: bottomSpacing, right: 0),
fixedHeight: precalculatedHeight
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let paragraphStyle = NSMutableParagraphStyle() | ||
| paragraphStyle.lineSpacing = configuration.lineSpacing | ||
| mutableAttri.addAttribute(.paragraphStyle, value: paragraphStyle, range: NSRange(location: 0, length: mutableAttri.length)) |
There was a problem hiding this comment.
The new code creates a fresh NSMutableParagraphStyle and applies it to the whole attributed string. This overwrites any existing paragraph style coming from the parser (e.g., blockquote indents, list indents, paragraphSpacing, lineHeightMultiple), which can break formatting for lists/quotes or multi-paragraph buffers. Instead, preserve existing .paragraphStyle attributes and only update lineSpacing (e.g., enumerate ranges, copy/mutate existing styles, and apply back; for ranges without a paragraph style, add one that only sets lineSpacing).
| let paragraphStyle = NSMutableParagraphStyle() | |
| paragraphStyle.lineSpacing = configuration.lineSpacing | |
| mutableAttri.addAttribute(.paragraphStyle, value: paragraphStyle, range: NSRange(location: 0, length: mutableAttri.length)) | |
| let fullRange = NSRange(location: 0, length: mutableAttri.length) | |
| mutableAttri.enumerateAttribute(.paragraphStyle, in: fullRange, options: []) { value, range, _ in | |
| let baseStyle: NSParagraphStyle | |
| if let existingStyle = value as? NSParagraphStyle { | |
| baseStyle = existingStyle | |
| } else { | |
| baseStyle = NSParagraphStyle.default | |
| } | |
| let mutableStyle = (baseStyle.mutableCopy() as? NSMutableParagraphStyle) ?? NSMutableParagraphStyle() | |
| mutableStyle.lineSpacing = configuration.lineSpacing | |
| mutableAttri.addAttribute(.paragraphStyle, value: mutableStyle, range: range) | |
| } | |
| let topSpacing = suppressTopSpacing ? 0 : (isInlineSegment ? 0 : configuration.paragraphTopSpacing) | ||
| let bottomSpacing = suppressBottomSpacing ? 0 : (isInlineSegment ? 0 : configuration.paragraphBottomSpacing) | ||
|
|
||
| let mutableAttri = NSMutableAttributedString(attributedString: attributedString) |
There was a problem hiding this comment.
Variable name mutableAttri is cryptic and inconsistent with nearby naming (attrString, codeAttrString). Consider renaming it to something clearer like mutableAttributedString for readability.
|
|
||
| ### 1.7.0 (2026-03-31) | ||
|
|
||
| - 🚀 **lineSpacing configuration** - Added the `lineSpacing` configuration option to support controlling the line height of the main text |
There was a problem hiding this comment.
Changelog entry says lineSpacing controls the “line height” of the main text, but the implementation uses NSParagraphStyle.lineSpacing (additional spacing between baselines), which is not the same as line height (e.g. minimumLineHeight/lineHeightMultiple). Please adjust the wording to “line spacing” (or update the implementation to actually control line height).
| - 🚀 **lineSpacing configuration** - Added the `lineSpacing` configuration option to support controlling the line height of the main text | |
| - 🚀 **lineSpacing configuration** - Added the `lineSpacing` configuration option to support controlling the line spacing of the main text |
|
|
||
| ### 1.7.0 (2026-03-31) | ||
|
|
||
| - 🚀 **lineSpacing配置** - 新增 `lineSpacing` 配置项,支持控制正文行高 |
There was a problem hiding this comment.
更新日志里写 lineSpacing 用于控制“正文行高”,但实现使用的是 NSParagraphStyle.lineSpacing(行间距/基线间额外间距),不等同于行高(如 minimumLineHeight/lineHeightMultiple)。建议把文案改为“行间距”,或调整实现以真正控制行高。
| - 🚀 **lineSpacing配置** - 新增 `lineSpacing` 配置项,支持控制正文行高 | |
| - 🚀 **行间距(lineSpacing)配置** - 新增 `lineSpacing` 配置项,支持控制正文行间距(段落基线间额外间距) |
No description provided.