Highlighting: Implement slice format for text-only highlight by ryoarmanda · Pull Request #1336 · MarkBind/markbind

ryoarmanda · 2020-08-27T18:53:13Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Enhancement to an existing feature

Resolves #1311

What is the rationale for this request?
Line highlighting highlights the full width of the code block, which can interfere with the visual structure of the code. It would be better to highlight only the text portion of the code, ignoring indentations.

What changes did you make? (Give an overview)
Added/updated syntax for line highlighting:

Line text-only highlighting: lineNum.
Range text-only highlighting: lineStart-lineEnd.
Line whole-line highlighting: Now use lineNumber[:] (called unbounded line-slice)
Range whole-line highlighting: Now use lineStart[:]-lineEnd or lineStart-lineEnd[:] (basically either of the ends is an unbounded line-slice)

Provide some example code that this change will affect:

Example fenced code (presenting all the line-highlighting capabilities supported):

```js {start-from=5 highlight-lines="5,6[:],7-8,9[:]-10"}
function f(x) {
  const foo = 1;
  const bar = 2;
  const baz = 3;
  const square = x * x;
  return square;
}
```

Example view:

Is there anything you'd like reviewers to focus on?
I'm not sure whether the code organization is proper already. Some of the lines I modified are just following the style of that particular line(s). If you have any suggestions for tidying up, please let me know in the review!

Testing instructions:
markbind serve -d (to compile the modified markbind.css)

Proposed commit message: (wrap lines at 72 characters)
Add slice format for text-only code highlight

The highlight from the highlight-lines attribute spans the entire
width of the block, which might interfere with the visual structure
of the code.

Let's add ways for authors to highlight only the text part of the code.

ang-zeyu

@ryoarmanda I saw that you tagged it ready for review but the [wip] tag is still there; If it's still a wip do ignore these where applicable:

Code quality can be generally improved with some processing model(s) / class(es). (e.g. ruleComponents)
Should we use [:] to highlight the entire line instead? @damithc @ryoarmanda
Reason: since we are going with stripping trailing/leading spaces by default (or not?)
Also, when we have [2:5] eventually the 2 would be counted from the start of the line, including whitespaces. (but this may actually be undesirable versus counting from the first non-whitespace character, which should be easier for the author)

ang-zeyu · 2020-08-30T14:39:48Z

