568649 - JSTerm keybindings: tab-completion

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[David Dahl :ddahl]

Implement tab-completion in the JSTerm input node to be used in each HeadsUpDisplay or JS Workspace

Comment 1

[David Dahl :ddahl]

The keydown handler is here:

http://hg.mozilla.org/users/ddahl_mozilla.com/heads-up-display/file/67ee7a904f36/toolkit/components/console/hudservice/HUDService.jsm#l2251

The JSTerm prototype starts here:

http://hg.mozilla.org/users/ddahl_mozilla.com/heads-up-display/file/67ee7a904f36/toolkit/components/console/hudservice/HUDService.jsm#l2160

"JSTerm" is the class used to create either a "terminal-like" input node to use with any "output node", or an entire "JS Workspace"

Some example tab-completion code is inside of Jesse Ruderman's JSShell: http://www.squarefree.com/shell/

Comment 2

[Julian Viereck]

Attachment: Work so far

This is a first iteration of completion. Things to try:

1. Open the console
2. start typing 'doc'
3. it should complete to 'document', where the text is selected from 'doc' to the end.
4. pressing <tab> will put the cursor at the end of the completion in this case. If there were more possible completions, then tab would iterate through them and shift_tab backwards iterate through them

Notes:
a) I've wrote some tests for completion (that passes) and some tests of the completion algo (that fails - the function JSPropertyProvider is not available within the testsuite. I guess that's trivial if you know about the testsuite things, but I couldn't figure it out.. Any idea?).
b) the matching algo is *VERY* simple at the moment. I've added some comments on the JSPropertyProvider function, how this could get improved.

Comment 3

[Julian Viereck]

Attachment: Fix the completion to complete `document . get` but not `document . ge `

Comment 4

[David Dahl :ddahl]

Comment on attachment 453689 [details] [diff] [review]
Work so far


> //////////////////////////////////////////////////////////////////////////
>+// JS Completer
>+//////////////////////////////////////////////////////////////////////////
>+
>+/**
>+ * Provides a list of properties, that are possible matches based on the passed
>+ * scope and inputValue.
>+ * TODO: This function is really simple. The provider algo could do more:
>+ *          - look into arrays like a[0].something
>+ *          - retrun the properties of the prototype as well (not possible
>+ *            at the moment as Object.getOwnPropertyNames is not implemented
>+ *            yet, see bug 518663).
>+ *          - parse 'from the back': document.getElementById(a.a<completion>
>+ *          - think about using narcissus for parsing and partly executing
>+ *            the input string. Problem: narcissus can only deal valid JS.
>+ *            If the input is `a[b[1].p` narcissus is going to fail because
>+ *            the first bracket is not closed. To use narcissus, the input
>+ *            has to be scaned for valid parts, that can get passed to
>+ *            narcissus. In this case, `b[1]` alone is valid.
>+ */

Since you are not using narcissus at this time, and who knows if it will be useful, you might want to add that comment to the bug to get a conversation going about it. (TODO comment is otherwise ok.)

>+  let ret = [];
>+  for (prop in obj) {
>+    ret.push(prop);
>+  }
>+
>+  ret = ret.filter(function(item) {
>+    return item.indexOf(matchProp) == 0;
>+  }).sort();
>+
>+  return {
>+    matchProp: matchProp,
>+    matches: ret


please use a more verbose variable name for the "ret" array

>+//////////////////////////////////////////////////////////////////////////
> // JSTerm
> //////////////////////////////////////////////////////////////////////////
> 
> /**
>  * JSTerm
>  *
>  * JavaScript Terminal: creates input nodes for console code interpretation
>  * and 'JS Workspaces'
>@@ -2340,21 +2396,21 @@ JSTerm.prototype = {
>       var target = aEvent.target;
>       var tmp;
> 
>       if (aEvent.ctrlKey) {
>         switch (aEvent.charCode) {
>           case 97:
>             // control-a
>             tmp = self.codeInputString;
>-              setTimeout(function() {
>-                self.inputNode.value = tmp;
>-                self.inputNode.setSelectionRange(0, 0);
>-              },0);
>-              break;
>+            setTimeout(function() {
>+              self.inputNode.value = tmp;
>+              self.inputNode.setSelectionRange(0, 0);
>+            },0);
>+            break;

indention changes only? 

>@@ -2384,33 +2440,50 @@ JSTerm.prototype = {
>           case 40:
>             // down arrow: history next
>             if (self.caretInLastLine()){
>               self.historyPeruse(false);
>             }
>             break;
>           case 9:
>             // tab key
>-            // TODO: this.tabComplete();
>-            // see bug 568649
>+            self.complete(aEvent.shiftKey);

why is the value of the shift key passed in?

>             var bool = aEvent.cancelable;
>             if (bool) {
>               aEvent.preventDefault();
>             }
>             else {
>               // noop
>             }
>             aEvent.target.focus();
>             break;
>+          case 8:
>+            // backspace key
>+            // necessary so that default is not reached.
>+            break;
>           default:
>+            // all not handled keys
>+            // Store the current inputNode value. If the value is the same
>+            // after keyDown event was handled (after 0ms) then the user
>+            // moved the cursor. If the value changed, then call the complete
>+            // function to show completion on new value.
>+            var value = self.inputNode.value;
>+            let setTimeout = aEvent.target.ownerDocument.defaultView.setTimeout;

we should probably make setTimeout a property of the parent object (self.setTimeout = ... at the beginning of the method where aEvent is passed in.)

>+            setTimeout(function() {
>+              if (self.inputNode.value !== value) {
>+                self.complete(false, true /** showHint **/);

