Add support for command-line editing to ddbg.

Add support for command-line editing to ddbg.

This is implemented using two new files, commando.dart and
terminfo.dart.  My hope is to eventually submit commando.dart to pub
for general use once the wrinkles are ironed out.

Commando supports:

- Emacs style editing keys:
  ^A (home)
  ^E (end)
  ^B or left arrow (back)
  ^F or right arrow (forward)
  ^K (kill from cursor)   [^Y not yet implemented]
  ^L (clear screen)
  ^U (clear line)
  backspace

- Command line history
  ^P or up arrow (history previous)
  ^N or down arrow (history next)

- Tab completion
  single-TAB (complete current command)
  double-TAB (list all completions)

I have not yet completed reasonable command line completion for ddbg.
Currently only first part of command is completed.

I have only tested this on Mac.  It may work on linux and definitely
will not work on Windows.  I need to implement a fallback mode for
windows.  I am leaving this to another cl, as the command line
debugger, as I believe that the command line debugger is not widely
used yet.

R=hausner@google.com

Committed: https://code.google.com/p/dart/source/detail?r=30585

[turnidge, 2013-11-20 18:06:31]

Hi Matthias,

Here are the changes to add the command line editing to ddbg.  Let me know if
you would like to see a demo or discuss any of the features.

Todd

[hausner, 2013-11-20 23:30:41]

I can't see anything for the moved file tools/ddbg/bin/ddbg.dart.  Also, in
order to make the changes in this file visible (as opposed to all green lines
for a new file) would it make sense to leave ddbg.dart in the tools directory,
and then once the command line changes are in, move the file in a separate
changelist?

[hausner, 2013-11-21 16:12:03]

Couple of comments on TermCap. Will look at the command line file next.

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
File tools/ddbg/lib/commando.dart (right):

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:1: // Copyright (c) 2012, the Dart project authors.
 Please see the AUTHORS file
2013

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/terminfo....
File tools/ddbg/lib/terminfo.dart (right):

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/terminfo....
tools/ddbg/lib/terminfo.dart:1: // Copyright (c) 2012, the Dart project authors.
 Please see the AUTHORS file
2013

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/terminfo....
tools/ddbg/lib/terminfo.dart:41: String cursorBack = _tputGetSequence('cub1');
Any reason why you make lines and cols a readonly getter, but these properties
can potentially be overwritten?

How about final String cursorBack = ....;

[turnidge, 2013-11-21 18:25:10]

Reuploaded the CL.  Please try again to see if this fixes the diff of ddbg.dart
for you.

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
File tools/ddbg/lib/commando.dart (right):

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:1: // Copyright (c) 2012, the Dart project authors.
 Please see the AUTHORS file
On 2013/11/21 16:12:03, hausner wrote:
> 2013

Done.

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/terminfo....
File tools/ddbg/lib/terminfo.dart (right):

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/terminfo....
tools/ddbg/lib/terminfo.dart:1: // Copyright (c) 2012, the Dart project authors.
 Please see the AUTHORS file
On 2013/11/21 16:12:03, hausner wrote:
> 2013

Done.

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/terminfo....
tools/ddbg/lib/terminfo.dart:41: String cursorBack = _tputGetSequence('cub1');
On 2013/11/21 16:12:03, hausner wrote:
> Any reason why you make lines and cols a readonly getter, but these properties
> can potentially be overwritten?
> 
> How about final String cursorBack = ....;

Done.

[hausner, 2013-11-21 19:24:56]

A few comments, but feel free to ignore  them. I mostly ask to spark a
discussion or learn how to use Dart effectively. We can discuss offline.

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
File tools/ddbg/lib/commando.dart (right):

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:16: static const int runeCtrlA   = 0x01;
I find this to be quite verbose to read. How about leaving the int out? It does
not buy you anything.

