Skip to content
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

Improve key binding match/matching check #709

Merged
merged 4 commits into from
Jun 3, 2024

Conversation

tompng
Copy link
Member

@tompng tompng commented May 27, 2024

First step of #708

This will make :matched :matching check simple and fast, especially after we merge MAPPING(size==256 array) and key_bindings(small hash)

# Before O(N)
key_mapping.keys.select { |lhs|
  start_with?(lhs, input)
}
...

# After O(1)
:matched if @key_bindings[key]
:matching if @matching_bytes[key]

Structure

@key_bindings = {
  [27, 91, 65] => :ed_prev_history,
  [27, 91, 50, 48, 48, 126] => :bracketed_paste_start,
}
@matching_bytes = {
  [27] => true,
  [27, 91] => true,
  [27, 91, 50] => true,
  [27, 91, 50, 48] => true,
  [27, 91, 50, 48, 48] => true,
}

Composite key binding

# Before
kb = @key_actors[@editing_mode_label].default_key_bindings.dup
kb.merge!(@additional_key_bindings[@editing_mode_label])
kb.merge!(@oneshot_key_bindings)

# After
Reline::KeyActor::Composite.new([@oneshot_key_bindings, @additional_key_bindings[@editing_mode_label], @key_actors[@editing_mode_label]])

KeyStroke#expand

# Before
expanded = key_stroke.expand(buffer).map{ |expanded_c|
  Reline::Key.new(expanded_c, expanded_c, false)
}

# After
expanded, rest_bytes = key_stroke.expand(buffer)

When key bindings is { 'abc' => :foo, 'ab' => :bar, 'xy' => :baz }, abxy should be [:bar, :baz].
Old implementation will expand abx to [Key(:bar), Key('x')] but it should be expanded: [Key(:bar)], rest_bytes: ['x']. The rest bytes x could be part of xy.

@@ -19,20 +19,10 @@ module Reline
class ConfigEncodingConversionError < StandardError; end

Key = Struct.new(:char, :combined_char, :with_meta) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we also convert this into a normal class?

lib/reline.rb Show resolved Hide resolved
lib/reline.rb Show resolved Hide resolved
lib/reline.rb Show resolved Hide resolved
lib/reline/config.rb Outdated Show resolved Hide resolved
end

def add_oneshot_key_binding(keystroke, target)
@oneshot_key_bindings[keystroke] = target
# IRB sets invalid keystroke [Reline::Key]. We should ignore it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we removed that from IRB?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet.
Even if we remove it, we need to support [Reline::Key.new(nil, 0xE4, true)] for a while, not to break an environment that new Reline is installed but IRB is not updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I'm aware of that and I'm not arguing we need to change the approach here 🙂 I just hadn't see any related PR on IRB so want to confirm.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened ruby/irb#963

lib/reline/config.rb Outdated Show resolved Hide resolved
lib/reline/key_actor/composite.rb Show resolved Hide resolved
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't test the changes manually, but they look good to me 👍

@tompng tompng merged commit 353ec23 into ruby:master Jun 3, 2024
40 checks passed
matzbot pushed a commit to ruby/ruby that referenced this pull request Jun 3, 2024
(ruby/reline#709)

* Improve key binding match/matching check

* Rename key_actors to default_key_bindings

* Make key_stroke.expand always return a value

* Update add_default_key_binding to use a add_default_key_binding_by_keymap internally

Co-authored-by: Stan Lo <[email protected]>

---------

ruby/reline@353ec236e2

Co-authored-by: Stan Lo <[email protected]>
@tompng tompng deleted the key_binding_algorithm branch June 3, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants