[Spice-devel] [PATCH spice-html5 v2] spice_auto: Add button to send Ctrl-Alt-Delete

[Jeremy White]

Hi Tomáš,

This is logically two patches, not one.  Could you split them?

Further comments inline.

On 09/14/2017 04:34 AM, Tomáš Bohdálek wrote:
> This already uses the added function sendCtrlAltDel().
> ---
>  inputs.js       | 9 ++++++---
>  spice_auto.html | 1 +
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/inputs.js b/inputs.js
> index 29f4970..0f77b66 100644
> --- a/inputs.js
> +++ b/inputs.js
> @@ -193,14 +193,17 @@ function sendCtrlAltDel()
>          update_modifier(true, KEY_LCtrl, sc);
>          update_modifier(true, KEY_Alt, sc);
>  
> -        key.code = KEY_KP_Decimal;
> +        key.code = common_scanmap[46];
>          msg.build_msg(SPICE_MSGC_INPUTS_KEY_DOWN, key);
>          sc.inputs.send_msg(msg);
>          msg.build_msg(SPICE_MSGC_INPUTS_KEY_UP, key);
>          sc.inputs.send_msg(msg);
>  
> -        if(Ctrl_state == false) update_modifier(false, KEY_LCtrl, sc);
> -        if(Alt_state == false) update_modifier(false, KEY_Alt, sc);
> +        /* We have to send this, otherwise the function will not work on Linux */
> +        update_modifier(false, KEY_KP_Decimal, sc);
> +
> +        update_modifier(false, KEY_Alt, sc);
> +        update_modifier(false, KEY_LCtrl, sc);

I'm afraid I find myself worried by these changes.  Why have we gone
from a nice constant to a magic number (46)?  You assert that the
function will not work on Linux, but you don't really say why.

With that said, the whole "if(Ctrl_state == false)" prior logic simply
looks wrong on the face of it.  It'd be nice if we could understand the
original authors motive for that particular bit of code.

>      }
>  }
>  
> diff --git a/spice_auto.html b/spice_auto.html
> index 2f04fc9..5c90cbc 100644
> --- a/spice_auto.html
> +++ b/spice_auto.html
> @@ -204,6 +204,7 @@
>  
>          <div id="login">
>              <span class="logo">SPICE</span>
> +            <button id="sendCtrlAltDelButton" onclick="sendCtrlAltDel();">Send Ctrl-Alt-Delete</button>
>          </div>
>  
>          <div id="spice-area">
> 

Why only spice_auto and not spice.html too?

I also have to confess that, as my primary use case is XSpice to Linux
systems, this button makes me wince.  Do we have any way of detecting
the OS of the remote side and doing this in a dynamic way?

Cheers,

Jeremy