Add code highlighting based on absolute positions #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:
This PR also adds automatic conversion of tabs to 4 spaces by default. The User Guide has been updated to reflect the changes.

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

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

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?

lhw-1 · 2025-02-05T02:30:04Z

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?

That would make sense - we'll just need to specify this behavior in the documentation :)

damithc · 2025-02-05T03:09:47Z

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 Note that a tab may be considered a different number of spaces by different tools: some use 2 spaces, some use 4, and some use 8 (e.g., GitHub). But yes, we can pick one (e.g., 4) and keep the users informed about it.

lhw-1 · 2025-02-16T03:23:20Z

@AgentHagu What is the current status of this PR?

AgentHagu · 2025-02-16T05:31:06Z

@lhw-1 Sorry for the lack of updates, currently fixing some issues with the tab conversion. The user guide has been updated to reflect the new behavior as well.

codecov · 2025-02-16T07:37:57Z

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 51.87%. Comparing base (8c9de42) to head (51c0d84).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
packages/core/src/lib/markdown-it/index.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2584   +/-   ##
=======================================
  Coverage   51.86%   51.87%           
=======================================
  Files         127      127           
  Lines        5474     5473    -1     
  Branches     1201     1201           
=======================================
  Hits         2839     2839           
+ Misses       2340     2339    -1     
  Partials      295      295

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

AgentHagu · 2025-02-17T11:47:06Z

Upon initial investigations, it seems that as of the current master branch, tab characters are 8 spaces wide by default. I've confirmed this behavior using a simple test with @IanCheah but if anyone would like to, they can also confirm it on their own.

Code:

Code with highlighting to indicate spaces or tabs

Actual output:

If the consensus is that tabs should instead be 4 spaces wide instead of 8, this PR will also target this issue with its auto-conversion of tabs to 4 spaces wide. The UG has also been updated to indicate this change. However, it is possible that existing MarkBind sites that use tabs in their fenced code will be affected by this change, since visually the tabs will be cut in half.

To avoid breaking existing sites, we could also choose to retain the 8 space wide tabs for backward compatibility. What does everyone think?

lhw-1 · 2025-02-18T05:18:20Z

@damithc in terms of the tab conversion to 4 instead of 8, would this comment still hold?

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.

Given that this is a visual change, and the only functionality (that I can think of at the moment) that might be affected by tab size is the highlight feature being merged in with this PR, I think it should be fine to make this change? I don't think the 8 spaces was an intentional implementation (given that 2 or 4 is the usual standard).

damithc · 2025-02-18T06:25:32Z

Given that this is a visual change, and the only functionality (that I can think of at the moment) that might be affected by tab size is the highlight feature being merged in with this PR, I think it should be fine to make this change? I don't think the 8 spaces was an intentional implementation (given that 2 or 4 is the usual standard).

@lhw-1 Yes, it should be fine. I don't use tabs in my code anyway. So, none of my MarkBind sites will be affected.

lhw-1 · 2025-02-26T06:43:47Z

@AgentHagu What is the status of this PR?

AgentHagu · 2025-02-26T08:39:42Z

@lhw-1 The PR should be finished, accomplishing the original task and adding auto conversion of tabs to 4 spaces.

lhw-1 · 2025-03-10T13:31:05Z

Once the PR is tested and good to go, we can merge it in. Thanks @AgentHagu!

lhw-1

LGTM! Works on testing. Thanks @AgentHagu for the PR!

github-actions · 2025-03-12T05:33:55Z

@lhw-1 Each PR must have a SEMVER impact label, please remember to label the PR properly.

lhw-1 · 2025-03-14T15:02:47Z

@all-contributors please add @AgentHagu for code

allcontributors · 2025-03-14T15:03:13Z

@lhw-1

I've put up a pull request to add @AgentHagu! 🎉

lhw-1 · 2025-03-14T15:13:12Z

@all-contributors please add @AgentHagu for code

