Drag and drop fixes (#1037)

Default to appending to existing blocks but allow control over the indent level where it can't cause invalid indentation.

Fix handling of whitespace in "while smooshing".

This makes a 4 space assumption, like tab, autoindent and code structure highlighting. However, drag and drop didn't previously do so, so worth flagging here.

Closes #856.
Closes #812.
Closes #800.
Closes #539.

Co-authored-by: Robert Knight <robert@microbit.org>

Author: microbit-matt-hillsdon

src/editor/codemirror/copypaste.ts

@@ -51,16 +51,17 @@ const copyPasteHandlers = () => [
         message: pasteContext.id,
       });
 
-      const lineNumber = view.state.doc.lineAt(
-        view.state.selection.ranges[0].from
-      ).number;
+      const line = view.state.doc.lineAt(view.state.selection.ranges[0].from);
+      const lineNumber = line.number;
+      const column = view.state.selection.ranges[0].from - line.from;
 
       view.dispatch(
         calculateChanges(
           view.state,
           pasteContext.codeWithImports,
           pasteContext.type,
           lineNumber,
+          Math.floor(column / 4),
           true
         )
       );

src/editor/codemirror/dnd.ts

@@ -26,13 +26,18 @@ interface LastDragPos {
   /**
    * The last drag position.
    */
-  line: number;
+  logicalPosition: LogicalPosition;
   /**
    * The inverse set of changes to the changes made for preview.
    */
   previewUndo: ChangeSet;
 }
 
