imho, this is an "advanced" binding, and should just be documented in manual, not cluttering the UI. my 2c

It is possible to use the "keyboard-grab" signal which is emitted when the grab changes.

(In reply to comment #2)
> It is possible to use the "keyboard-grab" signal which is emitted when the grab
> changes.

yes but it doesn't tell me what the change was so i can't really use it.

(In reply to comment #1)
> imho, this is an "advanced" binding, and should just be documented in manual,
> not cluttering the UI. my 2c

hmm.. maybe you are right.

Created attachment 295171 [details] [review]
Use "keyboard-grab" signal to detect the change of the grab

Review of attachment 295171 [details] [review]:

Good patch but I think we want a different hint displayed for this keyboard grab since the kbd grab is not really user-visible, like mouse-grab.

::: src/spice-display.vala
@@ +122,3 @@
                 mouse_grabbed = status != 0;
             });
+            display.keyboard_grab.connect((status) => {

Ah, I missed the fact that it has a status argument.

Review of attachment 295171 [details] [review]:

I wouldn't update this title here, but I let designers and maintainers decide.

Created attachment 295190 [details] [review]
Give hint about keyboard ungrab / grab

Review of attachment 295190 [details] [review]:

* Give hint -> Hint (since it can be used as verb itself)
* Use tend to use prefix in shortlog. "display: " would be appropriate here.
* A bit of description would be nice.

::: src/display-page.vala
@@ +99,3 @@
         string? hint = null;
         if (grabbed)
+            hint = _("All keys currently received by guest. Press & release Ctrl+Alt to change.");

I think 'guest' -> name of machine (in '' since it can have spaces).

@@ -99,3 @@
         string? hint = null;
         if (grabbed)
-            hint = _("(press [left] Ctrl+Alt keys to ungrab)");

We still want this for mouse grab. I think we should rename 'grabbed' to 'mouse_grabbed' and create a new property for keyboard grab.

@@ +101,3 @@
+            hint = _("All keys currently received by guest. Press & release Ctrl+Alt to change.");
+        else if (event_box.get_child ().has_focus)
+            hint = _("All keys currently received by client. Press & release Ctrl+Alt to change.");

similarly, client -> hostname.

::: src/display.vala
@@ +9,3 @@
     public bool can_grab_mouse { get; protected set; }
     public bool mouse_grabbed { get; protected set; }
+    public bool keyboard_grabbed { get; protected set; }

Addition of this property is a different logical change so lets put that in a separate patch.

(In reply to comment #4)
> (In reply to comment #1)
> > imho, this is an "advanced" binding, and should just be documented in manual,
> > not cluttering the UI. my 2c
> 
> hmm.. maybe you are right.

Thinking about accessibility, I think it would be good to provide the hint for disabled users who dont have a pointing device and have not yet read the manual.

Pavel, Do you intend to fix the patch as per last review?

Created attachment 296181 [details] [review]
display: add keyboard grabbed property

Created attachment 296182 [details] [review]
display-page: Hint about keyboard grab

Review of attachment 296181 [details] [review]:

Looks good. I'll Capitalize 'add' before pushing..

Review of attachment 296182 [details] [review]:

Some nitpicks on the description while you are fixing other things:

* "an user" -> "the user" (or just 'users').
* "is possible" -> "".
* Its also for sending keys to host system (e.g Super key after ungrab should launch overview mode of gnome-shell). I'd just remove the last line.
* Please put '.' after sentences.

::: src/display-page.vala
@@ +34,3 @@
         }
     }
+    private bool mouse_grabbed {

Separate change (along with update of its current usage below) so separate patch.

@@ +43,3 @@
+            return display != null ? display.keyboard_grabbed : false;
+        }
+    }

Ideally this should also go into separate patch but not biggie since its private.

@@ +103,2 @@
         string? hint = null;
+        if (can_grab_mouse) {

Do we really need to care about this? I'd go with much simpler logic:

if (mouse_grabbed)
   hint = _("(press [left] Ctrl+Alt keys to ungrab)");
else if (keyboard_grabbed)
   hint = _("All keys currently received by guest. Press & release Ctrl+Alt to change.");
else if ((event_box.get_child ().has_focus)
   hint = _("All keys currently received by client. Press & release Ctrl+Alt to change.");

Unless there is something wrong with this logic?

@@ +109,3 @@
+                hint = _("All keys currently received by guest. Press & release Ctrl+Alt to change.");
+            else if (event_box.get_child ().has_focus)
+                hint = _("All keys currently received by client. Press & release Ctrl+Alt to change.");

As pointed out in previous review:

* I think 'guest' -> name of machine (in '' since it can have spaces).
* similarly, client -> hostname.

@@ +137,3 @@
             });
         });