allcontributors · 2025-03-14T15:13:16Z

@lhw-1

@AgentHagu already contributed before to code

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

AgentHagu changed the title ~~Add optional absolute char indexing for highlighting~~ Add optional absolute char indexing for highlighting (WIP) Feb 6, 2025

AgentHagu added 2 commits February 10, 2025 12:36

Add default tab conversion

ff20a24

Update User Guide to reflect new behavior

23f9830

AgentHagu changed the title ~~Add optional absolute char indexing for highlighting (WIP)~~ Add optional absolute char indexing for highlighting Feb 10, 2025

Merge branch 'master' into leading-whitespace-highlighting

7f3d200

Fix issues with tab conversion

2cff619

AgentHagu added 3 commits February 16, 2025 14:04

Fix bug with failing test case

8c9849a

Merge branch 'master' into leading-whitespace-highlighting

35b674c

Update user guide to better reflect tab conversion behavior

3ed3d84

lhw-1 added 2 commits February 20, 2025 00:11

Merge branch 'master' into leading-whitespace-highlighting

841f9c4

Merge branch 'master' into leading-whitespace-highlighting

3cef17f

lhw-1 and others added 2 commits February 26, 2025 17:34

Merge branch 'master' into leading-whitespace-highlighting

876b010

Merge branch 'master' into leading-whitespace-highlighting

51c0d84

lhw-1 added the r.Minor Version resolver: increment by 0.1.0 label Mar 10, 2025

Merge branch 'master' into leading-whitespace-highlighting

75f66a6

lhw-1 approved these changes Mar 12, 2025

View reviewed changes

lhw-1 changed the title ~~Add optional absolute char indexing for highlighting~~ Add code highlighting based on absolute positions Mar 12, 2025

lhw-1 merged commit d44d608 into MarkBind:master Mar 12, 2025
11 checks passed

lhw-1 added this to the v5.6.0 milestone Mar 13, 2025

lhw-1 added the breakingChange 💥 Feature will behave significantly different, or is made obsolete label Mar 14, 2025

allcontributors bot mentioned this pull request Mar 14, 2025

add AgentHagu as a contributor for code #2643

Merged

AgentHagu deleted the leading-whitespace-highlighting branch March 14, 2025 15:45

Add code highlighting based on absolute positions #2584

Add code highlighting based on absolute positions #2584

Uh oh!

Conversation

AgentHagu commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gerteck left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gerteck Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

AgentHagu Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

AgentHagu commented Jan 20, 2025

Uh oh!

gerteck commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damithc commented Jan 20, 2025

Uh oh!

AgentHagu commented Jan 20, 2025

Uh oh!

damithc commented Jan 20, 2025

Uh oh!

AgentHagu commented Jan 21, 2025

Uh oh!

damithc commented Jan 21, 2025

Uh oh!

AgentHagu commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damithc commented Jan 21, 2025

Uh oh!

lhw-1 left a comment

Choose a reason for hiding this comment

Uh oh!

lhw-1 Jan 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lhw-1 Jan 25, 2025

Choose a reason for hiding this comment

Uh oh!

AgentHagu Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AgentHagu commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lhw-1 commented Feb 5, 2025

Uh oh!

damithc commented Feb 5, 2025

Uh oh!

lhw-1 commented Feb 16, 2025

Uh oh!

AgentHagu commented Feb 16, 2025

Uh oh!

codecov bot commented Feb 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AgentHagu commented Feb 17, 2025

Uh oh!

lhw-1 commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damithc commented Feb 18, 2025

Uh oh!

lhw-1 commented Feb 26, 2025

Uh oh!

AgentHagu commented Feb 26, 2025

Uh oh!

lhw-1 commented Mar 10, 2025

Uh oh!

lhw-1 left a comment

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

codecov bot commented Feb 16, 2025 •

edited

Loading

lhw-1 commented Feb 18, 2025 •

edited

Loading