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

 
 

bug #44095: readline configuration - tab autocompletion of paths

Submitted by:  None
Submitted on:  Mon 26 Jan 2015 07:36:01 PM UTC  
 
Category: InterpreterSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Feature Request
Status: FixedAssigned to: Lachlan Andrew <lachlan>
Originator Name: EmilOriginator Email: -unavailable-
Open/Closed: ClosedRelease: dev
Operating System: Any

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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>
Project 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>
Project 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>
Project MemberIn charge of this item.
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>
Project 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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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

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>
Project MemberIn charge of this item.
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>
Project 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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project 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>
Project Administrator
Mon 12 Oct 2015 03:29:28 PM UTC, comment #14:

Simple enough to get rid of the -reorder warning.

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

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

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.

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>
Project 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:

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>
Project Administrator
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

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

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>
Project Administrator
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>
Project Administrator
Tue 27 Jan 2015 04:40:02 PM UTC, comment #1:

I will just paste here from the IRC

Carnë Draug <carandraug>
Project 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 File(s):
   
   
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)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

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

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 24 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 01 Jun 2016 09:38:36 PM UTCjweStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Tue 31 May 2016 03:08:10 AM UTClachlanAttached File-=>Added bug_44095.v6.cset, #37314
      Severity1 - Wish=>3 - Normal
    Mon 15 Feb 2016 01:39:22 AM UTClachlanDependencies-=>bugs #34624 is dependent
    Wed 10 Feb 2016 05:07:01 AM UTClachlanAttached File-=>Added bug_44095.v5.cset, #36287
      StatusPatch Submitted=>Ready For Test
      Assigned toNone=>lachlan
    Tue 29 Dec 2015 05:22:29 AM UTClachlanAttached File-=>Added bug_44095.v4.diff, #35883
    Tue 22 Dec 2015 09:48:47 AM UTClachlanAttached File-=>Added bug_44095.v3.diff, #35833
    Fri 18 Dec 2015 09:03:11 AM UTClachlanAttached File-=>Added bug_44095.v2.cset, #35788
    Thu 17 Dec 2015 11:43:12 AM UTClachlanAttached File-=>Added bug_44095.cset, #35778
    Sun 11 Oct 2015 07:26:40 PM UTCrik5Attached File-=>Added fixtabcomplete.diff, #35156
    Sat 12 Sep 2015 09:20:07 PM UTCmtmillerDependencies-=>bugs #42291 is dependent
    Sat 12 Sep 2015 09:12:09 PM UTCmtmillerStatusNeed Info=>Patch Submitted
    Fri 03 Jul 2015 02:57:02 PM UTCjordighAttached File-=>Added fix-tab-completion.diff, #34372
    Mon 22 Jun 2015 08:38:00 PM UTCNoneAttached File-=>Added octave-3.6.2-directory-commands-can-autocomplete-across-slash.patch, #34298
    Wed 28 Jan 2015 03:04:32 AM UTCmtmillerCategoryNone=>Interpreter
      Severity3 - Normal=>1 - Wish
      Item GroupNone=>Feature Request
      StatusNone=>Need Info
      Release3.8.0=>dev
      Operating SystemMac OS=>Any
    Tue 27 Jan 2015 04:40:02 PM UTCcarandraugSummarytab autocompletion bug=>readline configuration - tab autocompletion of paths

    Back to the top


    Powered by Savane 3.1-cleanup1