Improved User Experience for History

[rubberduck203]

This PR changes the way clearing the current line works.
Instead of backspacing, it clears the line in one shot.
This speeds up the interface and works more like a typical terminal experience.

[rubberduck203]

Looks like I'm running into this issue.
https://github.com/dotnet/cli/issues/2780

Appveyor looks to be running a slightly older version than I am. (v2.1.101).

Works find on my Mac running 2.1.4.
Might have to wait until Appveyor catches up.

.NET Command Line Tools (2.1.4)

Product Information:
 Version:            2.1.4
 Commit SHA-1 hash:  5e8add2190

Runtime Environment:
 OS Name:     Mac OS X
 OS Version:  10.12
 OS Platform: Darwin
 RID:         osx.10.12-x64
 Base Path:   /usr/local/share/dotnet/sdk/2.1.4/

Microsoft .NET Core Shared Framework Host

  Version  : 2.0.5
  Build    : 17373eb129b3b05aa18ece963f8795d65ef8ea54

I also verified that it works on Travis.

https://travis-ci.org/rubberduck203/readline/jobs/372681532

[tonerdo]

@rubberduck203 can you please look into the test failures?

[rubberduck203]

It looks to me like Appveyor’s getting a stale commit somehow. I removed the code that was causing the failure, but the Debug build is failing. I’ll take another look later today.

[rubberduck203]

There we go. Super green.
Is there any chance of merging this and publishing an updated package soon?
I have a project pinned to my forked version at the moment and would like to move it back to the mainline package.

[Latency]

Looks okay to me now as long as its tested.

---

GNU Readline has the following:

• C-b
  Move back one character.
• C-f
  Move forward one character.
• DEL or Backspace
  Delete the character to the left of the cursor.
• C-d
  Delete the character underneath the cursor.
• Printing characters
  Insert the character into the line at the cursor.
• C-_ or C-x C-u
  Undo the last editing command. You can undo all the way back to an empty line.

[rubberduck203]

All good except C-d, but I checked out the tip of your master branch and I'm seeing the same problem.
It doesn't delete the character. It just shifts the cursor forward one character. Same behavior as C-f.

[Latency]

That is a bit more difficult to do without termios esc-seqs in Windows.

Can you try something like this:

1. Obtain current cursor position.

#include <windows.h> 

void gotoxy(int x, int y)
{
  COORD coord;
  coord.X = x;
  coord.Y = y;
  SetConsoleCursorPosition(GetStdHandle(STD_OUTPUT_HANDLE), coord);
}

2. Move to current position Y + 1 and add a space.

   gotoxy(x, y+1);
   Console.Write(" ");
   gotoxy(x-1, y);

[rubberduck203]

Are you asking me to fix a bug that also exists on master? I’m not following what you want me to do.

[Latency]

Since you are in there testing stuff out, could you let us know what needs fixed so we can patch it?

You had mentioned "All good except C-d," If that hotkey is broken, it is worth investigating.

[rubberduck203]

Sure. I should have a few minutes sometimes soon. I’ll open an issue about it later so we can track that separately.

[rubberduck203]

I just discovered a bug in this implementation.
It crashes with an index out of range if the text exceeds the console buffer width.
Will work on a fix.

[Latency]

You may want to check the width of the terminal and compare that against your startColumn. I do know, that xterm/termios will wrap and typical is x_max=80 for console. This could be a possible cause, hence why I prefered the original method via the loop iterator to stride over the width as the preferred method of impl.

Granted!! It does appear suspect and needs attention.

[rubberduck203]

Yeah. You can get the correct information via the following formulas.

CursorLeft = totalTextLength % BufferWidth
rowInText = floor(totalTextLength / BufferWidth)
CursorTop = currentTop - rowInText

Or something close to that anyway.

The problem is that the total text length must include the length of the prompt text, which is currently unavailable to the KeyHandler. I spent yesterday trying to figure out how to calculate the prompt length, but may need to just pass the prompt text to the handler.

[Latency]

You can get the prompt environment variable from:

#if POSIX
 system( "\`env\` | grep PROMPT" );  //  [e.g. if it exists] and then do a <string>.Length comparison to evaluate.
#endif 

For non-POSIX based OS's, you may want to look at the equivalent for that. However, since we are deploying with .NET Core/STD... this should be sufficient. E.g. 'return $CWD.Length'

    internal static int PromptLength() {
      var process = new Process
      {
        StartInfo = new ProcessStartInfo("cmd")
        {
          UseShellExecute = false,
          RedirectStandardInput = true,
          RedirectStandardOutput = true,
          CreateNoWindow = true,
          Arguments = "/k",
        }
      };

      process.Start();

      try {
        process.StandardInput.WriteLine();
        process.StandardInput.Flush();
        var str = process.StandardOutput.ReadLine();
        return str?.Length ?? -1;
      } finally {
        process.Kill();
      }
    }

[Latency]

Revert changes to KeyHandlerTests.cs.

Reason:
Redundancy... See jump-table macros for ConsoleKeyInfo.

[rubberduck203]

No. It’s not a redundant test. During development I had all green tests, but still managed to cause an invalid access to an array. This is the test I wrote to cover the case.

[Latency]

Have you compared it against 'master'?

MoveCursorThenPreviousHistory() is already defined in the unit tests. It uses the method group as its parameter for Handle(...) which reuses the definitions already defined essentially being the equivalent to your explicit implementation.

E.g. LeftArrow, UpArrow

        [Fact]
        public void MoveCursorThenPreviousHistory()
        {
            _keyHandler.Handle(LeftArrow);
            _keyHandler.Handle(UpArrow);

            Assert.Equal("clear", _keyHandler.Text);
        }

[Latency]

I am not seeing anything wrong with what Toni merged from you 2 days ago in PR#40. Perhaps, you refined it since. This PR#39 shows different than what I had merged shortly after he did and the file I mentioned is stale.

I believe we are on the same page. The code above should be the final draft.

What I am not understanding is why the PR remains open if it has already been pulled from you and merged into master. My only guess is he CAN'T because the last file I am mentioning conflicts.

Toni likes to cherry pick stuff in his branch and really confuses the tracking on these PR's. He did it with my new VS2017 X-Platform build configurations 6 months ago and I was the one that wrote all that code hence my PR was never merged similarly. Go figure!!!

I will send him an email to try and get this sorted out.

[rubberduck203]

I’m confused. Which branch? I don’t see this on master. It’s in my master that I’ve been cherry-picking from for these PRs. I’ve had to cherry pick because I’m publishing an alternative nuget package based on my tree so I can use these changes in another project.

[Latency]

'Master' branch.

I don't have access to merge into 'master' but I pulled yours into mine from your PR-#43.

I am almost positive Tony can not merge this PR-#39 due to conflicts (e.g. reverting back to stale code).

Remove the changes for the last 2 files or he will be cherry picking the code for the 1st file into his build (thus losing the tracking and credit for your efforts), along with closing this PR.

This is what I am going to have to be doing as well since it is just 1 method a dozen lines long.

It would appear that your unit tests need merged from PR-#43? I cleaned up your branch and merged them into mine.
Just download the files or merge from my branch (for the 'tests' folder only) and overwrite in yours since you already have the request in.. and it reflects on your PR#-43.

I am positive Toni will reflect changing the versioning on things once he publishes the .nupkg.

Typically, from my experience, you do not mess with the versioning unless you have access to the master branch.