From 736bc18eb2165e9d0187de77daa63e7981e09156 Mon Sep 17 00:00:00 2001
From: Matt Hillsdon <matt.hillsdon@microbit.org>
Date: Mon, 22 Nov 2021 15:26:12 +0000
Subject: [PATCH 1/6] Failing test for import line break issue.

---
 src/editor/codemirror/edits.test.ts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/editor/codemirror/edits.test.ts b/src/editor/codemirror/edits.test.ts
index d53aa72ec..3e4d4e6ca 100644
--- a/src/editor/codemirror/edits.test.ts
+++ b/src/editor/codemirror/edits.test.ts
@@ -114,4 +114,12 @@ describe("edits", () => {
       "from microbit import *\nimport radio\n\nradio.off()\n\nwhile True:\n    display.scroll('Hello, World')\n"
     );
   });
+
+  it("multiple imports into empty doc", () => {
+    check(
+      "",
+      "from microbit import *\nimport music\n\n\ndisplay.scroll('score', delay=100, loop=True, wait=False)\nmusic.play(music.ODE)\n",
+      "from microbit import *\nimport music\n\n\ndisplay.scroll('score', delay=100, loop=True, wait=False)\nmusic.play(music.ODE)\n"
+    );
+  });
 });

From f25f6a8088709da55503afebccc927c20d0e7893 Mon Sep 17 00:00:00 2001
From: Matt Hillsdon <matt.hillsdon@microbit.org>
Date: Fri, 26 Nov 2021 10:22:33 +0000
Subject: [PATCH 2/6] Resequence.

---
 src/editor/codemirror/edits.ts | 140 ++++++++++++++++-----------------
 1 file changed, 70 insertions(+), 70 deletions(-)

diff --git a/src/editor/codemirror/edits.ts b/src/editor/codemirror/edits.ts
index f24dc485c..90bd050b8 100644
--- a/src/editor/codemirror/edits.ts
+++ b/src/editor/codemirror/edits.ts
@@ -21,76 +21,6 @@ type SimpleChangeSpec = {
   insert: string;
 };
 
