Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional absolute char indexing for highlighting #2584

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add optional absolute char indexing for highlighting #2584

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e4f0e2b
Update highlighter functionality to support new regex
AgentHagu Jan 13, 2025
78c8e84
Add new test cases for highlighter
AgentHagu Jan 13, 2025
46901a3
Update documentation for computeCharBounds
AgentHagu Jan 17, 2025
f679edb
Fix issue regarding failing test cases
AgentHagu Jan 20, 2025
fe14d75
Update symbol and position
AgentHagu Jan 20, 2025
7cb451b
Update documentation to follow idea of absolute char indexing
AgentHagu Jan 27, 2025
2245743
Refactor code to improve readability
AgentHagu Jan 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 27 additions & 8 deletions packages/core/src/lib/markdown-it/highlight/HighlightRuleComponent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { splitCodeAndIndentation } from './helper';

const LINESLICE_CHAR_REGEX = /(\d+)\[(\d*):(\d*)]/;
const LINESLICE_CHAR_REGEX = /(\d+)\[(\d*):(\d*)](\+?)/;
const LINESLICE_WORD_REGEX = /(\d+)\[(\d*)::(\d*)]/;
const LINEPART_REGEX = /(\d+)\[(["'])((?:\\.|[^\\])*?)\2]/;
const UNBOUNDED = -1;
Expand Down Expand Up @@ -43,22 +43,29 @@ export class HighlightRuleComponent {
const linesliceWordMatch = compString.match(LINESLICE_WORD_REGEX);
const sliceMatch = linesliceCharMatch || linesliceWordMatch;
if (sliceMatch) {
// There are four capturing groups: [full match, line number, start bound, end bound]
// There are four/five capturing groups: [full match, line number, start bound,
// end bound,highlight spaces]
const groups = sliceMatch.slice(1); // discard full match

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

let highlightSpaces = false;
if (sliceMatch === linesliceCharMatch) {
highlightSpaces = groups.pop() === '+';
}

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

if (isUnbounded) {
return new HighlightRuleComponent(lineNumber, true, []);
}

let bound = groups.map(x => (x !== '' ? parseInt(x, 10) : UNBOUNDED)) as [number, number];
const isCharSlice = sliceMatch === linesliceCharMatch;
bound = isCharSlice
? HighlightRuleComponent.computeCharBounds(bound, lines[lineNumber - 1])
? HighlightRuleComponent.computeCharBounds(bound, lines[lineNumber - 1], highlightSpaces)
: HighlightRuleComponent.computeWordBounds(bound, lines[lineNumber - 1]);

return new HighlightRuleComponent(lineNumber, true, [bound]);
Expand Down Expand Up @@ -95,38 +102,50 @@ export class HighlightRuleComponent {
* comparing the bounds and the line's range.
*
* If the bound does not specify either the start or the end bound, the computed bound will default
* to the start or end of line, excluding leading whitespaces.
* to the start or end of line. The bound will either include or exclude leading whitespaces depending
* on the user's preference.
*
* @param bound The user-defined bound
* @param line The given line
* @param highlightSpaces Whether to highlight leading whitespaces
* @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] {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! Thanks

const [indents] = splitCodeAndIndentation(line);
let [start, end] = bound;

if (start === UNBOUNDED) {
start = indents.length;
} else {
if (highlightSpaces) {
start = 0;
} else {
start = indents.length;
}
} else if (!highlightSpaces) {
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;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.


if (end === UNBOUNDED) {
end = line.length;
} else {
} else if (!highlightSpaces) {
end += indents.length;

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

return [start, end];
Expand Down
37 changes: 32 additions & 5 deletions packages/core/test/unit/lib/markdown-it/highlight/HighlightRuleComponent.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,28 +44,55 @@ describe('parseRuleComponent', () => {
});
});

describe('computeCharBounds', () => {
describe('computeCharBounds, no whitespace highlighting', () => {
test('computes character bounds correctly', () => {
const bounds = HighlightRuleComponent.computeCharBounds([2, 5], ' some text');
const bounds = HighlightRuleComponent.computeCharBounds([2, 5], ' some text', false);
expect(bounds).toEqual([4, 7]);
});

test('handles unbounded start correctly', () => {
const bounds = HighlightRuleComponent.computeCharBounds([-1, 4], ' some text');
const bounds = HighlightRuleComponent.computeCharBounds([-1, 4], ' some text', false);
expect(bounds).toEqual([2, 6]);
});

test('handles unbounded end correctly', () => {
const bounds = HighlightRuleComponent.computeCharBounds([3, -1], ' some text');
const bounds = HighlightRuleComponent.computeCharBounds([3, -1], ' some text', false);
expect(bounds).toEqual([5, ' some text'.length]);
});

test('handles out-of-range bounds correctly', () => {
const bounds = HighlightRuleComponent.computeCharBounds([30, 40], ' some text');
const bounds = HighlightRuleComponent.computeCharBounds([30, 40], ' some text', false);
expect(bounds).toEqual([' some text'.length, ' some text'.length]);
});
});

describe('computeCharBounds, with whitespace highlighting', () => {
test('computes character bounds correctly', () => {
const bounds = HighlightRuleComponent.computeCharBounds([2, 5], ' some text', true);
expect(bounds).toEqual([2, 5]);
});

test('handles unbounded start correctly', () => {
const bounds = HighlightRuleComponent.computeCharBounds([-1, 4], ' some text', true);
expect(bounds).toEqual([0, 4]);
});

test('handles unbounded end correctly', () => {
const bounds = HighlightRuleComponent.computeCharBounds([3, -1], ' some text', true);
expect(bounds).toEqual([3, ' some text'.length]);
});

test('handles out-of-range bounds correctly', () => {
const bounds = HighlightRuleComponent.computeCharBounds([30, 40], ' some text', true);
expect(bounds).toEqual([' some text'.length, ' some text'.length]);
});

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

Choose a reason for hiding this comment

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

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

Copy link
Author

@AgentHagu AgentHagu Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

expect(bounds).toEqual([0, 4]);
});
});

describe('computeWordBounds', () => {
test('computes word bounds correctly', () => {
const bounds = HighlightRuleComponent.computeWordBounds([1, 2], ' some text here');
Expand Down
Loading