Move this comment "showHint" above the line and be more verbose

>@@ -2454,19 +2527,104 @@ JSTerm.prototype = {
>   caretInLastLine: function JSTF_caretInLastLine()
>   {
>     var lastLineBreak = this.codeInputString.lastIndexOf("\n");
>     return (this.inputNode.selectionEnd > lastLineBreak);
>   },
> 
>   history: [],
> 
>-  tabComplete: function JSTF_tabComplete(aInputValue) {
>-    // parse input value:
>-    // TODO: see bug 568649
>+  /** Stores the data for the last completion. **/
nit: use "//" comment style

>+  lastCompletion: null,
>+
>+  /**
>+   * Completes the current typed text in the inputNode. Completion is only
>+   * performed, when the selection/cursor is at the end of the string.
>+   *
>+   * @param boolean backwards: Iterate the matches forward or backwards.
>+   * @param boolean showHint: If true, show the first matching only and do
>+   *                          not move the cursor at the end of the completed
>+   *                          inputValue if there is only one match.
>+   */
returns? 

>+  complete: function JSTF_complete(backwards, showHint) {
start method open braces on new line

>+    let inputNode = this.inputNode;
>+    let inputValue = inputNode.value;
>+    let selStart = inputNode.selectionStart, selEnd = inputNode.selectionEnd;
>+
>+    // 'Normalize' the selection so that end is always after start.
>+    if (selStart > selEnd) {
>+      let t = selStart;
use a more verbose variable name (t) when not a counter

>+      selStart = selEnd;
>+      selEnd = t;
>+    }
where is "t" used?

>+    // Only complete if the selection is at the end of the input.
>+    if (selEnd != inputValue.length) {
>+      this.lastCompletion = null;
>+      return;
>+    }
>+
>+    // Remove the selected text from the inputValue.
>+    inputValue = inputValue.substring(0, selStart);
>+
>+    let matches;
>+    let matchIndexToUse;
>+    let matchOffset;
>+    let completionStr;
>+
>+    // If there is a saved completion from last time and the used value for
>+    // completion stayed the same, then use the stored completion.
>+    if (this.lastCompletion && inputValue == this.lastCompletion.value) {
>+      matches = this.lastCompletion.matches;
>+      matchOffset = this.lastCompletion.matchOffset;
>+      if (backwards) {
>+        this.lastCompletion.index --;
>+      } else {
nit: else on newline

>+        this.lastCompletion.index ++;
>+      }
>+      matchIndexToUse = this.lastCompletion.index;
>+    } else {
nit: else on newline

>+      // Look up possible completion values.
>+      let completion = JSPropertyProvider(this.sandbox.window, inputValue);
>+      if (!completion) {
>+        return;
>+      }
>+      matches = completion.matches;
>+      matchIndexToUse = 0;
>+      matchOffset = completion.matchProp.length
>+      // Store this match;
>+      this.lastCompletion = {
>+        index: 0,
>+        value: inputValue,
>+        matches: matches,
>+        matchOffset: matchOffset
>+      };
>+    }
>+
>+    if (matches.length != 0) {
>+      // Ensure that the matchIndexToUse is always a valid array index.
>+      if (matchIndexToUse < 0) {
>+        matchIndexToUse = matches.length + (matchIndexToUse % matches.length);
>+        if (matchIndexToUse == matches.length) {
>+          matchIndexToUse = 0;
>+        }
>+      } else {
else on new line

>+        matchIndexToUse = matchIndexToUse % matches.length;
>+      }
>+
>+      completionStr = matches[matchIndexToUse].substring(matchOffset);
>+      this.inputNode.value = inputValue +  completionStr;
>+
>+      selEnd = inputValue.length + completionStr.length;
>+      if (matches.length == 1 && !showHint) {
>+        inputNode.setSelectionRange(selEnd, selEnd);
>+      } else {
else on new line

>+        inputNode.setSelectionRange(selStart, selEnd);
>+      }
>+    }
>   }
> };
> 
> /**
>  * JSTermFirefoxMixin
>  *
>  * JavaScript Terminal Firefox Mixin
>  *
>diff --git a/toolkit/components/console/hudservice/tests/browser/Makefile.in b/toolkit/components/console/hudservice/tests/browser/Makefile.in
>--- a/toolkit/components/console/hudservice/tests/browser/Makefile.in
>+++ b/toolkit/components/console/hudservice/tests/browser/Makefile.in
>@@ -50,15 +50,16 @@ _BROWSER_TEST_FILES = \
> _BROWSER_TEST_PAGES = \
> 	test-console.html \
> 	test-network.html \
> 	test-mutation.html \
> 	testscript.js \
> 	test-filter.html \
> 	test-observe-http-ajax.html \
> 	test-data.json \
>+	test-property-provider.html \
> 	$(NULL)
> 
> libs:: $(_BROWSER_TEST_FILES)
> 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/browser/$(relativesrcdir)
> 
> libs:: $(_BROWSER_TEST_PAGES)
> 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/browser/$(relativesrcdir)
>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js b/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js
>--- a/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js
>+++ b/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js
>@@ -46,30 +46,42 @@ XPCOMUtils.defineLazyGetter(this, "HUDSe
>   try {
>     return HUDService;
>   }
>   catch (ex) {
>     dump(ex + "\n");
>   }
> });
> 
>+XPCOMUtils.defineLazyGetter(this, "JSPropertyProvider", function () {
>+  Cu.import("resource://gre/modules/HUDService.jsm");
>+  try {
>+    return JSPropertyProvider;
>+  }
>+  catch (ex) {
>+    dump(ex + "\n");
>+  }
>+});
you can take the JSPropertyyProvider and add it as a property on jsterm itself, then you can use it like hud.jsterm.jsPropertyProvider and not need this lazy getter

>+// test property provider
>+function testPropertyProvider() {
>+  content.location = TEST_PROPERTY_PROVIDER_URI;
>+  executeSoon(function() {
>+    // This one FAILS!
>+    // Why is JSPropertyProvider undefined in this context?
above comment should clear this problem up.

>+    ok (JSPropertyProvider !== undefined, "JSPropertyProvider is defined");
>+
>+    ok(JSPropertyProvider(content, "thisIsNotDefined") == null,
>+      "Looked up matches for thisIsNotDefined");
>+
>+    let completion = JSPropertyProvider(context, 'testObj . test');
>+    ok(completion.matches.length == 1, "one match for testObj . test");
>+    ok(completion.matches[0] == "testProp", "the match is 'testProp'");
>+    ok(completion.matchProp == "test", "matching part is 'test'");
>+  });

looks good so far. Works well. I still need to play with it a bit more.

Comment 5

[David Dahl :ddahl]

Comment on attachment 453859 [details] [diff] [review]
Fix the completion to complete `document . get` but not `document . ge `

># HG changeset patch
># Parent 32f29e9826fab246afeb908d032ad76f5759e130
>
>Fix the completion to complete `document . get` but not `document . ge `.
>
>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
>--- a/toolkit/components/console/hudservice/HUDService.jsm
>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>@@ -2213,29 +2213,29 @@ function NodeFactory(aFactoryType, aName
>  *            narcissus. In this case, `b[1]` alone is valid.
>  */
> function JSPropertyProvider(aScope, aInputValue) {
>   let obj = aScope;
> 
>   let properties = aInputValue.split('.');
>   let matchProp;
>   if (properties.length > 1) {
>-      matchProp = properties[properties.length - 1].trim();
>+      matchProp = properties[properties.length - 1].trimLeft();
>       properties.pop();
>       for each (let prop in properties) {
>         prop = prop.trim();
>         // Check if prop is a getter function on obj. Functions can change other
>         // stuff so we can't execute them to get the next object. Stop here.
>         if (obj.__lookupGetter__(prop)) {
>           return null;
>         }
>         obj = obj[prop];
>       }
>   } else {
>-      matchProp = aInputValue.trim();
>+      matchProp = aInputValue.trimLeft();
>   }
> 
>   let ret = [];
>   for (prop in obj) {
>     ret.push(prop);
>   }
> 
>   ret = ret.filter(function(item) {
>@@ -2532,17 +2532,17 @@ JSTerm.prototype = {
> 
>   history: [],
> 
>   /** Stores the data for the last completion. **/
>   lastCompletion: null,
> 
>   /**
>    * Completes the current typed text in the inputNode. Completion is only
>-   * performed, when the selection/cursor is at the end of the string.
>+   * performed, if the selection/cursor is at the end of the string.
>    *
>    * @param boolean backwards: Iterate the matches forward or backwards.
>    * @param boolean showHint: If true, show the first matching only and do
>    *                          not move the cursor at the end of the completed
>    *                          inputValue if there is only one match.
>    */
>   complete: function JSTF_complete(backwards, showHint) {
>     let inputNode = this.inputNode;
>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js b/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js
>--- a/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js
>+++ b/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js
>@@ -306,16 +306,19 @@ function testPropertyProvider() {
> 
>     ok(JSPropertyProvider(content, "thisIsNotDefined") == null,
>       "Looked up matches for thisIsNotDefined");
> 
>     let completion = JSPropertyProvider(context, 'testObj . test');
>     ok(completion.matches.length == 1, "one match for testObj . test");
>     ok(completion.matches[0] == "testProp", "the match is 'testProp'");
>     ok(completion.matchProp == "test", "matching part is 'test'");
>+
>+    let completion = JSPropertyProvider(context, 'testObj . te ');
>+    ok(completion.matches.length == 0, "zero match for 'testObj . te'");
>   });
> }
> 
> function testCompletion() {
>   var HUD = HUDService.hudWeakReferences[hudId].get();
>   var jsterm = HUD.jsterm;
>   var input = jsterm.inputNode;
> 


Not going to review this yet, please combine with the next iteration of first patch.

Comment 6

[David Dahl :ddahl]

Oh! btw: your lazy getter does not work because you have not added it to the EXPORTED_SYMBOLS array in HUDService.jsm. No worries though, just expose it as a property on JSTerm's prototype.

Comment 7

[Julian Viereck]

Attachment: Folded patch, improved based on feedback, a few more comments

This is a folded patch of the patches provided in 453689 and 453859. It also included some changes based on the feedback from David on 453689.

Comment 8

[Julian Viereck]

(In reply to comment #4)
> (From update of attachment 453689 [details] [diff] [review])
> 
> > //////////////////////////////////////////////////////////////////////////
> >+// JS Completer
> >+//////////////////////////////////////////////////////////////////////////
> >+
> >+/**
> >+ * Provides a list of properties, that are possible matches based on the passed
> >+ * scope and inputValue.
> >+ * TODO: This function is really simple. The provider algo could do more:
> >+ *          - look into arrays like a[0].something
> >+ *          - retrun the properties of the prototype as well (not possible
> >+ *            at the moment as Object.getOwnPropertyNames is not implemented
> >+ *            yet, see bug 518663).
> >+ *          - parse 'from the back': document.getElementById(a.a<completion>
> >+ *          - think about using narcissus for parsing and partly executing
> >+ *            the input string. Problem: narcissus can only deal valid JS.
> >+ *            If the input is `a[b[1].p` narcissus is going to fail because
> >+ *            the first bracket is not closed. To use narcissus, the input
> >+ *            has to be scaned for valid parts, that can get passed to
> >+ *            narcissus. In this case, `b[1]` alone is valid.
> >+ */
> 
> Since you are not using narcissus at this time, and who knows if it will be
> useful, you might want to add that comment to the bug to get a conversation
> going about it.

Okay, let's do this!

The current completion code can complete only simple cases. It works fine for stuff like

    document.getElement

but it doesn't work on something that is nested

    document.getElementById(document.get

because it just takes a look at the string from the beginning and can't find the property `getElementById(document` of `document` then. To improve the algo, it should scan the string from the back and isolates the statement to complete on. This would be in this case

    document.getElementById( << Ignore this one
    document.get >> here goes the completion

JSTerm has code for this and it shouldn't take too long to get this into the current algo.

Going from there, the algo could be improved by providing not only completion for string with the form <property>.<property>.<someThing> but also do more fancy stuff

    a[0].par<complete>
     (5 + 'test').len<complete>

This requires a more complex analysis of the input string. There might be a chance to use Narcissus to do the job. The problems with using Narcissus are (so far I can see them):

a) it makes stuff more complex + you have to implement Narcissus into the current algo
b) Narcissus has to use a given scope as global scope (the scope of the page we're looking at) - is that possible?
c) Narcissus executes code. While looking for possible completions, it's not allowed to execute code in any form because that could change variables.

Currently I don't think the Narcissus approche should get implemented, because there is enough other stuff to do but it could be a nice enhancement for later.

Comment 9

[David Dahl :ddahl]

I'm going to CC: some JS team people on this as well.

Comment 10

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #8)
> c) Narcissus executes code. While looking for possible completions, it's not
> allowed to execute code in any form because that could change variables.

You'll have to tweak it to do a lookup on every property for a getter just to make sure it's not going to do anything. And because JavaScript is dynamically typed, it also reduces the usefulness tab-completion on input like:

shell = gBrowser.getBrowserAtIndex(i).findChildShell(docshell, uri);

This will have to be broken up into two lines. That's one thing, and people might not find it to be so bad, but when getters are involved, like gBrowser.browsers, I can imagine people getting frustrated because it's (seemingly) inexplicably failing to do tab-completion. You'd probably need to do some sort of styling on the text and a tooltip to indicate why it won't work.

Comment 11

[David Dahl :ddahl]

Comment on attachment 454012 [details] [diff] [review]
Folded patch, improved based on feedback, a few more comments

> //////////////////////////////////////////////////////////////////////////
>+// JS Completer
>+//////////////////////////////////////////////////////////////////////////
>+
>+/**
>+ * Provides a list of properties, that are possible matches based on the passed
>+ * scope and inputValue.
>+ * TODO: This function is really simple. The provider algo could do more:
>+ *          - look into arrays like a[0].something
>+ *          - retrun the properties of the prototype as well (not possible
s/return/retrun/
>+ *            at the moment as Object.getOwnPropertyNames is not implemented
>+ *            yet, see bug 518663).
>+ *          - parse 'from the back': document.getElementById(a.a<completion>
>+ *          - think about using narcissus for parsing and partly executing
>+ *            the input string. Problem: narcissus can only deal valid JS.
s/deal /deal with /
>+ *            If the input is `a[b[1].p` narcissus is going to fail because
>+ *            the first bracket is not closed. To use narcissus, the input
>+ *            has to be scaned for valid parts, that can get passed to
>+ *            narcissus. In this case, `b[1]` alone is valid.
>+ */

I would start a conversation about narcissus in this context - and what you actually want to accomplish with some JS hackers at Moz who may know how to open up some existing trace or jaeger-monkey APIs that can do what you want inside of a JS Sandbox.

Just file a bug in Core::Javascript and cc Jeff Walden or Blake Kaplan (mrbkap)

>+function JSPropertyProvider(aScope, aInputValue)
>+{
>+  let obj = aScope;
>+
>+  let properties = aInputValue.split('.');
>+  let matchProp;
>+  if (properties.length > 1) {
>+      matchProp = properties[properties.length - 1].trimLeft();
>+      properties.pop();
>+      for each (let prop in properties) {
>+        prop = prop.trim();
>+        // Check if prop is a getter function on obj. Functions can change other
>+        // stuff so we can't execute them to get the next object. Stop here.
>+        if (obj.__lookupGetter__(prop)) {
>+          return null;
If you return here do you lose other properties? should you continue instead? or save a string name of the getter?

>+        }
>+        obj = obj[prop];
>+      }
>+  }
>+  else {
>+      matchProp = aInputValue.trimLeft();
>+  }

>@@ -2384,33 +2449,57 @@ JSTerm.prototype = {
>           case 40:
>             // down arrow: history next
>             if (self.caretInLastLine()){
>               self.historyPeruse(false);
>             }
>             break;
>           case 9:
>             // tab key
>-            // TODO: this.tabComplete();
>-            // see bug 568649
>+            // If there are more then one possible completion, pressing tab
s/than/then/

>+  /**
>+   * Completes the current typed text in the inputNode. Completion is performed
>+   * only if the selection/cursor is at the end of the string. If no completion
>+   * is found, the current inputNode value and cursor/selection stay.
>+   *
>+   * @param int type possible values are
>+   *    - this.COMPLETE_FORWARD: If there is more then one possible completion
s/than/then/

>+   *          and the input value stayed the same compared to the last time this
>+   *          function was called, then the next completion of all possible
>+   *          completions is used. If the value changed, then the first possible
>+   *          completion is used and the selection is set from the current
>+   *          cursor position to the end of the completed text.
>+   *          If there is only one possible completion, then this completion
>+   *          value is used and the cursor is put at the end of the completion.
>+   *    - this.COMPLETE_BACKWARD: Same as this.COMPLETE_FORWARD but if the
>+   *          value stayed the same as the last time the function was called,
>+   *          then the previous completion of all possible completions is used.
>+   *    - this.COMPLETE_HINT_ONLY: If there is more then one possible
/s/than/then/

>+      completionStr = matches[matchIndexToUse].substring(matchOffset);
>+      this.inputNode.value = inputValue +  completionStr;
>+
>+      selEnd = inputValue.length + completionStr.length;
>+
>+      // If there is more then one possible completion or the completed part
s/than/then/

> 
>+// test property provider
>+function testPropertyProvider() {
>+  content.location = TEST_PROPERTY_PROVIDER_URI;
>+  executeSoon(function() {
>+    var HUD = HUDService.hudWeakReferences[hudId].get();
>+    var jsterm = HUD.jsterm;
>+
>+    ok (jsterm.propertyProvider !== undefined, "JSPropertyProvider is defined");
>+
>+    ok(jsterm.propertyProvider(content, "thisIsNotDefined") == null,
>+      "Looked up matches for thisIsNotDefined");
>+
>+    let completion = jsterm.propertyProvider(context, 'testObj . test');
>+    ok(completion.matches.length == 1, "one match for testObj . test");
>+    ok(completion.matches[0] == "testProp", "the match is 'testProp'");
>+    ok(completion.matchProp == "test", "matching part is 'test'");
>+
>+    let completion = jsterm.propertyProvider(context, 'testObj . te ');
>+    ok(completion.matches.length == 0, "zero match for 'testObj . test'");
>+  });
>+}
>+
>+function testCompletion()
>+{
>+  var HUD = HUDService.hudWeakReferences[hudId].get();
>+  var jsterm = HUD.jsterm;
>+  var input = jsterm.inputNode;
>+
>+  // Test typing 'docu'.
>+  input.value = "docu";
>+  input.setSelectionRange(4, 4);
>+  jsterm.complete(jsterm.COMPLETE_HINT_ONLY);
>+  is(input.value, "document", "'docu' completion");
>+  is(input.selectionStart, 4, "start selection is alright");
>+  is(input.selectionEnd, 8, "end selection is alright");
>+
>+  // Test typing 'docu' and press tab.
>+  input.value = "docu";
>+  input.setSelectionRange(4, 4);
>+  jsterm.complete(jsterm.COMPLETE_FORWARD);
>+  is(input.value, "document", "'docu' tab completion");
>+  is(input.selectionStart, 8, "start selection is alright");
>+  is(input.selectionEnd, 8, "end selection is alright");
>+
>+  // Test typing 'document.getElem'.
>+  input.value = "document.getElem";
>+  input.setSelectionRange(16, 16);
>+  jsterm.complete(jsterm.COMPLETE_HINT_ONLY);
>+  is(input.value, "document.getElementById", "'document.getElem' completion");
>+  is(input.selectionStart, 16, "start selection is alright");
>+  is(input.selectionEnd, 23, "end selection is alright");
>+
>+  // Test pressing tab another time.
>+  jsterm.complete(jsterm.COMPLETE_FORWARD);
>+  is(input.value, "document.getElementsByClassName", "'document.getElem' another tab completion");
>+  is(input.selectionStart, 16, "start selection is alright");
>+  is(input.selectionEnd, 31, "end selection is alright");
>+
>+  // Test pressing shift_tab.
>+  jsterm.complete(jsterm.COMPLETE_BACKWARD);
>+  is(input.value, "document.getElementById", "'document.getElem' untab completion");
>+  is(input.selectionStart, 16, "start selection is alright");
>+  is(input.selectionEnd, 23, "end selection is alright");
>+}
>+

Seeing a bunch of Gecko warnings:
WARNING: NS_ENSURE_TRUE(count) failed: file /home/ddahl/code/moz/user_repos/HUD/mozilla/editor/libeditor/base/nsSelectionState.cpp, line 252
WARNING: NS_ENSURE_TRUE(bCollapsed) failed: file /home/ddahl/code/moz/user_repos/HUD/mozilla/editor/libeditor/text/nsTextEditRules.cpp, line 1009
WARNING: NS_ENSURE_TRUE(count) failed: file /home/ddahl/code/moz/user_repos/HUD/mozilla/editor/libeditor/base/nsSelectionState.cpp, line 492
WARNING: NS_ENSURE_TRUE(count) failed: file /home/ddahl/code/moz/user_repos/HUD/mozilla/editor/libeditor/base/nsSelectionState.cpp, line 492

I'll cc a platform hacker to help me look at that.

r+, just one question above and the typos and I will check it into the hud repo. This patch will undergo review again by a toolkit peer before it goes into m-c.

Comment 12

[Julian Viereck]

> >+function JSPropertyProvider(aScope, aInputValue)
> >+{
> >+  let obj = aScope;
> >+
> >+  let properties = aInputValue.split('.');
> >+  let matchProp;
> >+  if (properties.length > 1) {
> >+      matchProp = properties[properties.length - 1].trimLeft();
> >+      properties.pop();
> >+      for each (let prop in properties) {
> >+        prop = prop.trim();
> >+        // Check if prop is a getter function on obj. Functions can change other
> >+        // stuff so we can't execute them to get the next object. Stop here.
> >+        if (obj.__lookupGetter__(prop)) {
> >+          return null;
> If you return here do you lose other properties? should you continue instead?
> or save a string name of the getter?

This should be alright. Imagine you have a a string like

   foo.bar.<complete>

The code above does nothing else then setting

    obj = aScope.foo.bar

The properties of obj are used for completion later on (if a property is a getter, that's not a problem).

If foo or bar is a getter function, the lookup has to be exited, because otherwise the foo or bar getter function is executed to get the value of obj. This is not permitted as calling a function could have other side effects. That's why

> >+        if (obj.__lookupGetter__(prop)) {
> >+          return null;

returns at once.

Comment 13

[Julian Viereck]

Attachment: New patch improved based on comment 11

Updated typos as mentioned by David in comment 11.

Comment 14

[David Dahl :ddahl]

(In reply to comment #13)
> Created an attachment (id=454800) [details]
> New patch improved based on comment 11
> 
> Updated typos as mentioned by David in comment 11.

got stuck working a blocker all day, will push this in the morning

Comment 15

[Julian Viereck]

Attachment: Addition: Don't complete when using the delete key.

This is a small addition/behavior fix to 454800. When the user presses delete, completion should not take place. Otherwise if the user presses the delete key in the following situation 4 times

    docu<cursorPosition>ment

this will complete to

    docu<cursorPosition>ment

again.

===

Is this the right workflow? Should I open another bug for this small addition or upload one folded patch of this + the already reviewed patch 454800?

Comment 16

[David Dahl :ddahl]

Comment on attachment 454800 [details] [diff] [review]
New patch improved based on comment 11

Pushed into heads-up-display repo

Comment 17

[David Dahl :ddahl]

we should file a followup bug to implement more completion, forinstance:


things like:
typeof doc <tab>

does not work, which is OK for now.

Comment 18

[David Dahl :ddahl]

(In reply to comment #15)
> Is this the right workflow? Should I open another bug for this small addition
> or upload one folded patch of this + the already reviewed patch 454800?

For now, this is good.

Comment 19

[Julian Viereck]

Attachment: (Hopefully) Final patch

Comment 20

[Julian Viereck]

Attachment: Required patch from ddahls repo

exposes 'jsterm' on the HUD object. This patch is necessary to get the jsterm object when running unit tests (e.g. for tab completion).

Comment 21

[Julian Viereck]

Attachment: (Hopefully2) Final patch

Patch nearly the same as attachment 457364 [details] [diff] [review], but applies on trunk with attachment 457386 [details] [diff] [review] from this bug.

Comment 22

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 457392 [details] [diff] [review]
(Hopefully2) Final patch

+// JS Completer
+//////////////////////////////////////////////////////////////////////////
+
+var STATE_NORMAL = 0,
+  STATE_QUOTE = 2,
+  STATE_DQUOTE = 3;
+
+var OPEN_BODY = '{[('.split('');
+var CLOSE_BODY = '}])'.split('');
+var OPEN_CLOSE_BODY = {
+  '{': '}',
+  '[': ']',
+  '(': ')'
+};

these should be "consts".

for lines like these:
+        else if (c == ';') {
+          start = i + 1;

I might just use start++ or start += 1; if wanted to be really clear. Not really worth changing though, just a personal preference.

+      case STATE_DQUOTE:
+        if (c == '\\') {
+          i ++;
+        }

i++;

...
+/**
+ * Provides a list of properties, that are possible matches based on the passed
+ * scope and inputValue.
+ */
+function JSPropertyProvider(aScope, aInputValue)

You should describe your @params and @returns in your comment here.

+            setTimeout(function() {
+              self.inputNode.value = tmp;
+              self.inputNode.setSelectionRange(0, 0);
+            },0);

I'd include the space between , and 0 on the last line. Or remove it in the default: section to be consistent with the rest of the file.

+   * @return void <-
+   */
+  complete: function JSTF_complete(type)
+  {

should be @returns.

everything else looks good to me!

Comment 23

[Julian Viereck]

Attachment: (Hopefully3) Final patch

Note: Patch depends on bug 579073.

Comment 24

[Dietrich Ayala (:dietrich)]

Comment on attachment 457661 [details] [diff] [review]
(Hopefully3) Final patch

> //////////////////////////////////////////////////////////////////////////
>+// JS Completer
>+//////////////////////////////////////////////////////////////////////////
>+
>+const STATE_NORMAL = 0,
>+  STATE_QUOTE = 2,
>+  STATE_DQUOTE = 3;

line em up please

>+/**
>+ * Analyses a given string to find the last statement that is intessting for
>+ * later completion.

typo

>+ * @param   aStr    A string to analyse
>+ * @returns Object
>+ *          If there was an error in the string detected, then a object like

please consistently put comment text on same line or next line.

>+/**
>+ * Provides a list of properties, that are possible matches based on the passed
>+ * scope and inputValue.
>+ *
>+ * @param object aScope
>+ *                scope to use for the completion.
>+ * @param string aInputValue
>+ *                value that should be completed.
>+ *
>+ * @returns null or object
>+ *                if no completion valued could be computed, null is returned,
>+ *                otherwise a object with the following form is returned:
>+ *                {
>+ *                  matches: [ string, string, string ],
>+ *                  matchProp: last part of the inputValue that was used to find
>+ *                              the matches-strings.
>+ *                }

line up the comments with the type, capitalize beginnings of sentences, etc.

>+function JSPropertyProvider(aScope, aInputValue)
>+{
>+  let obj = aScope;
>+
>+  // Analysis the aInputValue and find the beginning of the last part that

analyse

this patch is fine to start. let's get it landed and get feedback on interaction, we can polish from there. r=me.

Comment 25

[Julian Viereck]

Attachment: [checked in] Improved patch

This is a rebased patch based on former attachment 457661 [details] [diff] [review] with small fixed as pointed out by :dietrich.

Comment 26

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 458279 [details] [diff] [review]
[checked in] Improved patch

changeset:   47924:d0f799546728
tag:         tip
user:        Julian Viereck <[e-mail]>
date:        Mon Jul 19 11:38:07 2010 -0300
summary:     bug 568649 - JSTerm keybindings - tab completion. r=dietrich

Comment 27

[Rob Campbell [:rc] (:robcee)]

I'm getting some errors after executing some text (with tab-completion) on the console.

10:06 < robcee> Error: inputValue is undefined
10:06 < robcee> Source File: resource://gre/modules/HUDService.jsm
10:06 < robcee> Line: 2683
10:07 < robcee> and
10:07 < robcee> Error: container is undefined
10:07 < robcee> Source File: resource://gre/modules/HUDService.jsm
10:07 < robcee> Line: 1429
10:08 < robcee> first is in HUDService.jsm>>complete: function JSTF_complete(type)
10:09 < robcee> second is in HUDService.jsm>>windowInitializer: function 
                HS_WindowInitalizer(aContentWindow)
10:12 < robcee> I'm getting the errors after typing into the console and doing some completion
10:12 < robcee> closing the console starts generating those errors

we'll need either a quick fix or I'll have to back this out.

Comment 28

[Rob Campbell [:rc] (:robcee)]

fix in bug 580001

Comment 29

[Rob Campbell [:rc] (:robcee)]

other fix in bug 579954