+interface LogicalPosition {
+  line: number;
+  indent: number | undefined;
+}
+
 export type CodeInsertType =
   /**
    * A potentially multi-line example snippet.
@@ -109,21 +114,23 @@ const dndHandlers = ({ sessionSettings, setSessionSettings }: DragTracker) => {
         if (dragContext) {
           event.preventDefault();
 
-          const visualLine = view.visualLineAtHeight(event.y || event.clientY);
-          const line = view.state.doc.lineAt(visualLine.from);
-
-          if (line.number !== lastDragPos?.line) {
-            debug("  dragover", line);
+          const logicalPosition = findLogicalPosition(view, event);
+          if (
+            logicalPosition.line !== lastDragPos?.logicalPosition.line ||
+            logicalPosition.indent !== lastDragPos?.logicalPosition.indent
+          ) {
+            debug("  dragover", logicalPosition);
             revertPreview(view);
 
             const transaction = calculateChanges(
               view.state,
               dragContext.code,
               dragContext.type,
-              line.number
+              logicalPosition.line,
+              logicalPosition.indent
             );
             lastDragPos = {
-              line: line.number,
+              logicalPosition,
               previewUndo: transaction.changes.invert(view.state.doc),
             };
             // Take just the changes, skip the selection updates we perform on drop.
@@ -189,16 +196,16 @@ const dndHandlers = ({ sessionSettings, setSessionSettings }: DragTracker) => {
         clearSuppressChildDragEnterLeave(view);
         event.preventDefault();
 
-        const visualLine = view.visualLineAtHeight(event.y || event.clientY);
-        const line = view.state.doc.lineAt(visualLine.from);
-
+        const logicalPosition = findLogicalPosition(view, event);
         revertPreview(view);
         view.dispatch(
           calculateChanges(
             view.state,
             dragContext.code,
             dragContext.type,
-            line.number
+            logicalPosition.line,
+            logicalPosition.indent,
+            false
           )
         );
         view.focus();
@@ -207,6 +214,24 @@ const dndHandlers = ({ sessionSettings, setSessionSettings }: DragTracker) => {
   ];
 };
 
+const findLogicalPosition = (
+  view: EditorView,
+  event: DragEvent
+): { line: number; indent: number | undefined } => {
+  const visualLine = view.visualLineAtHeight(event.y || event.clientY);
+  const line = view.state.doc.lineAt(visualLine.from);
+  const pos = view.posAtCoords({
+    x: event.x || event.clientX,
+    y: event.y || event.clientY,
+  });
+  const column = pos ? pos - visualLine.from : undefined;
+  const indent = column ? Math.floor(column / 4) : undefined;
+  return {
+    line: line.number,
+    indent,
+  };
+};
+
 interface DragTracker {
   sessionSettings: SessionSettings;
   setSessionSettings: (sessionSettings: SessionSettings) => void;

src/editor/codemirror/edits.test.ts

@@ -14,20 +14,33 @@ describe("edits", () => {
     additional,
     expected,
     line,
+    indentLevelHint,
     type,
   }: {
     initial: string;
     additional: string;
     expected: string;
     line?: number;
+    indentLevelHint?: number;
     type?: CodeInsertType;
   }) => {
+    // Tabs are more maintainable in the examples but we actually work with 4 space indents.
+    initial = initial.replaceAll("\t", "    ");
+    additional = additional.replaceAll("\t", "    ");
+    expected = expected.replaceAll("\t", "    ");
+
     const state = EditorState.create({
       doc: initial,
       extensions: [python()],
     });
     const transaction = state.update(
-      calculateChanges(state, additional, type ?? "example", line)
+      calculateChanges(
+        state,
+        additional,
+        type ?? "example",
+        line,
+        indentLevelHint
+      )
     );
     const actual = transaction.newDoc.sliceString(0);
     const expectedSelection = expected.indexOf("█");
@@ -224,6 +237,15 @@ describe("edits", () => {
       type: "example",
     });
   });
+  it("while True inside while True is a special case even when inserting after document end", () => {
+    check({
+      line: 10,
+      initial: "while True:\n    a = 1\n",
+      additional: "while True:\n    b = 2\n",
+      expected: `while True:\n    a = 1\n${"\n".repeat(7)}    b = 2\n`,
+      type: "example",
+    });
+  });
   it("inside while False is not a special case", () => {
     check({
       line: 2,
@@ -387,4 +409,88 @@ describe("edits", () => {
       type: "example",
     });
   });
+  it("is not misled by whitespace on blank lines - 1", () => {
+    check({
+      line: 3,
+      initial: "while True:\n    print('Hi')\n   \n",
+      additional: "print('Bye')",
+      expected: "while True:\n    print('Hi')\n    print('Bye')\n   \n",
+      type: "example",
+    });
+  });
+  it("is not misled by whitespace on blank lines - 2", () => {
+    check({
+      line: 2,
+      initial: "while True:\n  \n    print('Hi')\n",
+      additional: "print('Bye')",
+      expected: "while True:\n    print('Bye')\n  \n    print('Hi')\n",
+      type: "example",
+    });
+  });
+  it("uses indent hint", () => {
+    check({
+      line: 3,
+      initial: "if True:\n\tprint('a')\n",
+      additional: "print('b')",
+      expected: "if True:\n\tprint('a')\nprint('b')\n",
+      type: "example",
+      // This pulls it back a level
+      indentLevelHint: 0,
+    });
+
+    check({
+      line: 3,
+      initial: "if True:\n\tprint('a')\n",
+      additional: "print('b')",
+      expected: "if True:\n\tprint('a')\n\tprint('b')\n",
+      type: "example",
+      indentLevelHint: 1,
+    });
+  });
+  it("ignores indent hint when greater than calculated indent", () => {
+    check({
+      line: 3,
+      initial: "if True:\n\tprint('a')\n",
+      additional: "print('b')",
+      expected: "if True:\n\tprint('a')\n\tprint('b')\n",
+      type: "example",
+      // This is ignored
+      indentLevelHint: 2,
+    });
+  });
+  it("ignores indent hint when appending to while true", () => {
+    check({
+      line: 3,
+      initial: "while True:\n\tprint('a')\n",
+      additional: "print('b')",
+      expected: "while True:\n\tprint('a')\n\tprint('b')\n",
+      type: "example",
+      // This is ignored to make it easy to build typical while True micro:bit programs.
+      indentLevelHint: 0,
+    });
+  });
+  it("uses indent hint when nested in while True", () => {
+    check({
+      line: 4,
+      initial: "while True:\n\tif True:\n\t\tprint('a')\n\tpass\n",
+      additional: "print('b')",
+      expected:
+        "while True:\n\tif True:\n\t\tprint('a')\n\tprint('b')\n\tpass\n",
+      type: "example",
+      // By default it would indent under the if.
+      indentLevelHint: 1,
+    });
+  });
+  it("limits use of indent hint based on following line indent", () => {
+    check({
+      line: 4,
+      initial: "if True:\n\tif True:\n\t\tprint('a')\n\tprint('b')\n",
+      additional: "print('c')",
+      expected:
+        "if True:\n\tif True:\n\t\tprint('a')\n\tprint('c')\n\tprint('b')\n",
+      type: "example",
+      // By default it would indent under the if.
+      indentLevelHint: 0,
+    });
+  });
 });