[Qemu-devel] patch: add delay=<msecs> suboption to -display curses

[Qemu-devel] patch: add delay=<msecs> suboption to -display curses

[Dave Mielke]

[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]

The attached patch (qemu-curses-delay-1.patch) allows the user to specify that 
he needs -display curses to insert a delay in between key events. The current 
behaviour is that it inserts key events immediately, one right after another, 
which has proven to be too fast for some applications. Please let me know if 
there are any improvements you'd like me to make to this patch, with the goal 
that I'm hoping you'll accept it. A more detailed description of this patch is 
as follows:

Add support for the "-display curses" option to accept suboptions (-display 
curses[,option...), and add the "delay=<msecs>" suboption. This suboption 
causes a millisecond-based delay to be inserted in between key events so that 
they won't be inserted immediately one after another. The delay is performed 
using a qemu virtual clock timer. If the "delay" option isn't specified, or if 
"delay=0" is specified, then the timer isn't used, thus making the default be 
the original behaviour. The "=<msecs>" operand is optional - if it isn't 
specified then 20 is assumed.

-- 
Dave Mielke           | 2213 Fox Crescent | The Bible is the very Word of God.
Phone: 1-613-726-0014 | Ottawa, Ontario   | http://Mielke.cc/bible/
EMail: dave@mielke.cc | Canada  K2A 1H7   | http://FamilyRadio.com/

[-- Attachment #2: qemu-curses-delay-1.patch --]
[-- Type: text/plain, Size: 6598 bytes --]

diff --git a/include/ui/console.h b/include/ui/console.h
index 8a86617..d131260 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -338,6 +338,7 @@ static inline int vnc_display_pw_expire(DisplayState *ds, time_t expires)
 #endif
 
 /* curses.c */
+void curses_parse_options(const char *options);
 void curses_display_init(DisplayState *ds, int full_screen);
 
 /* input.c */
diff --git a/qemu-options.hx b/qemu-options.hx
index c2c0823..1e407bd 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -820,9 +820,11 @@ ETEXI
 
 DEF("display", HAS_ARG, QEMU_OPTION_display,
     "-display sdl[,frame=on|off][,alt_grab=on|off][,ctrl_grab=on|off]\n"
-    "            [,window_close=on|off]|curses|none|\n"
-    "            gtk[,grab_on_hover=on|off]|\n"
-    "            vnc=<display>[,<optargs>]\n"
+    "            [,window_close=on|off]\n"
+    "         none\n"
+    "         curses[,delay[=<msecs>]]\n"
+    "         gtk[,grab_on_hover=on|off]\n"
+    "         vnc=<display>[,<optargs>]\n"
     "                select display type\n", QEMU_ARCH_ALL)
 STEXI
 @item -display @var{type}
diff --git a/ui/curses.c b/ui/curses.c
index b044790..45719a4 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -176,6 +176,82 @@ static void curses_cursor_position(DisplayChangeListener *dcl,
 
 static kbd_layout_t *kbd_layout = NULL;
 
+struct KeyEvent {
+    struct KeyEvent *next;
+    int code;
+    bool down;
+};
+
+static struct KeyEvent *firstKeyEvent = NULL;
+static struct KeyEvent *lastKeyEvent = NULL;
+
+static unsigned long long keyEventDelay = 0;
+static QEMUTimer *keyEventTimer = NULL;
+
+static void curses_begin_key_event(void);
+
+static void curses_send_key_event(int code, bool down)
+{
+    qemu_input_event_send_key_number(NULL, code, down);
+}
+
+static void curses_end_key_event(void *opaque)
+{
+    struct KeyEvent *event = firstKeyEvent;
+
+    if ((firstKeyEvent = event->next))
+    {
+        curses_begin_key_event();
+    }
+    else
+    {
+        lastKeyEvent = NULL;
+    }
+
+    g_free(event);
+}
+
+static void curses_begin_key_event(void)
+{
+    struct KeyEvent *event = firstKeyEvent;
+
+    if (!keyEventTimer)
+    {
+        keyEventTimer = timer_new_ms(QEMU_CLOCK_VIRTUAL, curses_end_key_event, NULL);
+    }
+
+    curses_send_key_event(event->code, event->down);
+    timer_mod(keyEventTimer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + keyEventDelay);
+}
+
+static void curses_enqueue_key_event(int code, bool down)
+{
+    if (keyEventDelay)
+    {
+        struct KeyEvent *event;
+
+        event = g_malloc(sizeof(*event));
+        event->next = NULL;
+        event->code = code;
+        event->down = down;
+
+        if (lastKeyEvent)
+        {
+            lastKeyEvent->next = event;
+            lastKeyEvent = event;
+        }
+        else
+        {
+            firstKeyEvent = lastKeyEvent = event;
+            curses_begin_key_event();
+        }
+    }
+    else
+    {
+        curses_send_key_event(code, down);
+    }
+}
+
 static void curses_refresh(DisplayChangeListener *dcl)
 {
     int chr, nextchr, keysym, keycode, keycode_alt;
@@ -276,32 +352,32 @@ static void curses_refresh(DisplayChangeListener *dcl)
             /* since terminals don't know about key press and release
              * events, we need to emit both for each key received */
             if (keycode & SHIFT) {
-                qemu_input_event_send_key_number(NULL, SHIFT_CODE, true);
+                curses_enqueue_key_event(SHIFT_CODE, true);
             }
             if (keycode & CNTRL) {
-                qemu_input_event_send_key_number(NULL, CNTRL_CODE, true);
+                curses_enqueue_key_event(CNTRL_CODE, true);
             }
             if (keycode & ALT) {
-                qemu_input_event_send_key_number(NULL, ALT_CODE, true);
+                curses_enqueue_key_event(ALT_CODE, true);
             }
             if (keycode & ALTGR) {
-                qemu_input_event_send_key_number(NULL, GREY | ALT_CODE, true);
+                curses_enqueue_key_event(GREY | ALT_CODE, true);
             }
 
-            qemu_input_event_send_key_number(NULL, keycode, true);
-            qemu_input_event_send_key_number(NULL, keycode, false);
+            curses_enqueue_key_event(keycode, true);
+            curses_enqueue_key_event(keycode, false);
 
             if (keycode & ALTGR) {
-                qemu_input_event_send_key_number(NULL, GREY | ALT_CODE, false);
+                curses_enqueue_key_event(GREY | ALT_CODE, false);
             }
             if (keycode & ALT) {
-                qemu_input_event_send_key_number(NULL, ALT_CODE, false);
+                curses_enqueue_key_event(ALT_CODE, false);
             }
             if (keycode & CNTRL) {
-                qemu_input_event_send_key_number(NULL, CNTRL_CODE, false);
+                curses_enqueue_key_event(CNTRL_CODE, false);
             }
             if (keycode & SHIFT) {
-                qemu_input_event_send_key_number(NULL, SHIFT_CODE, false);
+                curses_enqueue_key_event(SHIFT_CODE, false);
             }
         } else {
             keysym = curses2qemu[chr];
@@ -349,6 +425,51 @@ static void curses_keyboard_setup(void)
     }
 }
 
+void curses_parse_options(const char *options)
+{
+    keyEventDelay = 0;
+
+    while (*options)
+    {
+        const char *next;
+        char *end;
+
+        if (!strstart(options, ",", &next))
+        {
+            break;
+        }
+        options = next;
+
+        if (strstart(options, "delay", &next))
+        {
+            options = next;
+            keyEventDelay = 20;
+
+            if (strstart(options, "=", &next))
+            {
+                options = next;
+
+                if (parse_uint(options, &keyEventDelay, &end, 10))
+                {
+                    break;
+                }
+                options = end;
+            }
+        }
+
+        else
+        {
+            break;
+        }
+    }
+
+    if (*options)
+    {
+        fprintf(stderr, "qemu: invalid Curses display option: %s\n", options);
+        exit(1);
+    }
+}
+
 static const DisplayChangeListenerOps dcl_ops = {
     .dpy_name        = "curses",
     .dpy_text_update = curses_update,
diff --git a/vl.c b/vl.c
index 709d8cd..bd6c583 100644
--- a/vl.c
+++ b/vl.c
@@ -2300,6 +2300,7 @@ static DisplayType select_display(const char *p)
     } else if (strstart(p, "curses", &opts)) {
 #ifdef CONFIG_CURSES
         display = DT_CURSES;
+        curses_parse_options(opts);
 #else
         fprintf(stderr, "Curses support is disabled\n");
         exit(1);

Re: [Qemu-devel] patch: add delay=<msecs> suboption to -display curses

[Peter Maydell]

On 25 May 2014 00:29, Dave Mielke <dave@mielke.cc> wrote:
> The attached patch (qemu-curses-delay-1.patch) allows the user to specify that
> he needs -display curses to insert a delay in between key events. The current
> behaviour is that it inserts key events immediately, one right after another,
> which has proven to be too fast for some applications. Please let me know if
> there are any improvements you'd like me to make to this patch, with the goal
> that I'm hoping you'll accept it. A more detailed description of this patch is
> as follows:
>
> Add support for the "-display curses" option to accept suboptions (-display
> curses[,option...), and add the "delay=<msecs>" suboption. This suboption
> causes a millisecond-based delay to be inserted in between key events so that
> they won't be inserted immediately one after another. The delay is performed
> using a qemu virtual clock timer. If the "delay" option isn't specified, or if
> "delay=0" is specified, then the timer isn't used, thus making the default be
> the original behaviour. The "=<msecs>" operand is optional - if it isn't
> specified then 20 is assumed.

Why is this a problem only for the curses UI frontend, and not for
any of the other UIs which might send key events?

thanks
-- PMM

Re: [Qemu-devel] patch: add delay=<msecs> suboption to -display curses

[Dave Mielke]

[quoted lines by Peter Maydell on 2014/05/25 at 01:04 +0100]

>Why is this a problem only for the curses UI frontend, and not for
>any of the other UIs which might send key events?

One reason is that most UIs send key events as they receive them from the 
keyboard, one at a time, whereas the curses UI gets a single key event from 
curses and then sends all of the scancodes which it takes to represent that 
key. In other words, a single key event from curses could result in as many as 
10 actual key events needing to be passed along - both the press and release 
for the actual key plus up to four modifiers (shift, control, alt, altgr).

Another reason is that it's not a problem for a lot of applications. I happened 
to hit it in an application, and, therefore, coded a fix for it.

-- 
Dave Mielke           | 2213 Fox Crescent | The Bible is the very Word of God.
Phone: 1-613-726-0014 | Ottawa, Ontario   | http://Mielke.cc/bible/
EMail: dave@mielke.cc | Canada  K2A 1H7   | http://FamilyRadio.com/

Re: [Qemu-devel] patch: add delay=<msecs> suboption to -display curses

[Peter Maydell]

On 25 May 2014 02:21, Dave Mielke <dave@mielke.cc> wrote:
> [quoted lines by Peter Maydell on 2014/05/25 at 01:04 +0100]
>>Why is this a problem only for the curses UI frontend, and not for
>>any of the other UIs which might send key events?
>
> One reason is that most UIs send key events as they receive them from the
> keyboard, one at a time, whereas the curses UI gets a single key event from
> curses and then sends all of the scancodes which it takes to represent that
> key. In other words, a single key event from curses could result in as many as
> 10 actual key events needing to be passed along - both the press and release
> for the actual key plus up to four modifiers (shift, control, alt, altgr).

Ah, I see. Still, I think it makes more sense for the queue and delay
to be in the common key handling code, not in the curses frontend
specifically.

thanks
-- PMM

Re: [Qemu-devel] patch: add delay=<msecs> suboption to -display curses

[Dave Mielke]

[quoted lines by Peter Maydell on 2014/05/25 at 10:11 +0100]

>Ah, I see. Still, I think it makes more sense for the queue and delay
>to be in the common key handling code, not in the curses frontend
>specifically.

Yes, you're right. While the curses UI is especially vulnerable to the problem, 
others could be as well, especially if they're driven by a software "user" 
rather than by a human being. Also, who's to say that there won't be a new UI 
in the future which won't run into the same problem. I'll move the code and 
submit a new patch.

Given that this'll require a brand new option, could you please let me know all 
the places that'll need updating insofar as documentation is concerned?

-- 
Dave Mielke           | 2213 Fox Crescent | The Bible is the very Word of God.
Phone: 1-613-726-0014 | Ottawa, Ontario   | http://Mielke.cc/bible/
EMail: dave@mielke.cc | Canada  K2A 1H7   | http://FamilyRadio.com/

Re: [Qemu-devel] patch: add delay=<msecs> suboption to -display curses

[Dave Mielke]

[quoted lines by Peter Maydell on 2014/05/25 at 10:11 +0100]

>Ah, I see. Still, I think it makes more sense for the queue and delay
>to be in the common key handling code, not in the curses frontend
>specifically.

The code has been moved. I can see a couple of possibilities insofar as an 
option for it is concerned. One is to extend the existing -k option to accept 
language= as an optional prefix for its currnet operand, and to add delay=. The 
other is to add a brand new -keydelay option. Which would be the better 
approach?

I'm assuming that qemu-options.hx is the only file that needs to be updated 
insofar as documentation is concerned. Is that correct?

-- 
Dave Mielke           | 2213 Fox Crescent | The Bible is the very Word of God.
Phone: 1-613-726-0014 | Ottawa, Ontario   | http://Mielke.cc/bible/
EMail: dave@mielke.cc | Canada  K2A 1H7   | http://FamilyRadio.com/

Re: [Qemu-devel] [Qemu-trivial] patch: add delay=<msecs> suboption to -display curses

[Michael Tokarev]

25.05.2014 03:29, Dave Mielke wrote:

> Add support for the "-display curses" option to accept suboptions (-display 
> curses[,option...), and add the "delay=<msecs>" suboption. This suboption 
> causes a millisecond-based delay to be inserted in between key events so that 

In addition to what Peter said, I think this suboption is named poorly.
Maybe it can be named, say, kbddelay, or keydelay, or something like that.
Just "delay" may be interpreted as _video_ delay, ie, delay updating picture
for so many millisecs.  After all, this is -display, which is about video,
and keyboard is a secondary function...

Also, you still haven't told us what application is having problem with it,
it is some mystery "some app".  And provided this hasn't been a problem for
so many years, maybe we shouldn't add it in the first place?

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-trivial] patch: add delay=<msecs> suboption to -display curses

[Dave Mielke]

[quoted lines by Michael Tokarev on 2014/05/25 at 16:41 +0400]

>In addition to what Peter said, I think this suboption is named poorly.
>Maybe it can be named, say, kbddelay, or keydelay, or something like that.
>Just "delay" may be interpreted as _video_ delay, ie, delay updating picture
>for so many millisecs.  After all, this is -display, which is about video,
>and keyboard is a secondary function...

Yes, that makes good sense.

>Also, you still haven't told us what application is having problem with it,
>it is some mystery "some app".  

Where I ran into it is DJGPP termios input on MS-DOS.

>And provided this hasn't been a problem for so many years, 

Perhaps it hasn't been observed before because the curses UI may not have that 
many users. I myself must use it as, being blind, it's much easier for me to 
work in text mode than in graphics mode.

>maybe we shouldn't add it in the first place?

Given that this feature does resolve a problem, given that it's default retains 
the original behaviour, and given that we have no idea whatsoever regarding 
when the problem may strike again, my personal opinion is that implementing it 
now would be a reasonable thing to do, especially since the current situation 
is that no one is being asked to figure it out, code it, etc.

-- 
Dave Mielke           | 2213 Fox Crescent | The Bible is the very Word of God.
Phone: 1-613-726-0014 | Ottawa, Ontario   | http://Mielke.cc/bible/
EMail: dave@mielke.cc | Canada  K2A 1H7   | http://FamilyRadio.com/