interrupts for inline evaluation #1690
Conversation
package.json
Outdated
| }, | ||
| { | ||
| "command": "language-julia.interrupt", | ||
| "key": "Ctrl+Shift+C", |
There was a problem hiding this comment.
This technically overwrites a default kebinding (Open New External Terminal), but that doesn't seem like a big deal to me.
There was a problem hiding this comment.
Hm, don't we have the other keyboard shortcut already? I think overwriting default keybindings is probably something we should avoid...
There was a problem hiding this comment.
Yes, but it's very convenient not to have to focus the REPL to interrupt code, imho. Either is fine though, so I can remove this.
There was a problem hiding this comment.
I kind of feel we need a generic solution for this... Here is one idea: we could have a setting that says "enable conflicting julia keybindings", and that toggles one of these context values, and then we could use that in when clauses for all the keybindings that we want to introduce but that conflict with existing keybindings. The default for the setting would be false, but it would make it very easy for a user to toggle that setting, and then they would automatically get a large number of "better" keybindings, at the cost of them overwriting existing ones.
There was a problem hiding this comment.
Yes, I like that option. I wanted to rework our keybindings anyways, so I can implement that option while I'm at it. Ok to do that in a follow up?
| msg = JSONRPC.get_next_message(conn_endpoint[]) | ||
|
|
||
| if is_dev | ||
| @async if is_dev |
There was a problem hiding this comment.
This shouldn't cause any problems, but is required for us to process multiple requests/notifications at the same time.
There was a problem hiding this comment.
I have to admit this makes me quite nervous because if affects absolutely everything, i.e. now the whole REPL code here needs to be reentrant safe, which is certainly not something we thought about when we wrote it initially...
But I also don't have a fantastic other idea ;) We could go back to the original model, where the code execution message is a notification that returns right away once the code running has been started, and then the server sends a notification back when the code has finished running, that way we wouldn't have these long-running code execution requests. I think that would limit the whole reentrant story to code execution, right?
|
Alright, every way to evaluate code now goes through the same mechanism. As a consequence, the |
davidanthoff
left a comment
davidanthoff
left a comment
There was a problem hiding this comment.
Cool!
I can't pretend to fully understand all of this, but I did have one comment on making the whole msg loop async.
package.json
Outdated
| }, | ||
| { | ||
| "command": "language-julia.interrupt", | ||
| "key": "Ctrl+Shift+C", |
There was a problem hiding this comment.
Hm, don't we have the other keyboard shortcut already? I think overwriting default keybindings is probably something we should avoid...
| msg = JSONRPC.get_next_message(conn_endpoint[]) | ||
|
|
||
| if is_dev | ||
| @async if is_dev |
There was a problem hiding this comment.
I have to admit this makes me quite nervous because if affects absolutely everything, i.e. now the whole REPL code here needs to be reentrant safe, which is certainly not something we thought about when we wrote it initially...
But I also don't have a fantastic other idea ;) We could go back to the original model, where the code execution message is a notification that returns right away once the code running has been started, and then the server sends a notification back when the code has finished running, that way we wouldn't have these long-running code execution requests. I think that would limit the whole reentrant story to code execution, right?
|
In reference to the maybe it's preferable to have a key combination like in Juno. For example : Ctrl + J, Ctrl + C |
|
Alright, removed the controversial keybinding. We're also only |
|
|
No description provided.