Collapse nested character markup to single level (BL-16387)#8019
Collapse nested character markup to single level (BL-16387)#8019StephenMcConnel wants to merge 3 commits into
Conversation
|
| Filename | Overview |
|---|---|
| src/BloomExe/Book/Book.cs | Adds two extra passes of <b>/<i> → <strong>/<em> conversion to handle one level of nested old-style tags, then a deduplication regex to collapse <strong><strong>, <em><em>, and <u><u> pairs; also adds RegexOptions.IgnoreCase to the empty-tag removal call. Logic traces correctly for all tested input shapes. |
| src/BloomTests/Book/BookTests.cs | Adds two new test methods covering nested same-tag collapse (<b><b>, <i><i>, <u><u>, <sup><sup>) and multi-tag paragraphs with mixed nesting, including intentional non-collapse of <sup><sup>. |
Reviews (6): Last reviewed commit: "Fix ticket number typo in comment: BL-16..." | Re-trigger Greptile
b857247 to
6c5d409
Compare
JohnThomson
left a comment
There was a problem hiding this comment.
, but a couple of ideas to consider. If you don't think it's worth it you can merge as-is.
@JohnThomson reviewed 2 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on StephenMcConnel).
src/BloomExe/Book/Book.cs line 2631 at r2 (raw file):
/// <summary> /// Convert old <b> and <i> to <strong> and <em> respectively. /// Also remove instances like </b><b> altogether since such markup is redundant.
Comment is rather specific, so needs update.
src/BloomExe/Book/Book.cs line 2695 at r2 (raw file):
inner = Regex.Replace( inner, @"<(strong|em|sup|u)>(<\1>.*?</\1>)</\1>",
This also only handles things that are doubled right at the boundary. Maybe that's enough, but I'm wondering if we should handle the case that there is text inside the outer element but outside the inner one.
6c5d409 to
a3f63e4
Compare
StephenMcConnel
left a comment
There was a problem hiding this comment.
@StephenMcConnel made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on JohnThomson).
src/BloomExe/Book/Book.cs line 2631 at r2 (raw file):
Previously, JohnThomson (John Thomson) wrote…
Comment is rather specific, so needs update.
Done. I added to the comment.
src/BloomExe/Book/Book.cs line 2695 at r2 (raw file):
Previously, JohnThomson (John Thomson) wrote…
This also only handles things that are doubled right at the boundary. Maybe that's enough, but I'm wondering if we should handle the case that there is text inside the outer element but outside the inner one.
Done.
d677a98 to
f8e71d3
Compare
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on StephenMcConnel).
src/BloomExe/Book/Book.cs line 2695 at r2 (raw file):
Previously, StephenMcConnel (Steve McConnel) wrote…
Done.
I may be misunderstanding. It looks to me as if both the previous and current versions will convert abc to abc. My point is that neither of them will convert abcdefghi to abcdefghi. And it seems at least plausible that doing so would be a good idea. The inner b doesn't make anything more bold, and may cause unexpected behavior when changing the boldness of things.
I'm open to reasoning that what I'm suggesting is not a good idea, or unnecessary or too difficult.
For example, <em><em>...</em></em> goes to <em>...</em>.
…e (BL-16387) Responds to review feedback on PR #8019: the collapse regex previously only removed markup doubled exactly at the boundary (<b><b>...</b></b>). It now also collapses cases with text inside the outer element but outside the inner one, e.g. <b>abc<b>def</b>ghi</b> -> <b>abcdefghi</b>, since the inner tag adds no formatting and can cause surprises when toggling the style. Also drop <sup> from the collapse set: nested superscript is semantically meaningful, so those are left as-is (attributes are still stripped from the inner tag). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
f8e71d3 to
6da1954
Compare
The nested-markup cleanup comment referenced BL-16378; the correct ticket for this work is BL-16387. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
StephenMcConnel
left a comment
There was a problem hiding this comment.
@StephenMcConnel made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on JohnThomson).
src/BloomExe/Book/Book.cs line 2695 at r2 (raw file):
Previously, JohnThomson (John Thomson) wrote…
I may be misunderstanding. It looks to me as if both the previous and current versions will convert abc to abc. My point is that neither of them will convert abcdefghi to abcdefghi. And it seems at least plausible that doing so would be a good idea. The inner b doesn't make anything more bold, and may cause unexpected behavior when changing the boldness of things.
I'm open to reasoning that what I'm suggesting is not a good idea, or unnecessary or too difficult.
Your attempt to illustrate your point didn't come through: you have to escape html markers like <b>.
I've updated the regex to handle what you want. It now collapses a same-tag pair even when there's text on either side of the inner tag, so <strong>abc<strong>def</strong>ghi</strong> goes to <strong>abcdefghi</strong> (and likewise for <em>/<u>).
After thinking about it, I dropped <sup> from this collapsing, since nested superscript is semantically meaningful (a superscript of a superscript renders higher and smaller), so collapsing these would change how the text displays.
For example, ... goes to ....
Devin review
This change is