bug#32501: Bouncing parentheses broken in REPL with vi-mode enabled

bug#32501: Bouncing parentheses broken in REPL with vi-mode enabled

[Daniel Tam]

Hi,


I've activated readline support for the Guile repl, but I've found that
if my inputrc enables vi-mode, then the bouncing parentheses feature
doesn't work. Disabling vi-mode does the trick.


Cheers,

Dan

bug#32501: Bouncing parentheses broken in REPL with vi-mode enabled

[Mark H Weaver]

Hi Daniel,

Daniel Tam <danooo.tam@gmail.com> writes:

> I've activated readline support for the Guile repl, but I've found that
> if my inputrc enables vi-mode, then the bouncing parentheses feature
> doesn't work. Disabling vi-mode does the trick.

Indeed.  For some reason that I cannot determine, the bouncing
parentheses feature is specifically disabled when the vi keymap is in
use.  The relevant function is 'init_bouncing_parens' in
guile-readline/readline.c:

  static void
  init_bouncing_parens ()
  {
    if (strncmp (rl_get_keymap_name (rl_get_keymap ()), "vi", 2))
      {
        rl_bind_key (')', match_paren);
        rl_bind_key (']', match_paren);
        rl_bind_key ('}', match_paren);
      }
  }

This is ancient code, predating version control, present in the original
import into CVS in 1999.

I looked at the source code of readline-7.0, and IIUC none of those keys
have mappings in the default vi keymap.

The right fix might be to simply remove the 'if' check above.  Would you
like to try it and report back?

      Thanks,
        Mark

bug#32501: Bouncing parentheses broken in REPL with vi-mode enabled

[Mark H Weaver]

Hi again,

> Daniel Tam <danooo.tam@gmail.com> writes:
>
>> I've activated readline support for the Guile repl, but I've found that
>> if my inputrc enables vi-mode, then the bouncing parentheses feature
>> doesn't work. Disabling vi-mode does the trick.
>
> Indeed.  For some reason that I cannot determine, the bouncing
> parentheses feature is specifically disabled when the vi keymap is in
> use.

I think I now see the reason for it.  I noticed that readline's default
vi keymap includes a binding for '%', which jumps to the paren matching
the one under the cursor.  That reminded me, from many years ago when I
used vi more often, that this is the way that old vi traditionally
allows matching parens to be found.

So, I guess the decision long ago to disable bouncing parens when in vi
mode was to match the way that emacs and vi behaved at that time.

However, I just tried modern vim, and I see that it now highlights
matching parens by default.  So, we should probably remove the 'if' to
match this newer behavior.

       Mark

bug#32501: Bouncing parentheses broken in REPL with vi-mode enabled

[John Cowan]

