Created attachment 147172 [details] [review]
Patch for this bug

Review of attachment 147172 [details] [review]:

Thanks for the patch!  Overall comments:

* I think we should consider adding inotify watches on each component of $PATH, and caching the executable list we read.  Otherwise if you have say a NFS directory in the path the UI can block for an unacceptable amount of time.  This is also true for the busy-laptop case.

* Have you looked at file:///usr/share/gtk-doc/html/gio/GFilenameCompleter.html  ? I'm not sure offhand if it searches $PATH or what.

::: js/ui/runDialog.js
@@ +188,3 @@
+                    if (!pref.length || file_name.substr(0, pref.length) == pref) {
+                        if (pref.length)
+                            possible.push(file_name.substr(pref.length))

Missing a ;

@@ +192,3 @@
+                            possible.push(file_name);
+                    }
+                }

Should call enumerator.close(null) here.  It'll probably be closed on unref, but best to be safe.

@@ +210,3 @@
+        }
+        return common;
+    },

This is not very efficient.  Instead, we could sort possibilities, then just take the common part from the first and last one, right?

Created attachment 147694 [details] [review]
Corrected patch

Use GFilenameCompleter for completing filename(it do not searches in $PATH).
On init add inotify watches on each component of $PATH and caching the executable list.

>This is not very efficient.  Instead, we could sort possibilities, then just
>take the common part from the first and last one, right?
No, search is O(n*ln(n)). This algorithm is O(n).

Review of attachment 147694 [details] [review]:

::: js/ui/runDialog.js
@@ +68,3 @@
+                    }
+                }));
+            }

Hmmm, a couple of things here.  First it'd be nicer to make this anonymous function into a member to avoid getting up to 7 indentations deep.

  this._monitors[i].connect("changed", Lang.bind(this, this._onChanged));

...

  onChanged: function() { 
     code here
  }

Another general problem we hit in the shell is doing expensive computation on changes.  Remember that since we are the display compositor, if we're not processing events and repainting, the UI is locked and the user gets unhappy.  

A concrete example of a pathological case we were hitting was larger OS upgrades via yum/apt/etc. which rapidly change all of the .desktop files; but they would also churn /usr/bin quite rapidly, and we'd be continually getting these "changed" notifications, even though the user isn't necessarily running Alt-F2 right now.

The trick we've used in other places is that the "changed" notfication just sets a dirty flag, like:

  onChanged: function() {
     this._dirty = true;
  }

Then inside your getCompletion, you can do:

   getCompletion: function(text) {
      if (this._dirty)
          this._reload();

      ...
   }

Your approach does avoid I/O in the main process after loading which is fairly clever; maybe we could keep a queue of changes (say up to 10) in this._changes instead of a dirty flag, and if it gets beyond that just give up and do a full reload?

If we wanted to be even more clever (I don't want to push too much on you before we get a first version of this patch in, you don't have to do all of this!) we could use GIO's asynchronous API to avoid blocking the mainloop at all.

@@ +230,3 @@
                 return true;
+                let text = o.get_text().concat('/a');
+            if (symbol == Clutter.slash) {

Is the 'a' a typo?

@@ +251,3 @@
+                    o.set_cursor_position(text.length + postfix.length);
+                    if (postfix[postfix.length - 1] == '/')
+                        this._getCompletion(text + postfix + 'a');

Hm, but there's also a hardcoded 'a' here.

Can you add a comment here about what's going on?

Thanks again for the patch by the way!  Quite looking forward to having this landed.

Review of attachment 147694 [details] [review]:

::: js/ui/runDialog.js
@@ +68,3 @@
+                    }
+                }));
+            }

Hmmm, a couple of things here.  First it'd be nicer to make this anonymous function into a member to avoid getting up to 7 indentations deep.

  this._monitors[i].connect("changed", Lang.bind(this, this._onChanged));

...

  onChanged: function() { 
     code here
  }

Another general problem we hit in the shell is doing expensive computation on changes.  Remember that since we are the display compositor, if we're not processing events and repainting, the UI is locked and the user gets unhappy.  

A concrete example of a pathological case we were hitting was larger OS upgrades via yum/apt/etc. which rapidly change all of the .desktop files; but they would also churn /usr/bin quite rapidly, and we'd be continually getting these "changed" notifications, even though the user isn't necessarily running Alt-F2 right now.

