Added tab prompter capability

[abates]

In an attempt to allow customization of the tab prompt (requested in #20) I've added a "tab prompter" callback to the tab completion. This isn't yet documented or unit tested. However, if this seems like a reasonable approach and would be accepted, then I'll add documentation and tests to the pull request. An example of bash-like tab prompting would be:

func(items []string, readNext liner.NextReader) (string, error) {
  if len(items) == 0 {
    return "", nil
  } else if len(items) == 1 {
    return items[0], nil
  } else if len(items) > completionQueryItems {
    fmt.Printf("\nDisplay all %d possibilities? (y or n) ", len(items))
    for {
      next, err := readNext()
      if err != nil {
        return "", err
      }

      if key, ok := next.(rune); ok {
        if unicode.ToLower(key) == 'n' {
          return "", nil
        } else if unicode.ToLower(key) == 'y' {
          break
        }
      }
    }
  }

  fmt.Printf("\n%v\n", items)
  return "", nil
}

The above example would present a very basic display of the possibilities when a single tab is pressed. While not exactly like bash (no tab-tab support) it certainly allows for customizing the prompt. Please let me know what you think.

Regards, Andrew

[peterh]

As I explained in #20, I strongly prefer tab cycling over scrollback pollution. I'm also trying to keep the number of configurable options in Liner down (although that seems to be slipping recently).

Also, the above example appears flexible, but I'm not sure you can do much beyond bash-style "Display all 778 possibilities? (y or n)", since you can't use Prompt (it's not reentrant), and you don't want to rebuild prompt out of readNext calls. You might be better off with a simple boolean flag to set bash-style completion (and then you can do tab-tab, too).

I really don't like bash style completion though, so I'd be more comfortable if the flag was an environment variable spelled BASH_STYLE_SCROLLBACK_POLLUTION_ON_TAB=1 instead of a programmatic flag, so your app can't force the wrong behaviour on me, but that's not actually a deal breaker.

I'm going to think about this for a bit before I close it. Please try to convince me that I should accept this pull request; I'd like to have more to think about. Also, please link me to your application that uses Liner, and explain how that application specifically benefits from bash-style completion. @ElPincheTopo feel free to comment here, too. Thanks.

[abates]

I guess it just comes down to a matter of preference. I understand your preference with the prompt behavior, and it's completely valid. My preference is to be able to see a list. It is common for me to be in a situation where the number of results in an autocomplete list is longer than I want to scroll through. Much of the time I just need to know the next few characters to type to finish the auto completion. For example, all of my media files are organized by directory. If I want to do something to a specific "Star Trek" movie, then I need to change to that directory first. I have many "Star Trek" movies, each in a directory by title. Some of the titles have colons, others do not. For instance:

abates@pms1:/data/media/Movies$ ls Star\ Trek
Star Trek (2009)/                             
Star Trek First Contact (1996)/               
Star Trek: Generations (1994)/                
Star Trek III The Search for Spock (1984)/    
Star Trek II The Wrath of Khan (1982)/        
Star Trek: Insurrection (1998)/               
Star Trek Into Darkness (2013)/               
Star Trek IV The Voyage Home (1986)/          
Star Trek Nemesis (2002)/                     
Star Trek: The Motion Picture (1979)/
Star Trek VI The Undiscovered Country (1991)/
Star Trek V: The Final Frontier (1989)/
abates@pms1:/data/media/Movies$ ls Star\ Trek

I don't want to tab through each possibility when I can just visually scan for the one I'm looking for, see if it has a colon or not, and then continue.

I do agree that this type of behavior should be assignable by the user (using flags, environment variables or whatever), but it seems like that selection should be in the actual program implementation rather than the library.

I don't currently have an application that is using liner, at least nothing that is pushed to github, but I want to write a media server that has a shell for managing the media assets. The motivation for a shell rather than just allowing the user to manage the directories/folders/files themselves is that this media server implementation would have an abstracted virtual filesystem that might span several systems in a cluster. The media shell would present a single unified interface to the underlying virtual filesystem. I would want tab completion to behave similar to bash. Others using the shell might want a different behavior. Therefore, I'd allow the user to specify the behavior (by env variable or program argument), but I would implement that in my shell, not in liner.

Thanks for taking the time to at least look at this.

Regards, Andrew

[ElPincheTopo]

I don't have much to add to what @abates already said, just that when I started using Liner I wanted bash-like completion because it was what I was familiar with. Now I have been using liner in several projects at work and I have get used to this type of completion, the only case when I do think bash-like is a lot better is in the case previously explained by @abates: when you have a lot of options, cycling through all of them takes a lot more time than just seeing which next char you need to type in order to make tab reduce its options to just one.

Although I understand @peterh in not having apps using your library force unwanted behavior on you, I agree with @abates that it should be the program using the library, and not the library who makes that decision. Maybe, the solution is having both options. An interface that allows the program that uses the library, select the type of completion they want in their program, and also an environment variable that can override this option. This way, the programmer can control how the library should act but the end-user can override it if they really have strong preferences towards one or another. On the downside, this approach adds two extra configuration options and I do think we should try to keep it simple.

[peterh]

Thanks. You have convinced me that Bash-style is desirable, and that an environment switch is a bad idea.

Do either of you have comments on the proposed SetTabPrompter(f TabPrompter) vs something like SetTabPrintsMatches(p bool)? Or even a suggestion for a better name than SetTabPrintsMatches?

[abates]

The only reason I chose a callback rather than a boolean, with the implementation directly in tabComplete, was to allow the downstream implementor to be able to fully control the display. My thinking was that at some point I might want ANSI colors (although I don't know if that would be doable on windows) in the tab prompt. Also, with a callback it is possible to move the current completion prompt into its own closure/function that is the default prompt.

It really doesn't matter to me what it is named, I was trying to follow the conventions I saw already in liner.

Andrew

[peterh]

Brainstorming: SetTabCompletionStyle(s liner.TabStyle) leaves more room for future styles (although not zsh—that would need full curses support)

type TabStyle int

const (
    Circular TabStyle = iota // Like Windows
    Prints                   // Like Bash
)

[peterh]

Re: ANSI colours. Colour is explicitly out of scope for Liner. It's possible on Windows (and since I first wrote Liner there now exist libraries to emulate ANSI on Windows. Some of them might even be pure Go, I haven't looked) but I intentionally excluded colour to make TERM=dumb as simple as possible to implement, and to keep Liner simple and easy to port. (Not that I have any plans to support another non-VT100 OS at this time, but I'd like to keep the possibility open).

[ElPincheTopo]

I like your enum-like solution for setting tab completion.

[abates]

@peterh, Do you want me to update my pull request per your constant proposal above?

Andrew

[peterh]

@abates Yes, please.

After thinking about it a bit more, the word Tab should appear in each constant (eg. TabCircular), to help make it clear that the constant goes with SetTabCompletionStyle() (since Go doesn't have real enums). Thanks in advance.

[abates]

I'll try to update this pull request in the next few days. I've been tied up with other projects so haven't had a chance to get to it.

Andrew

[abates]

I'm not exactly happy with the structure of things, but the committed changes should work as discussed here. @peterh, are things where you expect them to be? Also, any thoughts on my approach of using closures for the tab printers?

[peterh]

I like closures. Looks like a perfectly reasonable way to store per-tabstyle state.

Thanks for the update.

Comments on the pull request:

• In my tests, 00e52808a36bbc27fca04660dc59f2052b97508d causes single-item completion to fail (it erases my partial token). It works fine before this change. Could you please elaborate on what caused you to make this commit?
• On a similar note, if there are lots of possibilities, pressing tab erases my partial token. (aside: This might be easiest to fix by supporting the longest common prefix now instead of later; see "Future Work" below.)
• Off by one error in maxWidth (should be len(item)+1 to allow for a space between items).
• Columns need to be left-aligned, not right-aligned. See the '-' printf flag.
• tabDirection should be in line.go where it will be used, not in common.go.
• Please document all exported identifiers. TabStyle, TabCircular, TabPrints, and SetTabCompletionStyle all need documentation.
• Please submit changes in logical chunks. In this case, you might want a commit that migrates the old tab handler to closure-style, a commit that adds the new tab handler, and a commit that adds the public interface. I'd also accept everything squashed into one commit. What I don't want to see is a pull request that includes the old commit exposing NextReader, followed by another commit removing NextReader.
• Speaking of NextReader, please remove NextReader.

Style nits:

• Use :=. var tabPrinter func(tabDirection) (string, error); tabPrinter = should be tabPrinter :=
• Use the printf '' width opearator. strfmt := fmt.Sprintf("%%-%dv", maxWidth); fmt.Printf(strfmt, items[i+j]) should be `fmt.Printf("%-s", maxWidth, items[i+j])`

Future work (not necessary for this pull request, but I'm writing it down before I forget):

• If there is more than one item in the list, the first tab should return the longest common prefix (if any).
• If there is more than one item in the list, the first tab should beep.
• Print items sorted by columns, not rows (for consistency with bash).
• The "Display all?" prompt limit should be 24 lines, not 100 items. (This is unlike bash, which has an item limit. But I think a line limit makes a whole lot more sense).
• Decide what to do about huge lists. Support -more- internally? Shell out to $PAGER? Do nothing and let the user's terminal emulator scroll bar handle it?

[ElPincheTopo]

I think that invoking $PAGER is the smart think to do, but I don't think that would work on windows. Maybe call $PAGER on Unix-like and try and emulate this functionality for windows is the way to go.

[peterh]

Windows comes with a simplistic more. I have less installed on Windows, and it's often the contents of $PAGER. If someone were to go to the effort of implementing both, I'd suggest "Call $PAGER if it is set, otherwise do a simple -more- emulation" and leave the OS out of the decision. I might be happier with "Call $PAGER if it is set, otherwise let the user's terminal do the scrollback", as it's easier to maintain. But that's a decision for later.

[abates]

@peterh: I actually already have a longest common substring function that I wrote for another program, so I'll add that in. I also think I can fix/update everything you commented on, including the style comments. Regarding commits: do I need to start a new branch and submit a new PR for that, or is there a way for me to update this PR accordingly (chunk commits rather than squashed)? If not, I'll just squash all my commits.

Regards, Andrew

[ElPincheTopo]

@abates You can use git rebase to edit commits in the same branch, and the pull request will update accordingly after you push.

[abates]

Okay, here is the update:

• Cleaned up the commits
• Fixed single-item completion in TabPrints
• Fixed TabPrints where partial token was erased with multiple candidates
• Fixed off by one error in max column width
• Columns are now left aligned
• Moved tabDirection definition to line.go
• Added documentation for exported identifiers
• Added longest common prefix to TabPrints when multiple candidates are returned
• Items are now sorted by column rather than row

Still todo:

• [ ] Change prompt limit to 24 rows instead of 100 items
• [ ] Add ability to scroll a list that extends beyond the screen limits

I have reservations regarding changing the prompt limit. If we key on rows rather than items then we have to compute the number of rows first. For extremely long lists, computing the number of rows could introduce enough lag as to deteriorate the user experience. For these extremely long lists the user is likely to reject displaying the list anyhow. Shouldn't we allow canceling as quickly as possible?

Regarding scrolling: I took a look at GNU readline and they have a simple more built into the library directly (as opposed to shelling out). For forward-only scrolling I think this would be easy to implement here. If someone wants behavior more like "less" that would be more difficult to build in.

Regards, Andrew

[ElPincheTopo]

@abates Regarding the scroll paging I agree with @peterh, think the best choice is to call $PAGER on UNIX-like systems and more on windows if they are set/exist and otherwise let the terminal manage the scroll. Not only because it is easier to maintain(which is a nice bonus) but because it puts the user in control, if they decided to change $PAGER to another program or remove it completely for whatever reason, then it's their choice. Those environmental variables exist just for this cases, to let the user or sysadmin take control over this cases.

[abates]

@ElPincheTopo So if $PAGER is not set, dump everything to the screen? That's definitely not the expected behavior with BASH. Also, if we're going to do one thing for Unix-like systems and something different for Windows systems then I think the implementation is actually more complicated than a simple --more-- capability. Executing the contents of $PAGER should be easy and OS agnostic, but we should still fall back to a basic --more--

[peterh]

@ElPincheTopo @abates No, I definitely don't want to do anything different based on the OS.

If you want a fallback, we can exec.LookPath for $PAGER, less, and more, in that order, and only dump everything at once if none of the three are found. I really don't want to implement my own "more".

[peterh]

Pushed. Thanks!

Next time, could you please format your commit messages as a single line summary, a blank line, and then the paragraph describing the change? It makes gitk (and git shortlog, and a few other git tools) produce nicer looking output.

re: Performance of calculating number of rows on extremely long lists. The only thing that's O(n) is finding the longest item. In my tests, I ran out of RAM building the item list before I could make finding the longest item (and therefore calculating the number of rows) take more than 1s. If the huge list is coming from a directory tree, it's going to take significantly longer to build than the time it takes you to calculate the number of rows and columns. Even if the huge list is coming from

list := make([]string, 0, bigNum)
for i := 0; i < bigNum; i++ {
    list = append(list, strconv.Itoa(i))
}

it still takes approximately 65x as long (on my machine) to build the list as to calculate the number of rows and columns. So it appears that if a huge list has a performance problem, it was probably a problem long before Completer returned the list to us.

(Aside: If you're that concerned about performance, longestCommonPrefix appears to contain low-hanging fruit for optimization.)

[abates]

@peterh Thanks for merging! I will be more diligent with my commit messages in the future. Also, thanks for looking into the performance of calculating the maxWdith. I will submit a new PR for that change as well as a PR for the paging feature/enhancement.

Also, I agree that the longestCommonPrefix is not exactly a high performance lookup, but I went for simple to implement :)