Revise code structure settings. (#249)

Introduce two new options replacing L-shaped blocks:

- Full, with background highlighting and borders for the active block.
- Simple, with L-shaped borders.

Behind the scenes there's rather more configurability than before.
We might use this for future experimentation or, in time, drop it.

We may revisit the distinctiveness of the active block highlight (in
Full mode) and review the contrast with the syntax highlighting colors.
Leaving the issue open for now to discuss this and raise new issues as
required.

See #207

Author: microbit-matt-hillsdon

.circleci/config.yml

@@ -33,7 +33,7 @@ _steps:
   install_dependencies: &install_dependencies
     run: npm ci --cache .npm-cache && sudo npm i -g --registry https://npm.pkg.github.com/microbit-foundation @microbit-foundation/website-deploy-aws@0.2.0 @microbit-foundation/website-deploy-aws-config@0.4.2 @microbit-foundation/circleci-npm-package-versioner@1
   install_theme: &install_theme
-    run: npm install --no-save --registry https://npm.pkg.github.com/microbit-foundation @microbit-foundation/python-editor-next-microbit@0.1.0-dev.45
+    run: npm install --no-save --registry https://npm.pkg.github.com/microbit-foundation @microbit-foundation/python-editor-next-microbit@0.1.0-dev.53
   update_version: &update_version
     run: npm run ci:update-version
   build: &build

src/common/use-local-storage.test.tsx

@@ -0,0 +1,55 @@
+import { fireEvent, render, screen } from "@testing-library/react";
+import React from "react";
+import { useLocalStorage } from "./use-local-storage";
+
+interface TestState {
+  x: number;
+  y: number;
+  z: number;
+}
+
+const validate = (o: unknown): o is TestState => true;
+
+const Test = () => {
+  const [state, setState] = useLocalStorage<TestState>("key", validate, {
+    x: 1,
+    y: 1,
+    z: 1,
+  });
+  return (
+    <div>
+      <p data-testid="value">{state.x}</p>
+      <button onClick={() => setState({ ...state, x: state.x + 1 })}>
+        increment x
+      </button>
+    </div>
+  );
+};
+
+describe("useLocalStorage", () => {
+  it("reads/writes to/from local storage", () => {
+    localStorage.setItem("key", JSON.stringify({ x: 9, y: 10, z: 11 }));
+
+    render(<Test />);
+    fireEvent.click(screen.getByText("increment x"));
+
+    expect(JSON.parse(localStorage.getItem("key")!)).toEqual({
+      x: 10,
+      y: 10,
+      z: 11,
+    });
+  });
+
+  it("aligns top-level keys with default value", () => {
+    localStorage.setItem("key", JSON.stringify({ a: 1, z: 11 }));
+
+    render(<Test />);
+    fireEvent.click(screen.getByText("increment x"));
+
+    expect(JSON.parse(localStorage.getItem("key")!)).toEqual({
+      x: 2,
+      y: 1,
+      z: 11,
+    });
+  });
+});

src/common/use-local-storage.ts

@@ -17,10 +17,21 @@ export function useLocalStorage<T>(
     const value = localStorage.getItem(key);
     if (value !== null) {
       try {
-        const parsed = JSON.parse(value);
+        let parsed = JSON.parse(value);
+        // Ensure we get new top-level defaults.
+        parsed = {
+          ...defaultValue,
+          ...parsed,
+        };
+        // Remove any top-level keys that aren't present in defaults.
+        const unknownKeys = new Set(Object.keys(parsed));
+        Object.keys(defaultValue).forEach((k) => unknownKeys.delete(k));
+        unknownKeys.forEach((k) => delete parsed[k]);
+
         if (!validate(parsed)) {
           throw new Error("Invalid data stored in local storage");
         }
+
         return parsed;
       } catch (e) {
         // Better than exploding forever.

src/deployment/default/colors.ts

@@ -14,8 +14,11 @@ const colors = {
   brand: gray,
   gray,
   code: {
-    block: "rgba(52, 162, 235, 0.06)",
-    border: theme.colors.gray[400],
+    blockBorder: theme.colors.gray[400],
+    blockBackground: "rgba(185, 185, 185, 0.1)",
+    blockBackgroundActive: "lightyellow",
+    blockBorderActive: theme.colors.blue[400],
+
     comment: "gray",
     default: "black",
     keyword: "darkblue",

src/editor/EditorContainer.tsx

@@ -4,7 +4,7 @@
  * SPDX-License-Identifier: MIT
  */
 import { useProjectFileText } from "../project/project-hooks";
-import { useSettings } from "../settings/settings";
+import { codeStructureSettings, useSettings } from "../settings/settings";
 import { WorkbenchSelection } from "../workbench/use-selection";
 import Editor from "./codemirror/CodeMirror";
 
@@ -17,15 +17,15 @@ interface EditorContainerProps {
  * and wires it to the currently open file.
  */
 const EditorContainer = ({ selection }: EditorContainerProps) => {
-  const [{ fontSize, codeStructureHighlight }] = useSettings();
+  const [settings] = useSettings();
   const [defaultValue, onFileChange] = useProjectFileText(selection.file);
   return typeof defaultValue === "undefined" ? null : (
     <Editor
       defaultValue={defaultValue}
       location={selection.location}
       onChange={onFileChange}
-      fontSize={fontSize}
-      codeStructureHighlight={codeStructureHighlight}
+      fontSize={settings.fontSize}
+      codeStructureSettings={codeStructureSettings(settings)}
     />
   );
 };

src/editor/codemirror/CodeMirror.tsx

@@ -7,12 +7,12 @@ import { EditorSelection, EditorState } from "@codemirror/state";
 import { EditorView } from "@codemirror/view";
 import { useEffect, useMemo, useRef } from "react";
 import { useIntl } from "react-intl";
-import { CodeStructureHighlight } from "../../settings/settings";
 import { FileLocation } from "../../workbench/use-selection";
 import "./CodeMirror.css";
 import { editorConfig, themeExtensionsCompartment } from "./config";
 import {
-  structureHighlighting,
+  codeStructure,
+  CodeStructureSettings,
   structureHighlightingCompartment,
 } from "./structure-highlighting";
 import themeExtensions from "./themeExtensions";
@@ -24,7 +24,7 @@ interface CodeMirrorProps {
 
   location: FileLocation;
   fontSize: number;
-  codeStructureHighlight: CodeStructureHighlight;
+  codeStructureSettings: CodeStructureSettings;
 }
 
 /**
@@ -41,7 +41,7 @@ const CodeMirror = ({
   onChange,
   location,
   fontSize,
-  codeStructureHighlight,
+  codeStructureSettings,
 }: CodeMirrorProps) => {
   const elementRef = useRef<HTMLDivElement | null>(null);
   const viewRef = useRef<EditorView | null>(null);
@@ -51,9 +51,9 @@ const CodeMirror = ({
   const options = useMemo(
     () => ({
       fontSize,
-      codeStructureHighlight,
+      codeStructureSettings,
     }),
-    [fontSize, codeStructureHighlight]
+    [fontSize, codeStructureSettings]
   );
 
   useEffect(() => {
@@ -71,7 +71,7 @@ const CodeMirror = ({
           editorConfig,
           // Extensions we enable/disable based on props.
           structureHighlightingCompartment.of(
-            structureHighlighting(options.codeStructureHighlight)
+            codeStructure(options.codeStructureSettings)
           ),
           themeExtensionsCompartment.of(themeExtensions(options.fontSize)),
         ],
@@ -101,7 +101,7 @@ const CodeMirror = ({
           themeExtensions(options.fontSize)
         ),
         structureHighlightingCompartment.reconfigure(
-          structureHighlighting(options.codeStructureHighlight)
+          codeStructure(options.codeStructureSettings)
         ),
       ],
     });

src/editor/codemirror/structure-highlighting/doc-util.ts

@@ -9,12 +9,19 @@
 
 import { EditorState } from "@codemirror/state";
 
-export const skipTrailingBlankLines = (
-  state: EditorState,
-  position: number
-) => {
+/**
+ * Skips trailing comments (regardless of indent) and blank lines at the end of the body.
+ *
+ * @param state Document state.
+ * @param position Document character position.
+ * @returns End of last line we consider to be part of the body after skipping.
+ */
+export const skipBodyTrailers = (state: EditorState, position: number) => {
   let line = state.doc.lineAt(position);
-  while ((line.length === 0 || /^\s+$/.test(line.text)) && line.number >= 1) {
+  while (
+    line.number >= 1 &&
+    (line.length === 0 || /^\s+$/.test(line.text) || /^\s*#/.test(line.text))
+  ) {
     line = state.doc.line(line.number - 1);
   }
   return line.to;

src/editor/codemirror/structure-highlighting/index.ts

@@ -8,60 +8,32 @@
  */
 
 import { Compartment, Extension } from "@codemirror/state";
-import { CodeStructureHighlight } from "../../../settings/settings";
-import { baseTheme, themeTweakForBackgrounds } from "./theme";
+import { baseTheme } from "./theme";
 import { codeStructureView } from "./view";
 
 export const structureHighlightingCompartment = new Compartment();
 
+export type CodeStructureShape = "l-shape" | "box";
+export type CodeStructureBackground = "block" | "none";
+export type CodeStructureBorders = "borders" | "none" | "left-edge-only";
+
 export interface CodeStructureSettings {
-  shape: "l-shape" | "box";
-  background: "block" | "none";
-  borders: "borders" | "no-borders" | "left-edge-only";
+  shape: CodeStructureShape;
+  background: CodeStructureBackground;
+  borders: CodeStructureBorders;
 
-  hoverBackground?: boolean;
   cursorBackground?: boolean;
-  hoverBorder?: boolean;
-  cursorBorder?: boolean;
+  cursorBorder?: CodeStructureBorders;
 }
 
-// We'll switch to exporting this soon, see below.
-const codeStructure = (settings: CodeStructureSettings): Extension => [
-  codeStructureView(settings),
-  baseTheme,
-  settings.background !== "none" ||
-  settings.cursorBackground ||
-  settings.hoverBackground
-    ? themeTweakForBackgrounds
-    : [],
-];
-
-// This will go soon in favour of more fine-grained settings
-export const structureHighlighting = (
-  option: CodeStructureHighlight
-): Extension => {
-  switch (option) {
-    case "l-shapes":
-      return codeStructure({
-        shape: "l-shape",
-        background: "none",
-        borders: "left-edge-only",
-      });
-    case "boxes":
-      return codeStructure({
-        shape: "box",
-        background: "block",
-        borders: "no-borders",
-      });
-    case "l-shape-boxes":
-      return codeStructure({
-        shape: "l-shape",
-        background: "block",
-        borders: "no-borders",
-      });
-    case "none":
-      return [];
-    default:
-      return [];
-  }
+/**
+ * Creates a CodeMirror extension that provides structural highlighting
+ * based on the CodeMirror syntax tree. The intent is to aid code comprehension
+ * and provide clues when indentation isn't correct.
+ *
+ * @param settings Settings for the code structure CodeMirror extension.
+ * @returns A appropriately configured extension.
+ */
+export const codeStructure = (settings: CodeStructureSettings): Extension => {
+  return [codeStructureView(settings), baseTheme];
 };