The trick we've used in other places is that the "changed" notfication just sets a dirty flag, like:

  onChanged: function() {
     this._dirty = true;
  }

Then inside your getCompletion, you can do:

   getCompletion: function(text) {
      if (this._dirty)
          this._reload();

      ...
   }

Your approach does avoid I/O in the main process after loading which is fairly clever; maybe we could keep a queue of changes (say up to 10) in this._changes instead of a dirty flag, and if it gets beyond that just give up and do a full reload?

If we wanted to be even more clever (I don't want to push too much on you before we get a first version of this patch in, you don't have to do all of this!) we could use GIO's asynchronous API to avoid blocking the mainloop at all.

@@ +230,3 @@
                 return true;
+                let text = o.get_text().concat('/a');
+            if (symbol == Clutter.slash) {

Is the 'a' a typo?

@@ +251,3 @@
+                    o.set_cursor_position(text.length + postfix.length);
+                    if (postfix[postfix.length - 1] == '/')
+                        this._getCompletion(text + postfix + 'a');

Hm, but there's also a hardcoded 'a' here.

Can you add a comment here about what's going on?

Thanks again for the patch by the way!  Quite looking forward to having this landed.

Created attachment 148077 [details] [review]
Corrected patch

In this patch use GIO's asynchronous API.

g_file_enumerator_next_files_finish has incorrect annotation(Patch for gobject-introspection in next attachment).
g_file_enumerate_children_async doesn't work(Hack for gjs in next attachment).

Created attachment 148078 [details] [review]
Hack for gjs

Created attachment 148079 [details] [review]
Correct g_file_enumerator_next_files_finish annotation

Created attachment 148979 [details] [review]
Updated patch

Review of attachment 148979 [details] [review]:

Overall looks like a nice bit of code, thanks a lot for going through updating it!  Just have some smaller comments.  We should get this in and iterate further on it (I'd like to make Alt-F2 a search on apps + allow filenames and unix commands).

::: js/ui/runDialog.js
@@ +6,2 @@
 const GLib = imports.gi.GLib;
+const GObject = imports.gi.GObject;

This import isn't used?

@@ +38,3 @@
+function CommandCompleter() {
+
+const MAX_FILE_DELETED_BEFORE_INVALID = 10;

Typo?  Should be _updateInProgress?

@@ +45,3 @@
+function CommandCompleter() {
+
+const MAX_FILE_DELETED_BEFORE_INVALID = 10;

A bit ugly to mix together GLib and Gio.  Remember creating a GFile doesn't involve I/O, so you could write the is-directory test as:

let file = Gio.file_new_for_path(this._paths[i]);
if (file.query_type(Gio.FileQueryInfoFlags.NONE, null) != Gio.FileType.DIRECTORY)
  continue;

@@ +70,3 @@
+        } else {
+            this._enumerator.close(null);
+            this._enumerator = undefined;

I tend to prefer "null" instead of "undefined".

@@ +76,3 @@
+
+    _update : function(i) {
+        if (i == 0 && this._updataInProcess)

Typo here too.

@@ +101,3 @@
+        }
+        if (k === undefined) {
+            log("Error");

I'd just remove the log(); if someone sees just "Error" in their ~/.xsession-errors it'll be really hard to track down.

@@ +124,3 @@
+    getCompletion: function(text) {
+        let common = "";
+        let not_init = true;

JavaScript variables should be camelCase (we may not be totally consistent here in the code).

@@ +128,3 @@
+            this._update(0);
+            return common;
+        }

So what will happen here in practice is the user will open up Alt-F2, and start typing, hit TAB, and at that point is when we will start reading in all the directories (and your first TAB keypress won't do anything?)

It would be nicer to do this in the background say when the Alt-F2 dialog is first shown, so it's more likely to have completions by the time you hit TAB.

(In the future the UI should show some "Searching..." type indication ideally while we're reading the directories, but we can delay doing that and get this patch in now)

@@ +140,3 @@
+        }
+        function _hasPrefix(s1, prefix) {
+            return s1.substr(0, prefix.length) == prefix;

s1.indexOf(prefix) ==0

is probably more efficient on small strings since we don't allocate another string.

Created attachment 149945 [details] [review]
Corrected patch

Pushed; thanks a lot for the hard work on this patch Maxim!