Add optional absolute char indexing for highlighting #2584

AgentHagu · 2025-01-17T05:02:19Z

What is the purpose of this pull request?

Overview of changes:
This PR closes #2573. In summary, it allows users to now optionally highlight leading whitespace, giving them more options when using MarkBind.

Users can now add an optional + before their char bound specifier to indicate that the index should be considered as its absolute value, i.e. inclusive of leading whitespace/indentation. For example, previously 1[:4] would highlight the abcd in abcd. But now, by using +1[:4], MarkBind will highlight ab in abcd (i.e. it will also highlight the first 2 spaces in the string since it considers the end bound of 4 as its absolute value, rather than relative to the indentation level).

Anything you'd like to highlight/discuss:
nil

Testing instructions:
A test similar to the example brought up in #2573 can be used to test the new functionality. For example:

```{highlight-lines="1[:4]"}
  abcd
```

abcd should be highlighted, i.e. exclusive of the 2 whitespaces in the beginning.

```{highlight-lines="+1[:4]"}
  abcd
```

Now, ab should be highlighted, since the "+" symbol was added, so the absolute value of the bounds is considered in the bounds calculation.

Proposed commit message: (wrap lines at 72 characters)
Add optional absolute char indexing for highlighting

Currently, there is no way to include highlighting of leading
whitespace when using the highlight-lines attribute. Providing such
an option may be beneficial to users, giving them more freedom
when using MarkBind.

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

Major (when you make incompatible API changes)
Minor (when you add functionality in a backward compatible manner)
Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

what was the old feature that was made obsolete

any replacement feature (if any), and

how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

groups.pop() was called without considering the number of capturing groups within. This caused some issues with the end bound of word matching being popped and removed from the array.

gerteck

Looking at the use case of highlighting, it seems that the reason why this feature was developed this way was because of the usecase of highlighting code blocks, which are appropriately indented. (Given that the documentation is under the code formatting section).

Additionally, to capture whitespace from front of line, it could be better to add symbol in front rather than the back?

gerteck · 2025-01-20T02:31:48Z

packages/core/src/lib/markdown-it/highlight/HighlightRuleComponent.ts

   * @returns {[number, number]} The actual bound computed
   */
