-
-
Notifications
You must be signed in to change notification settings - Fork 292
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
Support running inside VS Code webview #1493
base: main
Are you sure you want to change the base?
Conversation
a717e7d
to
58fc26e
Compare
@pankgeorg can you make that last commit on main, not on this PR? |
frontend/components/Editor.js
Outdated
@@ -903,6 +907,9 @@ patch: ${JSON.stringify( | |||
} | |||
e.preventDefault() | |||
} else if (e.key.toLowerCase() === "s" && has_ctrl_or_cmd_pressed(e)) { | |||
// If VSCode is around, we shouldn't 'Set and run all remote changed remote cells. | |||
// Control + Save sends VSCode save | |||
if (vscode_available) return this.run_selected() |
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.
I don't understand this change, could you write a more elaborate comment @pankgeorg ?
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.
Maybe give a 1.2.3. timeline of what happens when you press Ctrl+S?
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.
I think the Ctrl+S
is pretty much the central TODO that's keeping us from releasing the extension publicly, once we figure this out it's all beautiful 🌟
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.
I reverted the change and I'm going all in to server-aware un-run changes. wip wip wip wip
src/notebook/Notebook.jl
Outdated
@@ -54,10 +54,26 @@ Base.@kwdef mutable struct Notebook | |||
wants_to_interrupt::Bool=false | |||
last_save_time::typeof(time())=time() | |||
last_hot_reload_time::typeof(time())=zero(time()) | |||
last_serialized_version::String = "" | |||
write_out_fs::Bool=true |
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 already have
Line 58 in c1ba792
disable_writing_notebook_files::Bool = false |
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.
The existing flag is session-wide and it prevents the save function from running at all. The new flag is per-notebook and it only prevents the final write-out to the file. Thus, we can run save event listeners and have the latest text representation of the file
src/notebook/Notebook.jl
Outdated
@@ -54,10 +54,26 @@ Base.@kwdef mutable struct Notebook | |||
wants_to_interrupt::Bool=false | |||
last_save_time::typeof(time())=time() | |||
last_hot_reload_time::typeof(time())=zero(time()) | |||
last_serialized_version::String = "" |
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.
Let's track this in pluto-vscode instead
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.
The benefits of having this here too are
- we prevent multiple file IO
- we prevent emitting events for file change (through listeners) if the file wasn't changed (that's a weaker argument - it can be part of the listener)
- we can compare this version to the
last_unsaved_serialized_version
(incoming) that is what we wantShift+Enter
to propagate to the VS Code workspace, beforeControl + S
actually does the filesave(!) (can also be tracked in the listener)
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.
all in all, I like it here for the above reasons - but we don't really need it
What about having a hash of the (string contents of the) file here?
Use vscode extension API instead of websocket when detected.
See the companion extension: https://github.com/JuliaComputing/pluto-vscode