From 77b60c8110b37f2c83f94babefd76fa08e476ed3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Tue, 2 Feb 2021 19:58:06 +0100 Subject: [PATCH] Integrate evaluation into UI (#17) * Render evaluation outputs and result * Fix auto-scrolling to not be interrupted by editor focus * Add cell output tests * Run formatter * Show cell status * Apply review suggestions * Change EEx strings to Live EEx --- assets/css/app.css | 14 ++ assets/js/cell/index.js | 4 + assets/js/session/index.js | 30 +-- lib/live_book/notebook/cell.ex | 1 - lib/live_book/session/data.ex | 25 ++- lib/live_book_web/live/cell.ex | 174 +++++++++++++++--- lib/live_book_web/live/icons.ex | 45 +++-- lib/live_book_web/live/insert_cell_actions.ex | 4 +- lib/live_book_web/live/section.ex | 3 + lib/live_book_web/live/session_live.ex | 15 ++ test/live_book/session/data_test.exs | 67 ++++++- test/live_book_web/live/session_live_test.exs | 83 +++++++-- 12 files changed, 378 insertions(+), 87 deletions(-) diff --git a/assets/css/app.css b/assets/css/app.css index f52d1fcd9..b94823072 100644 --- a/assets/css/app.css +++ b/assets/css/app.css @@ -121,3 +121,17 @@ iframe[hidden] { .bg-editor { background-color: #282c34; } + +.tiny-scrollbar::-webkit-scrollbar { + width: 0.4rem; + height: 0.4rem; +} + +.tiny-scrollbar::-webkit-scrollbar-thumb { + border-radius: 0.25rem; + @apply bg-gray-400; +} + +.tiny-scrollbar::-webkit-scrollbar-track { + @apply bg-gray-100; +} diff --git a/assets/js/cell/index.js b/assets/js/cell/index.js index 84855342b..f96fe3aeb 100644 --- a/assets/js/cell/index.js +++ b/assets/js/cell/index.js @@ -67,6 +67,10 @@ const Cell = { if (isActive(prevProps) && !isActive(this.props)) { this.liveEditor.blur(); } + + if (!prevProps.isFocused && this.props.isFocused) { + this.el.scrollIntoView({ behavior: "smooth", block: "center" }); + } }, }; diff --git a/assets/js/session/index.js b/assets/js/session/index.js index e6d948df4..5c279675b 100644 --- a/assets/js/session/index.js +++ b/assets/js/session/index.js @@ -14,7 +14,7 @@ const Session = { this.props = getProps(this); // Keybindings - document.addEventListener("keydown", (event) => { + this.handleDocumentKeydown = (event) => { if (event.shiftKey && event.key === "Enter" && !event.repeat) { if (this.props.focusedCellId !== null) { // If the editor is focused we don't want it to receive the input @@ -27,33 +27,35 @@ const Session = { } else if (event.altKey && event.key === "k") { event.preventDefault(); this.pushEvent("move_cell_focus", { offset: -1 }); + } else if (event.ctrlKey && event.key === "Enter") { + event.stopPropagation(); + this.pushEvent("queue_cell_evaluation", {}); } - }); + }; + + document.addEventListener("keydown", this.handleDocumentKeydown, true); // Focus/unfocus a cell when the user clicks somewhere - document.addEventListener("click", (event) => { + this.handleDocumentClick = (event) => { // Find the parent with cell id info, if there is one const cell = event.target.closest("[data-cell-id]"); const cellId = cell ? cell.dataset.cellId : null; if (cellId !== this.props.focusedCellId) { this.pushEvent("focus_cell", { cell_id: cellId }); } - }); + }; + + document.addEventListener("click", this.handleDocumentClick); }, updated() { - const prevProps = this.props; this.props = getProps(this); - - // When a new cell gets focus, center it nicely on the page - if ( - this.props.focusedCellId && - this.props.focusedCellId !== prevProps.focusedCellId - ) { - const cell = this.el.querySelector(`#cell-${this.props.focusedCellId}`); - cell.scrollIntoView({ behavior: "smooth", block: "center" }); - } }, + + destroyed() { + document.removeEventListener("keydown", this.handleDocumentKeydown); + document.removeEventListener("click", this.handleDocumentClick); + } }; function getProps(hook) { diff --git a/lib/live_book/notebook/cell.ex b/lib/live_book/notebook/cell.ex index 8f3f84f32..98944d522 100644 --- a/lib/live_book/notebook/cell.ex +++ b/lib/live_book/notebook/cell.ex @@ -18,7 +18,6 @@ defmodule LiveBook.Notebook.Cell do id: id(), type: type(), source: String.t(), - # TODO: expand on this outputs: list(), metadata: %{atom() => term()} } diff --git a/lib/live_book/session/data.ex b/lib/live_book/session/data.ex index f9b80b36f..b3dbbd09b 100644 --- a/lib/live_book/session/data.ex +++ b/lib/live_book/session/data.ex @@ -326,22 +326,35 @@ defmodule LiveBook.Session.Data do |> set_cell_info!(cell.id, evaluation_status: :ready) end - defp add_cell_evaluation_stdout({data, _} = data_actions, _cell, _string) do + defp add_cell_evaluation_stdout({data, _} = data_actions, cell, string) do data_actions |> set!( - # TODO: add stdout to cell outputs - notebook: data.notebook + notebook: + Notebook.update_cell(data.notebook, cell.id, fn cell -> + %{cell | outputs: add_output(cell.outputs, string)} + end) ) end - defp add_cell_evaluation_response({data, _} = data_actions, _cell, _response) do + defp add_cell_evaluation_response({data, _} = data_actions, cell, response) do data_actions |> set!( - # TODO: add result to outputs - notebook: data.notebook + notebook: + Notebook.update_cell(data.notebook, cell.id, fn cell -> + %{cell | outputs: add_output(cell.outputs, response)} + end) ) end + defp add_output([], output), do: [output] + + defp add_output([head | tail], output) when is_binary(head) and is_binary(output) do + # Merge consecutive string outputs + [head <> output | tail] + end + + defp add_output(outputs, output), do: [output | outputs] + defp finish_cell_evaluation(data_actions, cell, section) do data_actions |> set_cell_info!(cell.id, diff --git a/lib/live_book_web/live/cell.ex b/lib/live_book_web/live/cell.ex index 52e765ee0..a04860c28 100644 --- a/lib/live_book_web/live/cell.ex +++ b/lib/live_book_web/live/cell.ex @@ -17,14 +17,16 @@ defmodule LiveBookWeb.Cell do def render_cell_content(%{cell: %{type: :markdown}} = assigns) do ~L""" -
- -
+ <%= if @focused do %> +
+ +
+ <% end %>
"> - <%= render_editor(@cell) %> + <%= render_editor(@cell, @cell_info) %>
@@ -35,26 +37,44 @@ defmodule LiveBookWeb.Cell do def render_cell_content(%{cell: %{type: :elixir}} = assigns) do ~L""" -
- - -
+ <%= if @focused do %> +
+ + +
+ <% end %> - <%= render_editor(@cell) %> + <%= render_editor(@cell, @cell_info, show_status: true) %> + + <%= if @cell.outputs != [] do %> +
+ <%= render_outputs(@cell.outputs) %> +
+ <% end %> """ end - defp render_editor(cell) do - ~E""" -
- <%= render_editor_content_placeholder(cell.source) %> + defp render_editor(cell, cell_info, opts \\ []) do + show_status = Keyword.get(opts, :show_status, false) + assigns = %{cell: cell, cell_info: cell_info, show_status: show_status} + + ~L""" +
+
+ <%= render_editor_content_placeholder(@cell.source) %> +
+ + <%= if @show_status do %> +
+ <%= render_cell_status(@cell_info) %> +
+ <% end %>
""" end @@ -64,13 +84,17 @@ defmodule LiveBookWeb.Cell do # or and editors are mounted, so show neat placeholders immediately. defp render_markdown_content_placeholder("" = _content) do - ~E""" + assigns = %{} + + ~L"""
""" end defp render_markdown_content_placeholder(_content) do - ~E""" + assigns = %{} + + ~L"""
@@ -82,13 +106,17 @@ defmodule LiveBookWeb.Cell do end defp render_editor_content_placeholder("" = _content) do - ~E""" + assigns = %{} + + ~L"""
""" end defp render_editor_content_placeholder(_content) do - ~E""" + assigns = %{} + + ~L"""
@@ -98,4 +126,98 @@ defmodule LiveBookWeb.Cell do
""" end + + defp render_outputs(outputs) do + assigns = %{outputs: outputs} + + ~L""" +
+ <%= for output <- Enum.reverse(@outputs) do %> +
+
+ <%= render_output(output) %> +
+
+ <% end %> +
+ """ + end + + defp render_output(output) when is_binary(output) do + assigns = %{output: output} + + ~L""" +
<%= @output %>
+ """ + end + + defp render_output({:ok, value}) do + inspected = inspect(value, pretty: true, width: 140) + assigns = %{inspected: inspected} + + ~L""" +
<%= @inspected %>
+ """ + end + + defp render_output({:error, kind, error, stacktrace}) do + formatted = Exception.format(kind, error, stacktrace) + assigns = %{formatted: formatted} + + ~L""" +
<%= @formatted %>
+ """ + end + + defp render_cell_status(%{evaluation_status: :evaluating}) do + assigns = %{} + + ~L""" +
+
Evaluating
+ + + + +
+ """ + end + + defp render_cell_status(%{evaluation_status: :queued}) do + assigns = %{} + + ~L""" +
+
Queued
+ + + + +
+ """ + end + + defp render_cell_status(%{validity_status: :evaluated}) do + assigns = %{} + + ~L""" +
+
Evaluated
+
+
+ """ + end + + defp render_cell_status(%{validity_status: :stale}) do + assigns = %{} + + ~L""" +
+
Stale
+
+
+ """ + end + + defp render_cell_status(_), do: nil end diff --git a/lib/live_book_web/live/icons.ex b/lib/live_book_web/live/icons.ex index 320b79869..6d54b2a41 100644 --- a/lib/live_book_web/live/icons.ex +++ b/lib/live_book_web/live/icons.ex @@ -1,44 +1,55 @@ defmodule LiveBookWeb.Icons do - import Phoenix.HTML import Phoenix.HTML.Tag + import Phoenix.LiveView.Helpers @doc """ Returns icon svg tag. """ - @spec svg(atom(), keyword()) :: Phoenix.HTML.safe() def svg(name, attrs \\ []) def svg(:chevron_right, attrs) do - ~e""" - + assigns = %{attrs: heroicon_svg_attrs(attrs)} + + ~L""" + <%= tag(:svg, @attrs) %> + + """ - |> heroicon_svg(attrs) end def svg(:play, attrs) do - ~e""" - - + assigns = %{attrs: heroicon_svg_attrs(attrs)} + + ~L""" + <%= tag(:svg, @attrs) %> + + + """ - |> heroicon_svg(attrs) end def svg(:plus, attrs) do - ~e""" - + assigns = %{attrs: heroicon_svg_attrs(attrs)} + + ~L""" + <%= tag(:svg, @attrs) %> + + """ - |> heroicon_svg(attrs) end def svg(:trash, attrs) do - ~e""" - + assigns = %{attrs: heroicon_svg_attrs(attrs)} + + ~L""" + <%= tag(:svg, @attrs) %> + + """ - |> heroicon_svg(attrs) end # https://heroicons.com - defp heroicon_svg(svg_content, attrs) do + defp heroicon_svg_attrs(attrs) do heroicon_svg_attrs = [ xmlns: "http://www.w3.org/2000/svg", fill: "none", @@ -46,6 +57,6 @@ defmodule LiveBookWeb.Icons do stroke: "currentColor" ] - content_tag(:svg, svg_content, Keyword.merge(attrs, heroicon_svg_attrs)) + Keyword.merge(attrs, heroicon_svg_attrs) end end diff --git a/lib/live_book_web/live/insert_cell_actions.ex b/lib/live_book_web/live/insert_cell_actions.ex index 71b91d392..43d92d9b9 100644 --- a/lib/live_book_web/live/insert_cell_actions.ex +++ b/lib/live_book_web/live/insert_cell_actions.ex @@ -27,7 +27,9 @@ defmodule LiveBookWeb.InsertCellActions do end defp line() do - ~e""" + assigns = %{} + + ~L"""
""" end diff --git a/lib/live_book_web/live/section.ex b/lib/live_book_web/live/section.ex index ae17a72f3..85bb763fa 100644 --- a/lib/live_book_web/live/section.ex +++ b/lib/live_book_web/live/section.ex @@ -25,15 +25,18 @@ defmodule LiveBookWeb.Section do
<%= live_component @socket, LiveBookWeb.InsertCellActions, + id: "#{@section.id}:0", section_id: @section.id, index: 0 %> <%= for {cell, index} <- Enum.with_index(@section.cells) do %> <%= live_component @socket, LiveBookWeb.Cell, + id: cell.id, cell: cell, cell_info: @cell_infos[cell.id], focused: @selected and cell.id == @focused_cell_id, expanded: @selected and cell.id == @focused_cell_id and @focused_cell_expanded %> <%= live_component @socket, LiveBookWeb.InsertCellActions, + id: "#{@section.id}:#{index + 1}", section_id: @section.id, index: index + 1 %> <% end %> diff --git a/lib/live_book_web/live/session_live.ex b/lib/live_book_web/live/session_live.ex index 5d0118331..854ac823d 100644 --- a/lib/live_book_web/live/session_live.ex +++ b/lib/live_book_web/live/session_live.ex @@ -75,6 +75,7 @@ defmodule LiveBookWeb.SessionLive do
<%= for section <- @data.notebook.sections do %> <%= live_component @socket, LiveBookWeb.Section, + id: section.id, section: section, selected: section.id == @selected_section_id, cell_infos: @data.cell_infos, @@ -187,6 +188,20 @@ defmodule LiveBookWeb.SessionLive do end 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} + end + + def handle_event("queue_cell_evaluation", %{}, socket) do + if socket.assigns.focused_cell_id do + Session.queue_cell_evaluation(socket.assigns.session_id, socket.assigns.focused_cell_id) + end + + {:noreply, socket} + end + @impl true def handle_info({:operation, operation}, socket) do case Session.Data.apply_operation(socket.assigns.data, operation) do diff --git a/test/live_book/session/data_test.exs b/test/live_book/session/data_test.exs index 3686c80b1..1679d8360 100644 --- a/test/live_book/session/data_test.exs +++ b/test/live_book/session/data_test.exs @@ -252,14 +252,73 @@ defmodule LiveBook.Session.DataTest do end describe "apply_operation/2 given :add_cell_evaluation_stdout" do - test "update the cell output" do - # TODO assert against output being updated once we do so + test "updates the cell outputs" do + data = + data_after_operations!([ + {:insert_section, 0, "s1"}, + {:insert_cell, "s1", 0, :elixir, "c1"}, + {:queue_cell_evaluation, "c1"} + ]) + + operation = {:add_cell_evaluation_stdout, "c1", "Hello!"} + + assert {:ok, + %{ + notebook: %{ + sections: [ + %{ + cells: [%{outputs: ["Hello!"]}] + } + ] + } + }, []} = Data.apply_operation(data, operation) + end + + test "merges consecutive stdout results" do + data = + data_after_operations!([ + {:insert_section, 0, "s1"}, + {:insert_cell, "s1", 0, :elixir, "c1"}, + {:queue_cell_evaluation, "c1"}, + {:add_cell_evaluation_stdout, "c1", "Hello"} + ]) + + operation = {:add_cell_evaluation_stdout, "c1", " amigo!"} + + assert {:ok, + %{ + notebook: %{ + sections: [ + %{ + cells: [%{outputs: ["Hello amigo!"]}] + } + ] + } + }, []} = Data.apply_operation(data, operation) end end describe "apply_operation/2 given :add_cell_evaluation_response" do - test "update the cell output" do - # TODO assert against output being updated once we do so + test "updates the cell outputs" do + data = + data_after_operations!([ + {:insert_section, 0, "s1"}, + {:insert_cell, "s1", 0, :elixir, "c1"}, + {:queue_cell_evaluation, "c1"} + ]) + + operation = {:add_cell_evaluation_response, "c1", {:ok, [1, 2, 3]}} + + assert {:ok, + %{ + notebook: %{ + sections: [ + %{ + cells: [%{outputs: [{:ok, [1, 2, 3]}]}] + } + ] + } + }, []} = Data.apply_operation(data, operation) end test "marks the cell as evaluated" do diff --git a/test/live_book_web/live/session_live_test.exs b/test/live_book_web/live/session_live_test.exs index 6d3f62c80..819c27a1b 100644 --- a/test/live_book_web/live/session_live_test.exs +++ b/test/live_book_web/live/session_live_test.exs @@ -3,7 +3,7 @@ defmodule LiveBookWeb.SessionLiveTest do import Phoenix.LiveViewTest - alias LiveBook.{SessionSupervisor, Session} + alias LiveBook.{SessionSupervisor, Session, Delta} setup do {:ok, session_id} = SessionSupervisor.create_session() @@ -29,48 +29,42 @@ defmodule LiveBookWeb.SessionLiveTest do test "renders a newly inserted section", %{conn: conn, session_id: session_id} do {:ok, view, _} = live(conn, "/sessions/#{session_id}") - Session.insert_section(session_id, 0) - %{notebook: %{sections: [section]}} = Session.get_data(session_id) + section_id = insert_section(session_id) - assert render(view) =~ section.id + assert render(view) =~ section_id end test "renders an updated section name", %{conn: conn, session_id: session_id} do - Session.insert_section(session_id, 0) - %{notebook: %{sections: [section]}} = Session.get_data(session_id) + section_id = insert_section(session_id) {:ok, view, _} = live(conn, "/sessions/#{session_id}") - Session.set_section_name(session_id, section.id, "My section") + Session.set_section_name(session_id, section_id, "My section") wait_for_session_update(session_id) assert render(view) =~ "My section" end test "renders a newly inserted cell", %{conn: conn, session_id: session_id} do - Session.insert_section(session_id, 0) - %{notebook: %{sections: [section]}} = Session.get_data(session_id) + section_id = insert_section(session_id) {:ok, view, _} = live(conn, "/sessions/#{session_id}") - Session.insert_cell(session_id, section.id, 0, :markdown) - %{notebook: %{sections: [%{cells: [cell]}]}} = Session.get_data(session_id) + cell_id = insert_cell(session_id, section_id, :markdown) - assert render(view) =~ cell.id + assert render(view) =~ cell_id end test "un-renders a deleted cell", %{conn: conn, session_id: session_id} do - Session.insert_section(session_id, 0) - %{notebook: %{sections: [section]}} = Session.get_data(session_id) - Session.insert_cell(session_id, section.id, 0, :markdown) - %{notebook: %{sections: [%{cells: [cell]}]}} = Session.get_data(session_id) + section_id = insert_section(session_id) + cell_id = insert_cell(session_id, section_id, :markdown) {:ok, view, _} = live(conn, "/sessions/#{session_id}") - Session.delete_cell(session_id, cell.id) + Session.delete_cell(session_id, cell_id) wait_for_session_update(session_id) - refute render(view) =~ cell.id + refute render(view) =~ cell_id end end @@ -97,6 +91,40 @@ defmodule LiveBookWeb.SessionLiveTest do assert %{notebook: %{sections: [%{cells: [%{type: :markdown}]}]}} = Session.get_data(session_id) end + + test "queueing cell evaluation updates the shared state", %{conn: conn, session_id: session_id} do + section_id = insert_section(session_id) + cell_id = insert_cell(session_id, section_id, :elixir, "Process.sleep(5)") + + {:ok, view, _} = live(conn, "/sessions/#{session_id}") + + view + |> element("#session") + |> render_hook("queue_cell_evaluation", %{"cell_id" => cell_id}) + + assert %{cell_infos: %{^cell_id => %{evaluation_status: :evaluating}}} = + Session.get_data(session_id) + end + + test "queueing cell evaluation defaults to the focused cell if no cell id is given", + %{conn: conn, session_id: session_id} do + + section_id = insert_section(session_id) + cell_id = insert_cell(session_id, section_id, :elixir, "Process.sleep(5)") + + {:ok, view, _} = live(conn, "/sessions/#{session_id}") + + view + |> element("#session") + |> render_hook("focus_cell", %{"cell_id" => cell_id}) + + view + |> element("#session") + |> render_hook("queue_cell_evaluation", %{}) + + assert %{cell_infos: %{^cell_id => %{evaluation_status: :evaluating}}} = + Session.get_data(session_id) + end end defp wait_for_session_update(session_id) do @@ -105,4 +133,23 @@ defmodule LiveBookWeb.SessionLiveTest do Session.get_data(session_id) :ok end + + # Utils for sending session requests, waiting for the change to be applied + # and retrieving new ids if applicable. + + defp insert_section(session_id) do + Session.insert_section(session_id, 0) + %{notebook: %{sections: [section]}} = Session.get_data(session_id) + section.id + end + + defp insert_cell(session_id, section_id, type, content \\ "") do + Session.insert_cell(session_id, section_id, 0, type) + %{notebook: %{sections: [%{cells: [cell]}]}} = Session.get_data(session_id) + + delta = Delta.new(insert: content) + Session.apply_cell_delta(session_id, self(), cell.id, delta, 1) + + cell.id + end end