[-- Attachment #1: Type: text/plain, Size: 1405 bytes --]

Indeed, I pretty much only go into vi-mode in vi itself to use %; the rest
of the time, I remain an "ex" troglodyte whose only vi-commands are h, j,
k, l (and arrow analogues), x, %, and most importantly gQ.  Yes, even when
writing Lisp.

On Thu, Aug 23, 2018 at 12:05 AM Mark H Weaver <mhw@netris.org> wrote:

> Hi again,
>
> > Daniel Tam <danooo.tam@gmail.com> writes:
> >
> >> I've activated readline support for the Guile repl, but I've found that
> >> if my inputrc enables vi-mode, then the bouncing parentheses feature
> >> doesn't work. Disabling vi-mode does the trick.
> >
> > Indeed.  For some reason that I cannot determine, the bouncing
> > parentheses feature is specifically disabled when the vi keymap is in
> > use.
>
> I think I now see the reason for it.  I noticed that readline's default
> vi keymap includes a binding for '%', which jumps to the paren matching
> the one under the cursor.  That reminded me, from many years ago when I
> used vi more often, that this is the way that old vi traditionally
> allows matching parens to be found.
>
> So, I guess the decision long ago to disable bouncing parens when in vi
> mode was to match the way that emacs and vi behaved at that time.
>
> However, I just tried modern vim, and I see that it now highlights
> matching parens by default.  So, we should probably remove the 'if' to
> match this newer behavior.
>
>        Mark
>
>
>
>

[-- Attachment #2: Type: text/html, Size: 1901 bytes --]

bug#32501: Bouncing parentheses broken in REPL with vi-mode enabled

[Mark H Weaver]

Hello again,

I wrote:

> The relevant function is 'init_bouncing_parens' in
> guile-readline/readline.c:
>
>   static void
>   init_bouncing_parens ()
>   {
>     if (strncmp (rl_get_keymap_name (rl_get_keymap ()), "vi", 2))
>       {
>         rl_bind_key (')', match_paren);
>         rl_bind_key (']', match_paren);
>         rl_bind_key ('}', match_paren);
>       }
>   }

[...]

> The right fix might be to simply remove the 'if' check above.

Actually, it probably won't be that simple.  Whereas in emacs mode, we
can probably rely on these bindings being added to the correct keymap,
namely 'emacs_standard_keymap', when in vi mode it's likely that these
bindings would end up in the wrong keymap, namely 'vi_movement_keymap'.

When in vi mode, these mappings should be added to
'vi_insertion_keymap', and that probably involves using
'rl_bind_key_in_map' instead of 'rl_bind_key'.

       Mark

bug#32501: Bouncing parentheses broken in REPL with vi-mode enabled

[Mark H Weaver]

I wrote:

>> The right fix might be to simply remove the 'if' check above.
>
> Actually, it probably won't be that simple.  Whereas in emacs mode, we
> can probably rely on these bindings being added to the correct keymap,
> namely 'emacs_standard_keymap', when in vi mode it's likely that these
> bindings would end up in the wrong keymap, namely 'vi_movement_keymap'.
>
> When in vi mode, these mappings should be added to
> 'vi_insertion_keymap', and that probably involves using
> 'rl_bind_key_in_map' instead of 'rl_bind_key'.

And I guess 'rl_get_keymap_by_name' is the function we should use to get
the keymap.  From keymaps.h in readline:

  /* Return the keymap corresponding to a given name.  Names look like
     `emacs' or `emacs-meta' or `vi-insert'.  */
  extern Keymap rl_get_keymap_by_name PARAMS((const char *));

      Mark

bug#32501: Bouncing parentheses broken in REPL with vi-mode enabled

[Daniel Tam]

Hey Mark,


On 08/24/2018 04:09 PM, Mark H Weaver wrote:
> I wrote:
>
>>> The right fix might be to simply remove the 'if' check above.
>> Actually, it probably won't be that simple.  Whereas in emacs mode, we
>> can probably rely on these bindings being added to the correct keymap,
>> namely 'emacs_standard_keymap', when in vi mode it's likely that these
>> bindings would end up in the wrong keymap, namely 'vi_movement_keymap'.
>>
>> When in vi mode, these mappings should be added to
>> 'vi_insertion_keymap', and that probably involves using
>> 'rl_bind_key_in_map' instead of 'rl_bind_key'.
> And I guess 'rl_get_keymap_by_name' is the function we should use to get
> the keymap.  From keymaps.h in readline:
>
>   /* Return the keymap corresponding to a given name.  Names look like
>      `emacs' or `emacs-meta' or `vi-insert'.  */
>   extern Keymap rl_get_keymap_by_name PARAMS((const char *));
>
>       Mark
I'm excited to have a look but I have 0 knowledge of the code base (and
am just learning Scheme although that's probably not an issue in this
case whatsoever). Do you have any advice for specific
documentation/files (other than README and HACKING) that I should read
if I were to attempt this? As well as which branch to base my changes onto.

Daniel

bug#32501: Bouncing parentheses broken in REPL with vi-mode enabled

[Mark H Weaver]

Hi Daniel,

Daniel Tam <danooo.tam@gmail.com> writes:
> I'm excited to have a look but I have 0 knowledge of the code base (and
> am just learning Scheme although that's probably not an issue in this
> case whatsoever).

It's okay, please don't feel any obligation to work on this.  Sometimes
people are impatient to fix a problem ASAP, and that's why I wrote these
preliminary emails.

It's probably better for you to focus on learning Scheme for now, rather
than mucking around in the underbelly of an implementation :)

I appreciate you taking the time to file this bug report.  That's quite
useful by itself.

      Thanks!
        Mark

bug#32501: Bug #32501 - Bouncing parentheses broken in REPL with vi-mode enabled

[Sean]

[-- Attachment #1: Type: text/plain, Size: 217 bytes --]

   set blink-matching-paren on






Adding this line to your  ~/.inputrc provides this functionality, even with vi-mode enabled.



This bug could be closed, perhaps with a documentation note somewhere.

[-- Attachment #2: Type: text/html, Size: 584 bytes --]