From dbccadfdcfc778b9f20ee0af2ac42db3d42f629e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Sat, 30 Oct 2021 12:02:26 +0200 Subject: [PATCH] Redesign save to file modal (#663) * Redesign save to file * Always show Save when a file is present * Fix indentation --- lib/livebook_web/helpers.ex | 29 ++ .../live/file_select_component.ex | 17 -- .../live/session_live/persistence_live.ex | 259 +++++++++++------- test/livebook_web/live/session_live_test.exs | 22 +- 4 files changed, 199 insertions(+), 128 deletions(-) diff --git a/lib/livebook_web/helpers.ex b/lib/livebook_web/helpers.ex index 7a92f477e..7d409f3df 100644 --- a/lib/livebook_web/helpers.ex +++ b/lib/livebook_web/helpers.ex @@ -3,6 +3,8 @@ defmodule LivebookWeb.Helpers do alias LivebookWeb.Router.Helpers, as: Routes + alias Livebook.FileSystem + @doc """ Renders a component inside the `Livebook.ModalComponent` component. @@ -246,4 +248,31 @@ defmodule LivebookWeb.Helpers do defdelegate ansi_string_to_html(string), to: LivebookWeb.Helpers.ANSI defdelegate ansi_string_to_html_lines(string), to: LivebookWeb.Helpers.ANSI + + @doc """ + Renders an icon representing the given file system. + """ + def file_system_icon(assigns) + + def file_system_icon(%{file_system: %FileSystem.Local{}} = assigns) do + ~H""" + <.remix_icon icon="hard-drive-2-line leading-none" /> + """ + end + + def file_system_icon(%{file_system: %FileSystem.S3{}} = assigns) do + ~H""" + + S3 + + """ + end + + @doc """ + Formats the given file system into a descriptive label. + """ + def file_system_label(file_system) + + def file_system_label(%FileSystem.Local{}), do: "Local disk" + def file_system_label(%FileSystem.S3{} = fs), do: fs.bucket_url end diff --git a/lib/livebook_web/live/file_select_component.ex b/lib/livebook_web/live/file_select_component.ex index b03ece23c..8cccb71ac 100644 --- a/lib/livebook_web/live/file_select_component.ex +++ b/lib/livebook_web/live/file_select_component.ex @@ -224,23 +224,6 @@ defmodule LivebookWeb.FileSelectComponent do """ end - defp file_system_icon(%{file_system: %FileSystem.Local{}} = assigns) do - ~H""" - <.remix_icon icon="hard-drive-2-line" /> - """ - end - - defp file_system_icon(%{file_system: %FileSystem.S3{}} = assigns) do - ~H""" - - S3 - - """ - end - - defp file_system_label(%FileSystem.Local{}), do: "Local disk" - defp file_system_label(%FileSystem.S3{} = fs), do: fs.bucket_url - defp file(%{file_info: %{file: file}, renaming_file: file} = assigns) do ~H"""
diff --git a/lib/livebook_web/live/session_live/persistence_live.ex b/lib/livebook_web/live/session_live/persistence_live.ex index 57243ffef..d836d5c14 100644 --- a/lib/livebook_web/live/session_live/persistence_live.ex +++ b/lib/livebook_web/live/session_live/persistence_live.ex @@ -33,7 +33,9 @@ defmodule LivebookWeb.SessionLive.PersistenceLive do session: session, running_files: running_files, attrs: attrs, - new_attrs: attrs + new_attrs: attrs, + draft_file: nil, + saved_file: nil )} end @@ -42,67 +44,106 @@ defmodule LivebookWeb.SessionLive.PersistenceLive do ~H"""

- File + Save to file

-
- <.switch_checkbox - name="persist_outputs" - label="Persist outputs" - checked={@new_attrs.persist_outputs} /> + +
+ <.switch_checkbox + name="persist_outputs" + label="Persist outputs" + checked={@new_attrs.persist_outputs} /> +
+ Autosave + <.select + name="autosave_interval_s" + selected={@new_attrs.autosave_interval_s} + options={[ + {5, "every 5 seconds"}, + {30, "every 30 seconds"}, + {60, "every minute"}, + {600, "every 10 minutes"}, + {nil, "never"} + ]} /> +
+
- Autosave - <.select - name="autosave_interval_s" - selected={@new_attrs.autosave_interval_s} - options={[ - {5, "every 5 seconds"}, - {30, "every 30 seconds"}, - {60, "every minute"}, - {600, "every 10 minutes"}, - {nil, "never"} - ]} /> + File: + <%= if @new_attrs.file do %> + + + [<.file_system_icon file_system={@new_attrs.file.file_system} />] + + + + <%= @new_attrs.file.path %> + + + + <% else %> + + no file selected + + + <% end %>
-
- <.choice_button - active={@new_attrs.file != nil} - phx-click="set_persistence_type" - phx-value-type="file"> - Save to file - - <.choice_button - active={@new_attrs.file == nil} - phx-click="set_persistence_type" - phx-value-type="memory"> - Memory only - -
- <%= if @new_attrs.file do %> -
- <%= live_component LivebookWeb.FileSelectComponent, - id: "persistence_file_select", - file: @new_attrs.file, - extnames: [LiveMarkdown.extension()], - running_files: @running_files, - submit_event: if(disabled?(@new_attrs, @attrs, @running_files), do: nil, else: :save) %> + <%= if @draft_file do %> +
+
+ <%= live_component LivebookWeb.FileSelectComponent, + id: "persistence_file_select", + file: @draft_file, + extnames: [LiveMarkdown.extension()], + running_files: @running_files, + submit_event: :confirm_file do %> +
+ + +
+ <% end %> +
+
+ File: <%= normalize_file(@draft_file).path %> +
<% end %> -
+
<%= if @new_attrs.file do %> -
- File: <%= normalize_file(@new_attrs.file).path %> -
- <% end %> -
-
+ <% else %> + + <% end %>
@@ -110,14 +151,21 @@ defmodule LivebookWeb.SessionLive.PersistenceLive do end @impl true - def handle_event("set_persistence_type", %{"type" => type}, socket) do - file = - case type do - "file" -> socket.assigns.attrs.file || Livebook.Config.default_dir() - "memory" -> nil - end + def handle_event("open_file_select", %{}, socket) do + file = socket.assigns.new_attrs.file || Livebook.Config.default_dir() + {:noreply, assign(socket, draft_file: file)} + end - {:noreply, put_new_file(socket, file)} + def handle_event("close_file_select", %{}, socket) do + {:noreply, assign(socket, draft_file: nil)} + end + + def handle_event("confirm_file", %{}, socket) do + handle_confirm_file(socket) + end + + def handle_event("clear_file", %{}, socket) do + {:noreply, socket |> put_new_file(nil) |> assign(draft_file: nil)} end def handle_event( @@ -134,57 +182,50 @@ defmodule LivebookWeb.SessionLive.PersistenceLive do |> put_new_attr(:autosave_interval_s, autosave_interval_s)} end - def handle_event("save", %{}, socket) do - handle_save(socket) + def handle_event("save", %{}, %{assigns: assigns} = socket) do + %{new_attrs: new_attrs, attrs: attrs} = assigns + new_attrs = Map.update!(new_attrs, :file, &normalize_file/1) + diff = map_diff(new_attrs, attrs) + + if Map.has_key?(diff, :file) do + Session.set_file(assigns.session.pid, diff.file) + end + + notebook_attrs_diff = Map.take(diff, [:autosave_interval_s, :persist_outputs]) + + if notebook_attrs_diff != %{} do + Session.set_notebook_attributes(assigns.session.pid, notebook_attrs_diff) + end + + if new_attrs.file do + Session.save_sync(assigns.session.pid) + end + + running_files = + [new_attrs.file | assigns.running_files] + |> List.delete(attrs.file) + |> Enum.reject(&is_nil/1) + + {:noreply, + assign(socket, + running_files: running_files, + attrs: assigns.new_attrs, + saved_file: new_attrs.file + )} end @impl true def handle_info({:set_file, file, _file_info}, socket) do - {:noreply, put_new_file(socket, file)} + {:noreply, assign(socket, draft_file: file)} end - def handle_info(:save, socket) do - handle_save(socket) + def handle_info(:confirm_file, socket) do + handle_confirm_file(socket) end - defp handle_save(%{assigns: assigns} = socket) do - file = normalize_file(assigns.new_attrs.file) - autosave_interval_s = assigns.new_attrs.autosave_interval_s - - if file != assigns.attrs.file do - Session.set_file(assigns.session.pid, file) - end - - if autosave_interval_s != assigns.attrs.autosave_interval_s do - Session.set_notebook_attributes(assigns.session.pid, %{ - autosave_interval_s: autosave_interval_s - }) - end - - Session.set_notebook_attributes(assigns.session.pid, %{ - persist_outputs: assigns.new_attrs.persist_outputs - }) - - Session.save_sync(assigns.session.pid) - - running_files = - if file do - [file | assigns.running_files] - else - List.delete(assigns.running_files, file) - end - - if file do - # After saving the file reload the directory contents, - # so that the new file gets shown. - send_update(LivebookWeb.FileSelectComponent, - id: "persistence_file_select", - running_files: running_files, - force_reload: true - ) - end - - {:noreply, assign(socket, running_files: running_files, attrs: assigns.new_attrs)} + defp handle_confirm_file(socket) do + file = normalize_file(socket.assigns.draft_file) + {:noreply, socket |> put_new_file(file) |> assign(draft_file: nil)} end defp parse_optional_integer(string) do @@ -242,13 +283,15 @@ defmodule LivebookWeb.SessionLive.PersistenceLive do end) end - defp disabled?(new_attrs, attrs, running_files) do - if normalize_file(new_attrs.file) == attrs.file do - new_attrs.persist_outputs == attrs.persist_outputs and - new_attrs.autosave_interval_s == attrs.autosave_interval_s - else - not file_savable?(normalize_file(new_attrs.file), running_files) - end + defp savable?(new_attrs, attrs, running_files, draft_file) do + new_attrs = Map.update!(new_attrs, :file, &normalize_file/1) + valid_file? = new_attrs.file == attrs.file or file_savable?(new_attrs.file, running_files) + valid_file? and draft_file == nil + end + + defp same_attrs?(new_attrs, attrs) do + new_attrs = Map.update!(new_attrs, :file, &normalize_file/1) + new_attrs == attrs end defp file_savable?(nil, _running_files), do: true @@ -256,4 +299,8 @@ defmodule LivebookWeb.SessionLive.PersistenceLive do defp file_savable?(file, running_files) do not FileSystem.File.dir?(file) and file not in running_files end + + defp map_diff(left, right) do + Map.new(Map.to_list(left) -- Map.to_list(right)) + end end diff --git a/test/livebook_web/live/session_live_test.exs b/test/livebook_web/live/session_live_test.exs index 1195c3008..6fc0edc76 100644 --- a/test/livebook_web/live/session_live_test.exs +++ b/test/livebook_web/live/session_live_test.exs @@ -210,7 +210,7 @@ defmodule LivebookWeb.SessionLiveTest do describe "persistence settings" do @tag :tmp_dir - test "saving to file shows the newly created file", + test "saving to file shows the newly created file in file selector", %{conn: conn, session: session, tmp_dir: tmp_dir} do {:ok, view, _} = live(conn, "/sessions/#{session.id}/settings/file") @@ -219,7 +219,7 @@ defmodule LivebookWeb.SessionLiveTest do path = Path.join(tmp_dir, "notebook.livemd") view - |> element("button", "Save to file") + |> element("button", "Choose a file") |> render_click() view @@ -227,7 +227,15 @@ defmodule LivebookWeb.SessionLiveTest do |> render_change(%{path: path}) view - |> element(~s{button[phx-click="save"]}, "Save") + |> element(~s{button[phx-click="confirm_file"]}, "Choose") + |> render_click() + + view + |> element(~s{button}, "Save now") + |> render_click() + + view + |> element("button", "Change file") |> render_click() assert view @@ -245,19 +253,23 @@ defmodule LivebookWeb.SessionLiveTest do path = Path.join(tmp_dir, "notebook.livemd") view - |> element("button", "Save to file") + |> element("button", "Choose a file") |> render_click() view |> element(~s{form[phx-change="set_path"]}) |> render_change(%{path: path}) + view + |> element(~s{button[phx-click="confirm_file"]}, "Choose") + |> render_click() + view |> element(~s{form[phx-change="set_options"]}) |> render_change(%{persist_outputs: "true"}) view - |> element(~s{button[phx-click="save"]}, "Save") + |> element(~s{button}, "Save now") |> render_click() assert %{notebook: %{persist_outputs: true}} = Session.get_data(session.pid)