Address issue #560: preference to hide horizontal rules in preview - #561
Conversation
…eview AI-generated markdown frequently inserts --- separators between sections. Most theme CSS already renders a border/rule under major headings for visual separation, so the rendered result ends up with two lines back-to-back. Adds a "Hide horizontal rules" checkbox to the Rendering preferences pane (default on) that suppresses <hr> output via a new Hoedown renderer patch, without needing to touch the markdown source.
schuyler
left a comment
There was a problem hiding this comment.
Thanks for the PR, @macnotes!
Issues
- Flag never set:
HOEDOWN_HTML_HIDE_HORIZONTAL_RULESis never OR-ed intorendererFlags— the composer atMacDown/Code/Document/MPDocument.m:230-242doesn't consulthtmlHideHorizontalRules, so the newhrulecallback is never installed and the checkbox has no effect on preview, export, or copy-as-HTML. - Coverage for the flag composition: a test asserting
(preferences.rendererFlags & HOEDOWN_HTML_HIDE_HORIZONTAL_RULES) != 0when the preference is on would catch this specific gap, which a renderer-level test passing the flag directly would not. - Default-on affects existing users: the default lives in
loadDefaultUserDefaults, which runs on every launch, so once the wiring is fixed every existing user's documents silently lose their<hr>output — worth calling out the intent explicitly, since this removes visible content rather than adding behavior.
Suggestions
- Quick Look has its own flags composer at
MacDownCore/MPQuickLookPreferences.m:148-164that won't pick up the preference, so previews in Finder would still show rules unless it's wired up too or the exclusion is stated. - The other patched callbacks are installed unconditionally and test
state->flagsinternally, so following that pattern here would keep the flag handling in one place. MPMarkdownRenderingTests.malready hasrenderMarkdown:withExtensions:rendererFlags:and atestHorizontalRulesfixture, which makes rendering tests for---/***/___, rules inside blockquotes and lists, and a guard thatTitle\n---still renders<h2>fairly cheap to add.
Generated by Claude Code
The hrule-hiding renderer patch from the initial commit was never
consulted: MPPreferences(Hoedown)'s -rendererFlags never OR-ed in
HOEDOWN_HTML_HIDE_HORIZONTAL_RULES, so the checkbox had no effect on
preview, export, or copy-as-HTML.
- Compose the flag into -rendererFlags in MPDocument.m.
- Wire the same preference into Quick Look, which reads prefs directly
via CFPreferences and has its own renderer callbacks (hoedown_html_patch
isn't linked into MacDownCore), so it needed its own hrule stripper.
- Declare the previously-undeclared MPPreferences (Hoedown) category in
MPPreferences.h so -rendererFlags/-extensionFlags are callable (and
testable) outside MPDocument.m.
- Add a regression test asserting the preference is actually reflected
in rendererFlags's bitmask -- a renderer-level test that sets the flag
directly would not have caught the wiring gap.
- Add rendering tests for rules in blockquotes/lists and a guard that a
setext heading ("Title" underlined with ---) still renders as <h2>
rather than being swallowed as a hidden rule.
Related to schuyler#560
|
Thanks for the thorough review — fixed all three issues: Flag never set: `MPPreferences (Hoedown)`'s `-rendererFlags` (in `MPDocument.m`) now OR's in `HOEDOWN_HTML_HIDE_HORIZONTAL_RULES` when `htmlHideHorizontalRules` is on, matching the pattern of the other flags in that method. Also declared that category properly in `MPPreferences.h` (it previously had no header declaration at all, callable only via loose message-sends from within `MPDocument.m`) so it's callable — and testable — from elsewhere. Test coverage for flag composition: Added `testHideHorizontalRulesFlagComposition` in `MPPreferencesTests.m`, asserting `(preferences.rendererFlags & HOEDOWN_HTML_HIDE_HORIZONTAL_RULES)` directly, which is exactly the gap a renderer-level test wouldn't have caught. Quick Look: `MPQuickLookPreferences` gets a new `-hideHorizontalRules` reader (default YES, matching the app default) and `MPQuickLookRenderer.m` installs a local no-op `hrule` callback when it's set — mirroring the existing `mp_quicklook_render_blockcode`/`_header` pattern, since `hoedown_html_patch.c` isn't linked into `MacDownCore`. Also added the suggested rendering tests: `---`/`***`/`___` hiding, rules inside blockquotes/lists (with assertions that surrounding content still renders), and a guard that a setext heading (`Title` underlined with `---`) still renders as < h2 > rather than being swallowed as a hidden rule. Left the "install unconditionally, check `state->flags` internally" suggestion alone — Quick Look's renderer overrides are a separate, parallel implementation (no shared code with `hoedown_html_patch.c`), so there's no single place to centralize that check across both call sites On the default-on note: issue #560 explicitly asked for "default on," so that's intentional — flagging in case it's worth a mention in release notes since it does change what existing users see in previews of documents using `---` as a separator. |
|
Honestly, given the complexity of this PR, and perhaps its limited value to the average user, it won't hurt my feelings at all if you want to reject it. But for me it's a nice time-saver. |
|
According to Copilot:
#include "hoedown/document.h"
#include "hoedown/renderer.h"Sorry for all the back and forth. If I could edit this branch, I'd just go ahead and make the change and push again |
hoedown_html_patch.h declares functions using hoedown_renderer_data and hoedown_list_flags without including the header that defines them. MPPreferencesTests.m imports hoedown_html_patch.h without first importing a Hoedown header, so those types were undeclared and the build failed. Reviewed by an independent Opus subagent per the Rule of Two: verified the types are declared in hoedown/document.h (not hoedown/renderer.h, which doesn't exist in this project's vendored Hoedown), confirmed double-include safety via document.h's include guards, and confirmed the fix resolves the MPPreferencesTests.m failure. Approved with one non-blocking nit (a pre-existing redundant hoedown_buffer typedef, left as-is). Related to schuyler#560
…e-560-hide-hr-4c11266f-95a1-4aee-aa05-f48d8b17b66b # Conflicts: # MacDown/Code/Document/MPRenderer.m # MacDown/Code/Extension/hoedown_html_patch.c
…box overlap htmlHideHorizontalRules now defaults to off rather than silently changing render output for existing users. Adds a note under the checkbox explaining that it targets the case where AI-generated Markdown inserts --- section breaks that duplicate a rule the theme already draws under headings. Also fixes an unrelated pre-existing bug in the same xib: the "Syntax highlighted code block" checkbox had no vertical Auto Layout constraint, leaving its Y position ambiguous and causing it to render on top of the CSS theme dropdown row. Reviewed by an independent Opus subagent per the Rule of Two: confirmed both default-value changes are consistent, no test asserts the old default, the new note's constraints are well-formed, and the checkbox fix leaves exactly one path to a fixed Y position. Verified locally with ibtool --compile (zero errors/warnings/notices) and a project build. Related to schuyler#560
The note explaining AI-generated section breaks was convoluted for a preferences pane. The checkbox title stands on its own. Reviewed by an independent Opus subagent per the Rule of Two: confirmed no dangling references to the removed elements, the vertical constraint chain remains a single unambiguous top-to-bottom path, and the corrected design-height hint (406) matches the measured Auto Layout fitting size. Related to schuyler#560
Related to #560
Summary
---separators between sections. Most theme CSS already renders a border/rule under major headings for visual separation, so the rendered result ends up with two lines back-to-back.<hr>output via a new Hoedown renderer patch, without needing to touch the markdown source.Test plan
xcodebuild ... -scheme MacDown)---lines produce no<hr>; with it off,<hr>renders as before