Re: [Qemu-devel] [PULL 4/7] ui: add multimedia keys

[Gerd Hoffmann <kraxel@redhat.com>]

On Thu, 2017-07-27 at 18:45 +0100, Daniel P. Berrange wrote:
> On Thu, Jul 27, 2017 at 04:00:22PM +0200, Gerd Hoffmann wrote:
> > Add multimedia keys to QKeyCodes and to the keymaps.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Message-id: 20170726152918.11995-5-kraxel@redhat.com
> > ---
> >  ui/input-keymap.c | 44
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  qapi-schema.json  | 28 +++++++++++++++++++++++++++-
> >  2 files changed, 71 insertions(+), 1 deletion(-)
> > 
> > diff --git a/ui/input-keymap.c b/ui/input-keymap.c
> > index 7461e1edde..ae781beae9 100644
> > --- a/ui/input-keymap.c
> > +++ b/ui/input-keymap.c
> > @@ -116,6 +116,28 @@ static int linux_to_qcode[KEY_CNT] = {
> >      [KEY_LEFTMETA]       = Q_KEY_CODE_META_L,
> >      [KEY_RIGHTMETA]      = Q_KEY_CODE_META_R,
> >      [KEY_MENU]           = Q_KEY_CODE_MENU,
> > +
> > +    [KEY_SLEEP]          = Q_KEY_CODE_SLEEP,
> > +    [KEY_WAKEUP]         = Q_KEY_CODE_WAKE,
> > +    [KEY_CALC]           = Q_KEY_CODE_CALCULATOR,
> > +    [KEY_MAIL]           = Q_KEY_CODE_MAIL,
> > +    [KEY_COMPUTER]       = Q_KEY_CODE_COMPUTER,
> > +
> > +    [KEY_STOP]           = Q_KEY_CODE_AC_STOP,
> 
> We already have a Q_KEY_CODE_STOP that I think is the correct mapping
> for KEY_STOP, so this new Q_KEY_CODE_AC_STOP looks like a dupe.

Oops, missed that indeed.

> > +    [KEY_BOOKMARKS]      = Q_KEY_CODE_AC_BOOKMARKS,
> > +    [KEY_BACK]           = Q_KEY_CODE_AC_BACK,
> > +    [KEY_FORWARD]        = Q_KEY_CODE_AC_FORWARD,
> > +    [KEY_HOMEPAGE]       = Q_KEY_CODE_AC_HOME,
> > +    [KEY_REFRESH]        = Q_KEY_CODE_AC_REFRESH,
> > +    [KEY_FIND]           = Q_KEY_CODE_AC_SEARCH,
> 
> Similarly we already have a Q_KEY_CODE_FIND that should map to
> KEY_FIND, so this Q_KEY_CODE_AC_SEARCH is another dup AFAICT.

Yes.

> I'm curious what the 'AC_' prefix on all these is indicating ?
> Do we actually need it ?

Seems to stand for "application control".

# grep " AC " include/standard-headers/linux/input-event-codes.h 
 * AC - Application Control
#define KEY_STOP                128     /* AC Stop */
#define KEY_PROPS               130     /* AC Properties */
#define KEY_UNDO                131     /* AC Undo */
[ ... ]

I think it is better to keep it, even if it is inconsistent because we
already have stop + find without prefix.   Just dropping the ac_ prefix
will not work for some keys, ac_home for example.

> Missing Q_KEY_CODE_MEDIASELECT entry - presumably it was supposed
> to map to  KEY_MEDIA

There is KEY_SELECT too, but not KEY_MEDIASELECT.  Hmm.

cheers,
  Gerd