Re: [PATCH RFC v2] Add support for LED state extension to gvnc

["Daniel P. Berrange" <dan berrange com>]

On Wed, May 08, 2013 at 11:04:39AM +0800, Lei Li wrote:
> On 05/07/2013 05:12 PM, Daniel P. Berrange wrote:
> > On Tue, May 07, 2013 at 03:35:02PM +0800, Lei Li wrote:
> > > On 05/07/2013 12:24 PM, Lei Li wrote:
> > > > On 05/06/2013 02:47 AM, Daniel P. Berrange wrote:
> > > > > On Sun, May 05, 2013 at 10:16:16PM +0800, Lei Li wrote:
> > > > > > diff --git a/src/libgvnc_sym.version b/src/libgvnc_sym.version
> > > > > > index 9adec25..20593ba 100644
> > > > > > --- a/src/libgvnc_sym.version
> > > > > > +++ b/src/libgvnc_sym.version
> > > > > > @@ -87,6 +87,7 @@
> > > > > >      vnc_connection_set_audio_format;
> > > > > >      vnc_connection_get_audio_format;
> > > > > >      vnc_connection_set_audio;
> > > > > > +        vnc_connection_get_ledstate;
> > > > > Indentation is off wrt other lines
> > > > Oh, seems it needs a tab not blanks here..
> > > >
> > > > > > diff --git a/src/vncconnection.c b/src/vncconnection.c
> > > > > > index 4b25a96..f86442c 100644
> > > > > > --- a/src/vncconnection.c
> > > > > > +++ b/src/vncconnection.c
> > > > > > @@ -217,6 +217,7 @@ struct _VncConnectionPrivate
> > > > > >      guint8 zrle_pi;
> > > > > >      int zrle_pi_bits;
> > > > > >  +    int ledstate;
> > > > > What does this integer represent ?  I'm guessing you're somehow
> > > > > using it as a bitfield ? If so, then you should be defining an
> > > > > enum with some constants for each bit in the header file.
> > > > Yes, you are right.
> > > > OK, sure.
> > > Sorry, my misunderstanding before. 'ledstate' is not a bitfield. In the implementation
> > > over Qemu VNC server, it's just an integer.
> > What does that integer represent though ? QEMU must have some sematics for it
> > and you must define some semantics for it in the RFB protocol extension.  Just
> > saying it is an integer is meaningless over QEMU side
>
> The 'ledstate' represented in QEMU as:
>
> static int current_led_state(VncState *vs)
> {
>     int ledstate = 0;
>
>     if (vs->modifiers_state[0x46]) {
>         ledstate |= QEMU_SCROLL_LOCK_LED;
>     }
>     if (vs->modifiers_state[0x45]) {
>         ledstate |= QEMU_NUM_LOCK_LED;
>     }
>     if (vs->modifiers_state[0x3a]) {
>         ledstate |= QEMU_CAPS_LOCK_LED;
>     }
>
>     return ledstate;
> }
>
> #define QEMU_SCROLL_LOCK_LED (1 << 0)
> #define QEMU_NUM_LOCK_LED    (1 << 1)
> #define QEMU_CAPS_LOCK_LED   (1 << 2)

So include these constants in the GVNC patch so that people can
interpret the contents of the led-state integer.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|