[Home]

Summary:ASTERISK-14859: [patch] CLI to honor user's ~/.editrc file
Reporter:Kirill Katsnelson (kkm)Labels:
Date Opened:2009-09-22 03:25:54Date Closed:2011-05-05 16:20:06
Priority:MajorRegression?No
Status:Closed/CompleteComponents:General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 015929-astcli-editrc-trunk.240324.diff
( 1) astcli-editrc-v2.diff
Description:Asterisk CLI ignores command line keybinding file ~/.editrc. By default, Home, End and Delete keys are not bound to meaningful commands. The attached simple patch fixes that, in such a way that:

1. If the environment variable EDITRC is set, it is taken to point to an alternative bindings file.
2. Otherwise, if HOME is set, the file is looked up in ${HOME}/.editrc

If exists, the file is processed by the editline library el_source() function.

The following sample .editrc file binds Home and End to move to the beginning and end of command line respectively, Delete to delete character right, and C-left and C-right to move by word:

bind \\e[3~ ed-delete-next-char
bind \\e[1~ ed-move-to-beg
bind \\e[4~ ed-move-to-end
bind \\eOC  vi-next-space-word
bind \\eOD  vi-prev-space-word
Comments:By: Jason Parker (jparker) 2009-09-22 13:58:21

If they do nothing currently, why don't we just add these to Asterisk, instead of requiring an rc file for it?
By: Kirill Katsnelson (kkm) 2009-09-22 17:37:30

Yes that can be done (or in addition to). This is less in line with general support of different terminals and keyboards by programs.

In support of my approach: (a) Asterisk uses generic library that many other programs use, but initializes it rather improperly and thus ignores a common rc file. (b) Hardcoding terminal escape sequences is not as good as allowing them to be configurable

In support of your approach: (a) Not many programs really use editline; readline is used far more often, and that one uses different rc file with different syntax (~/.inputrc) (b) Who uses terminals with non-VT100 keyboard codes, anyhow?

So, what do you think? I find the arguments of the both sides valid and reasonable.
By: Michiel van Baak (mvanbaak) 2009-09-22 17:48:46

Why not take the best of both:
1) Just add these to asterisk
2) Make them configurable if you want to act them differently from what is in asterisk

1 means ppl actually get usefull keys by default without having to mess with the configfile, while 2 gives ppl that want stuff weirder the ability to configure it.
By: Kirill Katsnelson (kkm) 2009-09-22 18:05:46

Agree.

Should I attach a new patch to this same ticket?
By: Michiel van Baak (mvanbaak) 2009-09-23 00:53:28

Yes please.
By: Kirill Katsnelson (kkm) 2009-09-23 02:21:41

Uploaded new patch against the original file (disregard the first please -- am I missing a button to remove it?)

Added the same 5 key bindings listed in the original description example as defaults, but still allowing user's .editrc to override.

License "PENDING" indicates that I did something wrong when submitted it, or is it expected?
By: Michiel van Baak (mvanbaak) 2009-09-23 02:46:36

License PENDING means legal department did not have a look at it yet.
This is expected. They will have a look at it in the next couple of days (maybe faster, cant really say)
Untill then we will have to wait because we cannot see the patch yet.

Thanks for the followup.
By: Kirill Katsnelson (kkm) 2009-09-23 03:50:02

No problem at all. Thanks for the explanation.
By: Leif Madsen (lmadsen) 2009-09-30 10:22:55

Removed first patch per the reporter.
By: Kirill Katsnelson (kkm) 2010-01-15 00:19:05.000-0600

Added a patch against latest trunk (rev. 240324).
By: Sean Bright (seanbright) 2010-01-15 18:33:48.000-0600

I notice in your patch you stop using 'editor' and start using 'tenv' but they appear to do the same thing.  Is there a reason for that change?

Edit: Ah.  I see you re-use the same variable later for other purposes.  Make sense.


By: Sean Bright (seanbright) 2010-01-15 18:47:31.000-0600

Also, reading the man page for el_source() says:
<pre>
 Initialise editline by reading the contents of file.  el_parse() is called
 for each line in file. If file is NULL, try $PWD/.editrc then $HOME/.editrc.
</pre>
So is it sufficient to simply call el_source(el, NULL) after determining that EDITRC is NULL or empty instead of checking HOME and building the path ourselves?


By: Kirill Katsnelson (kkm) 2010-01-16 04:57:19.000-0600

+ /* Not using el_source(el, NULL) as that does not work on non-BSD systems! */

I do not remember why off the top of my head, but the bottom line it would not work on linux and that should be quite a popular platform choice for Asterisk.
By: Sean Bright (seanbright) 2010-01-16 16:03:32.000-0600

Ah.  Not sure how I missed that in the patch.
By: Sean Bright (seanbright) 2010-03-19 10:12:04

Your patch is now integrated into my editline-update branch at:

   http://svn.digium.com/svn/asterisk/team/seanbright/editline-update/

We've updated the editline library that is shipped with asterisk which doesn't exhibit the same issue with el_source() that the previously bundled version did, so I haven't included that part in the branch.
By: Kirill Katsnelson (kkm) 2010-03-19 15:35:25

Thank you. Is this branch likely to make it into the 1.8 release?
By: Sean Bright (seanbright) 2010-03-20 10:04:56

I think so, yes.
By: Tzafrir Cohen (tzafrir) 2010-03-21 05:42:20

Small nit: use the variable name "AST_EDITMODE" instead of "AST_EDITOR"?

I normally prefer the vi editor but the emacs edit mode. Please don't try to force me to state I want an emacs AST_EDITOR ;-)
By: Sean Bright (seanbright) 2010-03-23 09:21:04

Sure.  Will update the branch.
By: Digium Subversion (svnbot) 2010-07-27 16:57:08

Repository: asterisk
Revision: 280019

U   branches/1.8/build_tools/menuselect-deps.in
U   branches/1.8/configure
U   branches/1.8/configure.ac
U   branches/1.8/include/asterisk/autoconfig.h.in
U   branches/1.8/include/asterisk/term.h
_U  branches/1.8/main/
U   branches/1.8/main/Makefile
U   branches/1.8/main/asterisk.c
U   branches/1.8/main/cli.c
D   branches/1.8/main/editline/
U   branches/1.8/main/term.c
U   branches/1.8/main/xmldoc.c
U   branches/1.8/makeopts.in

------------------------------------------------------------------------
r280019 | seanbright | 2010-07-27 16:57:05 -0500 (Tue, 27 Jul 2010) | 23 lines

Add ability to use system libedit and update bundled libedit.

The version of libedit that is bundled with asterisk is old and has some bugs.
This patch updates the bundled version of libedit within asterisk, and also
updates asterisk to use the system libedit instead if one is available (and
pkg-config is available).  This review integrates several patches from other
users specifically kkm and tzafrir.

(closes issue ASTERISK-14859)
Reported by: kkm
Patches:
     015929-astcli-editrc-trunk.240324.diff uploaded by kkm (license 888)

(issue ASTERISK-15652)
Reported by: jw-asterisk

(closes issue ASTERISK-15821)
Reported by: tzafrir
Patches:
     0001-allow-using-system-copy-of-libedit.patch uploaded by tzafrir (license 46)

Review: https://reviewboard.asterisk.org/r/807/

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=280019
By: Digium Subversion (svnbot) 2011-05-05 16:20:02

Repository: asterisk
Revision: 317395

U   trunk/main/asterisk.c

------------------------------------------------------------------------
r317395 | seanbright | 2011-05-05 16:20:01 -0500 (Thu, 05 May 2011) | 16 lines

Add some new editline bindings by default, and allow for user specified configuration.

I excluded the part of this patch that used the HOME environment variable since
the built-in editline library goes to great lengths to disallow that.  Instead
only settings the EDITRC environment variable will use a user specified file.

Also, the default environment variable use to determine the edit more is
AST_EDITMODE instead of AST_EDITOR (although the latter is still supported).

(closes issue ASTERISK-14859)
Reported by: kkm
Patches:
     astcli-editrc-v2.diff uploaded by kkm (license 888)
     015929-astcli-editrc-trunk.240324.diff uploaded by kkm (license 888)
Tested by: seanbright

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=317395