diff --git a/assets/css/components.css b/assets/css/components.css index ce5ba866be5..9bfeda4bf12 100644 --- a/assets/css/components.css +++ b/assets/css/components.css @@ -91,6 +91,10 @@ @apply relative inline-block w-14 h-7 mr-2 select-none transition; } + .switch-button--disabled { + @apply pointer-events-none opacity-50; + } + .switch-button__checkbox { @apply appearance-none absolute block w-7 h-7 rounded-full bg-gray-400 border-[5px] border-gray-100 cursor-pointer transition-all duration-300; } diff --git a/assets/js/session/index.js b/assets/js/session/index.js index c37d07968fe..aa2107703f3 100644 --- a/assets/js/session/index.js +++ b/assets/js/session/index.js @@ -316,8 +316,6 @@ function handleDocumentKeyDown(hook, event) { insertCellBelowFocused(hook, "markdown"); } else if (keyBuffer.tryMatch(["M"])) { insertCellAboveFocused(hook, "markdown"); - } else if (keyBuffer.tryMatch(["S"])) { - addSection(hook); } } } @@ -642,10 +640,6 @@ function insertCellAboveFocused(hook, type) { } } -function addSection(hook) { - hook.pushEvent("add_section", {}); -} - function insertFirstCell(hook, type) { const sectionIds = getSectionIds(); diff --git a/lib/livebook/notebook.ex b/lib/livebook/notebook.ex index f071632904e..fdbb15d88f0 100644 --- a/lib/livebook/notebook.ex +++ b/lib/livebook/notebook.ex @@ -94,6 +94,26 @@ defmodule Livebook.Notebook do %{notebook | sections: sections} end + @doc """ + Inserts `section` below the parent section. + + Cells below the given index are moved to the newly inserted section. + """ + @spec insert_section_into(t(), Section.id(), non_neg_integer(), Section.t()) :: t() + def insert_section_into(notebook, section_id, index, section) do + {sections_above, [parent_section | sections_below]} = + Enum.split_while(notebook.sections, &(&1.id != section_id)) + + {cells_above, cells_below} = Enum.split(parent_section.cells, index) + + sections = + sections_above ++ + [%{parent_section | cells: cells_above}, %{section | cells: cells_below}] ++ + sections_below + + %{notebook | sections: sections} + end + @doc """ Inserts `cell` at the given `index` within section identified by `section_id`. """ @@ -106,11 +126,24 @@ defmodule Livebook.Notebook do @doc """ Deletes section with the given id. + + All cells are moved to the previous section if present. """ @spec delete_section(t(), Section.id()) :: t() def delete_section(notebook, section_id) do - {_, notebook} = pop_in(notebook, [Access.key(:sections), access_by_id(section_id)]) - notebook + sections = + case Enum.split_while(notebook.sections, &(&1.id != section_id)) do + {[], [_section | sections_below]} -> + sections_below + + {sections_above, [section | sections_below]} -> + {prev_section, sections_above} = List.pop_at(sections_above, length(sections_above) - 1) + + sections_above ++ + [%{prev_section | cells: prev_section.cells ++ section.cells} | sections_below] + end + + %{notebook | sections: sections} end @doc """ diff --git a/lib/livebook/session.ex b/lib/livebook/session.ex index 42b51694945..6d897b6dff8 100644 --- a/lib/livebook/session.ex +++ b/lib/livebook/session.ex @@ -115,6 +115,14 @@ defmodule Livebook.Session do GenServer.cast(name(session_id), {:insert_section, self(), index}) end + @doc """ + Asynchronously sends section insertion request to the server. + """ + @spec insert_section_into(id(), Section.id(), non_neg_integer()) :: :ok + def insert_section_into(session_id, section_id, index) do + GenServer.cast(name(session_id), {:insert_section_into, self(), section_id, index}) + end + @doc """ Asynchronously sends cell insertion request to the server. """ @@ -127,9 +135,9 @@ defmodule Livebook.Session do @doc """ Asynchronously sends section deletion request to the server. """ - @spec delete_section(id(), Section.id()) :: :ok - def delete_section(session_id, section_id) do - GenServer.cast(name(session_id), {:delete_section, self(), section_id}) + @spec delete_section(id(), Section.id(), boolean()) :: :ok + def delete_section(session_id, section_id, delete_cells) do + GenServer.cast(name(session_id), {:delete_section, self(), section_id, delete_cells}) end @doc """ @@ -352,14 +360,20 @@ defmodule Livebook.Session do {:noreply, handle_operation(state, operation)} end + def handle_cast({:insert_section_into, client_pid, section_id, index}, state) do + # Include new id in the operation, so it's reproducible + operation = {:insert_section_into, client_pid, section_id, index, Utils.random_id()} + {:noreply, handle_operation(state, operation)} + end + def handle_cast({:insert_cell, client_pid, section_id, index, type}, state) do # Include new id in the operation, so it's reproducible operation = {:insert_cell, client_pid, section_id, index, type, Utils.random_id()} {:noreply, handle_operation(state, operation)} end - def handle_cast({:delete_section, client_pid, section_id}, state) do - operation = {:delete_section, client_pid, section_id} + def handle_cast({:delete_section, client_pid, section_id, delete_cells}, state) do + operation = {:delete_section, client_pid, section_id, delete_cells} {:noreply, handle_operation(state, operation)} end diff --git a/lib/livebook/session/data.ex b/lib/livebook/session/data.ex index 80205681f29..78463b9144c 100644 --- a/lib/livebook/session/data.ex +++ b/lib/livebook/session/data.ex @@ -80,8 +80,9 @@ defmodule Livebook.Session.Data do @type operation :: {:insert_section, pid(), index(), Section.id()} + | {:insert_section_into, pid(), Section.id(), index(), Section.id()} | {:insert_cell, pid(), Section.id(), index(), Cell.type(), Cell.id()} - | {:delete_section, pid(), Section.id()} + | {:delete_section, pid(), Section.id(), delete_cells :: boolean()} | {:delete_cell, pid(), Cell.id()} | {:move_cell, pid(), Cell.id(), offset :: integer()} | {:move_section, pid(), Section.id(), offset :: integer()} @@ -179,6 +180,18 @@ defmodule Livebook.Session.Data do |> wrap_ok() end + def apply_operation(data, {:insert_section_into, _client_pid, section_id, index, id}) do + with {:ok, _section} <- Notebook.fetch_section(data.notebook, section_id) do + section = %{Section.new() | id: id} + + data + |> with_actions() + |> insert_section_into(section_id, index, section) + |> set_dirty() + |> wrap_ok() + end + end + def apply_operation(data, {:insert_cell, _client_pid, section_id, index, type, id}) do with {:ok, _section} <- Notebook.fetch_section(data.notebook, section_id) do cell = %{Cell.new(type) | id: id} @@ -191,39 +204,24 @@ defmodule Livebook.Session.Data do end end - def apply_operation(data, {:delete_section, _client_pid, id}) do - with {:ok, section} <- Notebook.fetch_section(data.notebook, id) do + def apply_operation(data, {:delete_section, _client_pid, id, delete_cells}) do + with {:ok, section} <- Notebook.fetch_section(data.notebook, id), + true <- section != hd(data.notebook.sections) or delete_cells do data |> with_actions() - |> delete_section(section) + |> delete_section(section, delete_cells) |> set_dirty() |> wrap_ok() + else + _ -> :error end end def apply_operation(data, {:delete_cell, _client_pid, id}) do with {:ok, cell, section} <- Notebook.fetch_cell_and_section(data.notebook, id) do - case data.cell_infos[cell.id].evaluation_status do - :evaluating -> - data - |> with_actions() - |> clear_evaluation() - |> add_action({:stop_evaluation, section}) - - :queued -> - data - |> with_actions() - |> unqueue_cell_evaluation(cell, section) - |> unqueue_dependent_cells_evaluation(cell) - |> mark_dependent_cells_as_stale(cell) - - _ -> - data - |> with_actions() - |> mark_dependent_cells_as_stale(cell) - end - |> delete_cell(cell) - |> add_action({:forget_evaluation, cell, section}) + data + |> with_actions() + |> delete_cell(cell, section) |> set_dirty() |> wrap_ok() end @@ -332,26 +330,14 @@ defmodule Livebook.Session.Data do end def apply_operation(data, {:cancel_cell_evaluation, _client_pid, id}) do - with {:ok, cell, section} <- Notebook.fetch_cell_and_section(data.notebook, id) do - case data.cell_infos[cell.id].evaluation_status do - :evaluating -> - data - |> with_actions() - |> clear_evaluation() - |> add_action({:stop_evaluation, section}) - |> wrap_ok() - - :queued -> - data - |> with_actions() - |> unqueue_cell_evaluation(cell, section) - |> unqueue_dependent_cells_evaluation(cell) - |> mark_dependent_cells_as_stale(cell) - |> wrap_ok() - - _ -> - :error - end + with {:ok, cell, section} <- Notebook.fetch_cell_and_section(data.notebook, id), + true <- data.cell_infos[cell.id].evaluation_status in [:evaluating, :queued] do + data + |> with_actions() + |> cancel_cell_evaluation(cell, section) + |> wrap_ok() + else + _ -> :error end end @@ -491,6 +477,14 @@ defmodule Livebook.Session.Data do ) end + defp insert_section_into({data, _} = data_actions, section_id, index, section) do + data_actions + |> set!( + notebook: Notebook.insert_section_into(data.notebook, section_id, index, section), + section_infos: Map.put(data.section_infos, section.id, new_section_info()) + ) + end + defp insert_cell({data, _} = data_actions, section_id, index, cell) do data_actions |> set!( @@ -499,18 +493,29 @@ defmodule Livebook.Session.Data do ) end - defp delete_section({data, _} = data_actions, section) do + defp delete_section(data_actions, section, delete_cells) do + {data, _} = + data_actions = + if delete_cells do + data_actions + |> reduce(Enum.reverse(section.cells), &delete_cell(&1, &2, section)) + else + data_actions + end + data_actions |> set!( notebook: Notebook.delete_section(data.notebook, section.id), section_infos: Map.delete(data.section_infos, section.id), - deleted_sections: [section | data.deleted_sections] + deleted_sections: [%{section | cells: []} | data.deleted_sections] ) - |> reduce(section.cells, &delete_cell_info/2) end - defp delete_cell({data, _} = data_actions, cell) do + defp delete_cell({data, _} = data_actions, cell, section) do data_actions + |> cancel_cell_evaluation(cell, section) + |> mark_dependent_cells_as_stale(cell) + |> add_action({:forget_evaluation, cell, section}) |> set!( notebook: Notebook.delete_cell(data.notebook, cell.id), deleted_cells: [cell | data.deleted_cells] @@ -630,6 +635,8 @@ defmodule Livebook.Session.Data do |> set_section_info!(section.id, evaluating_cell_id: nil) end + defp mark_dependent_cells_as_stale(data_actions, %Cell.Markdown{}), do: data_actions + defp mark_dependent_cells_as_stale({data, _} = data_actions, cell) do dependent = dependent_cells_with_section(data, cell.id) mark_cells_as_stale(data_actions, dependent) @@ -751,6 +758,24 @@ defmodule Livebook.Session.Data do end) end + defp cancel_cell_evaluation({data, _} = data_actions, cell, section) do + case data.cell_infos[cell.id].evaluation_status do + :evaluating -> + data_actions + |> clear_evaluation() + |> add_action({:stop_evaluation, section}) + + :queued -> + data_actions + |> unqueue_cell_evaluation(cell, section) + |> unqueue_dependent_cells_evaluation(cell) + |> mark_dependent_cells_as_stale(cell) + + _ -> + data_actions + end + end + defp unqueue_dependent_cells_evaluation({data, _} = data_actions, cell) do dependent = dependent_cells_with_section(data, cell.id) unqueue_cells_evaluation(data_actions, dependent) diff --git a/lib/livebook_web/helpers.ex b/lib/livebook_web/helpers.ex index 2127ace9ce5..a2f4483038c 100644 --- a/lib/livebook_web/helpers.ex +++ b/lib/livebook_web/helpers.ex @@ -135,13 +135,18 @@ defmodule LivebookWeb.Helpers do @doc """ Renders a checkbox input styled as a switch. """ - def render_switch(name, checked, label) do - assigns = %{name: name, checked: checked, label: label} + def render_switch(name, checked, label, opts \\ []) do + assigns = %{ + name: name, + checked: checked, + label: label, + disabled: Keyword.get(opts, :disabled, false) + } ~L"""
<%= @label %> -
""" diff --git a/lib/livebook_web/live/session_live/section_component.ex b/lib/livebook_web/live/session_live/section_component.ex index 19a46368aa8..c48c7cb74df 100644 --- a/lib/livebook_web/live/session_live/section_component.ex +++ b/lib/livebook_web/live/session_live/section_component.ex @@ -41,9 +41,10 @@ defmodule LivebookWeb.SessionLive.SectionComponent do - + <% end %> @@ -54,8 +55,7 @@ defmodule LivebookWeb.SessionLive.SectionComponent do id: "#{@section_view.id}:#{index}", persistent: false, section_id: @section_view.id, - insert_cell_index: index, - insert_section_index: nil %> + insert_cell_index: index %> <%= live_component LivebookWeb.SessionLive.CellComponent, id: cell_view.id, session_id: @session_id, @@ -65,8 +65,7 @@ defmodule LivebookWeb.SessionLive.SectionComponent do id: "#{@section_view.id}:last", persistent: @section_view.cell_views == [], section_id: @section_view.id, - insert_cell_index: length(@section_view.cell_views), - insert_section_index: @index + 1 %> + insert_cell_index: length(@section_view.cell_views) %> diff --git a/lib/livebook_web/live/session_live/shortcuts_component.ex b/lib/livebook_web/live/session_live/shortcuts_component.ex index 7ee5088d57f..bfbb61f8e01 100644 --- a/lib/livebook_web/live/session_live/shortcuts_component.ex +++ b/lib/livebook_web/live/session_live/shortcuts_component.ex @@ -24,7 +24,6 @@ defmodule LivebookWeb.SessionLive.ShortcutsComponent do %{seq: ["m"], desc: "Insert Markdown cell below"}, %{seq: ["N"], desc: "Insert Elixir cell above"}, %{seq: ["M"], desc: "Insert Markdown cell above"}, - %{seq: ["S"], desc: "Add section"}, %{seq: ["d", "d"], desc: "Delete cell"}, %{seq: ["e", "e"], desc: "Evaluate cell"}, %{seq: ["e", "s"], desc: "Evaluate section"}, diff --git a/lib/livebook_web/router.ex b/lib/livebook_web/router.ex index e84344c7c9b..9ac71c529a3 100644 --- a/lib/livebook_web/router.ex +++ b/lib/livebook_web/router.ex @@ -33,6 +33,7 @@ defmodule LivebookWeb.Router do live "/sessions/:id/settings/file", SessionLive, :file_settings live "/sessions/:id/cell-settings/:cell_id", SessionLive, :cell_settings live "/sessions/:id/cell-upload/:cell_id", SessionLive, :cell_upload + live "/sessions/:id/delete-section/:section_id", SessionLive, :delete_section get "/sessions/:id/images/:image", SessionController, :show_image live_dashboard "/dashboard", metrics: LivebookWeb.Telemetry diff --git a/test/livebook/session/data_test.exs b/test/livebook/session/data_test.exs index fb96939836a..40e76f3e5a6 100644 --- a/test/livebook/session/data_test.exs +++ b/test/livebook/session/data_test.exs @@ -48,6 +48,49 @@ defmodule Livebook.Session.DataTest do end end + describe "apply_operation/2 given :insert_section_into" do + test "returns an error given invalid section id" do + data = Data.new() + operation = {:insert_section_into, self(), "nonexistent", 0, "s1"} + assert :error = Data.apply_operation(data, operation) + end + + test "adds new section below the given one and section info" do + data = + data_after_operations!([ + {:insert_section, self(), 0, "s1"} + ]) + + operation = {:insert_section_into, self(), "s1", 0, "s2"} + + assert {:ok, + %{ + notebook: %{ + sections: [%{id: "s1"}, %{id: "s2"}] + }, + section_infos: %{"s2" => _} + }, []} = Data.apply_operation(data, operation) + end + + test "moves cells below the given index to the newly inserted section" do + data = + data_after_operations!([ + {:insert_section, self(), 0, "s1"}, + {:insert_cell, self(), "s1", 0, :elixir, "c1"}, + {:insert_cell, self(), "s1", 1, :elixir, "c2"} + ]) + + operation = {:insert_section_into, self(), "s1", 1, "s2"} + + assert {:ok, + %{ + notebook: %{ + sections: [%{cells: [%{id: "c1"}]}, %{cells: [%{id: "c2"}]}] + } + }, []} = Data.apply_operation(data, operation) + end + end + describe "apply_operation/2 given :insert_cell" do test "returns an error given invalid section id" do data = Data.new() @@ -95,7 +138,7 @@ defmodule Livebook.Session.DataTest do describe "apply_operation/2 given :delete_section" do test "returns an error given invalid section id" do data = Data.new() - operation = {:delete_section, self(), "nonexistent"} + operation = {:delete_section, self(), "nonexistent", true} assert :error = Data.apply_operation(data, operation) end @@ -105,7 +148,7 @@ defmodule Livebook.Session.DataTest do {:insert_section, self(), 0, "s1"} ]) - operation = {:delete_section, self(), "s1"} + operation = {:delete_section, self(), "s1", true} empty_map = %{} assert {:ok, @@ -117,6 +160,82 @@ defmodule Livebook.Session.DataTest do deleted_sections: [%{id: "s1"}] }, []} = Data.apply_operation(data, operation) end + + test "returns error when cell deletion is disabled for the first cell" do + data = + data_after_operations!([ + {:insert_section, self(), 0, "s1"}, + {:insert_cell, self(), "s1", 0, :elixir, "c1"} + ]) + + operation = {:delete_section, self(), "s1", false} + assert :error = Data.apply_operation(data, operation) + end + + test "keeps cells when cell deletion is disabled" do + data = + data_after_operations!([ + {:insert_section, self(), 0, "s1"}, + {:insert_cell, self(), "s1", 0, :elixir, "c1"}, + {:insert_section, self(), 1, "s2"}, + {:insert_cell, self(), "s2", 0, :elixir, "c2"} + ]) + + operation = {:delete_section, self(), "s2", false} + + assert {:ok, + %{ + notebook: %{ + sections: [%{id: "s1", cells: [%{id: "c1"}, %{id: "c2"}]}] + }, + deleted_cells: [] + }, []} = Data.apply_operation(data, operation) + end + + test "deletes cells when cell deletion is enabled" do + data = + data_after_operations!([ + {:insert_section, self(), 0, "s1"}, + {:insert_cell, self(), "s1", 0, :elixir, "c1"}, + {:insert_section, self(), 1, "s2"}, + {:insert_cell, self(), "s2", 0, :elixir, "c2"} + ]) + + operation = {:delete_section, self(), "s2", true} + + assert {:ok, + %{ + notebook: %{ + sections: [%{id: "s1", cells: [%{id: "c1"}]}] + }, + deleted_cells: [%{id: "c2"}] + }, + [{:forget_evaluation, %{id: "c2"}, %{id: "s2"}}]} = + Data.apply_operation(data, operation) + end + + test "marks evaluated child cells as stale when cells get deleted" do + data = + data_after_operations!([ + {:insert_section, self(), 0, "s1"}, + {:insert_cell, self(), "s1", 0, :elixir, "c1"}, + {:insert_section, self(), 1, "s2"}, + {:insert_cell, self(), "s2", 0, :elixir, "c2"}, + # Evaluate both cells + {:set_runtime, self(), NoopRuntime.new()}, + {:queue_cell_evaluation, self(), "c1"}, + {:add_cell_evaluation_response, self(), "c1", @eval_resp, @eval_meta}, + {:queue_cell_evaluation, self(), "c2"}, + {:add_cell_evaluation_response, self(), "c2", @eval_resp, @eval_meta} + ]) + + operation = {:delete_section, self(), "s1", true} + + assert {:ok, + %{ + cell_infos: %{"c2" => %{validity_status: :stale}} + }, _actions} = Data.apply_operation(data, operation) + end end describe "apply_operation/2 given :delete_cell" do @@ -207,6 +326,26 @@ defmodule Livebook.Session.DataTest do }, _actions} = Data.apply_operation(data, operation) end + test "deleting a markdown cell does not change child cell validity" do + data = + data_after_operations!([ + {:insert_section, self(), 0, "s1"}, + {:insert_cell, self(), "s1", 0, :markdown, "c1"}, + {:insert_cell, self(), "s1", 1, :elixir, "c2"}, + # Evaluate the elixir cell + {:set_runtime, self(), NoopRuntime.new()}, + {:queue_cell_evaluation, self(), "c2"}, + {:add_cell_evaluation_response, self(), "c2", @eval_resp, @eval_meta} + ]) + + operation = {:delete_cell, self(), "c1"} + + assert {:ok, + %{ + cell_infos: %{"c2" => %{validity_status: :evaluated}} + }, _actions} = Data.apply_operation(data, operation) + end + test "returns forget evaluation action" do data = data_after_operations!([ diff --git a/test/livebook/session_test.exs b/test/livebook/session_test.exs index 7f7dee6a5e1..747383abc4a 100644 --- a/test/livebook/session_test.exs +++ b/test/livebook/session_test.exs @@ -37,15 +37,15 @@ defmodule Livebook.SessionTest do end end - describe "delete_section/2" do + describe "delete_section/3" do test "sends a delete opreation to subscribers", %{session_id: session_id} do Phoenix.PubSub.subscribe(Livebook.PubSub, "sessions:#{session_id}") pid = self() {section_id, _cell_id} = insert_section_and_cell(session_id) - Session.delete_section(session_id, section_id) - assert_receive {:operation, {:delete_section, ^pid, ^section_id}} + Session.delete_section(session_id, section_id, false) + assert_receive {:operation, {:delete_section, ^pid, ^section_id, false}} end end