-  static computeCharBounds(bound: [number, number], line: string): [number, number] {
+  static computeCharBounds(bound: [number, number], line: string,
+                           highlightSpaces: boolean): [number, number] {


isHighlightSpaces instead of highlightSpaces for boolean name choice would be better.

Good catch! Thanks

AgentHagu · 2025-01-20T05:21:40Z

Additionally, to capture whitespace from front of line, it could be better to add symbol in front rather than the back?

Good idea, I'll try moving the symbol to the front, such as an underscore like "_1[2:4]" to make it more intuitive

gerteck · 2025-01-20T06:07:21Z

Additionally, to capture whitespace from front of line, it could be better to add symbol in front rather than the back?

Good idea, I'll try moving the symbol to the front, such as an underscore like "_1[2:4]" to make it more intuitive

Sorry, would something like

1[_:4] where _ replaces the digit if we want to include the whitespace? Not sure if it will complicate things, but it seems most intuitive to me, since it relates to the start of the highlight / line, without adding anything extra in the syntax, given it would be left empty.

Does anyone else have any suggestions?

damithc · 2025-01-20T07:01:57Z

Thanks @AgentHagu for this PR, and others for comments.
In terms of feature design, what if we treat spaces as regular characters, without using any special syntax? Would that be cleaner?
Don't worry about backward compatibility. It is unlikely to affect any existing MarkBind sites, as this is a very much an edge case of usage.

AgentHagu · 2025-01-20T13:05:04Z

Thanks @AgentHagu for this PR, and others for comments. In terms of feature design, what if we treat spaces as regular characters, without using any special syntax? Would that be cleaner? Don't worry about backward compatibility. It is unlikely to affect any existing MarkBind sites, as this is a very much an edge case of usage.

Thanks for the feedback! Just to clarify, do you mean having char-bound highlighting ignore the indentation level by default? So the spaces within the indents are now automatically considered in the char bounds?

damithc · 2025-01-20T14:27:16Z

Thanks for the feedback! Just to clarify, do you mean having char-bound highlighting ignore the indentation level by default? So the spaces within the indents are now automatically considered in the char bounds?

Oh, so that's why spaces were not counted before! I didn't realise. So, the current counting is relative to the first non-space char, by default?
Perhaps the solution is to introduce a notation to indicate absolute char counting instead?

AgentHagu · 2025-01-21T02:10:15Z

Oh, so that's why spaces were not counted before! I didn't realise. So, the current counting is relative to the first non-space char, by default?

Yep, by default the indentation level is checked and accounted for and the index counting is relative to the first non-whitespace char.

Perhaps the solution is to introduce a notation to indicate absolute char counting instead?

To clarify, do you mean introducing something like 1[0.3] instead of 1[0:3] for whitespace-inclusive char-bound highlighting? If so, I think this could be a good solution that doesn't affect existing MarkBind sites and also provides new options to users. It also means we wouldn't need the special syntax anymore.

damithc · 2025-01-21T02:17:01Z

To clarify, do you mean introducing something like 1[0.3] instead of 1[0:3] for whitespace-inclusive char-bound highlighting? If so, I think this could be a good solution that doesn't affect existing MarkBind sites and also provides new options to users. It also means we wouldn't need the special syntax anymore.

@AgentHagu I was thinking of something like +1[0:3] where + indicates numbers are absolute i.e., similar to your current syntax but the meaning is different.

AgentHagu · 2025-01-21T02:38:41Z

@AgentHagu I was thinking of something like +1[0:3] where + indicates numbers are absolute i.e., similar to your current syntax but the meaning is different.

I see. In that case, I believe the implementation should already support absolute char counting, so only the syntax and PR description needs to be updated

damithc · 2025-01-21T04:32:32Z

Just so we don't forget later, this is a user-visible change, which means UG needs to be updated as well. But you can wait till the feature behaviour is finalised before adding the UG update to the PR.

lhw-1

Thanks for the PR, @AgentHagu!

Revisiting a discussion point that we brought up earlier - in the cases of other kinds of whitespaces being present (e.g. tab or newline), how do we handle it? E.g. I can see a situation where the user copy-pastes in an existing chunk of code with whitespace, and non-space whitespace characters are added in directly. This may just be a documentation problem though given that this is not highly likely & usually these are converted to whitespaces in many IDEs etc, what do you think? Or is there a tweak we can do that will efficiently cover these cases?

lhw-1 · 2025-01-25T05:13:51Z

packages/core/src/lib/markdown-it/highlight/HighlightRuleComponent.ts

      const lineNumber = HighlightRuleComponent
        .isValidLineNumber(groups.shift() ?? '', 1, lines.length, lineNumberOffset);
      if (!lineNumber) return null;

      const isUnbounded = groups.every(x => x === '');
+


Suggested change

lhw-1 · 2025-01-25T05:24:51Z

packages/core/src/lib/markdown-it/highlight/HighlightRuleComponent.ts

      start += indents.length;
      // Clamp values
      if (start < indents.length) {
        start = indents.length;
      } else if (start > line.length) {
        start = line.length;
      }
+    } else if (start > line.length) {
+      start = line.length;
    }


The conditional logic is slightly convoluted for me here. If start !== UNBOUNDED is true, and isHighlightSpaces is true, and start > line.length is false, what happens? (Will this even happen in theory?)

Also, given that Lines 129-130 and 132-133 are duplicated, I wonder if there is a way to simplify this chunk of code. We don't need to shy away from ternary conditional operators if they make the code easier to understand.

If start !== UNBOUNDED is true, and isHighlightSpaces is true, and start > line.length is false, what happens? (Will this even happen in theory?)

In theory, it's possible for that to occur. For example, for some text, start = 2 would be bounded and <= line.length and if isHighlightSpaces is true, then that means we should leave the start value as is, since we wish to use the absolute value of those bounds.

Also, given that Lines 129-130 and 132-133 are duplicated, I wonder if there is a way to simplify this chunk of code. We don't need to shy away from ternary conditional operators if they make the code easier to understand.

Thanks for the feedback! I'll see if there's a way to simplify it more to improve readability.

lhw-1 · 2025-01-25T05:28:17Z

packages/core/test/unit/lib/markdown-it/highlight/HighlightRuleComponent.test.ts

+  });
+
+  test('handles line-length end correctly', () => {
+    const bounds = HighlightRuleComponent.computeCharBounds([0, 4], '  abcd', true);


Why abcd here in particular? (Let me know if it's a non-issue)

There's no underlying reason for abcd, I can update the test-case to follow the other test-cases' string input of ' some text'.

AgentHagu · 2025-01-27T05:16:39Z

Thanks for the PR, @AgentHagu!

Revisiting a discussion point that we brought up earlier - in the cases of other kinds of whitespaces being present (e.g. tab or newline), how do we handle it? E.g. I can see a situation where the user copy-pastes in an existing chunk of code with whitespace, and non-space whitespace characters are added in directly. This may just be a documentation problem though given that this is not highly likely & usually these are converted to whitespaces in many IDEs etc, what do you think? Or is there a tweak we can do that will efficiently cover these cases?

Thanks for the feedback!

After investigating, it seems that tabs are treated as 1 "character" by the current code, leading to weird highlighting such as the following:

For e.g., the highlight rule for line 4 is +4[0:2], but rather than highlighting the first 2 characters of the indentation, it highlights the first 2 tabs as a whole.

My current idea is to see whether it's possible to convert such tabs to the corresponding number of whitespace, which should remedy the issue automatically. What do you think of this solution?

AgentHagu added 4 commits January 13, 2025 22:58

Update highlighter functionality to support new regex

e4f0e2b

Add new test cases for highlighter

78c8e84

Update documentation for computeCharBounds

46901a3

Fix issue regarding failing test cases

f679edb

groups.pop() was called without considering the number of capturing groups within. This caused some issues with the end bound of word matching being popped and removed from the array.

gerteck reviewed Jan 20, 2025

View reviewed changes

Update symbol and position

fe14d75

lhw-1 reviewed Jan 25, 2025

View reviewed changes

Update documentation to follow idea of absolute char indexing

7cb451b

Refactor code to improve readability

2245743

AgentHagu changed the title ~~Add optional leading whitespace highlighting~~ Add optional absolute char indexing for highlighting Jan 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional absolute char indexing for highlighting #2584

Add optional absolute char indexing for highlighting #2584

AgentHagu commented Jan 17, 2025 •

edited

Loading

gerteck left a comment •

edited

Loading

gerteck Jan 20, 2025

AgentHagu Jan 20, 2025

AgentHagu commented Jan 20, 2025

gerteck commented Jan 20, 2025 •

edited

Loading

damithc commented Jan 20, 2025

AgentHagu commented Jan 20, 2025

damithc commented Jan 20, 2025

AgentHagu commented Jan 21, 2025

damithc commented Jan 21, 2025

AgentHagu commented Jan 21, 2025 •

edited

Loading

damithc commented Jan 21, 2025

lhw-1 left a comment

lhw-1 Jan 25, 2025

lhw-1 Jan 25, 2025

AgentHagu Jan 27, 2025

lhw-1 Jan 25, 2025

AgentHagu Jan 27, 2025 •

edited

Loading

AgentHagu commented Jan 27, 2025 •

edited

Loading

Add optional absolute char indexing for highlighting #2584

Are you sure you want to change the base?

Add optional absolute char indexing for highlighting #2584

Conversation

AgentHagu commented Jan 17, 2025 • edited Loading

gerteck left a comment • edited Loading

Choose a reason for hiding this comment

gerteck Jan 20, 2025

Choose a reason for hiding this comment

AgentHagu Jan 20, 2025

Choose a reason for hiding this comment

AgentHagu commented Jan 20, 2025

gerteck commented Jan 20, 2025 • edited Loading

damithc commented Jan 20, 2025

AgentHagu commented Jan 20, 2025

damithc commented Jan 20, 2025

AgentHagu commented Jan 21, 2025

damithc commented Jan 21, 2025

AgentHagu commented Jan 21, 2025 • edited Loading

damithc commented Jan 21, 2025

lhw-1 left a comment

Choose a reason for hiding this comment

lhw-1 Jan 25, 2025

Choose a reason for hiding this comment

lhw-1 Jan 25, 2025

Choose a reason for hiding this comment

AgentHagu Jan 27, 2025

Choose a reason for hiding this comment

lhw-1 Jan 25, 2025

Choose a reason for hiding this comment

AgentHagu Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

AgentHagu commented Jan 27, 2025 • edited Loading

AgentHagu commented Jan 17, 2025 •

edited

Loading

gerteck left a comment •

edited

Loading

gerteck commented Jan 20, 2025 •

edited

Loading

AgentHagu commented Jan 21, 2025 •

edited

Loading

AgentHagu Jan 27, 2025 •

edited

Loading

AgentHagu commented Jan 27, 2025 •

edited

Loading