-const calculateImportChangesInternal = (
-  allCurrent: ImportNode[],
-  required: RequiredImport
-): SimpleChangeSpec[] => {
-  const from = allCurrent.length
-    ? allCurrent[allCurrent.length - 1].node.to
-    : 0;
-  const to = from;
-  const prefix = to > 0 ? "\n" : "";
-
-  if (!required.name) {
-    // Module import.
-    if (
-      allCurrent.find(
-        (c) => !c.names && c.module === required.module && !c.alias
-      )
-    ) {
-      return [];
-    } else {
-      return [{ from, to, insert: `${prefix}import ${required.module}` }];
-    }
-  } else if (required.name === "*") {
-    // Wildcard import.
-    if (
-      allCurrent.find(
-        (c) =>
-          c.names?.length === 1 &&
-          c.names[0].name === "*" &&
-          c.module === required.module
-      )
-    ) {
-      return [];
-    } else {
-      return [
-        { from, to, insert: `${prefix}from ${required.module} import *` },
-      ];
-    }
-  } else {
-    // Importing some name from a module.
-    const partMatches = allCurrent.filter(
-      (c) =>
-        c.names &&
-        !(c.names?.length === 1 && c.names[0].name === "*") &&
-        c.module === required.module
-    );
-    const fullMatch = partMatches.find((nameImport) =>
-      nameImport.names?.find((n) => n.name === required.name && !n.alias)
-    );
-    if (fullMatch) {
-      return [];
-    } else if (partMatches.length > 0) {
-      return [
-        {
-          from: partMatches[0].node.to,
-          to: partMatches[0].node.to,
-          insert: `, ${required.name}`,
-        },
-      ];
-    } else {
-      return [
-        {
-          from,
-          to,
-          insert: `${prefix}from ${required.module} import ${required.name}`,
-        },
-      ];
-    }
-  }
-};
-
 /**
  * A representation of an import node.
  * The CodeMirror tree isn't easy to work with so we convert to these.
@@ -183,6 +113,76 @@ export const calculateChanges = (state: EditorState, addition: string) => {
   return changes;
 };
 
+const calculateImportChangesInternal = (
+  allCurrent: ImportNode[],
+  required: RequiredImport
+): SimpleChangeSpec[] => {
+  const from = allCurrent.length
+    ? allCurrent[allCurrent.length - 1].node.to
+    : 0;
+  const to = from;
+  const prefix = to > 0 ? "\n" : "";
+
+  if (!required.name) {
+    // Module import.
+    if (
+      allCurrent.find(
+        (c) => !c.names && c.module === required.module && !c.alias
+      )
+    ) {
+      return [];
+    } else {
+      return [{ from, to, insert: `${prefix}import ${required.module}` }];
+    }
+  } else if (required.name === "*") {
+    // Wildcard import.
+    if (
+      allCurrent.find(
+        (c) =>
+          c.names?.length === 1 &&
+          c.names[0].name === "*" &&
+          c.module === required.module
+      )
+    ) {
+      return [];
+    } else {
+      return [
+        { from, to, insert: `${prefix}from ${required.module} import *` },
+      ];
+    }
+  } else {
+    // Importing some name from a module.
+    const partMatches = allCurrent.filter(
+      (c) =>
+        c.names &&
+        !(c.names?.length === 1 && c.names[0].name === "*") &&
+        c.module === required.module
+    );
+    const fullMatch = partMatches.find((nameImport) =>
+      nameImport.names?.find((n) => n.name === required.name && !n.alias)
+    );
+    if (fullMatch) {
+      return [];
+    } else if (partMatches.length > 0) {
+      return [
+        {
+          from: partMatches[0].node.to,
+          to: partMatches[0].node.to,
+          insert: `, ${required.name}`,
+        },
+      ];
+    } else {
+      return [
+        {
+          from,
+          to,
+          insert: `${prefix}from ${required.module} import ${required.name}`,
+        },
+      ];
+    }
+  }
+};
+
 const currentImports = (state: EditorState): ImportNode[] => {
   const tree = syntaxTree(state);
   return topLevelImports(tree, (from, to) => state.sliceDoc(from, to));

From 83cbbe5a879cc36ad165525a5f0ce5d34e95559f Mon Sep 17 00:00:00 2001
From: Matt Hillsdon <matt.hillsdon@microbit.org>
Date: Fri, 26 Nov 2021 11:30:02 +0000
Subject: [PATCH 3/6] Tests pass.

---
 src/editor/codemirror/edits.test.ts | 147 +++++++++++++++++-----------
 src/editor/codemirror/edits.ts      |  66 ++++++++++---
 2 files changed, 142 insertions(+), 71 deletions(-)

diff --git a/src/editor/codemirror/edits.test.ts b/src/editor/codemirror/edits.test.ts
index 3e4d4e6ca..dc537e5ed 100644
--- a/src/editor/codemirror/edits.test.ts
+++ b/src/editor/codemirror/edits.test.ts
@@ -9,7 +9,15 @@ import { calculateChanges } from "./edits";
 import { EditorView } from "@codemirror/view";
 
 describe("edits", () => {
-  const check = (initial: string, additional: string, expected: string) => {
+  const check = ({
+    initial,
+    additional,
+    expected,
+  }: {
+    initial: string;
+    additional: string;
+    expected: string;
+  }) => {
     const state = EditorState.create({
       doc: initial,
       extensions: [python()],
@@ -24,102 +32,123 @@ describe("edits", () => {
   };
 
   it("first import from case - wildcard", () => {
-    check("", "from microbit import *", "from microbit import *\n\n");
+    check({
+      initial: "",
+      additional: "from microbit import *",
+      expected: "from microbit import *\n\n\n",
+    });
   });
 
   it("first import from case - name", () => {
-    check(
-      "",
-      "from random import randrange",
-      "from random import randrange\n\n"
-    );
+    check({
+      initial: "",
+      additional: "from random import randrange",
+      expected: "from random import randrange\n\n\n",
+    });
   });
 
   it("first import module case", () => {
-    check("", "import audio", "import audio\n\n");
+    check({
+      initial: "",
+      additional: "import audio",
+      expected: "import audio\n\n\n",
+    });
   });
 
   it("existing import module case", () => {
-    check("import audio", "import audio", "import audio");
+    check({
+      initial: "import audio",
+      additional: "import audio",
+      expected: "import audio",
+    });
   });
 
   it("existing import module case - as variant", () => {
-    check(
-      "import audio as foo",
-      "import audio",
-      "import audio as foo\nimport audio"
-    );
+    check({
+      initial: "import audio as foo",
+      additional: "import audio",
+      expected: "import audio as foo\nimport audio\n",
+    });
   });
 
   it("existing import from case - wildcard", () => {
-    check(
-      "from microbit import *",
-      "from microbit import *",
-      "from microbit import *"
-    );
+    check({
+      initial: "from microbit import *",
+      additional: "from microbit import *",
+      expected: "from microbit import *",
+    });
   });
 
   it("existing import from case - name", () => {
-    check(
-      "from random import randrange",
-      "from random import randrange",
-      "from random import randrange"
-    );
+    check({
+      initial: "from random import randrange",
+      additional: "from random import randrange",
+      expected: "from random import randrange",
+    });
   });
 
   it("existing import from case - alias", () => {
-    check(
-      "from random import randrange as foo",
-      "from random import randrange",
-      "from random import randrange as foo, randrange"
-    );
+    check({
+      initial: "from random import randrange as foo",
+      additional: "from random import randrange",
+      expected: "from random import randrange as foo, randrange",
+    });
   });
 
   it("existing from import new name", () => {
-    check(
-      "from random import getrandbits",
-      "from random import randrange",
-      "from random import getrandbits, randrange"
-    );
+    check({
+      initial: "from random import getrandbits",
+      additional: "from random import randrange",
+      expected: "from random import getrandbits, randrange",
+    });
   });
 
   it("copes with invalid imports", () => {
-    check(
-      "import\nfrom\n",
-      "from random import randrange",
-      "from random import randrange\n\nimport\nfrom\n"
-    );
+    check({
+      initial: "import\nfrom\n",
+      additional: "from random import randrange",
+      expected: "from random import randrange\n\n\nimport\nfrom\n",
+    });
   });
 
   it("combo imports", () => {
-    check(
-      "from microbit import *\nfrom random import randrange\nimport radio\n",
-      "from microbit import *\nfrom random import rantint\nimport micropython\n",
-      "from microbit import *\nfrom random import randrange, rantint\nimport radio\nimport micropython\n"
-    );
+    check({
+      initial:
+        "from microbit import *\nfrom random import randrange\nimport radio\n",
+      additional:
+        "from microbit import *\nfrom random import rantint\nimport micropython\n",
+      expected:
+        "from microbit import *\nfrom random import randrange, rantint\nimport radio\nimport micropython\n",
+    });
   });
 
   it("non-import content separated and appended", () => {
-    check(
-      "from microbit import *",
-      "from microbit import *\nwhile True:\n    display.scroll('Hello, World')\n",
-      "from microbit import *\nwhile True:\n    display.scroll('Hello, World')\n"
-    );
+    check({
+      initial: "from microbit import *",
+      additional:
+        "from microbit import *\nwhile True:\n    display.scroll('Hello, World')\n",
+      expected:
+        "from microbit import *\nwhile True:\n    display.scroll('Hello, World')\n",
+    });
   });
 
   it("non-import content separated and existing content", () => {
-    check(
-      "from microbit import *\n\nwhile True:\n    display.scroll('Hello, World')\n",
-      "import radio\n\nradio.off()",
-      "from microbit import *\nimport radio\n\nradio.off()\n\nwhile True:\n    display.scroll('Hello, World')\n"
-    );
+    check({
+      initial:
+        "from microbit import *\n\nwhile True:\n    display.scroll('Hello, World')\n",
+      additional: "import radio\n\nradio.off()",
+      expected:
+        "from microbit import *\nimport radio\n\nradio.off()\n\nwhile True:\n    display.scroll('Hello, World')\n",
+    });
   });
 
   it("multiple imports into empty doc", () => {
-    check(
-      "",
-      "from microbit import *\nimport music\n\n\ndisplay.scroll('score', delay=100, loop=True, wait=False)\nmusic.play(music.ODE)\n",
-      "from microbit import *\nimport music\n\n\ndisplay.scroll('score', delay=100, loop=True, wait=False)\nmusic.play(music.ODE)\n"
-    );
+    check({
+      initial: "",
+      additional:
+        "from microbit import *\nimport music\n\n\ndisplay.scroll('score', delay=100, loop=True, wait=False)\nmusic.play(music.ODE)\n",
+      expected:
+        "from microbit import *\nimport music\n\n\ndisplay.scroll('score', delay=100, loop=True, wait=False)\nmusic.play(music.ODE)\n",
+    });
   });
 });
diff --git a/src/editor/codemirror/edits.ts b/src/editor/codemirror/edits.ts
index 90bd050b8..c59d00ef4 100644
--- a/src/editor/codemirror/edits.ts
+++ b/src/editor/codemirror/edits.ts
@@ -77,9 +77,13 @@ export const calculateChanges = (state: EditorState, addition: string) => {
   );
   const allCurrentImports = currentImports(state);
 
+  const insertPoint = defaultImportInsertPoint(state, allCurrentImports);
   const changes = requiredImports.flatMap((required) =>
-    calculateImportChangesInternal(allCurrentImports, required)
+    calculateImportChangesInternal(insertPoint, allCurrentImports, required)
   );
+  if (insertPoint.used) {
+    changes[0].insert = insertPoint.prefix + changes[0].insert;
+  }
   if (changes.length > 0 && allCurrentImports.length === 0) {
     // Two blank lines separating the imports from everything else.
     changes[changes.length - 1].insert += "\n\n";
@@ -114,15 +118,10 @@ export const calculateChanges = (state: EditorState, addition: string) => {
 };
 
 const calculateImportChangesInternal = (
+  importInsertPoint: DefaultImportInsertPoint,
   allCurrent: ImportNode[],
   required: RequiredImport
 ): SimpleChangeSpec[] => {
-  const from = allCurrent.length
-    ? allCurrent[allCurrent.length - 1].node.to
-    : 0;
-  const to = from;
-  const prefix = to > 0 ? "\n" : "";
-
   if (!required.name) {
     // Module import.
     if (
@@ -132,7 +131,9 @@ const calculateImportChangesInternal = (
     ) {
       return [];
     } else {
-      return [{ from, to, insert: `${prefix}import ${required.module}` }];
+      return [
+        { from: importInsertPoint.from, insert: `import ${required.module}\n` },
+      ];
     }
   } else if (required.name === "*") {
     // Wildcard import.
@@ -147,7 +148,10 @@ const calculateImportChangesInternal = (
       return [];
     } else {
       return [
-        { from, to, insert: `${prefix}from ${required.module} import *` },
+        {
+          from: importInsertPoint.from,
+          insert: `from ${required.module} import *\n`,
+        },
       ];
     }
   } else {
@@ -174,15 +178,53 @@ const calculateImportChangesInternal = (
     } else {
       return [
         {
-          from,
-          to,
-          insert: `${prefix}from ${required.module} import ${required.name}`,
+          from: importInsertPoint.from,
+          insert: `from ${required.module} import ${required.name}\n`,
         },
       ];
     }
   }
 };
 
+/**
+ * The default point to insert imports.
+ *
+ * This tracks whether its used and exposes a prefix to add to the first import
+ * if it's used.
+ *
+ * This helps address the case where the document is a single line containing
+ * an import, as in that scenario there is no trailing line break.
+ */
+class DefaultImportInsertPoint {
+  used: boolean = false;
+  private _from;
+
+  constructor(from: number, public prefix: string = "") {
+    this._from = from;
+  }
+
+  get from() {
+    this.used = true;
+    return this._from;
+  }
+}
+
+const defaultImportInsertPoint = (
+  state: EditorState,
+  allCurrent: ImportNode[]
+): DefaultImportInsertPoint => {
+  if (allCurrent.length) {
+    const line = state.doc.lineAt(allCurrent[allCurrent.length - 1].node.to);
+    // We want the point after the line break, i.e. the start of the next line
+    // If it doesn't exist, we want the end of this line.
+    if (state.doc.lines > line.number) {
+      return new DefaultImportInsertPoint(state.doc.line(line.number + 1).from);
+    }
+    return new DefaultImportInsertPoint(line.to, "\n");
+  }
+  return new DefaultImportInsertPoint(0);
+};
+
 const currentImports = (state: EditorState): ImportNode[] => {
   const tree = syntaxTree(state);
   return topLevelImports(tree, (from, to) => state.sliceDoc(from, to));

From 8d0173c3f73c6464ace88ebdab1db535e2635518 Mon Sep 17 00:00:00 2001
From: Matt Hillsdon <matt.hillsdon@microbit.org>
Date: Fri, 26 Nov 2021 11:37:36 +0000
Subject: [PATCH 4/6] Better comments.

---
 src/editor/codemirror/edits.ts | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/editor/codemirror/edits.ts b/src/editor/codemirror/edits.ts
index c59d00ef4..dc3ea3fd2 100644
--- a/src/editor/codemirror/edits.ts
+++ b/src/editor/codemirror/edits.ts
@@ -213,13 +213,14 @@ const defaultImportInsertPoint = (
   state: EditorState,
   allCurrent: ImportNode[]
 ): DefaultImportInsertPoint => {
-  if (allCurrent.length) {
+  if (allCurrent.length > 0) {
     const line = state.doc.lineAt(allCurrent[allCurrent.length - 1].node.to);
-    // We want the point after the line break, i.e. the start of the next line
-    // If it doesn't exist, we want the end of this line.
+    // We want the point after the line break, i.e. the start of the next line.
     if (state.doc.lines > line.number) {
       return new DefaultImportInsertPoint(state.doc.line(line.number + 1).from);
     }
+    // If it doesn't exist, we want the end of this line, but need to insert a
+    // line break later if we actually insert anything.
     return new DefaultImportInsertPoint(line.to, "\n");
   }
   return new DefaultImportInsertPoint(0);

From 852b25eb907234e4e545da41889c90159d9d69b3 Mon Sep 17 00:00:00 2001
From: Matt Hillsdon <matt.hillsdon@microbit.org>
Date: Fri, 26 Nov 2021 12:04:27 +0000
Subject: [PATCH 5/6] Saner selection handling.

This will all be revisted soon.
---
 src/editor/active-editor-hooks.tsx | 13 +------------
 src/editor/codemirror/edits.ts     | 15 ++++++++++++++-
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/src/editor/active-editor-hooks.tsx b/src/editor/active-editor-hooks.tsx
index 77fb0e652..d8819ecec 100644
--- a/src/editor/active-editor-hooks.tsx
+++ b/src/editor/active-editor-hooks.tsx
@@ -50,18 +50,7 @@ export class ActiveEditorActions {
    * @param code The code with any required imports.
    */
   insertCode = (code: string): void => {
-    const state = this.view.state;
-    const changes = calculateChanges(state, code);
-    const changeSet = state.changes(changes);
-    const lastChange = changes[changes.length - 1];
-    const updatedSelection =
-      changeSet.mapPos(lastChange.from) + lastChange.insert.trimEnd().length;
-    const transaction = state.update({
-      changes: changeSet,
-      selection: { anchor: updatedSelection },
-      scrollIntoView: true,
-    });
-    this.view.dispatch(transaction);
+    this.view.dispatch(calculateChanges(this.view.state, code));
     this.view.focus();
   };
 }
diff --git a/src/editor/codemirror/edits.ts b/src/editor/codemirror/edits.ts
index dc3ea3fd2..f92f2b0bb 100644
--- a/src/editor/codemirror/edits.ts
+++ b/src/editor/codemirror/edits.ts
@@ -113,8 +113,21 @@ export const calculateChanges = (state: EditorState, addition: string) => {
         "\n" +
         (relation === Relation.Before ? "\n" : ""),
     });
+
+    const changeSet = state.changes(changes);
+    return state.update({
+      changes: changeSet,
+      scrollIntoView: true,
+      selection: {
+        anchor: changeSet.mapPos(insertionPoint, 1),
+      },
+    });
   }
-  return changes;
+
+  return state.update({
+    changes: state.changes(changes),
+    scrollIntoView: true,
+  });
 };
 
 const calculateImportChangesInternal = (

From 3f93a06b89b0c56cf5bb0e07f74af1032305b364 Mon Sep 17 00:00:00 2001
From: Matt Hillsdon <matt.hillsdon@microbit.org>
Date: Fri, 26 Nov 2021 12:05:58 +0000
Subject: [PATCH 6/6] Fix compile error.

The test could now be extended to test the selection changes, but we
we're about to revisit I've left that for now.
---
 src/editor/codemirror/edits.test.ts | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/editor/codemirror/edits.test.ts b/src/editor/codemirror/edits.test.ts
index dc537e5ed..4ea47f559 100644
--- a/src/editor/codemirror/edits.test.ts
+++ b/src/editor/codemirror/edits.test.ts
@@ -23,9 +23,7 @@ describe("edits", () => {
       extensions: [python()],
     });
     const view = new EditorView({ state });
-    const transaction = state.update({
-      changes: calculateChanges(state, additional),
-    });
+    const transaction = state.update(calculateChanges(state, additional));
     view.dispatch(transaction);
     const actual = view.state.sliceDoc(0);
     expect(actual).toEqual(expected);