Created attachment 198410 [details] [review]
patch to add tab completion

Review of attachment 198410 [details] [review]:

::: js/ui/lookingGlass.js
@@ +42,3 @@
 const HISTORY_KEY = 'looking-glass-history';
 
+function range(begin, end){

Python programmer spotted. :)

@@ +54,3 @@
+AutoComplete.prototype = {
+    properties : {
+        doubleTabDelay : 500   //time between tabs for them to count as a double-tab event

This would usually be done as a constant:

  // Time between tabs for them to count as a double-tab event.
  const AUTO_COMPLETE_DOUBLE_TAB_DELAY = 500;

@@ +60,3 @@
+        this._entry = entry;
+        this._entry.connect('key-press-event', Lang.bind(this, this._entryKeyPressEvent));
+        this._last_tab_time = global.get_current_time();

this._lastTabTime

We use camel-case.

@@ +68,3 @@
+        //insert characters of text not already included in head at cursor position.  i.e., if text="abc" and head="a",
+        //the string "bc" will be appended to this._entry
+        let insertCompletionText = Lang.bind(this, function (text, head) {

This should be a function on the prototype.

@@ +92,3 @@
+            ].join('');
+            
+            if (common_word.length > 0) {

This code is a bit too Python-y with list comprehensions and things, when basic string manipulation would be more clear:

    // Sort by length to grab the shortest word.
    let wordsByLength = event.completions.slice();
    wordsByLength.sort(function(a, b) { return a.length - b.length });
    let word = wordsByLength[0];
    let hasCommonWord = event.completions.every(function(w) { return w.slice(0, word.length) == word });

    if (hasCommonWord) {

@@ +97,3 @@
+                this.emit('suggest', {completions: event.completions});
+            }
+        }else if (event.completions.length > 1 && event.tabType === 'double') {

} else if (...) {

@@ +126,3 @@
+    //walk expr backwards from start, looking for the beginning of an
+    //expression suitable for passing to eval.
+    _getExpressionOffset : function (expr, start) {

I'm scared. I really doubt this function is 100% safe.

(also, this should probably go in the module along with all the helpers)

@@ +133,3 @@
+        };
+
+        let findMatchingQuote = function (expr, offset) {

A raw, unescaped quote isn't always a String delimiter in JavaScript:

    let foo = /"/;

@@ +166,3 @@
+
+                if (currChar == openBrace && depth == 0) {
+                    return i

;

@@ +207,3 @@
+        //things with non-word characters or that start with a number
+        //are not accessible via .foo notation and so aren't returned
+        let isValidMethod = function (w) {

Well, you could try to be smart and append ['you are smart'] or something.

@@ +225,3 @@
+            //make the list of properties unique
+            let tmp = {};
+            for each (let e in allProps) {

We don't use the for...each statement.

    if (typeof obj !== 'object')
        return [];

    // Use a map so that we can eliminate duplicate entries.
    let propsUnique = {};
    for (let i = 0; i < allProps.length; i ++) {
        if (isValidMethod(allProps[i]))
            propsUnique[allProps[i]] = true;
    }
    return Object.keys(allProps).sort();

@@ +239,3 @@
+        if (matches) {
+            base = matches[1];
+            attr_head = matches[2];

We usually use destructuring assignment for this:

    let [expr, base, attrHead] = matches;

Created attachment 198505 [details] [review]
Add-tab-completion-to-lookingGlass.patch

Updated with all the revisions.  _getExpressionOffset now tolerates regexs.  I am fairly sure its behavior is correct.  I have not yet moved it to utils because I am not sure it would be reused by anything....

Review of attachment 198505 [details] [review]:

Please squash both patches together using git rebase or git commit --amend

Created attachment 198507 [details] [review]
Add-tab-completion-to-lookingGlass.patch

Created attachment 198680 [details] [review]
Add-tab-completion-to-lookingGlass.patch

Reorganized functions to bring helpers out of AutoComplete.prototype

Created attachment 198685 [details] [review]
Add-tab-completion-to-lookingGlass.patch

More functions moved from inline to module scope

Review of attachment 198685 [details] [review]:

::: js/ui/lookingGlass.js
@@ +46,3 @@
+
+/*
+ * A few functions for parsing strings of javascript text.

Use //-style comments in JavaScript code.

@@ +74,3 @@
+    }
+    return -1;
+};

No semi.

@@ +104,3 @@
+        return findTheBrace(expr, offset - 1);
+        
+    };

No semi.

@@ +114,3 @@
+// value and offset.  This function is meant to take a string like
+// "foo(Obj.We.Are.Completing" and allow you to extract "Obj.We.Are.Completing"
+function getExpressionOffset (expr, offset) {

function getExpressionOffset(expr, offset) {

@@ +134,3 @@
+// Things with non-word characters or that start with a number
+// are not accessible via .foo notation and so aren't returned
+function isValidMethod(w) {

isValidPropertyName

@@ +152,3 @@
+// return ["foo", "bar", ...] but the list will not include "4",
+// since methods accessed with '.' notation must star with a letter or _.
+function getMethodsFromExpression (expr) {

There's nothing method-specific about this. "getPropertyNamesFromExpression".

@@ +210,3 @@
+                this.emit('suggest', {completions: event.completions});
+            }
+        }else if (event.completions.length > 1 && event.tabType === 'double') {

} else if

@@ +222,3 @@
+        }
+        if (event.get_key_symbol() == Clutter.Tab) {
+            let [completions, attr_head] = this._getCompletions(text);

attrHead

@@ +225,3 @@
+            let curr_time = global.get_current_time();
+            if ((curr_time - this._lastTabTime) < AUTO_COMPLETE_DOUBLE_TAB_DELAY) {
+                this.emit('completion-request', {tabType : 'double',

{ tabType: 'double',

@@ +240,3 @@
+    // the string "bc" will be appended to this._entry
+    _insertCompletionText : function (text, head) {
+        let additional_completion_text = text.slice(head.length);

additionalCompletionText

@@ +249,3 @@
+    _getCompletions : function (text) {
+        let methods = [];
+        let [expr, base, attrHead] = ['', '', ''];

let expr, base;
let attrHead = '';

@@ +259,3 @@
+            if (offset >= 0) {
+                base = base.slice(offset);
+                methods = getMethodsFromExpression(base).filter(function(attr){ return attr.match("^"+attrHead); });

.filter(function(attr) {
    return attr,match('^' + attrHead);
});

@@ +1079,3 @@
 
+        this._autoComplete = new AutoComplete(this._entry);
+        this._autoComplete.connect("suggest", Lang.bind(this, function(a,e){ this._pushCompletions(e.completions)}));

Use single quotes for untranslatable text.

Lang.bind(this, function(a, e) {
    this._pushCompletions(e.completions);
});

@@ +1135,3 @@
+        
+        let line = new Clutter.Rectangle({ name: 'Separator' });
+        let padBin = new St.Bin({ name: 'Separator', x_fill: true, y_fill: true });

You should not have two actors with the same name.

Created attachment 198691 [details] [review]
Add-tab-completion-to-lookingGlass.patch

More cleanup.

Created attachment 198694 [details] [review]
Add-tab-completion-to-lookingGlass.patch

One more try!

Review of attachment 198694 [details] [review]:

You seem to have some code style issues. I've filled in a few of them, but you have some missing semicolons, names_like_this, and some bad whitespace issues.

    function Thing() {
        this._init();
    }

    Thing.prototype = {
        _init: function() {
        },

        fooThing: function(actor, event) {
            let thisIsAThing = 0;
            thisIsAThing++;
            thisIsAThing = thisIsAThing + 5;
            if (thisIsAThing == 5)
                log('Hello, world!');
        }
    };

::: js/ui/lookingGlass.js
@@ +142,3 @@
+// the prototype chain ourselves
+function getAllProps(obj) {
+    if (!obj) {

Should the empty string or the number 0 not have properties? Something like:

    if (obj === null || obj === undefined)
        return [];

might be better.

@@ +185,3 @@
+        this._lastTabTime = global.get_current_time();
+
+        this.connect('completion-request', Lang.bind(this, this._completionRequestEvent));

It's untraditional to connect to your own signals. Just call _complete instead of wrapping it in a signal.

@@ +201,3 @@
+            let word = event.completions[0];
+            let numLettersInCommon = 0;
+            while (event.completions.every(function(w) { return w.charAt(numLettersInCommon) == word.charAt(numLettersInCommon); }) && numLettersInCommon < word.length) {

This is a bit strange as well. I'd write it as:

    function getCommonPrefix(words) {
        let word = words[0];
        for (let i = 0; i < word.length; i++) {
            for (let w = 1; w < words.length; w++) {
                if (words[w].charAt(i) != word.charAt(i))
                    return i;
            }
        }
        return word.length;
    }

@@ +204,3 @@
+                ++numLettersInCommon;
+            }
+            let common_prefix = word.slice(0, numLettersInCommon);

commonPrefix

@@ +217,3 @@
+
+    _entryKeyPressEvent : function (actor, event) {
+        let cursor_pos = this._entry.clutter_text.get_cursor_position();

cursorPos

@@ +262,3 @@
+                base = base.slice(offset);
+                methods = getPropertyNamesFromExpression(base).filter(function(attr) { 
+                    return attr.match("^" + attrHead); 

return attr.slice(0, attrHead.length) == attrHead;

There's a potential that attrHead could contain some regex-like things like ".", so we should avoid those.

@@ +1132,3 @@
+    
+    _pushCompletions: function(completions) {
+        let actor = new St.BoxLayout({ vertical: true });

You never seem to destroy this actor. You should probably create one on first creation and just change the text and visibility.

Created attachment 199747 [details] [review]
Add tab-completion to lookingGlass

Review of attachment 199747 [details] [review]:

The JS parsing is something that I think needs to have unit tests. Splitting it out into its own module in misc/ and writing a test along the lines of the URL matching code[0] would be a big thing to make me worry less about it. Try to do all the crazy JS you can think of, and give it incomplete and invalid strings like:

 * foo['a
 * foo()['b'
 * obj.foo()('a', 1, 2, 'b')()
 * foo.[
 * foo]]]()))]
 * 123'ab"
 * Main.foo.bar = 3; bar.

and so on to make sure that it doesn't spin in an infinite loop or modify the context while trying to match things. You may be able to use something like this as well as a custom commandHeader that shadows all globals like "window", "imports", "global" to test a modification on a context:

    const HARNESS_COMMAND_HEADER = "let imports = this; let global = this; let window = this;" + 
                                   "let Main = this;" +
                                   "let r = this;" + ...;

    function testModification(codeString) {
        // Need to use var here because of how the with() block works.
        var obj = {};
        with (obj) {
           try {
               eval(HARNESS_COMMAND_HEADER + codeString);
           } except(e) {
               JsUnit.assertNotEquals("Code '" + codeString + "' is valid code", e.constructor, SyntaxError);
           }
        }
        let propertyNames = Object.getOwnPropertyNames(obj);
        JsUnit.assertEquals("The context '" + JSON.stringify(obj) + "' was not modified", propertyNames.length, 0);
    }

[0] See tests/unit/url.js : http://git.gnome.org/browse/gnome-shell/tree/tests/unit/url.js

::: js/ui/lookingGlass.js
@@ +188,3 @@
+
+// Returns a list of global keywords derived from commandHeader
+function getGlobalCompletions() {

I'd prefer this done to the global list immediately.

    function _getGlobalCompletions() {
        // Grab our globals.
        let completions = Object.getOwnPropertyNames(window);

        // Try to extract something from commandHeader.
        let defines = commandHeader.split(';');
        for (let i = 0; i < defines.length; i ++) {
            let match = defines[i].match(/const (\w+) =/);
            if (match) {
                let [base, keyword] = match;
                completions.push(keyword);
            }
        }

        return completions.sort();
    }

    const AUTO_COMPLETE_GLOBAL_KEYWORDS = _getGlobalCompletions();

@@ +215,3 @@
+        this.globalCompletions = getGlobalCompletions();
+
+        //this.connect('completion-request', Lang.bind(this, this._processCompletionRequest));

Kill this.

@@ +226,3 @@
+        if (event.completions.length == 1) {
+            this.additionalCompletionText(event.completions[0], event.attrHead);
+            this.emit('completion', {completion: event.completions[0], type: 'whole-word' });

Needs whitespace.

@@ +297,3 @@
+                [expr, attrHead] = matches;
+                methods = this.globalCompletions.filter(function(attr) {
+                        return attr.slice(0, attrHead.length) == attrHead;

Fix the whitespace here.

@@ +1183,3 @@
+            cmdTxt.clutter_text.line_wrap = true;
+            actor.add(cmdTxt);
+            actor.cmdTxt = cmdTxt;

Don't use expando properties when "this" would work just as well.

  this._completionText

@@ +1203,3 @@
+        this._completionActor.show();
+        Tweener.removeTweens(this._completionActor);
+        Tweener.addTween(this._completionActor, { time: 0.2 / St.get_slow_down_factor(),

0.2 needs to be a constant.

@@ +1214,3 @@
+            Tweener.addTween(this._completionActor, { time: 0.2 / St.get_slow_down_factor(),
+                                                      transition: 'easeOutQuad',
+                                                      height: 5,

5?

Created attachment 199798 [details] [review]
Add tab-completion to lookingGlass

Review of attachment 199798 [details] [review]:

The patch doesn't apply against master. Did you code against an old branch?

::: js/ui/lookingGlass.js
@@ +47,3 @@
+const AUTO_COMPLETE_GLOBAL_KEYWORDS = ['Array', 'Boolean', 'Date', 'false', 
+                                       'Function', 'function', 'Math', 'new', 
+                                       'Object', 'RegEx', 'String', 'true'].concat(JsParse.getDeclaredConstants(commandHeader));

Any reason you didn't use my technique of Object.getOwnPropertyNames(window)?

Created attachment 199993 [details] [review]
Add tab-completion to lookingGlass

Review of attachment 199993 [details] [review]:

* You have a lot of whitespace errors. See about deleting them. If you use emacs, using 'M-x delete-trailing-whitespace' should be enough.
* jsParse.js needs to go in Makefile.am
* Trying it out, I like the behaviour.
* Not a fan of the italics, though.
* Hitting <TAB> shouldn't re-animate the completions. Just make it do nothing.
* Sometimes after expanding the completion the actor moves down and then disappears. Make it fade in/out as well as moving up so that this doesn't look ugly.

::: js/ui/lookingGlass.js
@@ +46,3 @@
+const AUTO_COMPLETE_DOUBLE_TAB_DELAY = 500;
+const AUTO_COMPLETE_SHOW_COMPLETION_ANIMATION_DURATION = 0.2;
+const AUTO_COMPLETE_GLOBAL_KEYWORDS = [ 'true', 'false', 'undefined', 'new' ]

window.undefined exists, window.null does not.

@@ +47,3 @@
+const AUTO_COMPLETE_SHOW_COMPLETION_ANIMATION_DURATION = 0.2;
+const AUTO_COMPLETE_GLOBAL_KEYWORDS = [ 'true', 'false', 'undefined', 'new' ]
+                                      .concat(Object.getOwnPropertyNames(window).filter(function(a){ return a.charAt(0) != '_' }))

I would prefer this to be in a function.

    function _getAutoGlobalKeywords() {
        const keywords = ['true', 'false', 'null', 'new'];
        const windowProperties = Object.getOwnPropertyNames(windowProperties);
        const headerProperties = JsParse.getDeclaredConstants;
        return keywords.concat(windowProperties).concat(headerProperties);
    }

const AUTO_COMPLETE_GLOBAL_KEYWORDS = _getAutoGlobalKeywords();

@@ +49,3 @@
+                                      .concat(Object.getOwnPropertyNames(window).filter(function(a){ return a.charAt(0) != '_' }))
+                                      .concat(JsParse.getDeclaredConstants(commandHeader));
+                                      //  ['Array', 'Boolean', 'Date', 'false', 

Don't leave this junk here.

Created attachment 200238 [details] [review]
Add tab-completion to lookingGlass

Everything should be good now.  I did the italics to separate the tab completions from looking like the rest of the text, though now that the lookingGlass theme has changed I guess it is not quite as necessary.  I tried to make the color grey, but the odd thing is, no matter how I tried to change the color in .lg-completions-text in the css file, the change didn't seem to be reflected in the rendered text...Am I missing something silly here?

Review of attachment 200238 [details] [review]:

Trying this out, I'm comfortable and happy with it. If you don't have git access, just yell at me and I'll commit it.

Modified commit message a bit and fixed some whitespace issues. Otherwise, pushed as is. Thanks!