Implement reordering cells using keyboard (#63)

* Implement moving cells with keyboard shortcuts

* Add tests for cell movement operation

* Refactor

* Does not mark cells as stale if Elixir cells did not change order

assets/js/session/index.js

@@ -70,6 +70,10 @@ const Session = {
           this.pushEvent("move_cell_focus", { offset: 1 });
         } else if (key === "k") {
           this.pushEvent("move_cell_focus", { offset: -1 });
+        } else if (key === "J") {
+          this.pushEvent("move_cell", { offset: 1 });
+        } else if (key === "K") {
+          this.pushEvent("move_cell", { offset: -1 });
         } else if (key === "n") {
           this.pushEvent("insert_cell_below_focused", { type: "elixir" });
         } else if (key === "N") {

lib/live_book/notebook.ex

@@ -162,6 +162,23 @@ defmodule LiveBook.Notebook do
     %{notebook | sections: sections}
   end
 
+  @doc """
+  Moves cell within the given section at the specified position to a new position.
+  """
+  @spec move_cell(t(), Section.id(), non_neg_integer(), non_neg_integer()) :: t()
+  def move_cell(notebook, section_id, from_idx, to_idx) do
+    update_section(notebook, section_id, fn section ->
+      {cell, cells} = List.pop_at(section.cells, from_idx)
+
+      if cell do
+        cells = List.insert_at(cells, to_idx, cell)
+        %{section | cells: cells}
+      else
+        section
+      end
+    end)
+  end
+
   @doc """
   Returns a list of Elixir cells that the given cell depends on.
 
@@ -169,7 +186,7 @@ defmodule LiveBook.Notebook do
   """
   @spec parent_cells(t(), Cell.id()) :: list(Cell.t())
   def parent_cells(notebook, cell_id) do
-    with {:ok, _, section} <- LiveBook.Notebook.fetch_cell_and_section(notebook, cell_id) do
+    with {:ok, _, section} <- fetch_cell_and_section(notebook, cell_id) do
       # A cell depends on all previous cells within the same section.
       section.cells
       |> Enum.take_while(&(&1.id != cell_id))
@@ -187,7 +204,7 @@ defmodule LiveBook.Notebook do
   """
   @spec child_cells(t(), Cell.id()) :: list(Cell.t())
   def child_cells(notebook, cell_id) do
-    with {:ok, _, section} <- LiveBook.Notebook.fetch_cell_and_section(notebook, cell_id) do
+    with {:ok, _, section} <- fetch_cell_and_section(notebook, cell_id) do
       # A cell affects all the cells below it within the same section.
       section.cells
       |> Enum.drop_while(&(&1.id != cell_id))

lib/live_book/session.ex

@@ -132,6 +132,14 @@ defmodule LiveBook.Session do
     GenServer.cast(name(session_id), {:delete_cell, cell_id})
   end
 
+  @doc """
+  Asynchronously sends cell move request to the server.
+  """
+  @spec move_cell(id(), Cell.id(), integer()) :: :ok
+  def move_cell(session_id, cell_id, offset) do
+    GenServer.cast(name(session_id), {:move_cell, cell_id, offset})
+  end
+
   @doc """
   Asynchronously sends cell evaluation request to the server.
   """
@@ -315,6 +323,11 @@ defmodule LiveBook.Session do
     {:noreply, handle_operation(state, operation)}
   end
 
+  def handle_cast({:move_cell, cell_id, offset}, state) do
+    operation = {:move_cell, cell_id, offset}
+    {:noreply, handle_operation(state, operation)}
+  end
+
   def handle_cast({:queue_cell_evaluation, cell_id}, state) do
     case ensure_runtime(state) do
       {:ok, state} ->

lib/live_book/session/data.ex

@@ -67,6 +67,7 @@ defmodule LiveBook.Session.Data do
           | {:insert_cell, Section.id(), index(), Cell.type(), Cell.id()}
           | {:delete_section, Section.id()}
           | {:delete_cell, Cell.id()}
+          | {:move_cell, Cell.id(), offset :: integer()}
           | {:queue_cell_evaluation, Cell.id()}
           | {:add_cell_evaluation_stdout, Cell.id(), String.t()}
           | {:add_cell_evaluation_response, Cell.id(), Evaluator.evaluation_response()}
@@ -204,6 +205,19 @@ defmodule LiveBook.Session.Data do
     end
   end
 
+  def apply_operation(data, {:move_cell, id, offset}) do
+    with {:ok, cell, section} <- Notebook.fetch_cell_and_section(data.notebook, id),
+         true <- offset != 0 do
+      data
+      |> with_actions()
+      |> move_cell(cell, section, offset)
+      |> set_dirty()
+      |> wrap_ok()
+    else
+      _ -> :error
+    end
+  end
+
   def apply_operation(data, {:queue_cell_evaluation, id}) do
     with {:ok, cell, section} <- Notebook.fetch_cell_and_section(data.notebook, id),
          :elixir <- cell.type,
@@ -408,6 +422,41 @@ defmodule LiveBook.Session.Data do
     |> set!(cell_infos: Map.delete(data.cell_infos, cell.id))
   end
 
+  defp move_cell({data, _} = data_actions, cell, section, offset) do
+    idx = Enum.find_index(section.cells, &(&1 == cell))
+    new_idx = (idx + offset) |> clamp_index(section.cells)
+
+    updated_notebook = Notebook.move_cell(data.notebook, section.id, idx, new_idx)
+    {:ok, updated_section} = Notebook.fetch_section(updated_notebook, section.id)
+
+    elixir_cell_ids_before = elixir_cell_ids(section.cells)
+    elixir_cell_ids_after = elixir_cell_ids(updated_section.cells)
+
+    # If the order of Elixir cells stays the same, no need to invalidate anything
+    affected_cells =
+      if elixir_cell_ids_before != elixir_cell_ids_after do
+        affected_from_idx = min(idx, new_idx)
+        Enum.slice(updated_section.cells, affected_from_idx..-1)
+      else
+        []
+      end
+
+    data_actions
+    |> set!(notebook: updated_notebook)
+    |> mark_cells_as_stale(affected_cells)
+    |> unqueue_cells_evaluation(affected_cells, section)
+  end
+
+  defp elixir_cell_ids(cells) do
+    cells
+    |> Enum.filter(&(&1.type == :elixir))
+    |> Enum.map(& &1.id)
+  end
+
+  defp clamp_index(index, list) do
+    index |> max(0) |> min(length(list) - 1)
+  end
+
   defp queue_cell_evaluation(data_actions, cell, section) do
     data_actions
     |> update_section_info!(section.id, fn section ->
@@ -464,10 +513,15 @@ defmodule LiveBook.Session.Data do
   end
 
   defp mark_dependent_cells_as_stale({data, _} = data_actions, cell) do
+    child_cells = Notebook.child_cells(data.notebook, cell.id)
+    mark_cells_as_stale(data_actions, child_cells)
+  end
+
+  defp mark_cells_as_stale({data, _} = data_actions, cells) do
     invalidated_cells =
-      data.notebook
+      Enum.filter(cells, fn cell ->
-      |> Notebook.child_cells(cell.id)
+        cell.type == :elixir and data.cell_infos[cell.id].validity_status == :evaluated
-      |> Enum.filter(fn cell -> data.cell_infos[cell.id].validity_status == :evaluated end)
+      end)
 
     data_actions
     |> reduce(invalidated_cells, &set_cell_info!(&1, &2.id, validity_status: :stale))
@@ -519,13 +573,16 @@ defmodule LiveBook.Session.Data do
   end
 
   defp unqueue_dependent_cells_evaluation({data, _} = data_actions, cell, section) do
-    queued_dependent_cells =
+    dependent_cells = Notebook.child_cells(data.notebook, cell.id)
-      data.notebook
+    unqueue_cells_evaluation(data_actions, dependent_cells, section)
-      |> Notebook.child_cells(cell.id)
+  end
-      |> Enum.filter(fn cell -> data.cell_infos[cell.id].evaluation_status == :queued end)
+
+  defp unqueue_cells_evaluation({data, _} = data_actions, cells, section) do
+    queued_cells =
+      Enum.filter(cells, fn cell -> data.cell_infos[cell.id].evaluation_status == :queued end)
 
     data_actions
-    |> reduce(queued_dependent_cells, &unqueue_cell_evaluation(&1, &2, section))
+    |> reduce(queued_cells, &unqueue_cell_evaluation(&1, &2, section))
   end
 
   defp set_notebook_name({data, _} = data_actions, name) do

lib/live_book_web/live/session_live.ex

@@ -300,6 +300,14 @@ defmodule LiveBookWeb.SessionLive do
     end
   end
 
+  def handle_event("move_cell", %{"offset" => offset}, socket) do
+    if socket.assigns.focused_cell_id do
+      Session.move_cell(socket.assigns.session_id, socket.assigns.focused_cell_id, offset)
+    end
+
+    {:noreply, socket}
+  end
+
   def handle_event("set_insert_mode", %{"enabled" => enabled}, socket) do
     if socket.assigns.focused_cell_id do
       {:noreply, assign(socket, insert_mode: enabled)}

lib/live_book_web/live/session_live/shortcuts_component.ex

@@ -10,6 +10,8 @@ defmodule LiveBookWeb.SessionLive.ShortcutsComponent do
       %{seq: "?", desc: "Open this help modal"},
       %{seq: "j", desc: "Focus next cell"},
       %{seq: "k", desc: "Focus previous cell"},
+      %{seq: "J", desc: "Move cell down"},
+      %{seq: "K", desc: "Move cell up"},
       %{seq: "i", desc: "Switch to insert mode"},
       %{seq: "n", desc: "Insert Elixir cell below"},
       %{seq: "m", desc: "Insert Markdown cell below"},

test/live_book/session/data_test.exs

@@ -211,6 +211,175 @@ defmodule LiveBook.Session.DataTest do
     end
   end
 
+  describe "apply_operation/2 given :move_cell" do
+    test "returns an error given invalid cell id" do
+      data = Data.new()
+      operation = {:move_cell, "nonexistent", 1}
+      assert :error = Data.apply_operation(data, operation)
+    end
+
+    test "returns an error given no offset" do
+      data =
+        data_after_operations!([
+          {:insert_section, 0, "s1"},
+          {:insert_cell, "s1", 0, :elixir, "c1"}
+        ])
+
+      operation = {:move_cell, "c1", 0}
+      assert :error = Data.apply_operation(data, operation)
+    end
+
+    test "given negative offset moves the cell and marks relevant cells as stale" do
+      data =
+        data_after_operations!([
+          {:insert_section, 0, "s1"},
+          # Add cells
+          {:insert_cell, "s1", 0, :elixir, "c1"},
+          {:insert_cell, "s1", 1, :elixir, "c2"},
+          {:insert_cell, "s1", 2, :elixir, "c3"},
+          {:insert_cell, "s1", 3, :elixir, "c4"},
+          # Evaluate cells
+          {:queue_cell_evaluation, "c1"},
+          {:add_cell_evaluation_response, "c1", {:ok, nil}},
+          {:queue_cell_evaluation, "c2"},
+          {:add_cell_evaluation_response, "c2", {:ok, nil}},
+          {:queue_cell_evaluation, "c3"},
+          {:add_cell_evaluation_response, "c3", {:ok, nil}},
+          {:queue_cell_evaluation, "c4"},
+          {:add_cell_evaluation_response, "c4", {:ok, nil}}
+        ])
+
+      operation = {:move_cell, "c3", -1}
+
+      assert {:ok,
+              %{
+                notebook: %{
+                  sections: [
+                    %{
+                      cells: [%{id: "c1"}, %{id: "c3"}, %{id: "c2"}, %{id: "c4"}]
+                    }
+                  ]
+                },
+                cell_infos: %{
+                  "c1" => %{validity_status: :evaluated},
+                  "c2" => %{validity_status: :stale},
+                  "c3" => %{validity_status: :stale},
+                  "c4" => %{validity_status: :stale}
+                }
+              }, []} = Data.apply_operation(data, operation)
+    end
+
+    test "given positive offset moves the cell and marks relevant cells as stale" do
+      data =
+        data_after_operations!([
+          {:insert_section, 0, "s1"},
+          # Add cells
+          {:insert_cell, "s1", 0, :elixir, "c1"},
+          {:insert_cell, "s1", 1, :elixir, "c2"},
+          {:insert_cell, "s1", 2, :elixir, "c3"},
+          {:insert_cell, "s1", 3, :elixir, "c4"},
+          # Evaluate cells
+          {:queue_cell_evaluation, "c1"},
+          {:add_cell_evaluation_response, "c1", {:ok, nil}},
+          {:queue_cell_evaluation, "c2"},
+          {:add_cell_evaluation_response, "c2", {:ok, nil}},
+          {:queue_cell_evaluation, "c3"},
+          {:add_cell_evaluation_response, "c3", {:ok, nil}},
+          {:queue_cell_evaluation, "c4"},
+          {:add_cell_evaluation_response, "c4", {:ok, nil}}
+        ])
+
+      operation = {:move_cell, "c2", 1}
+
+      assert {:ok,
+              %{
+                notebook: %{
+                  sections: [
+                    %{
+                      cells: [%{id: "c1"}, %{id: "c3"}, %{id: "c2"}, %{id: "c4"}]
+                    }
+                  ]
+                },
+                cell_infos: %{
+                  "c1" => %{validity_status: :evaluated},
+                  "c2" => %{validity_status: :stale},
+                  "c3" => %{validity_status: :stale},
+                  "c4" => %{validity_status: :stale}
+                }
+              }, []} = Data.apply_operation(data, operation)
+    end
+
+    test "moving a markdown cell does not change validity" do
+      data =
+        data_after_operations!([
+          {:insert_section, 0, "s1"},
+          # Add cells
+          {:insert_cell, "s1", 0, :elixir, "c1"},
+          {:insert_cell, "s1", 1, :markdown, "c2"},
+          # Evaluate the Elixir cell
+          {:queue_cell_evaluation, "c1"},
+          {:add_cell_evaluation_response, "c1", {:ok, nil}}
+        ])
+
+      operation = {:move_cell, "c2", -1}
+
+      assert {:ok,
+              %{
+                cell_infos: %{
+                  "c1" => %{validity_status: :evaluated}
+                }
+              }, []} = Data.apply_operation(data, operation)
+    end
+
+    test "affected queued cell is unqueued" do
+      data =
+        data_after_operations!([
+          {:insert_section, 0, "s1"},
+          # Add cells
+          {:insert_cell, "s1", 0, :elixir, "c1"},
+          {:insert_cell, "s1", 1, :elixir, "c2"},
+          # Evaluate the Elixir cell
+          {:queue_cell_evaluation, "c1"},
+          {:queue_cell_evaluation, "c2"}
+        ])
+
+      operation = {:move_cell, "c2", -1}
+
+      assert {:ok,
+              %{
+                cell_infos: %{
+                  "c2" => %{evaluation_status: :ready}
+                }
+              }, []} = Data.apply_operation(data, operation)
+    end
+
+    test "does not invalidate the moved cell if the order of Elixir cells stays the same" do
+      data =
+        data_after_operations!([
+          {:insert_section, 0, "s1"},
+          # Add cells
+          {:insert_cell, "s1", 0, :elixir, "c1"},
+          {:insert_cell, "s1", 1, :markdown, "c2"},
+          {:insert_cell, "s1", 2, :elixir, "c3"},
+          # Evaluate cells
+          {:queue_cell_evaluation, "c1"},
+          {:add_cell_evaluation_response, "c1", {:ok, nil}},
+          {:queue_cell_evaluation, "c3"},
+          {:add_cell_evaluation_response, "c3", {:ok, nil}}
+        ])
+
+      operation = {:move_cell, "c1", 1}
+
+      assert {:ok,
+              %{
+                cell_infos: %{
+                  "c1" => %{validity_status: :evaluated},
+                  "c3" => %{validity_status: :evaluated}
+                }
+              }, []} = Data.apply_operation(data, operation)
+    end
+  end
+
   describe "apply_operation/2 given :queue_cell_evaluation" do
     test "returns an error given invalid cell id" do
       data = Data.new()