From 682ee396d03c231de1ff46e79ff162c07c72c014 Mon Sep 17 00:00:00 2001
From: Benjamin Philip <67545861+Benjamin-Philip@users.noreply.github.com>
Date: Tue, 20 Apr 2021 15:42:29 +0530
Subject: [PATCH] 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                    |  10 +
 lib/livebook/notebook.ex                      |  22 ++
 lib/livebook/session.ex                       |  13 ++
 lib/livebook/session/data.ex                  |  33 ++-
 lib/livebook_web/live/session_live.ex         |   7 +
 .../live/session_live/section_component.ex    |  16 ++
 test/livebook/session/data_test.exs           | 210 ++++++++++++++++++
 7 files changed, 308 insertions(+), 3 deletions(-)

diff --git a/assets/js/session/index.js b/assets/js/session/index.js
index 57b5a2277..a8e4c954b 100644
--- a/assets/js/session/index.js
+++ b/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);
diff --git a/lib/livebook/notebook.ex b/lib/livebook/notebook.ex
index 792f19087..1d2d8cc82 100644
--- a/lib/livebook/notebook.ex
+++ b/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.
   """
diff --git a/lib/livebook/session.ex b/lib/livebook/session.ex
index 117f9b1f9..bc3c112d1 100644
--- a/lib/livebook/session.ex
+++ b/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} ->
diff --git a/lib/livebook/session/data.ex b/lib/livebook/session/data.ex
index 776b956b8..f251aeb75 100644
--- a/lib/livebook/session/data.ex
+++ b/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)
-    cells_with_section_after = Notebook.elixir_cells_with_section(updated_notebook)
+    data_actions
+    |> 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
diff --git a/lib/livebook_web/live/session_live.ex b/lib/livebook_web/live/session_live.ex
index 3a76496bd..69a6bb497 100644
--- a/lib/livebook_web/live/session_live.ex
+++ b/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}
diff --git a/lib/livebook_web/live/session_live/section_component.ex b/lib/livebook_web/live/session_live/section_component.ex
index e774e9616..3d972c87c 100644
--- a/lib/livebook_web/live/session_live/section_component.ex
+++ b/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") %>
diff --git a/test/livebook/session/data_test.exs b/test/livebook/session/data_test.exs
index f518caf08..d0a1ee31a 100644
--- a/test/livebook/session/data_test.exs
+++ b/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()