bugGNU Octave - Bugs: bug #44095, readline configuration - tab...

 
 

bug #44095: readline configuration - tab autocompletion of paths

Submitter:  None
Submitted:  Mon 26 Jan 2015 07:36:01 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  lachlan
Originator Name:  Emil Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * dev
Operating System:  * Any Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 01 Jun 2016 09:38:05 PM UTC, comment #28: 

I edited the changeset slightly and pushed it to default.  Thanks for working on this.  I know it will be appreciated by many.

John W. Eaton <jwe>
Group administrator
Wed 01 Jun 2016 08:04:05 PM UTC, comment #27: 

Lachlan,

[OT] sorry for responding so late. I have had a number (~10) of your csets for several bugs/patches in my Windows builds but had no time/priority to test all of them. I did report about some of them; in the mean time several have been applied.

I think/hope next weekend I'll make the next Linux build & Windows cross-build and try this new cset.

Thanks

Philip Nienhuis <philipnienhuis>
Group Member
Tue 31 May 2016 03:08:10 AM UTC, comment #26: 

Try the attached, Philip.  It applies for me on hg id f625afd1ef55.

Given the number of requests for this, I'm going to increase the severity to "normal" from "wish".  I really hope it makes it into 4.2.

(file #37314)

Lachlan Andrew <lachlan>
Mon 30 May 2016 10:04:47 PM UTC, comment #25: 

Patch doesn't apply cleanly anymore as of end of May.

In a previous build it worked AFAICS.

Philip Nienhuis <philipnienhuis>
Group Member
Wed 10 Feb 2016 05:07:01 AM UTC, comment #24: 

The attached changeset has addressed all of jwe's concerns that I could identify.

The other change from v4 is that it now uses file_ops::tilde_expand to expand paths starting with user names.  It doesn't expand partial user names, partly because Octave's password object doesn't support it, and partly because a tilde followed by something not a username is likely to be a Matlab-style negation.

(file #36287)

Lachlan Andrew <lachlan>
Tue 29 Dec 2015 05:22:29 AM UTC, comment #23: 

The previous patch didn't treat "~" as the home directory.  The new patch does.

I also put the function do_completer_word_break_hook() back as a member of gnu_readline, since that makes the fix for bug #34444 simpler.

Is there anything else I need to do to get this applied?

Season's greetings,
Lachlan

(file #35883)

Lachlan Andrew <lachlan>
Tue 22 Dec 2015 09:48:47 AM UTC, comment #22: 

Thanks for the feedback, John.  I've made the easy changes, and attempted the other ones.  In particular:

1. I don't think I added any char* functions to the command_editor class.  The only char* function I added was gnu_readline::do_completer_word_break_hook().  The quoting functions such as


char *gnu_readline::command_quoter (char *text, int matches, char *qcp)


also return char *.  To avoid any confusion, I've made the function not a member of any class.  Let me know if there is something else you had in mind.

2. To me, a filename is the last component of a path (unless that path is to a directory).  Still, I replaced "path" by "filename" in the code.  Is that what you wanted?

3. I've avoided the malloc by writing an accessor function to Readline's variable, which is cleaner anyway.

Sorry that I don't know how to produce a changeset based on what in my repository are two commits.  I'm providing a diff instead.  Once you're happy with it, I'll figure out how to produce a changeset :)

(file #35833)

Lachlan Andrew <lachlan>
Sun 20 Dec 2015 06:39:21 PM UTC, comment #21: 

Thanks for working on this feature.

I have a few minor comments:

Please use gnulib::malloc in C++ code.  Or, since we are managing the memory, use Octave's strsave funtion and operator delete[].

Please use Octave's file_stat class instead of copying functions from gnulib.  We want to avoid cut and pasted code as much as possible.

In C++ code, please use 0 instead of NULL.

To be consistent with other commit messages, please write the initial line as

description (bug #NNNNN)

instead of "Fixed bug..."

One of my goals for the command_editor class was to use std::string instead of bare char* pointers where possible for the hook functions and provide wrappers as needed for interfacing with readline.  For example, look at the way the quoting/dequoting or completion functions work.  But maybe that isn't really necessary, I don't know.  In any case, it would be nice to be consistent and not have a mix of some functions using char* and others std::string.

I know that the term "path" is often used interchangably with "filename" but I'd prefer to use "path" when describing a list of directory names.

John W. Eaton <jwe>
Group administrator
Fri 18 Dec 2015 09:03:11 AM UTC, comment #20: 

The attached changeset (bug_44095.v2.cset) is cleaner than the previous on.  It uses a hook in gnu readline to change the word delimiters if the last part of the command line (excluding what follows a (back)slash) is a valid directory.

Its main advantage over the previous patch is that it gets auto-quoting right.  Its main disadvantage is that the existence of a file or directory can inhibit completion of a variable.

It also results in a trailing '/' on Windows, like the status quo, whereas the previous patch fixed that.  I think that using  rl_directory_completion_hook  will be a cleaner way to fix that.

(file #35788)

Lachlan Andrew <lachlan>
Thu 17 Dec 2015 11:43:12 AM UTC, comment #19: 

The attached changeset does roughly what jwe was asking for.

Whenever a completion occurs, it looks for file completions as well as variable or function completions, by looking at the space-delimited tail of the command line.

It makes a first cut at handling quoted strings, but has lots of rough edges still.

It uses a hack to detect if a string is a directory -- copying from libgnu-hg.  Is there an Octave "is_directory" function or some such?

It also needs to remove the check "matches > 0" (i.e., at least one variable/function is a valid completion) from the code that returns a non-empty completion.  Will that cause troubles?

(file #35778)

Lachlan Andrew <lachlan>
Tue 01 Dec 2015 01:05:05 AM UTC, comment #18: 

Libor, why do you need to return a pointer to a string literal?  Can't the literal be stored in a static or global variable before being referenced?

Lachlan Andrew <lachlan>
Tue 13 Oct 2015 11:55:27 PM UTC, comment #17: 

The "assignment from incompatible pointer type" error is caused by 'const' specifier. I misread the GNU readline documentation: "It should return a character string like rl_completer_word_break_characters to be used to perform the current completion.", where rl_completer_word_break_characters is const char , but rl_cpvfunc_t is a function that should return char , while I'm assigning pointer to a function returning const char *.

It is necessary to remove the const specifier all the way up from the chain leading to dir_word_breaker in input.cc. But then you'll hit another kind of warning for returning pointer to a string literal without const: http://stackoverflow.com/questions/59670/how-to-get-rid-of-deprecated-conversion-from-string-constant-to-char-warnin . Choose yourself, which one is better;)

Libor Morkovsky <liborm>
Tue 13 Oct 2015 03:49:17 PM UTC, comment #16: 

Adding potential filename completion just about everywhere seems natural.  I just opened the perl debugger to get a CLI and it does that.  The shell seems to do the same as well.

Rik <rik5>
Group administrator
Tue 13 Oct 2015 02:21:03 PM UTC, comment #15: 

I wasn't thinking of maintaining a dynamic list or providing programmable completion.  Instead, what about always attempting filename completion in addition to completing function and variable names?  Or perhaps limiting filename completion to cases where the input looks like a character string or command syntax?

John W. Eaton <jwe>
Group administrator
Mon 12 Oct 2015 03:29:28 PM UTC, comment #14: 

Simple enough to get rid of the -reorder warning.


 gnu_readline::gnu_readline ()
   : command_editor (), previous_startup_hook (0),
-    previous_pre_input_hook (0), word_break_function(0),
-    previous_event_hook (0), completion_function (0),
+    previous_pre_input_hook (0), previous_event_hook (0),
+    completion_function (0), word_break_function(0),
     quoting_function (0), dequoting_function (0),
     char_is_quoted_function (0), user_accept_line_function (0)


However, what about "warning: assignment from incompatible pointer type".  That was the one that seemed more serious since it is in a C file.  C will happily let you interpret one thing as another simply by changing the pointer.  Normally, one ends up with segfaults and other oddities, and I'm not seeing that, but I still think it would be best to fix or understand that warning.

Rik <rik5>
Group administrator
Mon 12 Oct 2015 08:29:21 AM UTC, comment #13: 

The -Wreorder warnings are not a big deal, they're telling you that the order of initialization of members is not the one you use in initializer list of the constructor, but rather follows the order of the declarations, see http://stackoverflow.com/questions/1828037/whats-the-point-of-g-wreorder

There is no interdependency between the member fields, so you can try to reorder the initializer list to get rid of this warning.

Regarding the extension of this patch to user defined commands - I'd say that the current patch on it's own brings enough value to most of the users, so I'd consider the "dynamic" list a separate issue.

Having said that, the implementation would probably be quite straightforward, just keep the same list inside the 'octave' language, allowing the user to modify it at will. But there comes the need for some extensive validation...

Libor Morkovsky <liborm>
Sun 11 Oct 2015 10:36:44 PM UTC, comment #12: 

Given that users can define their own functions in Octave that may be called using "command" syntax, and those functions could easily accept filenames as arguments, maybe there is some other approach we could take other than trying to maintain a list of functions that accept filename arguments?

John W. Eaton <jwe>
Group administrator
Sun 11 Oct 2015 07:26:40 PM UTC, comment #11: 

The number of functions that accept a filename and might benefit from this patch is quite large (84, and I'm sure I missed some).  The list I found is below.


addpath
audioinfo
audioread
audiowrite
bunzip2
bzip2
canonicalize_file_name
cd
chdir
command_line_path
copyfile
delete
dir
dir_in_loadpath
dlmread
dlmwrite
doc_cache_create
doc_cache_file
edit
exec
exist
fileattrib
file_in_loadpath
file_in_path
fileparts
fileread
fopen
fullfile
gen_doc_cache
genpath
get_help_text_from_file
glob
gunzip
gzip
hgload
hgsave
history_file
imfinfo
importdata
imread
imshow
imwrite
info_file
info_program
is_absolute_filename
isdir
is_rooted_relative_filename
javaaddpath
javarmpath
link
load
ls
lstat
make_absolute_filename
makeinfo_program
mex
mkdir
mkfifo
mkoctfile
movefile
octave_core_file_name
rmdir
rmpath
run
rundemos
runtests
save
saveas
savepath
source
system
tempname
texi_macros_file
textread
tilde_expand
unlink
unpack
untar
unzip
urlread
urlwrite
what
which


I reduced the list using as criteria frequency of usage and interactive utility.  For example, when renaming files it is often files in the same directory for which the existing completion mechanism will work.  On the other hand, when you use 'source' it is precisely because the script does not have a '.m' extension and isn't necessarily on the path.  The list I used is


  static std::string dirfns[] = {"cd ", "ls ", "dir ", "load ", "save ",
                                 "saveas ", "edit ", "exist ", "run ",
                                 "source ", "addpath "};


I changed is_completing_dirfns() to use pre-computed values and compare() rather than find().  Together, this achieved a 6.5X speedup which is important since this function can be called frequently.  If we decide to add more functions to the list we should probably move from a direct linear search to a hash lookup.

I've uploaded the latest version of the patch to this bug report.  I did not commit it to the repository because I am getting two warnings during compilation.


liboctave/util/cmd-edit.cc: In constructor 'gnu_readline::gnu_readline()':
liboctave/util/cmd-edit.cc:214:18: warning: 'gnu_readline::word_break_function' will be initialized after [-Wreorder]
liboctave/util/cmd-edit.cc:210:18: warning:   'int (* gnu_readline::previous_event_hook)()' [-Wreorder]
liboctave/util/cmd-edit.cc:237:1: warning:   when initialized here [-Wreorder]

liboctave/util/oct-rl-edit.c: In function 'octave_rl_set_word_break_function':
liboctave/util/oct-rl-edit.c:340:33: warning: assignment from incompatible pointer type [enabled by default]


This needs someone who understands Readline better than me to take a look.  I will say, however, that everything works even with these warnings.


(file #35156)

Rik <rik5>
Group administrator
Mon 14 Sep 2015 06:39:38 PM UTC, comment #10: 

Just skimming the patch for bug #34624, the only substantial difference is few more keywords in the directory manipulation commands list:


+  static std::string dirfns_commands[] = {
+    "cd", "ls", "load", "save", "rename", "rmdir",
+    "movefile", "copyfile", "unlink"
+  };


Which I think would definitely be useful and do not require much additional testing.

I tested the current patch with ';:-<space>' over sevral levels of nesting, and everything looks fine, readline auto-quotes the current completion as required.

Libor Morkovsky <liborm>
Sat 12 Sep 2015 09:15:14 PM UTC, comment #9: 

There is some commonality between this patch and the one attached to bug #34624, we should combine the features of both patches.

Mike Miller <mtmiller>
Group Member
Sat 12 Sep 2015 09:12:09 PM UTC, comment #8: 

I just took a quick look at this patch, it seems mostly ok to me. I haven't tested it or looked thoroughly enough to push it yet, but I think it's a strong start.

If anything your commit msg is a little too wordy, Something like


* input.cc (dir_word_breaker): New function.


is sufficient.

I'd be interested in testing what the behavior is for directories with odd characters in them: a directory with a hyphen or other operator characters, directories that contain quotes or semicolons, etc.

Thanks for your work on this so far!

Mike Miller <mtmiller>
Group Member
Wed 09 Sep 2015 03:08:56 PM UTC, comment #7: 

Is the commit message okay like this, or needs some more work?

Libor Morkovsky <liborm>
Fri 17 Jul 2015 08:14:02 AM UTC, comment #6: 

Looks like the rich markup broke the commit message. One more try, wrapped at 80 chars per line.


directory commands can autocomplete across slash (bug #44095)

* libinterp/corefcn/input.cc (dir_word_breaker) Add a context sensitive
  function returning word delimiters for GNU readline, limiting the
  character set to only space, tab and newline for directory commands
  - currently 'ls' and 'cd'.
  (generate_completion) Register the dir_word_breaker function with
  current command_editor.
* liboctave/util/cmd-edit.cc (gnu_readline::*, command_editor::*) Add
  word break callback setter mimicking the structure of other callbacks.
* liboctave/util/cmd-edit.h (command_editor::*) Add word  break callback
  setter mimicking the structure of other callbacks.
* liboctave/util/oct-rl-edit.c (octave_rl_set_word_break_function) Add a
  function that sets GNU readline variable rl_completion_word_break_hook.
* liboctave/util/oct-rl-edit.h Add a type definition and setter function
  for GNU readline variable rl_completion_word_break_hook.


Libor Morkovsky <liborm>
Fri 17 Jul 2015 07:59:16 AM UTC, comment #5: 

The rebased version looks good to me.

I created the patch with git, because I used the tarball as source - had some checkout issues with hg, and I'm more familiar with git..

My suggested commit message, I tried to be concise:

directory commands can autocomplete across slash (bug #44095)

  • libinterp/corefcn/input.cc (dir_word_breaker) Add a context sensitive

  function returning word delimiters for GNU readline, limiting  the
  character set to only space, tab and newline for directory commands
  - currently 'ls' and 'cd'.
  (generate_completion) Register the dir_word_breaker function with
  current command_editor.

  • liboctave/util/cmd-edit.cc (gnu_readline::*, command_editor::*) Add

  word break callback setter mimicking the structure of other callbacks.

  • liboctave/util/cmd-edit.h (command_editor::*) Add word  break callback

  setter mimicking the structure of other callbacks.

  • liboctave/util/oct-rl-edit.c (octave_rl_set_word_break_function)  Add a

  function that sets GNU readline variable rl_completion_word_break_hook.

  • liboctave/util/oct-rl-edit.h Add a type definition and setter function

  for GNU readline variable rl_completion_word_break_hook.

Libor Morkovsky <liborm>
Fri 03 Jul 2015 02:57:02 PM UTC, comment #4: 

The patch formatting looks weird. Did you create this patch with git?

Okay, I rebased the thing. Can you please review the rebased version? I had to resolve one conflict in the ctor-initaliser of gnu_readline.

Also, please help me write a commit message:

http://wiki.octave.org/Commit_message_guidelines


(file #34372)

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Mon 22 Jun 2015 08:38:00 PM UTC, comment #3: 

I'm attaching a patch - it's against 3.6.2, because that's what we're using. But it should be quite easy to port it to newer versions.

The change is very simple - it uses the existing infrastructure of recognizing if 'is_completing_dirfns()', and if so, it limits the word splitting characters of GNU readline to minimum (space, tab, newline) using 'rl_completion_word_break_hook'.

I did not test/think much about how weird things can people do with their paths, so maybe some other reduced set of word breakers would be more appropriate.

But it works for sensible paths, and can be extended further. That's probably what most users need.

liborm

(file #34298)

Anonymous
Wed 28 Jan 2015 03:04:32 AM UTC, comment #2: 

So should we redefine this bug to be about adding support for context-sensitive readline completion in the Octave interpreter? Because that seems like the only way to differentiate between division operator and path separator.

Mike Miller <mtmiller>
Group Member
Tue 27 Jan 2015 04:40:02 PM UTC, comment #1: 

I will just paste here from the IRC


on your example, suppose you do "cd AA/A<tab>" you will get "cd AA/AA".
The reason is not because it thinks there is an AA inside AA. The problem is
that after a "/" it starts to autocomplete again from your current directory
and is autocompleting the AA on your current directory


and the reason it does that is because when you are auto completing, if
readline sees a "/" it thinks you are dividing so starts autocompling again.
It does not see as path separator. However, it will act correctly if you
"cd 'AA/A<tab". When you do this, it sees you are autocompleting a string,
so you don't want a division, it most likely is a path

note that this also happens you have a dash on your path "a-file-name" because
it gets confused with a subtraction

you could fix this by configuring readline to see / and - as parts of
filepaths if they come after a cd command for example


Carnë Draug <carandraug>
Group Member
Mon 26 Jan 2015 07:36:01 PM UTC, original submission:  

When browsing files I've noticed that tab autocompletion sometimes gave wrong results. It turned out that it happened only if a child folder was a prefix of a parent folder.
For example, if you have folder AA and folder A inside it. Then you will get wrong autocompletion (AA instead of A).


Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #37314:  bug_44095.v6.cset added by lachlan (7KiB - text/x-diff)
file #36287:  bug_44095.v5.cset added by lachlan (7KiB - application/octet-stream)
file #35883:  bug_44095.v4.diff added by lachlan (6KiB - text/x-diff)
file #35833:  bug_44095.v3.diff added by lachlan (6KiB - text/x-diff)
file #35788:  bug_44095.v2.cset added by lachlan (7KiB - application/octet-stream)
file #35778:  bug_44095.cset added by lachlan (9KiB - application/octet-stream)
file #35156:  fixtabcomplete.diff added by rik5 (8KiB - application/x-download)
file #34372:  fix-tab-completion.diff added by jordigh (7KiB - text/x-diff)

 

Carbon-Copy List
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by liborm (Posted a comment)
  • -email is unavailable- added by jordigh (Updated the item)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by None (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 24 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-06-01 jwe StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2016-05-31 lachlan Attached File- Added bug_44095.v6.cset, #37314
        Severity1 - Wish 3 - Normal
    2016-02-15 lachlan Dependencies- bugs #34624 is dependent
    2016-02-10 lachlan Attached File- Added bug_44095.v5.cset, #36287
        StatusPatch Submitted Ready For Test
        Assigned toNone lachlan
    2015-12-29 lachlan Attached File- Added bug_44095.v4.diff, #35883
    2015-12-22 lachlan Attached File- Added bug_44095.v3.diff, #35833
    2015-12-18 lachlan Attached File- Added bug_44095.v2.cset, #35788
    2015-12-17 lachlan Attached File- Added bug_44095.cset, #35778
    2015-10-11 rik5 Attached File- Added fixtabcomplete.diff, #35156
    2015-09-12 mtmiller Dependencies- bugs #42291 is dependent
    2015-09-12 mtmiller StatusNeed Info Patch Submitted
    2015-07-03 jordigh Attached File- Added fix-tab-completion.diff, #34372
    2015-06-22 None Attached File- Added octave-3.6.2-directory-commands-can-autocomplete-across-slash.patch, #34298
    2015-01-28 mtmiller CategoryNone Interpreter
        Severity3 - Normal 1 - Wish
        Item GroupNone Feature Request
        StatusNone Need Info
        Release3.8.0 dev
        Operating SystemMac OS Any
    2015-01-27 carandraug Summarytab autocompletion bug readline configuration - tab autocompletion of paths

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code