Cleanup; more tests

Author: pokey

packages/cursorless-vscode/src/ide/vscode/VSCodeScopeVisualizer/VscodeFancyRangeHighlighter/generateDecorationsForCharacterRange/handleMultipleLines.test.ts

@@ -1,28 +1,34 @@
 import assert = require("assert");
-import { BorderStyle, DecorationStyle } from "../decorationStyle.types";
+import { BorderStyle } from "../decorationStyle.types";
 import { handleMultipleLines } from "./handleMultipleLines";
 import { Range } from "@cursorless/common";
+import { map } from "itertools";
 
-type CharacterOffsets = [number, number];
+const solid = BorderStyle.solid;
+const porous = BorderStyle.porous;
+const none = BorderStyle.none;
 
-interface StyledOffsets {
-  style: DecorationStyle;
-  offsets: CharacterOffsets;
-}
+type CharacterOffsets = [number, number];
 
 /**
- * Compact representation of an input to `handleMultipleLines`. The first
- * element is the character offsets of the first line, and the rest are the
- * character offsets of the end of the remaining lines.  We use a single number
- * for lines after the first because they always start at character 0.
+ * Compact representation of an input to `handleMultipleLines`:
+ *
+ * [firstLine, ...rest]
+ *
+ * The first element is the character offsets of the first line, and the rest
+ * are the character offsets of the end of the remaining lines.  We use a single
+ * number for lines after the first because they always start at character 0.
  */
 type Input = [CharacterOffsets, ...number[]];
 
 /**
- * Compact representation of the expected highlights for a single line. The
- * first element is the line number, the second is the character offsets, and
- * the third is the border styles for the top, bottom, left, and right borders
- * respectively.
+ * Compact representation of the expected highlights for a single line:
+ *
+ * [lineNumber, [start, end], [top, right, bottom, left]
+ *
+ * The first element is the line number, the second is the character offsets of
+ * the highlight, and the third is the border style for each side of the
+ * highlight.
  */
 type LineDecorations = [
   number,
@@ -35,10 +41,6 @@ interface TestCase {
   expected: LineDecorations[];
 }
 
-const solid = BorderStyle.solid;
-const porous = BorderStyle.porous;
-const none = BorderStyle.none;
-
 const testCases: TestCase[] = [
   {
     input: [[0, 1], 1],
@@ -71,28 +73,63 @@ const testCases: TestCase[] = [
       [2, [0, 0], [porous, solid, solid, porous]],
     ],
   },
+  {
+    input: [[2, 3], 1],
+    expected: [
+      [0, [2, 3], [solid, porous, solid, solid]],
+      [1, [0, 1], [solid, solid, solid, porous]],
+    ],
+  },
+  {
+    input: [[1, 3], 4, 2],
+    expected: [
+      [0, [1, 3], [solid, porous, none, solid]],
+
+      [1, [0, 1], [solid, none, none, porous]],
+      [1, [1, 2], [none, none, none, none]],
+      [1, [2, 3], [none, none, solid, none]],
+      [1, [3, 4], [porous, porous, solid, none]],
+
+      [2, [0, 2], [none, solid, solid, porous]],
+    ],
+  },
+  {
+    input: [[0, 2], 1],
+    expected: [
+      [0, [0, 1], [solid, none, none, solid]],
+      [0, [1, 2], [solid, porous, solid, none]],
+      [1, [0, 1], [none, solid, solid, porous]],
+    ],
+  },
+  {
+    input: [[0, 2], 1, 0],
+    expected: [
+      [0, [0, 1], [solid, none, none, solid]],
+      [0, [1, 2], [solid, porous, porous, none]],
+      [1, [0, 1], [none, porous, solid, porous]],
+      [2, [0, 0], [none, solid, solid, porous]],
+    ],
+  },
 ];
 
 suite("handleMultipleLines", () => {
   for (const testCase of testCases) {
     test(JSON.stringify(testCase.input), () => {
       const [firstLine, ...rest] = testCase.input;
 
-      const actual = [
-        ...handleMultipleLines([
+      const actual: LineDecorations[] = map(
+        handleMultipleLines([
           new Range(0, firstLine[0], 0, firstLine[1]),
           ...rest.map((end, index) => new Range(index + 1, 0, index + 1, end)),
         ]),
-      ];
-      assert.deepStrictEqual(
-        actual,
-        testCase.expected.map(
-          ([lineNumber, [start, end], [top, right, bottom, left]]) => ({
-            range: new Range(lineNumber, start, lineNumber, end),
-            style: { top, right, bottom, left },
-          }),
-        ),
+        ({ range, style }) => [
+          range.start.line,
+          [range.start.character, range.end.character],
+          [style.top, style.right, style.bottom, style.left],
+        ],
       );
+
+      assert.deepStrictEqual(actual, testCase.expected);
     });
   }
 });

packages/cursorless-vscode/src/ide/vscode/VSCodeScopeVisualizer/VscodeFancyRangeHighlighter/generateDecorationsForCharacterRange/handleMultipleLines.ts

@@ -37,7 +37,7 @@ export function* handleMultipleLines(
  * previous and next lines.
  */
 function* handleLine(lineInfo: LineInfo): Iterable<StyledRange> {
-  const { lineNumber, previousLine, currentLine, nextLine } = lineInfo;
+  const { lineNumber, currentLine, nextLine } = lineInfo;
 
   /** A list of "events", corresponding to the start or end of a line */
   const events: Event[] = getEvents(lineInfo);
@@ -47,39 +47,36 @@ function* handleLine(lineInfo: LineInfo): Iterable<StyledRange> {
    * the fly.
    */
   const currentDecoration: Omit<DecorationStyle, "right"> = {
-    // Draw a solid top line if whatever is above us isn't part of our range.
-    // Otherwise draw no line so it merges in with the line above.
-    top:
-      previousLine == null || previousLine.isFirst
-        ? BorderStyle.solid
-        : BorderStyle.none,
-    // Analogous to above, but only care if we're last; doesn't matter if next
-    // line is last because it is guaranteed to start at char 0
+    // Start with a solid top border. We'll switch to no border when previous
+    // line begins.  Don't need to worry about porous because only the first
+    // line can start after char 0.
+    top: BorderStyle.solid,
+
+    // Start with a solid bottom border if we're the last line, otherwise no
+    // border because we'll blend with the next line.
     bottom: currentLine.isLast ? BorderStyle.solid : BorderStyle.none,
+
     // Start with a porous border if we're continuing from previous line
     left: currentLine.isFirst ? BorderStyle.solid : BorderStyle.porous,
   };
 
   let currentOffset = currentLine.start;
   let yieldedAnything = false;
-  let isDone = false;
-
-  for (const { offset, lineType, isStart } of events) {
-    if (isDone) {
-      break;
-    }
 
-    if (offset > currentOffset) {
+  // NB: The `loop` label here allows us to break out of the loop from inside
+  // the switch statement.
+  loop: for (const event of events) {
+    if (event.offset > currentOffset) {
       // If we've moved forward at all since the last event, yield a decoration
       // for the range between the last event and this one.
       yield {
-        range: new Range(lineNumber, currentOffset, lineNumber, offset),
+        range: new Range(lineNumber, currentOffset, lineNumber, event.offset),
         style: {
           ...currentDecoration,
           // If we're done with this line, draw a border, otherwise don't, so that
           // it merges in with the next decoration for this line.
           right:
-            offset === currentLine.end
+            event.offset === currentLine.end
               ? currentLine.isLast
                 ? BorderStyle.solid
                 : BorderStyle.porous
@@ -88,40 +85,30 @@ function* handleLine(lineInfo: LineInfo): Iterable<StyledRange> {
       };
       yieldedAnything = true;
       currentDecoration.left = BorderStyle.none;
+      currentOffset = event.offset;
     }
 
-    switch (lineType) {
+    switch (event.lineType) {
       case LineType.previous:
         // Use no top border when overlapping with previous line so it visually
         // merges; otherwise use porous border to show nice cutoff effect.
-        currentDecoration.top = isStart ? BorderStyle.none : BorderStyle.porous;
+        currentDecoration.top = event.isLineStart
+          ? BorderStyle.none
+          : BorderStyle.porous;
         break;
-      case LineType.current:
-        if (!isStart) {
-          isDone = true;
-        }
+      case LineType.current: // event.isLineStart === false
+        break loop;
+      case LineType.next: // event.isLineStart === false
+        currentDecoration.bottom = nextLine!.isLast
+          ? BorderStyle.solid
+          : BorderStyle.porous;
         break;
-      case LineType.next:
-        // Blend with next line while it is overlapping with us; then switch
-        // to solid or porous, depending if it is the last line.
-        if (isStart) {
-          currentDecoration.bottom = BorderStyle.none;
-        } else {
-          currentDecoration.bottom = nextLine!.isLast
-            ? BorderStyle.solid
-            : BorderStyle.porous;
-        }
-        break;
-    }
-
-    if (currentOffset < offset) {
-      // This guard is necessary so we don't accidentally jump backward if an
-      // adjacent line starts before we do.
-      currentOffset = offset;
     }
   }
 
   if (!yieldedAnything) {
+    // If current line is empty, then we didn't yield anything in the loop above,
+    // so yield a decoration for the whole line.
     yield {
       range: new Range(
         lineNumber,
@@ -137,12 +124,26 @@ function* handleLine(lineInfo: LineInfo): Iterable<StyledRange> {
   }
 }
 
-interface Event {
+interface PreviousLineEvent {
   offset: number;
-  lineType: LineType;
-  isStart: boolean;
+  lineType: LineType.previous;
+  isLineStart: boolean;
 }
 
+interface CurrentLineEvent {
+  offset: number;
+  lineType: LineType.current;
+  isLineStart: false;
+}
+
+interface NextLineEvent {
+  offset: number;
+  lineType: LineType.next;
+  isLineStart: false;
+}
+
+type Event = PreviousLineEvent | CurrentLineEvent | NextLineEvent;
+
 enum LineType {
   previous = -1,
   current = 0,
@@ -162,12 +163,12 @@ function getEvents({ previousLine, currentLine, nextLine }: LineInfo) {
       {
         offset: previousLine.start,
         lineType: LineType.previous,
-        isStart: true,
+        isLineStart: true,
       },
       {
         offset: previousLine.end,
         lineType: LineType.previous,
-        isStart: false,
+        isLineStart: false,
       },
     );
   }
@@ -177,14 +178,14 @@ function getEvents({ previousLine, currentLine, nextLine }: LineInfo) {
   events.push({
     offset: currentLine.end,
     lineType: LineType.current,
-    isStart: false,
+    isLineStart: false,
   });
 
   if (nextLine != null) {
     events.push({
       offset: nextLine.end,
       lineType: LineType.next,
-      isStart: false,
+      isLineStart: false,
     });
   }
 
@@ -198,7 +199,7 @@ function getEvents({ previousLine, currentLine, nextLine }: LineInfo) {
       if (a.lineType === LineType.current) {
         return 1;
       }
-      return a.isStart ? -1 : 1;
+      return a.isLineStart ? -1 : 1;
     }
 
     return a.offset - b.offset;