Created attachment 139725 [details] [review]
LookingGlass - JavaScript prompt and interactive Clutter controller

*** Bug 561299 has been marked as a duplicate of this bug. ***

Very neat! Especially for so little code.

 - I think the name lookingGlass is extremely descriptive, a perfect match. Some confusion potential from http://www.sun.com/software/looking_glass/ , I think that just means we might want to keep the name out of the UI text.

 - I have a lot of trouble reading it. Changing the background to have an alpha of e0 rather than a0 helped. Changing the font from "12px Mono" to the default Monospace font "Monospace 10" helped a bit, but that was still too small. (10 * 96/72 == 13.3333 so bigger than 12px.) Picking up the GConf key /desktop/gnome/interface/monospace_font_name would be best. A patch I have half-done to make dark-on-light text more constrasty in clutter helped a bit. Changing to a brighter green (88ff66, say) helped a lot.

- Would be useful to predefine 'global'

- Suffer's from alt-tab breaking grab (bug 590686)

+        let line = new Clutter.Rectangle({ opacity: 0, height: 4 });
+        this.actor.append(line, Big.BoxPackFlags.NONE);
+        line = new Clutter.Rectangle({ color: GREY, height: 1 });
+        this.actor.append(line, Big.BoxPackFlags.NONE);
+        let line = new Clutter.Rectangle({ opacity: 0, height: 4 });

 Two of the line = have let's, not the third. (And two of the 'lines' are more spacers?)

 - Lot of trailing whitespace in the patch
 
 - With F11 and a fresh build of gobject-introspection, history saving is throwing:

    JS ERROR: !!!   Exception was: Error: Argument 'etag' (type utf8) may not be null
    JS ERROR: !!!     lineNumber = '0'
    JS ERROR: !!!     fileName = 'gjs_throw'
    JS ERROR: !!!     stack = 'Error("Argument 'etag' (type utf8) may not be null")@:0
("Argument 'etag' (type utf8) may not be null")@gjs_throw:0
@:0
()@/home/otaylor/gnome-shell/source/gnome-shell/js/ui/lookingGlass.js:232
()@/home/otaylor/gnome-shell/install/share/gjs-1.0/lang.js:110
'
    JS ERROR: !!!     message = 'Argument 'etag' (type utf8) may not be null'

 - Using the viewMore SVG seems a bit off. Isn't there some GTK+ stock icon that can be used?

+            let width = Math.min(global.stage.width * 0.07, 150);

 Reminds me of some IRS tax form

   The width of the inspect popup is the maximum of the width it needs
   and 7% of the width of the screen, whichever is less.

 Wouldn't just 150 be fine?

+                eventHandler.x = (global.stage.width)/2 - (eventHandler.width)/2;

You really, really, need to round this to a pixel position, the fuzzing in and out as the text changed was horrible.

+            let text = o.get_text();
+            text.replace('\n', ' ');
+            this._evaluate(text);

The replacement there needs some comment, unclear why that makes sense.

+                if (this._historyNavIndex >= this._history.length-1)

Space around operator.

- Think the 30 second timeout to save history is too long. Though it would be better if you saved history reliably on console-window close and when the user hit Alt-F2 to Alt-F2 <restart>

- 'it' to mean the last result is quite obscure. And you'd expect to be able to type r5 to access the thing that displays as 

  r5 = 42

Displaying r(5) = 42 would help discover that r() is a function. But what if you just set variables in the scope so you could type r5? Or I think ipython's convention of:

  _N - Nth result
  _  - Last result

is pretty nice. [Yeah, that "conflicts" with the idea of _ for gettext(), but not practically]

- I like the simplicity of click-on-the-clock activation except:

  - Clearly can't be on by default because it will confuse people
  - Can't be on by default, since clicking on the clock should give you calendar, etc.

  Maybe just hide it as a control-click on the clock for now. Once we have extensions, I could imagine this as if you have the extension turned on, I'd imagine it as probably some icon on the panel that you click on.

- No particular idea about how activation of this interacts with:

  - Activation of recorder
  - Activation of a performance HUD

 Don't think they really are the same thing so maybe they don't interact.

- I'm fine with this landing at any point you feel comfortable with it.

Updated for all comments and pushed.  Answers for some specific problems:

- Changing the background to have an alpha of e0

I compromised on d5.  I like the extra transparency...gives it an ephemeral feel.  The font suggestions were great - there must not be hinting or something for 12px Mono because Monospace 10 fixes the kerning around >>>. 

- With F11 and a fresh build of gobject-introspection, history saving is
throwing:

Yeah, I had a fix for this uncommitted.  Pushed.

- Would be useful to predefine 'global'

Some of this we should figure out how to do nicely for the shell proper...I really want color().

- I like the simplicity of click-on-the-clock activation except:

Yeah was just a temporary hack =) I've updated the patch to launch from "lg" in the run dialog.  That may still not be good enough, but probably for now.