static const runeXXX = 0xNN;

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:36: {this.prompt:'> ', this.completer:null}) {
Should there be spaces around :

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:52: var runes = text.runes.toList();
I'm learning as I read your code and the api docs.

the runes property returns an Iterable. (Why, I'm not sure, since codeUnits
returns a List.) You could use the iterable in the loop, like

for (var rune in text.runes) {
...
}

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:63: var char = runes[pos];
This would now be the rune variable from above.

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:72: if (_isControlCharacter(char)) {
isControlRune() ? (maybe rather not, unless we rename char to Hieroglyph

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:289: return new
String.fromCharCodes(line.runes.skipWhile(_isSpace));
Is this really correct? You are feeding runes into a method that expects char
codes. This will fail if there is a rune that consists of more than once char
code, no?

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:414: _replaceHistory(_currentLine, _linePos);
So does this mean that if at the prompt I hit up arrow twice, delete the last
character in that line, and then up arrow again, I modify the history? If so,
why is this behavior desirable?

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:613: int _screenWidth;
This gets initialized once only in the constructor. How does the Commando object
know when the terminal resizes? It if doesn't and this is a value that never
changes, why not make it final?

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:616: List<List<int>> _lines = [];
I would find a short description helpful that explains how these properties are
used, e.g. that _lines contains the history of previous commands, _linePos
indicates the position in the history when navigating though it, etc.

[hausner, 2013-11-21 22:30:01]

Last set of comments.

LGTM, nice improvement!

https://codereview.chromium.org/69343017/diff/310001/tools/ddbg/bin/ddbg.dart
File tools/ddbg/bin/ddbg.dart (right):

https://codereview.chromium.org/69343017/diff/310001/tools/ddbg/bin/ddbg.dart...
tools/ddbg/bin/ddbg.dart:83: typedef void HandlerType(response);
Would help to have a type on the parameter. Is it Map?

https://codereview.chromium.org/69343017/diff/310001/tools/ddbg/bin/ddbg.dart...
tools/ddbg/bin/ddbg.dart:114:
sendCmd(cmd).then(hidePrompt(handleGenericResponse));
I find it confusing that the "hidePrompt" function is a parameter to the closure
that is supposed to be executed when the future completes. It's difficult enough
to follow the control flow when futures are used. This makes it even harder I
think. Why not

cmdo.hide();
sendCmd(cmd).then(showPrompt(handleGenericResponse));

Or even put the cmdo.show() into the handleXXXX functions?

https://codereview.chromium.org/69343017/diff/310001/tools/ddbg/bin/ddbg.dart...
tools/ddbg/bin/ddbg.dart:387: void handleGetIsolatesResponse(response) {
Remove the void annotation?

https://codereview.chromium.org/69343017/diff/310001/tools/ddbg/bin/ddbg.dart...
tools/ddbg/bin/ddbg.dart:673: completer:debuggerCommandCompleter);
Spaces around :

[turnidge, 2013-11-22 19:59:57]

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
File tools/ddbg/lib/commando.dart (right):

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:16: static const int runeCtrlA   = 0x01;
On 2013/11/21 19:24:56, hausner wrote:
> I find this to be quite verbose to read. How about leaving the int out? It
does
> not buy you anything.
> 
> static const runeXXX = 0xNN;

Done.

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:36: {this.prompt:'> ', this.completer:null}) {
On 2013/11/21 19:24:56, hausner wrote:
> Should there be spaces around : 

Seems reasonable.  Done.

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:52: var runes = text.runes.toList();
On 2013/11/21 19:24:56, hausner wrote:
> I'm learning as I read your code and the api docs.
> 
> the runes property returns an Iterable. (Why, I'm not sure, since codeUnits
> returns a List.) You could use the iterable in the loop, like
> 
> for (var rune in text.runes) {
> ...
> }
> 

I pass the List into handleRegularSequence/handleControlSequence, which need a
little bit of lookahead and return the number of characters consumed.  Using a
list made this a bit easier for me but I'm not opposed to changing this if there
is some straightforward way.

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:72: if (_isControlCharacter(char)) {
On 2013/11/21 19:24:56, hausner wrote:
> isControlRune() ? (maybe rather not, unless we rename char to Hieroglyph

Why not?  Changed.

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:289: return new
String.fromCharCodes(line.runes.skipWhile(_isSpace));
On 2013/11/21 19:24:56, hausner wrote:
> Is this really correct? You are feeding runes into a method that expects char
> codes. This will fail if there is a rune that consists of more than once char
> code, no?

I do not know if this is correct or not. :-/

Added a TODO to investigate.  Let me know if this is insufficient.

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:414: _replaceHistory(_currentLine, _linePos);
On 2013/11/21 19:24:56, hausner wrote:
> So does this mean that if at the prompt I hit up arrow twice, delete the last
> character in that line, and then up arrow again, I modify the history? If so,
> why is this behavior desirable?

Isn't that weird?!  That's what gdb and bash do :-).  Really!  I wanted to
closely emulate the behavior that people would be used to.

Here's how it might be useful:

I go up 3 lines in my history and begin editing.
I realize I need something from another line.
I go down one line and look at the other line to get some info
I go back up one line and complete the line which is in progress.

In some sense, conceptually, the command line history is like a line editor
viewing a file.  Hitting return has the special meaning of adding a line to the
file.  I am sure it is all consistent on some plane I will eventually reach
after vast deliberation and self-improvement.

BUT, if you think the behavior is just lousy, we can change it.

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:613: int _screenWidth;
On 2013/11/21 19:24:56, hausner wrote:
> This gets initialized once only in the constructor. How does the Commando
object
> know when the terminal resizes? It if doesn't and this is a value that never
> changes, why not make it final?

Added a TODO.

My intention was to update this when the user clears the screen with ^L. 
Ideally, I would also like to get window change events (SIGWINCH?) but I don't
know how to do that in dart yet.

https://codereview.chromium.org/69343017/diff/150001/tools/ddbg/lib/commando....
tools/ddbg/lib/commando.dart:616: List<List<int>> _lines = [];
On 2013/11/21 19:24:56, hausner wrote:
> I would find a short description helpful that explains how these properties
are
> used, e.g. that _lines contains the history of previous commands, _linePos
> indicates the position in the history when navigating though it, etc.

Done.

https://codereview.chromium.org/69343017/diff/310001/tools/ddbg/bin/ddbg.dart
File tools/ddbg/bin/ddbg.dart (right):

https://codereview.chromium.org/69343017/diff/310001/tools/ddbg/bin/ddbg.dart...
tools/ddbg/bin/ddbg.dart:83: typedef void HandlerType(response);
On 2013/11/21 22:30:02, hausner wrote:
> Would help to have a type on the parameter. Is it Map?

The actual handlers did not have types, so I was copying them.  Will go add
types to the response params below...

https://codereview.chromium.org/69343017/diff/310001/tools/ddbg/bin/ddbg.dart...
tools/ddbg/bin/ddbg.dart:114:
sendCmd(cmd).then(hidePrompt(handleGenericResponse));
On 2013/11/21 22:30:02, hausner wrote:
> I find it confusing that the "hidePrompt" function is a parameter to the
closure
> that is supposed to be executed when the future completes. It's difficult
enough
> to follow the control flow when futures are used. This makes it even harder I
> think. Why not
> 
> cmdo.hide();
> sendCmd(cmd).then(showPrompt(handleGenericResponse));
> 
> Or even put the cmdo.show() into the handleXXXX functions?
>

Yeah, I didn't think this was exactly perfect either.

For now I went with your first suggestion -- cmdo.hide() explicitly.  I actually
renamed hidePrompt to "showPromptAfter" which reads a little better.

Longer term:

To make command completion work nicely, I would like to make command
dispatch/completion/help driven by data (basically a map from command to
dispatchers/handlers/completers/helpers).  Once this rewrite is done, I
anticipate that the hide/show will happen in just one place, which can be
well-documented.

https://codereview.chromium.org/69343017/diff/310001/tools/ddbg/bin/ddbg.dart...
tools/ddbg/bin/ddbg.dart:387: void handleGetIsolatesResponse(response) {
On 2013/11/21 22:30:02, hausner wrote:
> Remove the void annotation?

I wanted it to be a match for HandlerType, above.

Also, it makes it consistent with e.g. handleGetLibraryResponse.

https://codereview.chromium.org/69343017/diff/310001/tools/ddbg/bin/ddbg.dart...
tools/ddbg/bin/ddbg.dart:673: completer:debuggerCommandCompleter);
On 2013/11/21 22:30:02, hausner wrote:
> Spaces around :

Done.

[turnidge, 2013-11-22 20:59:04]

Committed patchset #15 manually as r30585 (presubmit successful).

[turnidge, 2013-11-22 21:00:13]

Unbranched ddbg.dart to make it easier for me to handle the conflict in the
file.  Will move it later as you suggested.

Committing based on your LGTM.  If you have edits to suggest will happily add
them in a follow-up.

Thanks for the thorough review.

[hausner, 2013-11-22 21:14:52]

Still LGTM.