Created attachment 143126 [details] [review]
Allow looking glass in the overview

Comment on attachment 143126 [details] [review]
Allow looking glass in the overview

Maybe split out the modal/keystack stuff?

>+    _getTooltip: function() {

If we're going to end up having tooltips anywhere else, it might make sense to split this out like DND.makeDraggable...

>+        this.actor.connect('key-press-event', Lang.bind(this, function(o, e) {
>+            this.close();
>+            return true;
>+        }));

any reason for that? (that's in developertoolbar)

>+   toggle: function() {

the indentation is off on that line (only has 3 spaces instead of 4)

>+    close : function() {
...
>+        this.actor.hide();

any reason you animate on open() but not close()?

>+    global.screen.connect('toggle-recording', _onDeveloperToolbarToggle);

That seems pretty lame, name-wise.

I'm going to have to add key-grabbing support inside gnome-shell for the Alt-Tab stuff (there's no way to let Alt-Tab use the mouse using mutter's Alt-Tab support). So once we have that, we can get rid of the shell-specific keybindings we added to mutter. (This should help de-kludge _globalKeyPressHandler too...)

>+function _onDeveloperToolbarToggle() {
>+    log("dev toolbar toggle");
>+    // Special case the state where we're currently recording
>+    if (recorder != null && recorder.is_recording()) {
>+        recorder.pause();
>+    } else {
>+        getDeveloperToolbar().toggle();
>+    }
>+}

Hm... I suppose that special case makes sense, unless you're trying to make a screencast of looking glass...

What would be really clever would be if the key just brought up the toolbar like normal, but if you then clicked on the record button again, it would pause/stop, and remove the last few seconds after you typed the hotkey from the recording. (I don't know gstreamer, but it seems like either you could add some sort of mark to the stream when opening the toolbar, so that you could later delete everything after that mark, or else you could start buffering the frames without actually passing them to the recording element, until you decided whether or not they were going to get recorded.)

(In reply to comment #2)
> (From update of attachment 143126 [details] [review])
> Maybe split out the modal/keystack stuff?
> 
> >+    _getTooltip: function() {
> 
> If we're going to end up having tooltips anywhere else, it might make sense to
> split this out like DND.makeDraggable...

The NBTK patch has tooltips as part of NbtkWidget.  But I wanted to get a sense for how well they'd work here.

> >+        this.actor.connect('key-press-event', Lang.bind(this, function(o, e) {
> >+            this.close();
> >+            return true;
> >+        }));
>
> any reason for that? (that's in developertoolbar)

I guess I could have just done Escape.

> 
> >+   toggle: function() {
> 
> the indentation is off on that line (only has 3 spaces instead of 4)
> 
> >+    close : function() {
> ...
> >+        this.actor.hide();
> 
> any reason you animate on open() but not close()?

To avoid it appearing in the screen recordign when you hit record, and having two animations when you opened looking glass was a bit too much.

> 
> >+    global.screen.connect('toggle-recording', _onDeveloperToolbarToggle);
> 
> That seems pretty lame, name-wise.

Yeah...should rename the mutter signal, I just didn't get to it in this patch.

> 
> I'm going to have to add key-grabbing support inside gnome-shell for the
> Alt-Tab stuff (there's no way to let Alt-Tab use the mouse using mutter's
> Alt-Tab support). So once we have that, we can get rid of the shell-specific
> keybindings we added to mutter. (This should help de-kludge
> _globalKeyPressHandler too...)

Ahh interesting.  That sounds great.

> What would be really clever would be if the key just brought up the toolbar
> like normal,

Hmm tricky, well this seems like something we can tackle a bit later if we want to record LG.

One thing I was thinking about on this bug is instead of having a separate toolbar, just move the buttons into the top of LG.  Would be less duplication for sure.

Review of attachment 143126 [details] [review]:

I'm a little fuzzy about what you are trying to achieve - there seems to be a couple of different ideas going on here:

 - The idea of a "developer toolbar"
 - The idea of being able to do modal things from the overlay

With the modality changes, you could have presumably just make Alt-F2 work in the overlay, right?

I'm not opposed to the idea of the developer toolbar, though I wonder if it would be better to consider just fold it into the looking glass as a minimized mode? - you start off with just a toolbar at the top of the screen with [Restart] [Record]            [ Inspect ] [ Console ]    or something and then [ Inspect ] [ Console ] expand out. And I'm not opposed to making it the way to start recording, though as Dan says, it's a little unintuitive to make it stop recording too. (Editing the stream to remove the last few seconds is going to be *hard* - but we could bringing up the toolbar pause the recording - and if you do anything other than click [ Stop Recording ] maybe it resumes - still not great for screencasting lookingGlass, but you could hack out the pause feature temporarily if you wanted to do that.)

But I don't like reusing the Control-Alt-Shift-R keybinding for this. A) It's designed to be mnemonic of recording B) it's called that in GConf and otherwise C) It's just too hard to type. I dont' know if I'm the only developer without a Super - if that's a rare problem, I'd suggest we bind it to <Super>L by default - then <Super>L Re_start <Super>L _Console <Super>L _Record have a nice two-handed rhythm.

::: data/gnome-shell.schemas
@@ +4,3 @@
       <schema>
+        <key>/schemas/desktop/gnome/shell/development_tools</key>
+        <applyto>/desktop/gnome/shell/development_tools</applyto>

I think we can wait to add this until we are closer to a final 3.0 release. For now, it seems useful to tell people "try X" without having to say "run gconf-tool <magic gunk>" first.

::: js/ui/lookingGlass.js
@@ +318,3 @@
+                                                                      24);
+        this.actor.append(iconTexture, Big.BoxPackFlags.NONE);
+        this.actor.connect('notify::hover', Lang.bind(this, this._onHoverChanged));

It seems that every notify::hover breaks down to "if (hovering) { do_start_hover(); } else { do_end_hover; }" I wonder if there should be startHover/endHover signals.

@@ +366,3 @@
+}
+
+function DeveloperToolbar() {

Assuming this stays in the current form as something distinction from lookingGlass, I would rather this isn't crammed into lookingGlass.js - there's no penalty for us having more .js files - it would be fine to have a developerToolbar.js and a toolbarAction.js [or toolbar.js]

::: js/ui/main.js
@@ +175,3 @@
+                                                    Clutter.ModifierType.CONTROL_MASK |
+                                                    Clutter.ModifierType.MOD1_MASK))) {
+            log("PRESS");

Leftover

@@ +200,3 @@
+ * the effect will be undone when an equal number of popModal() invocations
+ * have been made.
+ */

I think this is OK - to make it purely about the global stage, but I'm a little nervous that there are problems that are going to come up with a reference-counted pushModal without any coordination about who gets events. One pushModaler has a clutter grab, the other pushModaler puts up a cover-plate. Well, we can deal with them as they arise.

@@ +205,3 @@
 
+    modalCount += 1;
+    log("pushModal count: "+ modalCount);

Leftover

@@ +224,3 @@
 
+    modalCount -= 1;
+    log("pushModal count: "+ modalCount);

Leftover

@@ +251,3 @@
+        removeKeyboardFocusStack(actor);
+    });
+    keyboardFocusStack.push(actor);

This doesn't seem to be living up to the description of "saving the previous focus on a stack" - it looks like you won't succesfully return the focus to the entry when you go back to the normal overlay.

@@ +252,3 @@
+    });
+    keyboardFocusStack.push(actor);
+    log("acquire focus -> " + actor);

Maybe not a leftover, but inappropriate until we have log(Log.DEBUG, ...) or something. Right now our policy really is that log() is warnings-only.

::: js/ui/overview.js
@@ +67,3 @@
 TRANSPARENT_COLOR.from_pixel(0x00000000);
 
+const DEVELOPER_TOOLS_BORDER_COLOR = new Clutter.Color();

You don't actually use this

::: js/ui/runDialog.js
@@ -37,3 +44,3 @@
         this._internalCommands = { 'lg':
                                    Lang.bind(this, function() {
-                                       // Run in an idle to avoid recursive key grab problems
+                                       Main.getDeveloperToolbar().actionLookingGlass();

Do we actually need to keep the magic commands if we also have the developer toolbar?

Created attachment 143185 [details] [review]
[main] Change keyboard handling API to handle nested modal, focus chains

Rename beginModal/endModal to pushModal/popModal.  All of the current callers
just want to ensure that we're in a modal state; they don't actually need to
fail if we already are.

Add functions for grabbing the Clutter keyboard focus, while recording
the previous focus.

Created attachment 143186 [details] [review]
[Main] Enable Alt-f2 in overview

This isn't a long-term solution; what we really want is for Alt-F2 to
just be an application search with a hack to detect shell commands,
but in the short term this allows us to run the magic 'lg' command
from the overview.

Created attachment 143187 [details] [review]
[gnome-shell.schemas] Add a developer_tools boolean defaulting to true

This will be use to enable various internal tools which should only
be exposed to developers/testers.

Created attachment 143188 [details] [review]
[runDialog] Only enable internal commands if developer_tools is true

'r' is a bit too accidentally-hittable.

(In reply to comment #7)
> Created an attachment (id=143186) [details]
> [Main] Enable Alt-f2 in overview
> 
> This isn't a long-term solution; what we really want is for Alt-F2 to
> just be an application search with a hack to detect shell commands,
> but in the short term this allows us to run the magic 'lg' command
> from the overview.

or alternatively, make the run dialog and the overview search entry more like each other. (Or have Alt-F2 slide in the dash over the current workspace and have it exit as soon as you launch something or hit Esc?)

(In reply to comment #9)
> Created an attachment (id=143188) [details]
> [runDialog] Only enable internal commands if developer_tools is true
> 
> 'r' is a bit too accidentally-hittable.

well, we could just remove the "r" shortcut and require people to type "restart" in full

(In reply to comment #10)
> (In reply to comment #7)
> > Created an attachment (id=143186) [details] [details]
> > [Main] Enable Alt-f2 in overview
> > 
> > This isn't a long-term solution; what we really want is for Alt-F2 to
> > just be an application search with a hack to detect shell commands,
> > but in the short term this allows us to run the magic 'lg' command
> > from the overview.
> 
> or alternatively, make the run dialog and the overview search entry more like
> each other. (Or have Alt-F2 slide in the dash over the current workspace and
> have it exit as soon as you launch something or hit Esc?)

Yeah something like that, needs some designer input I guess but we can cross that when we get to it.

> (In reply to comment #9)
> > Created an attachment (id=143188) [details] [details]
> > [runDialog] Only enable internal commands if developer_tools is true
> > 
> > 'r' is a bit too accidentally-hittable.
> 
> well, we could just remove the "r" shortcut and require people to type
> "restart" in full

Over my dea...I mean, ah, I like the r shortcut =)

Review of attachment 143185 [details] [review]:

The modal part is OK, but I think the acquireKeyboardFocusStack() stuff needs some more thinking through - or maybe it should be tied closer to pushModal/popModal(), as discussed below.

::: js/ui/lookingGlass.js
@@ -355,3 +355,3 @@
             inspector.connect('closed', Lang.bind(this, function() {
                 this.actor.show();
-                global.stage.set_key_focus(this._entry);
+                Main.acquireKeyboardFocusStack(this._entry);

I'm not sure why we are setting the keyboard focus (either way) when the inspector is closed, but using a stack-based acquire() seems definitely wrong.

@@ -390,3 +390,3 @@
         notebook.connect('selection', Lang.bind(this, function (nb, child) {
             if (child == this._evalBox)
-                global.stage.set_key_focus(this._entry);
+                Main.acquireKeyboardFocusStack(this._entry);

Like many cases, "kind of a hack" comments need to explain what the hack is for,  it's very unclear to me.

Several times further down in this file, you call set_key_focus() and don't change it in this patch - triggering the "undefined semantics" ?

::: js/ui/main.js
@@ +158,3 @@
     if (type == Clutter.EventType.KEY_PRESS) {
         let symbol = event.get_key_symbol();
+        let state = event.get_state();

Stray in this patch

@@ +202,3 @@
+    if (!global.begin_modal(timestamp)) {
+        log("pushModal: invocation of begin_modal failed");
+        return;

You probably should make this function boolean-returning and not increment the modal count in this case.

@@ +227,3 @@
+ *
+ * Take the Clutter keyboard focus, while saving the previous
+ * focus on a stack.  If any actor on this stack is unmapped or destroyed

You still aren't saving the previous focus on the stack - as noted in my last set of comments - when the last acquireKeyboarFocusStack is released, then you don't restore the focus back to the original focus.

@@ +231,3 @@
+ *
+ * Note: Changing the Clutter keyboard focus directly when the stack
+ * created by this function is non-empty has undefined semantics.

perhaps, but it's something that's normal and expected. I don't think we want to implement something that only works with dialogs with *one* focus entry. We likely want to wrap stage.set_key_focus() into our own function (though I'm not sure we *need to*, but I don't think we want to tie this to pushing a level onto the keyboard focus stack - they are separate.

We could even tie this directly to pushModal()... make pushModal() save the previous focus and restore it at popModal(). [You might want then to pushModal(actor) to avoid stacking errors and to automatically popModal() if the actor gets unmapped/destroyed]

Review of attachment 143186 [details] [review]:

I can't think of any harm for accepting <any modifiers>+<F2> in the overlay, though I find it a bit confusing reading the code - it looks like a bug. since this is all a temporary hack, it's fine.

Review of attachment 143187 [details] [review]:

OK

(In reply to comment #12)
> Review of attachment 143185 [details] [review]:
> 
> The modal part is OK, but I think the acquireKeyboardFocusStack() stuff needs
> some more thinking through - or maybe it should be tied closer to
> pushModal/popModal(), as discussed below.

Right now they're often paired; the pairing comes basically from code that needs to run both in the overview and outside of it.  I imagine that most of our code that takes key focus will be inside the overview, so in the future having those places call pushModal will be unnecessary, since they know they're in the overview.

> ::: js/ui/lookingGlass.js
> @@ -355,3 +355,3 @@
>              inspector.connect('closed', Lang.bind(this, function() {
>                  this.actor.show();
> -                global.stage.set_key_focus(this._entry);
> +                Main.acquireKeyboardFocusStack(this._entry);
> 
> I'm not sure why we are setting the keyboard focus (either way) when the
> inspector is closed, but using a stack-based acquire() seems definitely wrong.

I think the problem was when switching tabs the entry got un-focused, but honestly I'm not sure why that was happening.  I'll debug.

> @@ -390,3 +390,3 @@
>          notebook.connect('selection', Lang.bind(this, function (nb, child) {
>              if (child == this._evalBox)
> -                global.stage.set_key_focus(this._entry);
> +                Main.acquireKeyboardFocusStack(this._entry);
> 
> Like many cases, "kind of a hack" comments need to explain what the hack is
> for,  it's very unclear to me.
> 
> Several times further down in this file, you call set_key_focus() and don't
> change it in this patch - triggering the "undefined semantics" ?

Hmmm?  Shouldn't be.

> @@ +227,3 @@
> + *
> + * Take the Clutter keyboard focus, while saving the previous
> + * focus on a stack.  If any actor on this stack is unmapped or destroyed
> 
> You still aren't saving the previous focus on the stack - as noted in my last
> set of comments - when the last acquireKeyboarFocusStack is released, then you
> don't restore the focus back to the original focus.

The stack is only from callers of acquireKeyboardFocusStack.  It does work (after I fixed more of lookingGlass.js to use it).  Look at what removeKeyboardFocusStack does.  

> perhaps, but it's something that's normal and expected. I don't think we want
> to implement something that only works with dialogs with *one* focus entry. We
> likely want to wrap stage.set_key_focus() into our own function (though I'm not
> sure we *need to*, but I don't think we want to tie this to pushing a level
> onto the keyboard focus stack - they are separate.

Not sure what you're saying about one entry; for separate:

> We could even tie this directly to pushModal()... make pushModal() save the
> previous focus and restore it at popModal(). [You might want then to
> pushModal(actor) to avoid stacking errors and to automatically popModal() if
> the actor gets unmapped/destroyed]

You could argue pushModal should also take a focus.  But as above I'd argue we do want an independent function to just move the clutter focus around for stuff that knows it's in the overview.

Review of attachment 143188 [details] [review]:

Think your commit message needs to explain that 'r' will still be accidentally hittable until we change the default on the gconf key.

::: js/ui/runDialog.js
@@ -37,4 +44,3 @@
         this._internalCommands = { 'lg':
                                    Lang.bind(this, function() {
-                                       // Run in an idle to avoid recursive key grab problems
-                                       Mainloop.idle_add(function() { Main.createLookingGlass().open(); });
+                                       Main.createLookingGlass().toggle();

I'm OK with slipping unrelated change in, but should be mentioned in the commit message

(In reply to comment #15)
> (In reply to comment #12)
> > Review of attachment 143185 [details] [review] [details]:
> > 
> > The modal part is OK, but I think the acquireKeyboardFocusStack() stuff needs
> > some more thinking through - or maybe it should be tied closer to
> > pushModal/popModal(), as discussed below.
> 
> Right now they're often paired; the pairing comes basically from code that
> needs to run both in the overview and outside of it.  I imagine that most of
> our code that takes key focus will be inside the overview, so in the future
> having those places call pushModal will be unnecessary, since they know they're
> in the overview.

I don't think it is acceptable to call the acquireKeyboardFocusStack() function unless you are effectively doing a modal dialog. Otherwise you won't deterministically release it. And if you are effectively doing a modal dialog, then it's fine to pushModal().

> > @@ -390,3 +390,3 @@
> >          notebook.connect('selection', Lang.bind(this, function (nb, child) {
> >              if (child == this._evalBox)
> > -                global.stage.set_key_focus(this._entry);
> > +                Main.acquireKeyboardFocusStack(this._entry);
> > 
> > Like many cases, "kind of a hack" comments need to explain what the hack is
> > for,  it's very unclear to me.
> > 
> > Several times further down in this file, you call set_key_focus() and don't
> > change it in this patch - triggering the "undefined semantics" ?
> 
> Hmmm?  Shouldn't be.

OK, one time. In LookingGlass.open()
 
> > @@ +227,3 @@
> > + *
> > + * Take the Clutter keyboard focus, while saving the previous
> > + * focus on a stack.  If any actor on this stack is unmapped or destroyed
> > 
> > You still aren't saving the previous focus on the stack - as noted in my last
> > set of comments - when the last acquireKeyboarFocusStack is released, then you
> > don't restore the focus back to the original focus.
> 
> The stack is only from callers of acquireKeyboardFocusStack.  It does work
> (after I fixed more of lookingGlass.js to use it).  Look at what
> removeKeyboardFocusStack does. 

I know it's working becaue you are acquiring the keyboard focus to the dash entry when opening the overview,  but what you say above implies it actually saves the stage.get_key_focus() at the time of calling, which it doesn't. (But could.)

> > perhaps, but it's something that's normal and expected. I don't think we want
> > to implement something that only works with dialogs with *one* focus entry. We
> > likely want to wrap stage.set_key_focus() into our own function (though I'm not
> > sure we *need to*, but I don't think we want to tie this to pushing a level
> > onto the keyboard focus stack - they are separate.
> 
> Not sure what you're saying about one entry; for separate:

What I'm saying is that there's no way to change the focus at the top of the stack without pushing a new level of focus.

> > We could even tie this directly to pushModal()... make pushModal() save the
> > previous focus and restore it at popModal(). [You might want then to
> > pushModal(actor) to avoid stacking errors and to automatically popModal() if
> > the actor gets unmapped/destroyed]
> 
> You could argue pushModal should also take a focus.  But as above I'd argue we
> do want an independent function to just move the clutter focus around for stuff
> that knows it's in the overview.

I think if you make pushModal save and popModal restore focus, then you can just use stage.set_key_focus() to move the focus around the screen. And in fact, since ClutterEntry is going to grab the focus on its own, without our intervention, and without even any notification we can hook to, there's no way wrapping set_key_focus() is going to work.

(You will need to pass an identifying actor to pushModal() then, since it will be important *which* modal is popped)

Created attachment 143197 [details] [review]
Change keyboard handling API to handle nested modal calls

Rename beginModal/endModal to pushModal/popModal.  All of the current callers
just want to ensure that we're in a modal state; they don't actually need to
fail if we already are.

These functions also now take the Clutter keyboard focus, while recording
the previous focus.

Created attachment 143199 [details] [review]
Change keyboard handling API to handle nested modal calls

Rename beginModal/endModal to pushModal/popModal.  All of the current callers
just want to ensure that we're in a modal state; they don't actually need to
fail if we already are.

These functions also now take the Clutter keyboard focus, while recording
the previous focus.

Review of attachment 143199 [details] [review]:

I'd prefer it if the actor passed to pushModal/popModal wasn't required to be the focus widget but was just some arbitrary actor (overview group, runDialog box, etc). You could pass in the focus actor secondarily, but I think it might be easier and cleaner to just focus the focus actor using actor.grab_key_focus() after pushing modal.

(In reply to comment #20)
> Review of attachment 143199 [details] [review]:
> 
> I'd prefer it if the actor passed to pushModal/popModal wasn't required to be
> the focus widget but was just some arbitrary actor (overview group, runDialog
> box, etc). You could pass in the focus actor secondarily, but I think it might
> be easier and cleaner to just focus the focus actor using
> actor.grab_key_focus() after pushing modal.

How would passing back the focus work then in the recursive call case?  Short of some sort of ShellWindow with a _get_default method or the like?

(In reply to comment #21)
> (In reply to comment #20)
> > Review of attachment 143199 [details] [review] [details]:
> > 
> > I'd prefer it if the actor passed to pushModal/popModal wasn't required to be
> > the focus widget but was just some arbitrary actor (overview group, runDialog
> > box, etc). You could pass in the focus actor secondarily, but I think it might
> > be easier and cleaner to just focus the focus actor using
> > actor.grab_key_focus() after pushing modal.
> 
> How would passing back the focus work then in the recursive call case?  Short
> of some sort of ShellWindow with a _get_default method or the like?

I don't understand the problem - every time you push you just call global.stage.get_key_focus() and that's what you restore to on the matched pop.

(In reply to comment #22)

> I don't understand the problem - every time you push you just call
> global.stage.get_key_focus() and that's what you restore to on the matched pop.

That would mean we'd have to go back to running LG from an idle in the run dialog, do you consider that correct?

Enter overview - modal stack is

 actor,     saved_focus
 (overview, <random actor)

Enter Alt-F2, modal stack is:

 (overview, <random actor)
 (runDialog, dash entry)

Enter LG, modal stack is:

 (overview, <random actor)
 (runDialog, dash entry)
 (lookingGlass, run_dialog entry)

Close runDialog, modal stack is:

 (overview, <random actor)
 (lookingGlass, dash entry)

[ Note the rewriting when removing an item from the middle ]

Close lookingGlass, focus is restored to dash entry, stack is:

 (overview, <random actor)

Oh, I see so you were thinking we would save a pair on the stack, and call global.stage.get_key_focus() at that time?

Yep

Attachment 143199 [details] pushed as 8a2bfd0 - Change keyboard handling API to handle nested modal calls