+    //                   it's more concise to write a for-loop so that we can perform both in one block.
+    for (let i = 0; i < highlightRules.length; i++) {
+      const rule = highlightRules[i];
+      const [a, b, c] = rule; // can be up to three items


since the processing model is getting heavy, let's create explicit class(es) for the intermediate processing result(s)

Oh yes, that would be nicer. I'll create a separate class file for the rule components and its helpers (e.g. isLineSlice)

ang-zeyu · 2020-08-30T14:44:07Z

-    const inRange = lineNumbersAndRanges.some(([start, end]) => {
-      if (start && end) {
-        return currentLineNumber >= start && currentLineNumber <= end;
+    // Edit (28/8/2020): I changed the approach from using some() to a simple for-loop. It is still O(n^2).


let's just remove the long comment from Note: The algorithm ...

this area is hardly a bottleneck in performance

Ah, okay, will do

ryoarmanda · 2020-09-01T06:07:30Z

Thanks for the review @ang-zeyu 👍

I'm still marking it WIP as I haven't updated the docs yet. Will remove it once it's done and have addressed the reviews.

Should we use [:] to highlight the entire line instead?

Ah so basically swapping the behaviour of e.g. 2-7 and 2[:]-7 right? I think it's a nice idea but it depends on what the expected common usage would be. For me personally I like to lean more into text-only highlighting as the default, and so reserving [:] just for line highlighting for convenience sake. But also would like to know your input as well @damithc

ang-zeyu · 2020-09-08T14:20:10Z

Should we use [:] to highlight the entire line instead? @damithc @ryoarmanda
Reason: since we are going with stripping trailing/leading spaces by default (or not?)
Also, when we have [2:5] eventually the 2 would be counted from the start of the line, including whitespaces. (but this may actually be undesirable versus counting from the first non-whitespace character, which should be easier for the author)

any thoughts? @damithc

damithc · 2020-09-08T15:15:34Z

I think the first thing to decide is if slice positions should include leading spaces or not.

pros of omitting leading spaces from counting: easier to count for authors

cons: not possible to start highlighting somewhere in the middle of leading spaces e.g., if I wanted to show how line wrapping requires 8 spaces for indentation, I may want to highlight the 8 leading spaces in a line to illustrate that.

As this is an advance feature not expected to be used frequently, perhaps we choose the more flexible option so that even rare cases become possible?

Regarding the use of [:], the advantage of using it to highlight the trimmed line (not the full line) is that the current syntax remains the same. The [s:e] syntax comes into play only if the user wants to do more advanced stuff with highlighting. This is assuming full line highlighting is going to remain the default.

ang-zeyu · 2020-09-09T10:24:48Z

I think the first thing to decide is if slice positions should include leading spaces or not.

pros of omitting leading spaces from counting: easier to count for authors

cons: not possible to start highlighting somewhere in the middle of leading spaces e.g., if I wanted to show how line wrapping requires 8 spaces for indentation, I may want to highlight the 8 leading spaces in a line to illustrate that.

As this is an advance feature not expected to be used frequently, perhaps we choose the more flexible option so that even rare cases become possible?

Agree here

Regarding the use of [:], the advantage of using it to highlight the trimmed line (not the full line) is that the current syntax remains the same. The [s:e] syntax comes into play only if the user wants to do more advanced stuff with highlighting. This is assuming full line highlighting is going to remain the default.

Wasn't #1311 to change the default behaviour of full line highlighting? 😮

In python (im assuming that's where the inspiration for this came from) omitting either s / e would slice all the way to the start / end, might be small plus to follow this too. (so if you want full line highlighting it would be [:])

damithc · 2020-09-09T10:45:04Z

Wasn't #1311 to change the default behaviour of full line highlighting? 😮

In python (im assuming that's where the inspiration for this came from) omitting either s / e would slice all the way to the start / end, might be small plus to follow this too. (so if you want full line highlighting it would be [:])

Oh, right :-p
OK, I'm sold. Let's do that.

ang-zeyu

Nice work extracting out the logic, let's try to remove the use of magic numbers next too:

ang-zeyu · 2020-10-05T13:27:29Z

+
+  isUnboundedSlice() {
+    return this.isSlice()
+      && this.components[1] === -1


let's remove the use of magic numbers in favour of named instance properties, so we don't need to refer back to the usage to understand which is which

ang-zeyu · 2020-10-05T13:29:11Z

+  shouldApplyHighlight(lineNumber) {
+    const compares = this.rule.map(comp => comp.compareLine(lineNumber));
+    if (this.isLineRange()) {
+      return (compares[0] <= 0) && (compares[1] >= 0);


could the magic numbers be removed?

but if this is a necessary part of the algorithm, maybe you could use some named constant declarations here, which are self-documenting

const whatIsThis = compares[0]

Yeah, compareLine is basically emulating what Java's compareTo does, returning either a positive integer, zero, or a negative integer depending on whether the given line number is before, at, or after the component's line number. So I guess it is inevitable to compare whether it's <= 0 like the above to ensure that the line number is within the bounds.

I'll just name these checks as constant declarations.

ang-zeyu · 2020-10-05T13:35:25Z

+  }
+
+  isLineSlice() {
+    return this.rule.length === 1 && this.rule[0].isSlice();


Since its used only in applyHighlight and is rather simple, perhaps we could just use a named constant instead? (ignore if this is going to be removed when dealing with the below)

ang-zeyu

Mostly looks good now 👍

Just a couple more nits:

ang-zeyu · 2020-10-07T11:12:28Z

+
+You can specify highlighting in many different ways, depending on how you want it to be. There are two main variants:
+
+##### Text-only highlighting


could try super / regular bold if h6 dosen't work too well

ang-zeyu · 2020-10-07T11:14:39Z

 <include src="codeAndOutputCode.md" boilerplate >
 <variable name="code">
-```java {highlight-lines="2,4,6-8"}
+```java {highlight-lines="1,3[:],6-8,10[:]-12"}


to clarify, what happens if its 10-12[:]?

It does the same thing as 10[:]-12 at the moment, highlights full-line.

ang-zeyu · 2020-10-07T11:16:44Z

+    /**
+     * @type Array<HighlightRuleComponent>
+     */
+    this.rule = rule;


let's use plural for arrays

or ruleComponents perhaps

ang-zeyu · 2020-10-07T11:20:57Z

+    return this.rule.length === 2;
+  }
+
+  static parseRule(ruleString) {


hmm is there a pattern to the organization of methods?

maybe we could shift this and the components counterpart below right below their constructors since its also somewhat of a constructor

I wrote it in the order of usage flow: parsing of the rule, post-parse processing (offset line numbers), test for application, and lastly the application of the rule itself. I feel like this way is intuitive if you want to read the whole class and understand the sequence of process.

Although, the special, structural checks like isLineSlice (and isUnboundedSlice on the components side) does not fit too well on this scheme. I put it where it was now (on top of everything, after the constructor) if any developers want to understand what are the possible variants of the structure possible for this class before reading deep into the class methods.

Not sure if this is the best way, I don't have a strong preference really. What do you think?

ok, thanks for the explanation!

How about shifting isLineRange to the bottom at least then? Then we can keep the constructors together (parseRule and constructor) while still fitting to your scheme. Likewise for the Component version

Okay then 👍 will be shifted to the bottom of the class

ang-zeyu · 2020-10-07T11:24:06Z

+
+class HighlightRuleComponent {
+  constructor(lineNumber, isSlice, bounds) {
+    this.lineNumber = lineNumber;


nice cleanup here 👍

ang-zeyu · 2020-10-07T11:31:43Z

+  }
+
+  offsetLines(offset) {
+    this.rule.map(comp => comp.offsetLineNumber(offset));


ang-zeyu · 2020-10-07T11:33:19Z

+    return HighlightRule._highlightTextOnly(line);
+  }
+
+  static _splitCodeAndIndentation(codeStr) {


let's keep related methods together; i.e. below _highlightTextOnly

ang-zeyu

~~Sorry, missed it out earlier~~

~~Let's add / modify some simple tests to testCodeBlocks.md in the test_site as well, so we can detect unintended changes in the future~~

Oops

Lgtm, thanks! @ryoarmanda

The highlight from the highlight-lines attribute spans the entire width of the block, which might interfere with the visual structure of the code. Let's add ways for authors to highlight only the text part of the code.

ryoarmanda added 12 commits August 28, 2020 02:44

Highlighting: Implement slice format for text-only highlight

c553ffe

Remove logger

013f546

Add newlines on highlighting return values

7f24ffd

Use match instead of matchAll

1308371

Update comments

fede374

Tidy parseAndZeroLineNumber

4922de9

Add test for line-slice highlight format

ad5beb3

Fix parseAndZeroLineNumber and rename to parseRule

95601fc

Fix test

d3772b8

Fix parseRule

8a79480

Add text-only range highlighting

89170f7

Update comment

1857b23

ryoarmanda marked this pull request as ready for review August 29, 2020 08:44

Update test to include range text-only highlight

8f0f345

ang-zeyu reviewed Aug 30, 2020

View reviewed changes

Remove comment on higlight approach

f7f3662

ryoarmanda added 6 commits October 4, 2020 23:00

Swap behaviour of line number and line empty slice

3dc9270

Refactor highlighting utility

26e8de2

Update highlight syntax reference

0acb089

Merge branch 'master' into code-highlight-text-only

c2c759c

Update codeblock CSS to select all highlighted elements

25ad0bb

Improve docs

546ed13

ryoarmanda changed the title ~~[WIP] Highlighting: Implement slice format for text-only highlight~~ Highlighting: Implement slice format for text-only highlight Oct 5, 2020

ang-zeyu reviewed Oct 5, 2020

View reviewed changes

Address review

ffebdf7

ang-zeyu reviewed Oct 7, 2020

View reviewed changes

ryoarmanda added 2 commits October 13, 2020 18:17

Address reviews

0c38f1a

Shift structure checks to bottom

4f2c6af

ang-zeyu reviewed Oct 19, 2020

View reviewed changes

ang-zeyu approved these changes Oct 19, 2020

View reviewed changes

ang-zeyu merged commit c5f79a1 into MarkBind:master Oct 19, 2020


		You can specify highlighting in many different ways, depending on how you want it to be. There are two main variants:

		##### Text-only highlighting

Uh oh!

Conversation

ryoarmanda commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ang-zeyu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryoarmanda commented Sep 1, 2020

Uh oh!

ang-zeyu commented Sep 8, 2020

Uh oh!

damithc commented Sep 8, 2020

Uh oh!

ang-zeyu commented Sep 9, 2020

Uh oh!

damithc commented Sep 9, 2020

Uh oh!

ang-zeyu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ang-zeyu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryoarmanda Oct 13, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

ryoarmanda Oct 19, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ang-zeyu 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ryoarmanda commented Aug 27, 2020 •

edited

Loading

ryoarmanda Oct 13, 2020 •

edited

Loading

ryoarmanda Oct 19, 2020 •

edited

Loading

ang-zeyu left a comment •

edited

Loading