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

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

In a previous build it worked AFAICS.

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)

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.

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)

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;)

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?

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...

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)

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

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

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.

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)

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