JSTerm keybindings: tab-completion

RESOLVED FIXED

Status

defect
P1
normal
RESOLVED FIXED
9 years ago
Last year

People

(Reporter: ddahl, Assigned: julian.viereck)

Tracking

({uiwanted})

Trunk
x86
All
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

Reporter

Description

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

Updated

9 years ago
Assignee: ddahl → jviereck
Reporter

Updated

9 years ago
Depends on: 573102
Reporter

Comment 1

9 years ago
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/
Reporter

Updated

9 years ago
Priority: -- → P1
Assignee

Comment 2

9 years ago
Posted patch Work so far (obsolete) — Splinter Review
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.
Assignee

Updated

9 years ago
Attachment #453689 - Flags: review?(ddahl)
Reporter

Comment 4

9 years ago
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.
Attachment #453689 - Flags: review?(ddahl) → review-
Reporter

Updated

9 years ago
Attachment #453859 - Flags: review?(ddahl) → review-
Reporter

Comment 5

9 years ago
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.
Reporter

Comment 6

9 years ago
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.
Assignee

Comment 7

9 years ago
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.
Attachment #453689 - Attachment is obsolete: true
Attachment #453859 - Attachment is obsolete: true
Attachment #454012 - Flags: review?(ddahl)
Assignee

Comment 8

9 years ago
(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.
Reporter

Comment 9

9 years ago
I'm going to CC: some JS team people on this as well.
(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.
Reporter

Updated

9 years ago
Keywords: uiwanted
Reporter

Updated

9 years ago
Attachment #454012 - Flags: review?(ddahl) → review+
Reporter

Comment 11

9 years ago
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.
Assignee

Comment 12

9 years ago
> >+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.
Assignee

Comment 13

9 years ago
Updated typos as mentioned by David in comment 11.
Attachment #454012 - Attachment is obsolete: true
Attachment #454800 - Flags: review?(ddahl)
Reporter

Comment 14

9 years ago
(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
Assignee

Comment 15

9 years ago
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?
Attachment #455116 - Flags: review?(ddahl)
Reporter

Comment 16

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

Pushed into heads-up-display repo
Attachment #454800 - Flags: review?(ddahl) → review+
Reporter

Comment 17

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


things like:
typeof doc <tab>

does not work, which is OK for now.
Reporter

Comment 18

9 years ago
(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.
Reporter

Updated

9 years ago
Attachment #455116 - Flags: review?(ddahl) → review+
Assignee

Comment 19

9 years ago
Posted patch (Hopefully) Final patch (obsolete) — Splinter Review
Attachment #454800 - Attachment is obsolete: true
Attachment #455116 - Attachment is obsolete: true
Attachment #457364 - Flags: review?(dietrich)
Assignee

Comment 20

9 years ago
exposes 'jsterm' on the HUD object. This patch is necessary to get the jsterm object when running unit tests (e.g. for tab completion).
Attachment #457364 - Attachment is obsolete: true
Attachment #457386 - Flags: review?(dietrich)
Attachment #457364 - Flags: review?(dietrich)
Assignee

Comment 21

9 years ago
Posted patch (Hopefully2) Final patch (obsolete) — Splinter Review
Patch nearly the same as attachment 457364 [details] [diff] [review], but applies on trunk with attachment 457386 [details] [diff] [review] from this bug.
Attachment #457392 - Flags: review?(dietrich)
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!
Depends on: 579073
Assignee

Comment 23

9 years ago
Posted patch (Hopefully3) Final patch (obsolete) — Splinter Review
Note: Patch depends on bug 579073.
Attachment #457386 - Attachment is obsolete: true
Attachment #457392 - Attachment is obsolete: true
Attachment #457661 - Flags: review?(dietrich)
Attachment #457386 - Flags: review?(dietrich)
Attachment #457392 - Flags: review?(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.
Attachment #457661 - Flags: review?(dietrich) → review+
Assignee

Comment 25

9 years ago
This is a rebased patch based on former attachment 457661 [details] [diff] [review] with small fixed as pointed out by :dietrich.
Attachment #457661 - Attachment is obsolete: true
Assignee

Updated

9 years ago
Status: NEW → ASSIGNED
Whiteboard: [checked-in]
Assignee

Updated

9 years ago
Whiteboard: [checked-in] → [checkin-needed]
Comment on attachment 458279 [details] [diff] [review]
[checked in] Improved patch

changeset:   47924:d0f799546728
tag:         tip
user:        Julian Viereck <jviereck@mozilla.com>
date:        Mon Jul 19 11:38:07 2010 -0300
summary:     bug 568649 - JSTerm keybindings - tab completion. r=dietrich
Attachment #458279 - Attachment description: Improved patch → [checked in] Improved patch
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
fix in bug 580001
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.