diff --git a/assets/js/session/index.js b/assets/js/session/index.js index f8f47e0d7..5aaae1145 100644 --- a/assets/js/session/index.js +++ b/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") { diff --git a/lib/live_book/notebook.ex b/lib/live_book/notebook.ex index ea76b76ba..4de3ef79c 100644 --- a/lib/live_book/notebook.ex +++ b/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)) diff --git a/lib/live_book/session.ex b/lib/live_book/session.ex index 072571e81..821dd87e2 100644 --- a/lib/live_book/session.ex +++ b/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} -> diff --git a/lib/live_book/session/data.ex b/lib/live_book/session/data.ex index a5b863b6c..097680041 100644 --- a/lib/live_book/session/data.ex +++ b/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 - |> Notebook.child_cells(cell.id) - |> Enum.filter(fn cell -> data.cell_infos[cell.id].validity_status == :evaluated end) + Enum.filter(cells, fn cell -> + cell.type == :elixir and data.cell_infos[cell.id].validity_status == :evaluated + 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 = - data.notebook - |> Notebook.child_cells(cell.id) - |> Enum.filter(fn cell -> data.cell_infos[cell.id].evaluation_status == :queued end) + dependent_cells = Notebook.child_cells(data.notebook, cell.id) + unqueue_cells_evaluation(data_actions, dependent_cells, section) + 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 diff --git a/lib/live_book_web/live/session_live.ex b/lib/live_book_web/live/session_live.ex index 61e992672..897d55448 100644 --- a/lib/live_book_web/live/session_live.ex +++ b/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)} diff --git a/lib/live_book_web/live/session_live/shortcuts_component.ex b/lib/live_book_web/live/session_live/shortcuts_component.ex index 6b9925a94..65e55594c 100644 --- a/lib/live_book_web/live/session_live/shortcuts_component.ex +++ b/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"}, diff --git a/test/live_book/session/data_test.exs b/test/live_book/session/data_test.exs index c14d985fb..be5aab9b3 100644 --- a/test/live_book/session/data_test.exs +++ b/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()