Allow re-ordering of sections (#221)

* Allow server-side re-ordering of sections

* `Livebook.Notenook.move_section` definition
* Management and implementation of requests
* tests

This commit allows a person to send a request to the server to move a
section. However, the functionality in not yet available in the UI.

* Allow "Move up" and "Move down" functionality for sections

* Rendering of up and down "arrows" at Section's side
* Request from UI on click

This commit enables a user to move a section upwards or downwards, much
like a cell. However, after the section moves, the focus is not changed
to it.

* Apply formatting

* Define a function to update cell status

* Defines a common function for `move_cell` and `move_section` to use to
update cell status.

assets/js/session/index.js

@@ -89,6 +89,10 @@ const Session = {
       handleSectionDeleted(this, sectionId);
     });
 
+    this.handleEvent("section_moved", ({ section_id: sectionId }) => {
+      handleSectionMoved(this, sectionId);
+    });
+
     this.handleEvent("cell_upload", ({ cell_id: cellId, url }) => {
       handleCellUpload(this, cellId, url);
     });
@@ -504,6 +508,12 @@ function handleSectionDeleted(hook, sectionId) {
   }
 }
 
+function handleSectionMoved(hook, sectionId) {
+  if (hook.state.focusedSectionId === sectionId) {
+    globalPubSub.broadcast("session", { type: "section_moved", sectionId });
+  }
+}
+
 function handleCellUpload(hook, cellId, url) {
   if (hook.state.focusedCellId !== cellId) {
     setFocusedCell(hook, cellId);

lib/livebook/notebook.ex

@@ -229,6 +229,28 @@ defmodule Livebook.Notebook do
     index |> max(0) |> min(length(list) - 1)
   end
 
+  @doc """
+  Moves section by the given offset.
+  """
+  @spec move_section(t(), Section.id(), integer()) :: t()
+  def move_section(notebook, section_id, offset) do
+    # We first find the index of the given section.
+    # Then we find its' new index from given offset.
+    # Finally, we move the section, and return the new notebook.
+
+    idx =
+      Enum.find_index(notebook.sections, fn
+        section -> section.id == section_id
+      end)
+
+    new_idx = (idx + offset) |> clamp_index(notebook.sections)
+
+    {section, sections} = List.pop_at(notebook.sections, idx)
+    sections = List.insert_at(sections, new_idx, section)
+
+    %{notebook | sections: sections}
+  end
+
   @doc """
   Returns a list of `{cell, section}` pairs including all Elixir cells in order.
   """

lib/livebook/session.ex

@@ -144,6 +144,14 @@ defmodule Livebook.Session do
     GenServer.cast(name(session_id), {:move_cell, self(), cell_id, offset})
   end
 
+  @doc """
+  Asynchronously sends section move request to the server.
+  """
+  @spec move_section(id(), Section.id(), integer()) :: :ok
+  def move_section(session_id, section_id, offset) do
+    GenServer.cast(name(session_id), {:move_section, self(), section_id, offset})
+  end
+
   @doc """
   Asynchronously sends cell evaluation request to the server.
   """
@@ -357,6 +365,11 @@ defmodule Livebook.Session do
     {:noreply, handle_operation(state, operation)}
   end
 
+  def handle_cast({:move_section, client_pid, section_id, offset}, state) do
+    operation = {:move_section, client_pid, section_id, offset}
+    {:noreply, handle_operation(state, operation)}
+  end
+
   def handle_cast({:queue_cell_evaluation, client_pid, cell_id}, state) do
     case ensure_runtime(state) do
       {:ok, state} ->

lib/livebook/session/data.ex

@@ -77,6 +77,7 @@ defmodule Livebook.Session.Data do
           | {:delete_section, pid(), Section.id()}
           | {:delete_cell, pid(), Cell.id()}
           | {:move_cell, pid(), Cell.id(), offset :: integer()}
+          | {:move_section, pid(), Section.id(), offset :: integer()}
           | {:queue_cell_evaluation, pid(), Cell.id()}
           | {:add_cell_evaluation_stdout, pid(), Cell.id(), String.t()}
           | {:add_cell_evaluation_response, pid(), Cell.id(), Evaluator.evaluation_response()}
@@ -229,6 +230,19 @@ defmodule Livebook.Session.Data do
     end
   end
 
+  def apply_operation(data, {:move_section, _client_pid, id, offset}) do
+    with {:ok, section} <- Notebook.fetch_section(data.notebook, id),
+         true <- offset != 0 do
+      data
+      |> with_actions()
+      |> move_section(section, offset)
+      |> set_dirty()
+      |> wrap_ok()
+    else
+      _ -> :error
+    end
+  end
+
   def apply_operation(data, {:queue_cell_evaluation, _client_pid, id}) do
     with {:ok, cell, section} <- Notebook.fetch_cell_and_section(data.notebook, id),
          :elixir <- cell.type,
@@ -455,8 +469,22 @@ defmodule Livebook.Session.Data do
   defp move_cell({data, _} = data_actions, cell, offset) do
     updated_notebook = Notebook.move_cell(data.notebook, cell.id, offset)
 
-    cells_with_section_before = Notebook.elixir_cells_with_section(data.notebook)
+    data_actions
-    cells_with_section_after = Notebook.elixir_cells_with_section(updated_notebook)
+    |> set!(notebook: updated_notebook)
+    |> update_cells_status_after_moved(data.notebook)
+  end
+
+  defp move_section({data, _} = data_actions, section, offset) do
+    updated_notebook = Notebook.move_section(data.notebook, section.id, offset)
+
+    data_actions
+    |> set!(notebook: updated_notebook)
+    |> update_cells_status_after_moved(data.notebook)
+  end
+
+  defp update_cells_status_after_moved({data, _} = data_actions, prev_notebook) do
+    cells_with_section_before = Notebook.elixir_cells_with_section(prev_notebook)
+    cells_with_section_after = Notebook.elixir_cells_with_section(data.notebook)
 
     affected_cells_with_section =
       cells_with_section_after
@@ -467,7 +495,6 @@ defmodule Livebook.Session.Data do
       |> Enum.map(fn {new, _old} -> new end)
 
     data_actions
-    |> set!(notebook: updated_notebook)
     |> mark_cells_as_stale(affected_cells_with_section)
     |> unqueue_cells_evaluation(affected_cells_with_section)
   end

lib/livebook_web/live/session_live.ex

@@ -300,6 +300,13 @@ defmodule LivebookWeb.SessionLive do
     {:noreply, socket}
   end
 
+  def handle_event("move_section", %{"section_id" => section_id, "offset" => offset}, socket) do
+    offset = ensure_integer(offset)
+    Session.move_section(socket.assigns.session_id, section_id, offset)
+
+    {:noreply, socket}
+  end
+
   def handle_event("queue_cell_evaluation", %{"cell_id" => cell_id}, socket) do
     Session.queue_cell_evaluation(socket.assigns.session_id, cell_id)
     {:noreply, socket}

lib/livebook_web/live/session_live/section_component.ex

@@ -17,6 +17,22 @@ defmodule LivebookWeb.SessionLive.SectionComponent do
           <%# ^ Note it's important there's no space between <h2> and </h2>
             because we want the content to exactly match section name. %>
         <div class="flex space-x-2 items-center" data-element="section-actions">
+          <span class="tooltip top" aria-label="Move up">
+            <button class="icon-button"
+              phx-click="move_section"
+              phx-value-section_id="<%= @section_view.id %>"
+              phx-value-offset="-1">
+              <%= remix_icon("arrow-up-s-line", class: "text-xl") %>
+            </button>
+          </span>
+          <span class="tooltip top" aria-label="Move down">
+            <button class="icon-button"
+              phx-click="move_section"
+              phx-value-section_id="<%= @section_view.id %>"
+              phx-value-offset="1">
+              <%= remix_icon("arrow-down-s-line", class: "text-xl") %>
+            </button>
+          </span>
           <span class="tooltip top" aria-label="Delete">
             <button class="icon-button" phx-click="delete_section" phx-value-section_id="<%= @section_view.id %>" tabindex="-1">
               <%= remix_icon("delete-bin-6-line", class: "text-xl") %>

test/livebook/session/data_test.exs

@@ -430,6 +430,216 @@ defmodule Livebook.Session.DataTest do
     end
   end
 
+  describe "apply_operation/2 given :move_section" do
+    test "returns an error given invalid section id" do
+      data = Data.new()
+      operation = {:move_section, self(), "nonexistent", 1}
+      assert :error = Data.apply_operation(data, operation)
+    end
+
+    test "returns an error given no offset" do
+      data =
+        data_after_operations!([
+          {:insert_section, self(), 0, "s1"},
+          {:insert_section, self(), 1, "s2"}
+        ])
+
+      operation = {:move_section, self(), "s2", 0}
+      assert :error = Data.apply_operation(data, operation)
+    end
+
+    test "given negative offset moves the section and marks relevant cells as stale" do
+      data =
+        data_after_operations!([
+          {:insert_section, self(), 0, "s1"},
+          {:insert_section, self(), 1, "s2"},
+          # Add cells
+          {:insert_cell, self(), "s1", 0, :elixir, "c1"},
+          {:insert_cell, self(), "s1", 1, :elixir, "c2"},
+          {:insert_cell, self(), "s2", 0, :elixir, "c3"},
+          {:insert_cell, self(), "s2", 1, :elixir, "c4"},
+          # Evaluate cells
+          {:queue_cell_evaluation, self(), "c1"},
+          {:add_cell_evaluation_response, self(), "c1", {:ok, nil}},
+          {:queue_cell_evaluation, self(), "c2"},
+          {:add_cell_evaluation_response, self(), "c2", {:ok, nil}},
+          {:queue_cell_evaluation, self(), "c3"},
+          {:add_cell_evaluation_response, self(), "c3", {:ok, nil}},
+          {:queue_cell_evaluation, self(), "c4"},
+          {:add_cell_evaluation_response, self(), "c4", {:ok, nil}}
+        ])
+
+      operation = {:move_section, self(), "s2", -1}
+
+      assert {:ok,
+              %{
+                notebook: %{
+                  sections: [
+                    %{
+                      cells: [%{id: "c3"}, %{id: "c4"}]
+                    },
+                    %{
+                      cells: [%{id: "c1"}, %{id: "c2"}]
+                    }
+                  ]
+                },
+                cell_infos: %{
+                  "c1" => %{validity_status: :stale},
+                  "c2" => %{validity_status: :stale},
+                  "c3" => %{validity_status: :stale},
+                  "c4" => %{validity_status: :stale}
+                }
+              }, []} = Data.apply_operation(data, operation)
+    end
+
+    test "given positive offset moves the section and marks relevant cells as stale" do
+      data =
+        data_after_operations!([
+          {:insert_section, self(), 0, "s1"},
+          {:insert_section, self(), 1, "s2"},
+          # Add cells
+          {:insert_cell, self(), "s1", 0, :elixir, "c1"},
+          {:insert_cell, self(), "s1", 1, :elixir, "c2"},
+          {:insert_cell, self(), "s2", 0, :elixir, "c3"},
+          {:insert_cell, self(), "s2", 1, :elixir, "c4"},
+          # Evaluate cells
+          {:queue_cell_evaluation, self(), "c1"},
+          {:add_cell_evaluation_response, self(), "c1", {:ok, nil}},
+          {:queue_cell_evaluation, self(), "c2"},
+          {:add_cell_evaluation_response, self(), "c2", {:ok, nil}},
+          {:queue_cell_evaluation, self(), "c3"},
+          {:add_cell_evaluation_response, self(), "c3", {:ok, nil}},
+          {:queue_cell_evaluation, self(), "c4"},
+          {:add_cell_evaluation_response, self(), "c4", {:ok, nil}}
+        ])
+
+      operation = {:move_section, self(), "s1", 1}
+
+      assert {:ok,
+              %{
+                notebook: %{
+                  sections: [
+                    %{
+                      cells: [%{id: "c3"}, %{id: "c4"}]
+                    },
+                    %{
+                      cells: [%{id: "c1"}, %{id: "c2"}]
+                    }
+                  ]
+                },
+                cell_infos: %{
+                  "c1" => %{validity_status: :stale},
+                  "c2" => %{validity_status: :stale},
+                  "c3" => %{validity_status: :stale},
+                  "c4" => %{validity_status: :stale}
+                }
+              }, []} = Data.apply_operation(data, operation)
+    end
+
+    test "marks relevant cells in further sections as stale" do
+      data =
+        data_after_operations!([
+          {:insert_section, self(), 0, "s1"},
+          {:insert_section, self(), 1, "s2"},
+          {:insert_section, self(), 2, "s3"},
+          # Add cells
+          {:insert_cell, self(), "s1", 0, :elixir, "c1"},
+          {:insert_cell, self(), "s2", 1, :elixir, "c2"},
+          {:insert_cell, self(), "s3", 0, :elixir, "c3"},
+          # Evaluate cells
+          {:queue_cell_evaluation, self(), "c1"},
+          {:add_cell_evaluation_response, self(), "c1", {:ok, nil}},
+          {:queue_cell_evaluation, self(), "c2"},
+          {:add_cell_evaluation_response, self(), "c2", {:ok, nil}},
+          {:queue_cell_evaluation, self(), "c3"},
+          {:add_cell_evaluation_response, self(), "c3", {:ok, nil}}
+        ])
+
+      operation = {:move_section, self(), "s1", 1}
+
+      assert {:ok,
+              %{
+                cell_infos: %{"c3" => %{validity_status: :stale}}
+              }, []} = Data.apply_operation(data, operation)
+    end
+
+    test "moving a section with only markdown cells does not change validity" do
+      data =
+        data_after_operations!([
+          {:insert_section, self(), 0, "s1"},
+          {:insert_section, self(), 1, "s2"},
+          # Add cells
+          {:insert_cell, self(), "s1", 0, :elixir, "c1"},
+          {:insert_cell, self(), "s2", 0, :markdown, "c2"},
+          # Evaluate the Elixir cell
+          {:queue_cell_evaluation, self(), "c1"},
+          {:add_cell_evaluation_response, self(), "c1", {:ok, nil}}
+        ])
+
+      operation = {:move_section, self(), "s2", -1}
+
+      assert {:ok,
+              %{
+                cell_infos: %{
+                  "c1" => %{validity_status: :evaluated}
+                }
+              }, []} = Data.apply_operation(data, operation)
+    end
+
+    test "affected queued cells are unqueued" do
+      data =
+        data_after_operations!([
+          {:insert_section, self(), 0, "s1"},
+          {:insert_section, self(), 1, "s2"},
+          # Add cells
+          {:insert_cell, self(), "s1", 0, :elixir, "c1"},
+          {:insert_cell, self(), "s2", 0, :elixir, "c2"},
+          # Evaluate the Elixir cell
+          {:queue_cell_evaluation, self(), "c1"},
+          {:queue_cell_evaluation, self(), "c2"}
+        ])
+
+      operation = {:move_section, self(), "s2", -1}
+
+      assert {:ok,
+              %{
+                cell_infos: %{
+                  "c2" => %{evaluation_status: :ready}
+                }
+              }, []} = Data.apply_operation(data, operation)
+    end
+
+    test "does not invalidate cells in moved section if the order of Elixir cells stays the same" do
+      data =
+        data_after_operations!([
+          {:insert_section, self(), 0, "s1"},
+          {:insert_section, self(), 1, "s2"},
+          {:insert_section, self(), 2, "s3"},
+          {:insert_section, self(), 3, "s4"},
+          # Add cells
+          {:insert_cell, self(), "s1", 0, :elixir, "c1"},
+          {:insert_cell, self(), "s2", 0, :markdown, "c2"},
+          {:insert_cell, self(), "s3", 0, :elixir, "c3"},
+          {:insert_cell, self(), "s4", 0, :markdown, "c4"},
+          # Evaluate cells
+          {:queue_cell_evaluation, self(), "c1"},
+          {:add_cell_evaluation_response, self(), "c1", {:ok, nil}},
+          {:queue_cell_evaluation, self(), "c3"},
+          {:add_cell_evaluation_response, self(), "c3", {:ok, nil}}
+        ])
+
+      operation = {:move_section, self(), "s4", -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()