Skip to content

Conversation

@bismitpanda
Copy link

@bismitpanda bismitpanda commented May 6, 2023

  • One '~' produces a subscript instead of strikethrough (when enabled)
  • "==" highlights the text inside it
  • "++" underlines the text inside

- One '~' produces a subscript instead of strikethrough
- "==" highlights the text inside it
@kivikakk
Copy link
Owner

kivikakk commented May 7, 2023

Hi there! Thanks so much for this high-quality submission!

There's one major issue, which is that this changes the behaviour of the strikethrough extension, even when subscript isn't enabled. This causes an incompatibility with GitHub Flavored Markdown.

I'd be happy to accept the proposed change if this was addressed -- I'd be okay with subscript being enabled changing strikethrough's behaviour accordingly, but not with it disabled (i.e. GFM defaults). Right now there's an "interesting" bug that results where enabling only strikethrough (not subscript) and then using one ~ results in emphasis being rendered (!); see https://github.com/kivikakk/comrak/actions/runs/4902153007/jobs/8760508796?pr=302#step:4:659.

I'll leave some more comments inline.


if self.options.extension.strikethrough
&& opener_char == b'~'
&& (opener_num_chars != closer_num_chars || opener_num_chars > 0)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect (though haven't checked) this will cause spec failure with this example. The spec requires three or more tildes to not generate a strikethrough; since we've already subtracted use_delims (2 for a strikethrough of length 2 or more) from opener_num_chars, if we had 3 or more, opener_num_chars > 0 would be true and so we'd return None here.

Copy link
Author

@bismitpanda bismitpanda May 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If subscript and strikethrough, both are enabled, it will be a subscript as well as a strikethrough\ like in cases of italics and bolds. Considering this, I removed the condition.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding and additional && to check if subscript is enabled. Like

if (self.options.extension.strikethrough
    && opener_char == b'~'
    && (opener_num_chars != closer_num_chars || opener_num_chars > 0))
    && !self.options.extension.subscript
{
    return None;
}

@bismitpanda
Copy link
Author

Can I add a new feature of inserted text i.e. basically underline using ++? Got inspiration from https://markdown-it.github.io/

- Added underlined/inserted text support using `++`
- Fixed to confirm GFM strikethrough specs when subscript not enabled
- Fixed some code based on PR suggestions
@bismitpanda
Copy link
Author

Can I get some info regarding the skip_chars array in Subject struct in src/parser/inline.rs@41:4?

@bismitpanda bismitpanda changed the title Adding support for subscript and highlighted text Adding support for subscript, highlighted and underlined text May 10, 2023
@bismitpanda
Copy link
Author

Any other changes/improvements required? As I see it is still open.

@kivikakk
Copy link
Owner

I'm currently sick and unable to review the PR. I will when I can.

@bismitpanda
Copy link
Author

any updates on this?

@kivikakk
Copy link
Owner

No updates. With my limited energy I've been prioritising bug fixes and upstream compliance.

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.

2 participants