-
Notifications
You must be signed in to change notification settings - Fork 423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement file management #2022
Conversation
Uffizzi Preview |
hash = :crypto.hash(:sha256, bucket_url) |> Base.url_encode64(padding: false) | ||
id = "s3-#{hash}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently we are using random ids generated when the file system is added, which is fine for settings (local). However, to persist a file link in a notebook, we need a deterministic file system id, so if multiple Livebook users use the same S3 bucket, the file link is exported/imported as expected.
I did the hash so that the id is fixed length and this way it also has a predictable character set. Also, even though buckets generally need authorisation, not listing the URL explicitly sounds like an advantage to me.
I considered a second field/function like :public_id
, but having both random :id
and the :public_id
is confusing, especially that both should be unique anyway. Also, having a deterministic :id
is what we do for hubs too, so it's better for consistency. To avoid breaking existing settings I added a migration with more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I also wonder if the ID should be the bucket itself but this at least avoids us from leaking bucket information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I would still like base64 encode the bucket URL to avoid weird characters in the ID, but not leaking sounds preferable anyway.
options: [ | ||
%{ | ||
name: "delete_cells", | ||
label: "Delete all cells in this section", | ||
default: first_section?, | ||
disabled: first_section? | ||
} | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When deleting a section we ask the user "Delete all cells in this section" and to do that we used a whole separate URL and modal. Now that we needed a similar option when deleting a file, I added this to the generic confirm we have, and now that it's all in Elixir it was actually very little code :D
I have some unasked-for UI/description suggestions from the perspective of a user (please feel free to disregard!)
|
I would stick with radio. The issue with checkboxes is that it doesn’t tell you what happens when it is unchecked. Radios make that clear. agreed on “files/“, we should tag it as code (my preference) or remove the slash. :) |
True! This may just be me, but I'd still simplify the labels pretty dramatically to something like "Link to file / Copy file" and "Link to URL / Download file" because I think that folks already have good intuition about what those things mean and too much description makes people question that intuition instead of clarifying it. |
end | ||
|
||
@impl true | ||
def handle_event("yo", %{}, socket) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yo!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just very minor nitpicks. Something else we should add to the TODO-list is the option to open/download a file directly from the sidebar? For URLs it is easy but for files it can be a bit tricky.
I have also added this issue, which is related to future work: #2027 Just so we don't forget. :) |
Co-authored-by: José Valim <jose.valim@dashbit.co>
Sounds good, in the future we may also want to have an zip export to download notebook with all the files. |
FTR automatic conversion script that will be useful for people with large notebook collections: convert_notebook_images.exs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are to branch and merge :)
First steps towards #1604. There is quite a lot of changes already, so I'm opening the PR early (or perhaps we just break it down into multiple PRs).
Overview
This adds a new sidebar with notebook files, which can either be stored (and managed) alongside the notebook in
files/
directory as attachments or point to some global file or URL as links. Information about those is stored in the .livemd file. When importing a notebook from URL we know that we need to download all of its attachments.We decided to unify this with images, so now inserting an image automatically adds an attachment and the urls are
![](files/image.jpeg)
(rather thanimages/
). We keep resolvingimages/
for backward compatibility for now.UI
Expand
Sidebar
Adding from file
Adding from URL
Adding other unlisted files from files/
Deleting a file
Copying link to notebook files
Next steps
Primary
image/*
image urls on .livemd import and add deprecation warningAdditions