+        display_grabbed_id = display.notify["keyboard-grabbed"].connect(() => {

display_grabbed_id is already being used for mouse-grabbed notify. Just like the property rename, you want to rename this field and add a new separate one for keyboard-grabbed notify.

@@ +159,3 @@
         event_box.add (widget);
         event_box.show_all ();
+        update_subtitle ();

Why do we need to bring this call down here?

(In reply to comment #14)

> Do we really need to care about this? I'd go with much simpler logic:
> 
> if (mouse_grabbed)
>    hint = _("(press [left] Ctrl+Alt keys to ungrab)");
> else if (keyboard_grabbed)
>    hint = _("All keys currently received by guest. Press & release Ctrl+Alt to
> change.");
> else if ((event_box.get_child ().has_focus)
>    hint = _("All keys currently received by client. Press & release Ctrl+Alt to
> change.");
> 
> Unless there is something wrong with this logic?
> 
Try to connect to a machine which doesn't have the spice vdagent. In that case changes of the subtitle looks strange.

> 
> @@ +159,3 @@
>          event_box.add (widget);
>          event_box.show_all ();
> +        update_subtitle ();
> 
> Why do we need to bring this call down here?

To avoid a runtime warning - the widget is not in the event_box and we do event_box.get_child ().has_focus...

Created attachment 296446 [details] [review]
display-page: Add keyboard grabbed property

Created attachment 296449 [details] [review]
display-page: Rename grabbed to mouse_grabbed

Created attachment 296450 [details] [review]
display-page: Hint about keyboard grab

Zeeshan, I don't know how to get the hostname, please add a patch for it.

(In reply to Pavel Grunt from comment #18)
> Created attachment 296450 [details] [review] [review]
> display-page: Hint about keyboard grab
> 
> Zeeshan, I don't know how to get the hostname, please add a patch for it.

g_get_host_name(), in vala that is: GLib.Environment.get_host_name().

(In reply to Pavel Grunt from comment #15)
> (In reply to comment #14)

Not a biggie but its usually better to use the 'Review' link to reply to inline comments.

> > Do we really need to care about this? I'd go with much simpler logic:
> > 
> > if (mouse_grabbed)
> >    hint = _("(press [left] Ctrl+Alt keys to ungrab)");
> > else if (keyboard_grabbed)
> >    hint = _("All keys currently received by guest. Press & release Ctrl+Alt to
> > change.");
> > else if ((event_box.get_child ().has_focus)
> >    hint = _("All keys currently received by client. Press & release Ctrl+Alt to
> > change.");
> > 
> > Unless there is something wrong with this logic?
> > 
> Try to connect to a machine which doesn't have the spice vdagent. In that
> case changes of the subtitle looks strange.

What happens in that case? Would be nice to find out why that is the case.

> > 
> > @@ +159,3 @@
> >          event_box.add (widget);
> >          event_box.show_all ();
> > +        update_subtitle ();
> > 
> > Why do we need to bring this call down here?
> 
> To avoid a runtime warning - the widget is not in the event_box and we do
> event_box.get_child ().has_focus...

That doesn't sound like a real solution. How about not calling .has_focus if there is no child yet? i-e lets add a null check to the 'if' condition making this call.

Created attachment 296563 [details] [review]
display-page: Hint about keyboard grab

Review of attachment 296563 [details] [review]:

Looks good and i'll push once you answer the question in inline comment.

::: src/display-page.vala
@@ +108,3 @@
+            hint = keyboard_grab_fmt.printf (machine.name);
+        else if (event_box.get_child () != null && event_box.get_child ().has_focus)
+            hint = keyboard_grab_fmt.printf (GLib.Environment.get_host_name ());

1. Would be nicer if event_box.get_child () was put into a variable but I can handle that for you before pushing.

2. Whatever happened to this issue:

>> Try to connect to a machine which doesn't have the spice vdagent. In that
>> case changes of the subtitle looks strange.

> What happens in that case? Would be nice to find out why that is the case.

You can't reproduce the issue anymore?

Review of attachment 296446 [details] [review]:

nitpick: Name the property as is: either '-' or '_' instead of space..

Review of attachment 296449 [details] [review]:

ack

Review of attachment 296563 [details] [review]:

> 
> 1. Would be nicer if event_box.get_child () was put into a variable but I
> can handle that for you before pushing.
Thank you
> 
> 2. Whatever happened to this issue:
> 
> >> Try to connect to a machine which doesn't have the spice vdagent. In that
> >> case changes of the subtitle looks strange.
> 
> > What happens in that case? Would be nice to find out why that is the case.
> 
> You can't reproduce the issue anymore?

The issue is that the subtitle is switching between "(press [left] Ctrl+Alt keys to ungrab)" and "All keys currently received by client. Press & release Ctrl+Alt to change.".
I think that just hiding / showing the subtitle is better.

(In reply to Pavel Grunt from comment #25)
> Review of attachment 296563 [details] [review] [review]:

Seems its very hard to click 'Review' button. :)

> > 2. Whatever happened to this issue:
> > 
> > >> Try to connect to a machine which doesn't have the spice vdagent. In that
> > >> case changes of the subtitle looks strange.
> > 
> > > What happens in that case? Would be nice to find out why that is the case.
> > 
> > You can't reproduce the issue anymore?
> 
> The issue is that the subtitle is switching between "(press [left] Ctrl+Alt
> keys to ungrab)" and "All keys currently received by client. Press & release
> Ctrl+Alt to change.".

Ah ok.

> I think that just hiding / showing the subtitle is better.

Yes, we shouldn't show any subtitle when mouse is grabbable but not grabbed.

Created attachment 296610 [details] [review]
display-page: Hint about keyboard grab

Created attachment 296697 [details] [review]
display-page: Improved hint for upgrabbing

Created attachment 296698 [details] [review]
display-page: Rename grabbed to mouse_grabbed

We'll soon want properties/fields for keyboard grab too so this is to
keep the code more clear/readable.

bsoletes: 296449 - display-page: Rename grabbed to mouse_grabbed

Created attachment 296699 [details] [review]
display: Add keyboard_grabbed property

We'll need this in following patches for monitoring keyboard grab changes.

Created attachment 296700 [details] [review]
display-page: Add keyboard_grabbed property

Created attachment 296701 [details] [review]
display-page: Hint about keyboard grab

Inform users how to release the keyboard grab.

bsoletes: 296610 - display-page: Hint about keyboard grab

So I fixed a few minor things and changed the actual patch (attachment 296701 [details] [review]) a lot! Now I know what you (Pavel) was talking about after some testing about the focus thing. The issue is that when you move mouse away from display widget, while the widget remains focused and most of the keys are still going to it, non-window-manager keys are not and spice declares this as !kbd_grab. i-e we are in a mixed weird state with no indication from spice about it.

So I decided to go with one of your original approaches and to only provide hint when kbd is grabbed completely. The main point was that if a first time user who doesn't have a pointing device (e.g blind users who rely heavily on keyboard navigation) and doesn't know of the shortcut, is easily able to figure out how to get out of the guest display.

Review of attachment 296697 [details] [review]:

looks good to me. Why didn't we do that in the first place, when we added the '[left]'...

Review of attachment 296698 [details] [review]:

Looks good otherwise.

::: src/display-page.vala
@@ +98,3 @@
         string? hint = null;
+        if (mouse_grabbed)
+            hint = _("(press [left] Ctrl+Alt keys to ungrab)");

Changing it back here?

Review of attachment 296699 [details] [review]:

ack

Review of attachment 296700 [details] [review]:

ack

Review of attachment 296701 [details] [review]:

::: src/display-page.vala
@@ -103,3 @@
         string? hint = null;
-        if (mouse_grabbed)
-            hint = _("(press [left] Ctrl+Alt keys to ungrab)");

this won't survive the rebase if you don't change back the comment in the needs-work patch

Review of attachment 296698 [details] [review]:

::: src/display-page.vala
@@ +98,3 @@
         string? hint = null;
+        if (mouse_grabbed)
+            hint = _("(press [left] Ctrl+Alt keys to ungrab)");

Meh! I thought i fixed in all commits. :(

Created attachment 296764 [details] [review]
display-page: Rename grabbed to mouse_grabbed

We'll soon want properties/fields for keyboard grab too so this is to
keep the code more clear/readable.

Co-author: Zeeshan Ali (Khattak) <zeeshanak@gnome.org>

Created attachment 296765 [details] [review]
display: Add keyboard_grabbed property

We'll need this in following patches for monitoring keyboard grab changes.

Co-author: Zeeshan Ali (Khattak) <zeeshanak@gnome.org>

Created attachment 296766 [details] [review]
display-page: Add keyboard_grabbed property

Co-author: Zeeshan Ali (Khattak) <zeeshanak@gnome.org>

Created attachment 296767 [details] [review]
display-page: Hint about keyboard grab

Inform users how to release the keyboard grab.

Co-author: Zeeshan Ali (Khattak) <zeeshanak@gnome.org>

Review of attachment 296764 [details] [review]:

ack for this patch but request for another in comment below.

::: src/display-page.vala
@@ +162,3 @@
         }
 
         if (display_can_grab_id != 0) {

I'd rename display_can_grab_id to can_grab_mouse_id too (in a seperate patch). Then we have no displays grabbing things anymore in this file.

Review of attachment 296765 [details] [review]:

ack

Review of attachment 296766 [details] [review]:

ack

Review of attachment 296767 [details] [review]:

::: src/display-page.vala
@@ +107,3 @@
+                hint = _("Press (left) Ctrl+Alt to ungrab");
+        } else if (keyboard_grabbed) {
+            hint = _("Press & release (left) Ctrl+Alt to ungrab keyboard.");

Does the user really have to release Ctrl+Alt? Does that mean that Ctrl+Alt+<any key> will be delivered to the VM? This would be a bad behaviour IMO. If not I don't think it's rectified to further increase code complexity for an own messsage just for keyboard grabbing.

The first message is pretty generic and the second doesn't add real information for me.

@@ +136,3 @@
         });
+        keyboard_grabbed_id = display.notify["keyboard-grabbed"].connect(() => {
+            update_subtitle ();

a few lines above an author states:

            // In some cases this is sent inside size_allocate (see bug #692465)
            // which causes the label change queue_resize to be ignored
            // So we delay the update_title call to an idle to work around this.
            Idle.add_full (Priority.HIGH, () => {
                update_title ();
                return false;
            });

This would apply here to, wouldn't it?

Review of attachment 296767 [details] [review]:

::: src/display-page.vala
@@ +107,3 @@
+                hint = _("Press (left) Ctrl+Alt to ungrab");
+        } else if (keyboard_grabbed) {
+            hint = _("Press & release (left) Ctrl+Alt to ungrab keyboard.");

No, just ctrl+alt, as already discussed on this bug. Stress on the word 'release' here.

Well second one is about ungrabbing kbd only (mouse is free) and in case of kbd ungrab, you have to release too (not only press) so we gotta specify.

@@ +136,3 @@
         });
+        keyboard_grabbed_id = display.notify["keyboard-grabbed"].connect(() => {
+            update_subtitle ();

Good catch. That will probably explain the gtk warnings I saw. I'll check..

Created attachment 296786 [details] [review]
display-page: Hint about keyboard grab

Although the gtk+ critical I'm getting turned out to be irrelvant to this, I've now put the title update call in an idle call..

Review of attachment 296767 [details] [review]:

::: src/display-page.vala
@@ +107,3 @@
+                hint = _("Press (left) Ctrl+Alt to ungrab");
+        } else if (keyboard_grabbed) {
+            hint = _("Press & release (left) Ctrl+Alt to ungrab keyboard.");

What I mean is, you don't have to release ctrl+alt to ungrab the keyboard IIUC.

So if you press Ctrl+Alt and hold, keyboard gets released (as the mouse would). So if you now add another key the whole thing will go to the host system so actually "Press (left) Ctrl+Alt to ungrab" is more precise than "Press & release"

Review of attachment 296767 [details] [review]:

::: src/display-page.vala
@@ +107,3 @@
+                hint = _("Press (left) Ctrl+Alt to ungrab");
+        } else if (keyboard_grabbed) {
+            hint = _("Press & release (left) Ctrl+Alt to ungrab keyboard.");

Yes, you *do* have to release ctrl+alt in this mode. You are confusing the (existing) case of mouse grab with the (new in spice) kbd grab.

Review of attachment 296786 [details] [review]:

I'm not able to test but given what you write is true this commit is good. So ack.

However in this case I dislike the behaviour by itself due to its inconsistency which is in turn not a Boxes issue.

Created attachment 296790 [details] [review]
display-page: More specific names for grab props

We'll soon want properties/fields for keyboard grab too so this is to
keep the code more clear/readable.

Co-author: Zeeshan Ali (Khattak) <zeeshanak@gnome.org>

Created attachment 296791 [details] [review]
display: Add keyboard_grabbed property

We'll need this in following patches for monitoring keyboard grab changes.

Co-author: Zeeshan Ali (Khattak) <zeeshanak@gnome.org>

Created attachment 296792 [details] [review]
display-page: Add keyboard_grabbed property

Co-author: Zeeshan Ali (Khattak) <zeeshanak@gnome.org>

Created attachment 296793 [details] [review]
display-page: Hint about keyboard grab

Inform users how to release the keyboard grab.

Co-author: Zeeshan Ali (Khattak) <zeeshanak@gnome.org>

Attachment 296697 [details] pushed as d3e9923 - display-page: Improved hint for upgrabbing
Attachment 296790 [details] pushed as 9028df0 - display-page: More specific names for grab props
Attachment 296791 [details] pushed as 8edbf41 - display: Add keyboard_grabbed property
Attachment 296792 [details] pushed as 265ff6f - display-page: Add keyboard_grabbed property
Attachment 296793 [details] pushed as 27435e0 - display-page: Hint about keyboard grab