Conversation
|
Sample custom keybinding configuration: |
|
@zzeekk Thanks for taking a pass at this – I like the idea of configurable key bindings. I'm not sure whether they should be configured for the whole server as you've done here. Maybe it should be per-user (or per-browser) instead? That would also make it all client-side. But, it would be annoying if you switch browsers and your hotkeys are all changed. Not to yak shave this, but maybe we need to have a system for persisting per-user settings on the server first. WDYT @jonathanindig ? |
|
I also thought about per-user hotkeys. Nevertheless you probably need some defaults. I suggest to split this in multiple parts:
|
|
I like the ability to customize hotkeys (the current hotkeys differ quite a lot w.r.t. jupyter), thanks @zzeekk for the PR. Personally, I find it sufficient to have globally configurable hotkeys, I don't think we need per-user settings |
There was a problem hiding this comment.
Thanks so much for this PR @zzeekk Really impressed with how you were able to dive into our messy codebase! 😅
Also, apologies for taking such a long time to review!
Overall, a mix of major and minor feedback. The main thing I'm worried about is the use of HotkeyInfo as the new configuration. A user can't add any new functionality with their config - all they can do is add a new mapping from keybinding -> hotkey.
So, description doesn't really seem useful here, and hide/vimOnly are internal implementation details to support vim mode, so they are definitely not relevant here.
Instead, I think there should be a set of allowed hotkeys (MoveUp, MoveDown, RunActive....), and the config just needs to be a mapping from key to binding. (Having a set of allowed keys somewhere would also help users determine what is available!)
I'm thinking about something like this:
ui:
custom_keybindings:
RunActive:
- Ctrl-Enter
- Cmd-Enter
InsertAbove:
... # and so on
(Note - I changed the order from Hotkey -> binding, and added the option of setting multiple bindings. I did this to emulate other hotkey bindings I'm familiar with, like IntelliJ, where you select the thing you want to do first and then choose how you want to do it and can set up multiple hotkeys if you like).
Thoughts @zzeekk @jeremyrsmith ?
| import {ClientBackup} from "../state/client_backup"; | ||
| import {ErrorStateHandler} from "../state/error_state"; | ||
| import {ServerState, ServerStateHandler} from "../state/server_state"; | ||
| import { logger } from "vega"; |
There was a problem hiding this comment.
Is this import used?
| /** | ||
| * Helper for moving current cell. | ||
| * | ||
| * @param direction Whether to insert below of above the anchor |
There was a problem hiding this comment.
| * @param direction Whether to insert below of above the anchor | |
| * @param direction Whether to insert below or above the anchor |
| final case class UI( | ||
| baseUri: String = "/" | ||
| baseUri: String = "/", | ||
| customKeybindings: Map[String, HotkeyInfo] = Map() |
There was a problem hiding this comment.
Could you please update https://github.com/polynote/polynote/blob/master/config-template.yml#L151 with an example of using these new options?
| identity = identity.map(i => Identity(i.name, i.avatar.map(ShortString))), | ||
| sparkTemplates = config.spark.flatMap(_.propertySets).getOrElse(Nil) | ||
| sparkTemplates = config.spark.flatMap(_.propertySets).getOrElse(Nil), | ||
| customKeybindings = config.ui.customKeybindings.asInstanceOf[TinyMap[TinyString, HotkeyInfo]] |
There was a problem hiding this comment.
@jeremyrsmith anything we can do to get rid of the asInstanceOf here? I see we also need to do that for the interpreters above on line 85.
| if (keysStr.includes("meta")) keyMods += monaco.KeyMod.WinCtrl; | ||
| const actualKeys = keysStr | ||
| .map( key => KeyCodeUtils.fromString(key)) | ||
| .filter( keyCode => !createSimpleKeybinding(keyCode, OS).isModifierKey()); |
There was a problem hiding this comment.
[nit] Could you please indent the .map and .filter
| .when("MoveDownJ", () => { | ||
| this.notebookState.selectCell(this.id, {relative: "below", editing: true}) | ||
| }) | ||
| .when("RunActive", () => { |
| .when("RunToCursor", () => { | ||
| this.dispatcher.runToActiveCell() | ||
| }) | ||
| .when("MoveAbove", () => { |
There was a problem hiding this comment.
I like MoveAbove and MoveBelow, great additions.
| this.notebookState.moveCell("below"); | ||
| return ["stopPropagation", "preventDefault"] | ||
| }) | ||
| .when("Merge", () => { |
There was a problem hiding this comment.
This is a tricky one. What should happen if someone tries to merge a Scala and Python cell together? Is there any way to undo this change easily?
Thoughts @jeremyrsmith ?
There was a problem hiding this comment.
We don't have a global undo buffer, currently. We just rely on Monaco for text (and a one-off thing for undoing cell deletion).
So we'd have to add a global undo mechanism first (and I suppose the "Click to undo cell deletion" thing would go away and get rolled into that?)
There was a problem hiding this comment.
But generally I don't think it should let you merge cells of different languages. Should just do nothing if you attempt to.
| } | ||
| return ["stopPropagation", "preventDefault"] | ||
| }) | ||
| .when("Split", () => { |
There was a problem hiding this comment.
This is definitely safer than Merging, but I'm a little on the fence about it. How easy is this to undo if hit accidentally?
Thoughts @jeremyrsmith ?
| const fullRange = this.getRange(); | ||
| const endRange = fullRange.setStartPosition(fullRange.endLineNumber, fullRange.endColumn); | ||
| // insert text in editor | ||
| this.editor.executeEdits("appendText-insert", [{range: endRange, text: '\r\n'+text}]); |
There was a problem hiding this comment.
Is \r needed here? We only use \n in other parts of the UI.
Hi @jeremyrsmith,
I tried to implement custom keybindings in config.yml as follow up for #1252.
I must say that i really like Polynotes code and found it easy to add functionality. Amazing how you built your own server protocol...
The draft implementation of this PR works, but i would like to have you opinion on some design decisions before finalizing it:
I found current cellHotkeys definition in cell.ts. It seems to me that they have the scope "cell", e.g. only being active when focus is on a notebook cell. Do you have plans for other scopes, e.g. hotkeys in the list of notebooks?
Should we then add a scope to the configuration entries in the configuration?
I dont like this static definitions like cellHotkeys too much in code - would you mind moving all these entries into the configuration file as default entries? It would allow to fully customize the hotkey experience...
More hotkey actions - in my project we used Spark-Notebook before. It had hotkeys for moving cells up/down and split/merge cells. There are other issues in Polynotes backlog about additional hotkeys (hotkey to toggle cell type from code to text #437,Add hotkey to switch to another tab #263). Would it be supported by you to extend Polynote in this way?
The current implementation looks a little bit confusing regarding the meaning of keybindings - is it a string, number or Object implementing Keybinding interface, or even Monacos SimpleKeybinding? Also the name hotkey / keybinding is used similar.
Would you agree that this might be a point to cleanup and maybe even have a target solution and naming in mind?
Some technical questions i was struggling with we might discuss directly in the code if ok for you.
I hope this suggestion finds you well - otherwise let me know if your process or format requirements are different.