Papierkorb / fancyline

Readline-esque library with fancy features
Mozilla Public License 2.0
79 stars 8 forks source link

Key aliasing, or perhaps just a few more sensible defaults. #8

Open collidedscope opened 3 years ago

collidedscope commented 3 years ago

Hello, Fancyliners! Thanks for bringing such a great library into the world.

I recently incorporated history into my toy language's REPL (made an absolute breeze by Fancyline, of course), but was dismayed to discover that Ctrl+P and Ctrl+N couldn't be used to seek up and down, respectively. These particular keybinds are a staple in many line editors, and I'm long past the point of shaking them out of muscle memory. I briefly considered hard-coding them in the history widget's handler, but of course that wouldn't do.

Thankfully, and as a testament to the library's excellent design, I found it fairly straightforward to enable the old habits:

include Fancyline::Key

fancy = Fancyline.new
history_widget = Fancyline::Widget::History.new

fancy.actions.set Control::CtrlP do |ctx|
  if history_widget.@history
    history_widget.show_entry ctx, -1
  else
    ctx.start_widget history_widget
  end
end

fancy.actions.set Control::CtrlN do |ctx|
  if history_widget.@history
    history_widget.show_entry ctx, +1
  else
    # Do nothing.
  end
end

This works and I'm content with it, but I was thinking it'd be pretty nice if instead one could simply say something like the following:

fancy.actions.alias Control::CtrlP, Control::Up
fancy.actions.alias Control::CtrlN, Control::Down

A mere @mapping[to] = @mapping[from] goes a surprisingly long way, but this aliasing wouldn't get picked up in widgets, where the action keys are hard-coded at the moment. I appreciate the disinclination to make Fancyline too configurable, but the structure is all there to make this creature comfort possible, insofar as ctx.fancyline.actions.mapping should suffice for getting hold of any aliases from within a Widget.

Would you be interested in having this functionality in the library? As long as it's limited to just global aliases (as opposed to something crazy like per-context), it seems to me that it wouldn't come with too much mess.

Thanks again for this excellent utility! Looking forward to your thoughts.

lagerfeuer commented 3 years ago

Hey @collidedscope, not the creator, but I'm keeping an eye on the library.

Now that you brought it up, an alias feature actually seems really useful. As an arrow key guy myself, I hadn't noticed until now, but you make a good case 😄

Given how widely used Ctrl+P and Ctrl+N are, it might make sense to even add them to the history widget by default in addition to the current default.

Nonetheless, the alias feature should be included.

If you have the time and will to put this into a PR, or maybe you already have a vision and/or fork ready, I'd be happy to merge it. Otherwise, I'm putting it on my radar.

Papierkorb commented 3 years ago

Hello!

I think having aliases on a global-scope would be fine to have! Though I'd be more happy for the aliasing-feature to have a more talking API, e.g. one of:

fancy.actions.alias Control::CtrlP, to: Control::Up
fancy.actions.set Control::CtrlP, alias_for: Control::Up

I have used Ruby and Crystal for many years and still can't tell you if the thing that I'm aliasing a new name to goes as first or second argument. Crystals fantastic feature of forcing a keyword argument solves this. :)

Also, I'd be fine with having them aliased by-default if they're widely used.

collidedscope commented 3 years ago

Really glad you guys are on board with this enhancement!

I've posted the obvious-to-me approach outside of a PR on the good chance that I'm going about it totally incorrectly, but please let me know where I've gone wrong should that prove to be the case.

I do think Ctrl+P and Ctrl+N should be enabled out of the box, but wasn't sure whether it'd be best to do that via the new aliasing or just stick them in DEFAULT, so I've left that alone for now.

The "conversational" invocation makes a great deal of sense here, precisely because aliasing can be so tricky to talk about with complete clarity. I went ahead and made it possible to do both suggestions, but would push harder for set x, alias_for: y if only one should go in; the from/to dichotomy is largely a matter of perspective, so I've gone with new and existing, which should be much harder to conflate.

lagerfeuer commented 3 years ago

@collidedscope So sorry for the late response, completely lost track of this issue. To answer your question, I quite like your approach and I think we can go down that route. I also like that it allows for multiple aliases. If you're still interested, I'd welcome a PR and would love to have this in the library.