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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 9 additions & 22 deletions lib/reline.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?

def match?(other)
case other
when Reline::Key
(other.char.nil? or char.nil? or char == other.char) and
(other.combined_char.nil? or combined_char.nil? or combined_char == other.combined_char) and
(other.with_meta.nil? or with_meta.nil? or with_meta == other.with_meta)
when Integer, Symbol
(combined_char and combined_char == other) or
(combined_char.nil? and char and char == other)
else
false
end
# For dialog_proc `key.match?(dialog.name)`
def match?(sym)
combined_char.is_a?(Symbol) && combined_char == sym
end
alias_method :==, :match?
end
CursorPos = Struct.new(:x, :y)
DialogRenderInfo = Struct.new(
Expand Down Expand Up @@ -400,9 +390,8 @@ def readline(prompt = '', add_hist = false)
end
case result
when :matched
expanded = key_stroke.expand(buffer).map{ |expanded_c|
Reline::Key.new(expanded_c, expanded_c, false)
}
expanded, rest_bytes = key_stroke.expand(buffer)
rest_bytes.reverse_each { |c| io_gate.ungetc(c) }
st0012 marked this conversation as resolved.
Show resolved Hide resolved
block.(expanded)
st0012 marked this conversation as resolved.
Show resolved Hide resolved
break
when :matching
Expand All @@ -416,9 +405,8 @@ def readline(prompt = '', add_hist = false)
if buffer.size == 1 and c == "\e".ord
read_escaped_key(keyseq_timeout, c, block)
else
expanded = buffer.map{ |expanded_c|
Reline::Key.new(expanded_c, expanded_c, false)
}
expanded, rest_bytes = key_stroke.expand(buffer)
rest_bytes.reverse_each { |c| io_gate.ungetc(c) }
block.(expanded)
end
break
Expand All @@ -442,9 +430,8 @@ def readline(prompt = '', add_hist = false)
return :next
when :matched
buffer << succ_c
expanded = key_stroke.expand(buffer).map{ |expanded_c|
Reline::Key.new(expanded_c, expanded_c, false)
}
expanded, rest_bytes = key_stroke.expand(buffer)
rest_bytes.reverse_each { |c| io_gate.ungetc(c) }
block.(expanded)
st0012 marked this conversation as resolved.
Show resolved Hide resolved
return :break
end
Expand Down
40 changes: 21 additions & 19 deletions lib/reline/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,20 @@ class InvalidInputrc < RuntimeError
attr_accessor :autocompletion

def initialize
@additional_key_bindings = {} # from inputrc
@additional_key_bindings[:emacs] = {}
@additional_key_bindings[:vi_insert] = {}
@additional_key_bindings[:vi_command] = {}
@oneshot_key_bindings = {}
@additional_key_bindings = { # from inputrc
emacs: Reline::KeyActor::Base.new,
vi_insert: Reline::KeyActor::Base.new,
vi_command: Reline::KeyActor::Base.new
}
@oneshot_key_bindings = Reline::KeyActor::Base.new
@editing_mode_label = :emacs
@keymap_label = :emacs
@keymap_prefix = []
@key_actors = {}
@key_actors[:emacs] = Reline::KeyActor::Emacs.new
@key_actors[:vi_insert] = Reline::KeyActor::ViInsert.new
@key_actors[:vi_command] = Reline::KeyActor::ViCommand.new
@default_key_bindings = {
emacs: Reline::KeyActor::Base.new(Reline::KeyActor::EMACS_MAPPING),
vi_insert: Reline::KeyActor::Base.new(Reline::KeyActor::VI_INSERT_MAPPING),
vi_command: Reline::KeyActor::Base.new(Reline::KeyActor::VI_COMMAND_MAPPING)
}
@vi_cmd_mode_string = '(cmd)'
@vi_ins_mode_string = '(ins)'
@emacs_mode_string = '@'
Expand All @@ -62,7 +64,7 @@ def reset
end

def editing_mode
@key_actors[@editing_mode_label]
@default_key_bindings[@editing_mode_label]
end

def editing_mode=(val)
Expand All @@ -74,7 +76,7 @@ def editing_mode_is?(*val)
end

def keymap
@key_actors[@keymap_label]
@default_key_bindings[@keymap_label]
end

def loaded?
Expand Down Expand Up @@ -133,26 +135,26 @@ def read(file = nil)

def key_bindings
# The key bindings for each editing mode will be overwritten by the user-defined ones.
kb = @key_actors[@editing_mode_label].default_key_bindings.dup
kb.merge!(@additional_key_bindings[@editing_mode_label])
kb.merge!(@oneshot_key_bindings)
kb
Reline::KeyActor::Composite.new([@oneshot_key_bindings, @additional_key_bindings[@editing_mode_label], @default_key_bindings[@editing_mode_label]])
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

return unless keystroke.all? { |c| c.is_a?(Integer) }

@oneshot_key_bindings.add(keystroke, target)
end

def reset_oneshot_key_bindings
@oneshot_key_bindings.clear
end

def add_default_key_binding_by_keymap(keymap, keystroke, target)
@key_actors[keymap].default_key_bindings[keystroke] = target
@default_key_bindings[keymap].add(keystroke, target)
end

def add_default_key_binding(keystroke, target)
@key_actors[@keymap_label].default_key_bindings[keystroke] = target
add_default_key_binding_by_keymap(@keymap_label, keystroke, target)
end

def read_lines(lines, file = nil)
Expand Down Expand Up @@ -192,7 +194,7 @@ def read_lines(lines, file = nil)
func_name = func_name.split.first
keystroke, func = bind_key(key, func_name)
next unless keystroke
@additional_key_bindings[@keymap_label][@keymap_prefix + keystroke] = func
@additional_key_bindings[@keymap_label].add(@keymap_prefix + keystroke, func)
end
end
unless if_stack.empty?
Expand Down
1 change: 1 addition & 0 deletions lib/reline/key_actor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ module Reline::KeyActor
end

require 'reline/key_actor/base'
require 'reline/key_actor/composite'
require 'reline/key_actor/emacs'
require 'reline/key_actor/vi_command'
require 'reline/key_actor/vi_insert'
28 changes: 22 additions & 6 deletions lib/reline/key_actor/base.rb
Original file line number Diff line number Diff line change
@@ -1,15 +1,31 @@
class Reline::KeyActor::Base
MAPPING = Array.new(256)
def initialize(mapping = [])
@mapping = mapping
@matching_bytes = {}
@key_bindings = {}
end

def get_method(key)
self.class::MAPPING[key]
@mapping[key]
end

def add(key, func)
(1...key.size).each do |size|
@matching_bytes[key.take(size)] = true
end
@key_bindings[key] = func
end

def matching?(key)
@matching_bytes[key]
end

def initialize
@default_key_bindings = {}
def get(key)
@key_bindings[key]
end

def default_key_bindings
@default_key_bindings
def clear
@matching_bytes.clear
@key_bindings.clear
end
end
17 changes: 17 additions & 0 deletions lib/reline/key_actor/composite.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
class Reline::KeyActor::Composite
def initialize(key_actors)
@key_actors = key_actors
end

def matching?(key)
@key_actors.any? { |key_actor| key_actor.matching?(key) }
end

def get(key)
@key_actors.each do |key_actor|
func = key_actor.get(key)
return func if func
end
nil
st0012 marked this conversation as resolved.
Show resolved Hide resolved
end
end
4 changes: 2 additions & 2 deletions lib/reline/key_actor/emacs.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
class Reline::KeyActor::Emacs < Reline::KeyActor::Base
MAPPING = [
module Reline::KeyActor
EMACS_MAPPING = [
# 0 ^@
:em_set_mark,
# 1 ^A
Expand Down
4 changes: 2 additions & 2 deletions lib/reline/key_actor/vi_command.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
class Reline::KeyActor::ViCommand < Reline::KeyActor::Base
MAPPING = [
module Reline::KeyActor
VI_COMMAND_MAPPING = [
# 0 ^@
:ed_unassigned,
# 1 ^A
Expand Down
4 changes: 2 additions & 2 deletions lib/reline/key_actor/vi_insert.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
class Reline::KeyActor::ViInsert < Reline::KeyActor::Base
MAPPING = [
module Reline::KeyActor
VI_INSERT_MAPPING = [
# 0 ^@
:ed_unassigned,
# 1 ^A
Expand Down
Loading
Loading