repl: Save notebooks through the project instead of the local filesystem - #62602
repl: Save notebooks through the project instead of the local filesystem#62602ArneshBanerjee wants to merge 2 commits into
Conversation
cla-bot
Bot
added
the
cla-signed
dinocosta
added
area:repl
MrSubidubi
left a comment
MrSubidubi
left a comment
There was a problem hiding this comment.
Thanks for this!
Looks good overall, will give it a second pass after the comment below
| fn save( | ||
| &mut self, | ||
| _options: SaveOptions, | ||
| project: Entity<Project>, | ||
| _window: &mut Window, | ||
| cx: &mut Context<Self>, | ||
| ) -> Task<Result<()>> { | ||
| let notebook = self.to_notebook(cx); | ||
| let path = self.notebook_item.read(cx).path.clone(); | ||
| let fs = project.read(cx).fs().clone(); | ||
| let project_path = self.notebook_item.read(cx).project_path.clone(); | ||
|
|
||
| self.mark_as_saved(cx); | ||
|
|
||
| cx.spawn(async move |_this, _cx| { | ||
| cx.spawn(async move |_this, cx| { | ||
| let json = | ||
| serde_json::to_string_pretty(¬ebook).context("Failed to serialize notebook")?; | ||
| fs.atomic_write(path, json).await?; | ||
| Ok(()) | ||
| let buffer = project | ||
| .update(cx, |project, cx| project.open_buffer(project_path, cx)) | ||
| .await?; | ||
| buffer.update(cx, |buffer, cx| buffer.set_text(json, cx)); | ||
| project | ||
| .update(cx, |project, cx| project.save_buffer(buffer, cx)) | ||
| .await | ||
| }) | ||
| } |
There was a problem hiding this comment.
The methods save and save_as are very identical at this point, and contain a lot of duplicate code.
Can we perhaps throw this into a method save_impl that takes everything needed plus an enum that lets us differentiate between Save and SaveAs, then do all the different stuff based on that enum?
There was a problem hiding this comment.
Sure, will take a look at it.
There was a problem hiding this comment.
Done, both now go through a shared save_impl that takes a SaveDestination enum (CurrentPath / NewPath). The only branch left is save_buffer vs save_buffer_as plus the path update that has to follow the move.
|
Pushed the refactor. Tests and clippy are green locally. Ready for another look. |
3 participants
Closes #59001
Saving a notebook wrote the file with
project.fs().atomic_write(abs_path, ...).Project::fs()is always the client's own filesystem, so for a remote project the remote path was resolved against the local machine: on Windows the drive letter got prepended and the atomic-write temp file used a backslash, producingC:/data/project/src_example\.tmpPuzw3d3andos error 3. The same thing fails on macOS/Linux clients withos error 2.Both
saveandsave_asnow go through the project's buffer machinery instead: open the buffer for the notebook'sProjectPath, set its text to the serialized notebook, thensave_buffer/save_buffer_as. Writes are routed over the remote connection for remote projects, and the open buffer stays in sync locally.save_asalso updates the item's project path and entry id, sincesave_buffer_asmoves the buffer to the new path and otherwise the next save would write back to the old file.Added a test that edits a cell, saves, and checks that the notebook buffer held by the project reflects the saved file. It fails against the old implementation.
Release Notes: