search

monome / maiden

web based editor and repl for norns
GNU General Public License v3.0
47 stars 33 forks source link

Fix CTRL+P in editor #166

Closed ryanlaws closed 4 years ago

ryanlaws commented 4 years ago

This fixes CTRL+P in the editor for "playing" the current script (fix #71). I tested it for the default Ace keybindings and for the Vim ones. The existing keybindings applied to the window, but were overridden by the Ace editor and it appears that their default/lower-precedence behaviors were prevented.

The significant change is twofold. First, the relevant activeBuffer and scriptRun() props are passed from EditActivity to its Editor. The former had to be aliased to activityActiveBuffer due to conflicts surfaced in testing. This is then wrapped in a getActiveBuffer() helper in render() to ensure freshness. Next, a commands prop is passed to said Editor, which is an array of commands associated with keys. The sole member of commands is a mapping of CTRL+P to a call to scriptRun with the active buffer.

Next, for Vim, the <C-p> mapping found in the defaults is just removed. The Vim keybindings take priority in Ace, and removing that binding from Vim forces it to fall back to the main binding list, which includes the aforementioned CTRL+P mapping.

ngwese commented 4 years ago

Awesome - will get this tested and merged (hopefully tomorrow)

ryanlaws commented 4 years ago

Thank you! I updated my comment above for readability by the way - it was sourced from the commit body and looked pretty bad in markdown.

pq commented 4 years ago

@ryanlaws: this is awesome! a few questions...

is there any reason to do this only for CTRL+P? are the other commands ok because there aren't problematic collisions?

the idea of the commandService was to consolidate service handling; is there any way this could be integrated more tightly there? (or was that just an odd way to go about encapsulating handlers?)

cheers!

ryanlaws commented 4 years ago

Huh, I must have been making that cleanup commit at the same time you were commenting.

are the other commands ok because there aren't problematic collisions?

Exactly. I've actually only tested CTRL+S and CTRL+P.

the idea of the commandService was to consolidate service handling

Ahh, I like this. The single command hanging out in the JSX felt dirty. It would be a lot cleaner and better for maintenance to create an adapter from the keyService.bindings that plugs into the commands prop in editor.js.

ryanlaws commented 4 years ago

Alrighty, I think that's an improvement!

I tested CTRL+B, CTRL+E, CTRL+P, and CTRL+S; those are all working for me. CTRL+; doesn't work and throws an exception but I think that already existed.

pq commented 4 years ago

💎 💎 💎

Looks awesome!

ryanlaws commented 4 years ago

If someone would like to test this with OSX that'd probably be good at least as a smoke test. If no one has an OSX machine at hand, I can set it up and test. More eyeballs are definitely better, though.

ngwese commented 4 years ago

I hope to have time this evening to test and merge. It’s been a busier week than I anticipated.

ryanlaws commented 4 years ago

No worries - thank you!

ryanlaws commented 4 years ago

Thank you!