improve/fix -curses mode

Hi,

This patchset is to improve/fix -curses mode. But this patchset is tested only on linux, so RFC. (BTW, tested with xterm/gnome-terminal on linux)

ui/curses: Fix monitor color with -curses when 256 colors ui/curses: Support line graphics chars on -curses mode ui/curses: Fix pageup/pagedown on -curses

 include/ui/console.h |   11 ++++++++++-
 ui/curses.c          |   45 +++++++++++++++++++++++++++++++++++++++++++++
 ui/curses_keys.h     |    8 ++++++--
 3 files changed, 61 insertions(+), 3 deletions(-)

Thanks.

OGAWA Hirofumi <[email protected]>

If TERM=xterm-256color, COLOR_PAIRS==256 and monitor passes chtype like 0x74xx. Then, the code uses uninitialized color pair. As result, monitor uses black for both of fg and bg color, i.e. terminal is filled by black.

To fix, this initialize above than 64 with default color (fg=white,bg=black).

FIXME: on 256 color, curses may be possible better vga color emulation.

---

 ui/curses.c |    3 +++
 1 file changed, 3 insertions(+)

diff -puN ui/curses.c~support-curses-256color ui/curses.c
@@ -343,6 +343,9 @@ static void curses_setup(void)
 
     for (i = 0; i < 64; i ++)
         init_pair(i, colour_default[i & 7], colour_default[i >> 3]);
+    /* Set default color for more than 64. (monitor uses 0x74xx for example) */
+    for (i = 64; i < COLOR_PAIRS; i ++)
+        init_pair(i, COLOR_WHITE, COLOR_BLACK);
 }
 
 static void curses_keyboard_setup(void)

_

This converts vga code to curses code in console_write_bh().

With this changes, we can see line graphics (for example, dialog uses) correctly.

---

 include/ui/console.h |   11 ++++++++++-
 ui/curses.c          |   42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)

diff -puN include/ui/console.h~support-curses-border include/ui/console.h
@@ -284,13 +284,22 @@ static inline pixman_format_code_t surfa
 #ifdef CONFIG_CURSES
 #include <curses.h>
 typedef chtype console_ch_t;
+extern chtype vga_to_curses[];
 #else
 typedef unsigned long console_ch_t;
 #endif
 static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
 {
-    if (!(ch & 0xff))
+    uint8_t c = ch;
+#ifdef CONFIG_CURSES
+    if (vga_to_curses[c]) {
+        ch &= ~(console_ch_t)0xff;
+        ch |= vga_to_curses[c];
+    }
+#else
+    if (c == '\0')
         ch |= ' ';
+#endif
     *dest = ch;
 }
 

_

Current KEY_NPAGE/KEY_PPAGE handling is broken on -curses. Those uses "GREY", but "KEY_MASK" masked out "GREY".

To fix, we have to use correct mask value - SCANCODE_KEYMASK.

Then, this adds support of "shift + pageup/pagedown". With this, -curses mode can use scroll-up/down as usual like other display modes.

---

 ui/curses_keys.h |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff -puN ui/curses_keys.h~support-curses-pageup ui/curses_keys.h
@@ -29,8 +29,7 @@
 #include "keymaps.h"
 
 
-#define KEY_RELEASE         0x80
-#define KEY_MASK            0x7f
+#define KEY_MASK            SCANCODE_KEYMASK
 #define GREY_CODE           0xe0
 #define GREY                SCANCODE_GREY
 #define SHIFT_CODE          0x2a

@@ -60,6 +59,8 @@ static const int curses2keysym[CURSES_KE
     ['\n'] = KEY_ENTER,
     [27] = 27,
     [KEY_BTAB] = '\t' | KEYSYM_SHIFT,
+    [KEY_SPREVIOUS] = KEY_PPAGE | KEYSYM_SHIFT,
+    [KEY_SNEXT] = KEY_NPAGE | KEYSYM_SHIFT,
 };
 
 static const int curses2keycode[CURSES_KEYS] = {

@@ -149,6 +150,9 @@ static const int curses2keycode[CURSES_K
     [KEY_IC] = 82 | GREY, /* Insert */
     [KEY_DC] = 83 | GREY, /* Delete */
 
+    [KEY_SPREVIOUS] = 73 | GREY | SHIFT, /* Shift + Page Up */
+    [KEY_SNEXT] = 81 | GREY | SHIFT, /* Shift + Page Down */
+
     ['!'] = 2 | SHIFT,
     ['@'] = 3 | SHIFT,
     ['#'] = 4 | SHIFT,

_

[meta-comment]

You sent your message with deep threading, with each message a reply to the previous, resulting in:

0/3
+ 1/3
+ 2/3
+ 3/3

and when someone replies, a mail client that sorts by references before age sees:

0/3
+ 1/3

| + 2/3
| | + 3/3
| | | + Re: 3/3
| | + Re: 2/3
| + Re: 1/3

+ Re: 0/3

which is rather confusing. Better is to use shallow threading, with each message in reply ONLY to the cover letter:

0/3
+ 1/3
+ 2/3
+ 3/3

and where replies are easier to follow in mail clients:

0/3
+ 1/3
| + Re: 1/3
+ 2/3
| + Re: 2/3
+ 3/3
| + Re: 3/3
+ Re: 0/3

You may want to double-check your git settings, as there are some knobs to tweak for shallow vs. default threading (I think shallow is default in newer git, but it was not always the case in really old git - but I don't remember the knob names off-hand).

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

But it breaks line graphics in vga text mode on gtk I suspect?

cheers,
Gerd

vga_to_curses[] is filled by curses_setup(). So if no -curses option, I think it should have no change to behavior. (BTW, there is no common section like kernel in qemu build, right? i.e. no need initializer to global value.)

Thanks.

OGAWA Hirofumi <[email protected]>

The patch is following reply removes unnecessary

	memset(vga_to_curses, 0, sizeof(vga_to_curses));

in curses_setup(). And tested line graphics works on both of gtk and curses.

Thanks.

OGAWA Hirofumi <[email protected]>

This converts vga code to curses code in console_write_bh().

With this changes, we can see line graphics (for example, dialog uses) correctly.

---

 include/ui/console.h |   11 ++++++++++-
 ui/curses.c          |   43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 1 deletion(-)

diff -puN include/ui/console.h~support-curses-border include/ui/console.h
@@ -284,13 +284,22 @@ static inline pixman_format_code_t surfa
 #ifdef CONFIG_CURSES
 #include <curses.h>
 typedef chtype console_ch_t;
+extern chtype vga_to_curses[];
 #else
 typedef unsigned long console_ch_t;
 #endif
 static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
 {
-    if (!(ch & 0xff))
+    uint8_t c = ch;
+#ifdef CONFIG_CURSES
+    if (vga_to_curses[c]) {
+        ch &= ~(console_ch_t)0xff;
+        ch |= vga_to_curses[c];
+    }
+#else
+    if (c == '\0')
         ch |= ' ';
+#endif
     *dest = ch;
 }
 

_

Ok, curses and gtk can't be active at the same time. But for curses and vnc it is possible (qemu -curses -vnc :$display), and you probably get a dissorted vga text mode over vnc then.

I think the mapping should happen in the curses code, when curses updates the screen. curses_update() function is probably the best place for it.

Also you don't need a init function, you can simply init the array with c99 initializers:

chtype vga_to_curses[256] = {
    [0x04] = ACS_DIAMOND,
    [ ... ]
};

OK. It would be possible though, but parsing at curses_update() is slower. It is why, I did at that. Well, I'll see.

This is not possible, because ACS_* is not constant (initialized by reading from termcap or such).

Thanks.

OGAWA Hirofumi <[email protected]>

I've checked callers and usage of console_write_ch(). Call path is following paths.

curses_refresh()

    graphic_hw_text_update(NULL, screen);
        ->text_update()
            vga_update_text()
                console_write_ch()

        ->text_update()
            jazz_led_text_update()
                console_write_ch()

        ->text_update()
            text_console_update()
                console_write_ch()

->text_update() path is only used for -curses mode. What do you think? (E.g. qemu is going to extend to use this for other purpose?)

I guess, rather qemu wants to remove the call of ->text_update() for micro optimization if curses display is not used. (I'm not sure how optimizes it right now though.)

Thanks.

OGAWA Hirofumi <[email protected]>

Hi,

Oh, right, text mode curses update and text mode framebuffer rendering take completely separate code paths. Completely forgot that, I rarely look into the curses code. Sorry for the extra trouble.

Unlikely. In theory other ui frontends could support vga text mode that way. But text mode is a dying relic, people use either graphics or a serial console these days, so I don't expect that to happen.

So, I still think it would be cleaner to implement the curses special char mapping in ui/curses.c. I can't imagine there is a noticable performance difference. But doing it like you did shouldn't break things either.

cheers,
Gerd

OK. For cleaner (purpose is separation of curses code), I introduced hw_text_update_cb() interface (looks like possible to use text data for other than curses interface too). But you may feel this is overkill.

Well, IMO, the patch makes clear purpose of this path in following reply.

Thanks.

OGAWA Hirofumi <[email protected]>

This converts vga code to curses code in console_write_bh().

With this changes, we can see line graphics (for example, dialog uses) correctly.

---

 include/ui/console.h |    9 ++------
 ui/console.c         |   13 ++++++++++++
 ui/curses.c          |   53 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 6 deletions(-)

diff -puN include/ui/console.h~support-curses-border include/ui/console.h
@@ -287,12 +287,9 @@ typedef chtype console_ch_t;
 #else
 typedef unsigned long console_ch_t;
 #endif
-static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
-{
-    if (!(ch & 0xff))
-        ch |= ' ';
-    *dest = ch;
-}
+typedef void (hw_text_update_t)(console_ch_t *, uint32_t);
+void register_hw_text_update_cb(hw_text_update_t *cb);
+void console_write_ch(console_ch_t *dest, uint32_t ch);
 
 typedef struct GraphicHwOps {
     void (*invalidate)(void *opaque);

diff -puN ui/console.c~support-curses-border ui/console.c
@@ -333,6 +333,19 @@ void qmp_screendump(const char *filename
     ppm_save(filename, surface, errp);
 }
 
+static hw_text_update_t *hw_text_update_cb;
+
+void register_hw_text_update_cb(hw_text_update_t *cb)
+{
+    hw_text_update_cb = cb;
+}
+
+void console_write_ch(console_ch_t *dest, uint32_t ch)
+{
+    if (hw_text_update_cb)
+        hw_text_update_cb(dest, ch);
+}
+
 void graphic_hw_text_update(QemuConsole *con, console_ch_t *chardata)
 {
     if (!con) {

_

Hi,

Waded through the code a bit. The whole text interface isn't cleanly abstracted. There are curses-specific assumptions all over the place and thats why there is a curses include in console.h (ideally there shouldn't be any outside ui/curses.c).

So I've changed my mind and I think adding this interface doesn't help much. It pretends we have a clean text mode interface, which we clearly don't have. Tex mode support needs some major work anyway should we add a second text mode interface at some point in the future. Using vga_to_curses in console.c makes at least pretty clear what is going on here.

Lets go with v2 of this patch.

There are some codestyle issues in the patch (and the others too), please fix them them (scripts/checkpatch.pl helps), then resend the whole series.

thanks,
Gerd

OK. I'll send with new thread, not this thread.

OGAWA Hirofumi <[email protected]>

Ah, ok. That isn't obvious from reading the code. Can you add a comment please?

thanks,
Gerd

Oh, sent updated patch already. I will resend a updated patch to this reply.

OGAWA Hirofumi <[email protected]>

This converts vga code to curses code in console_write_bh().

With this changes, we can see line graphics (for example, dialog uses) correctly.

---

 include/ui/console.h |    9 ++------
 ui/console.c         |   13 ++++++++++++
 ui/curses.c          |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 6 deletions(-)

diff -puN include/ui/console.h~support-curses-border include/ui/console.h
@@ -287,12 +287,9 @@ typedef chtype console_ch_t;
 #else
 typedef unsigned long console_ch_t;
 #endif
-static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
-{
-    if (!(ch & 0xff))
-        ch |= ' ';
-    *dest = ch;
-}
+typedef void (hw_text_update_t)(console_ch_t *, uint32_t);
+void register_hw_text_update_cb(hw_text_update_t *cb);
+void console_write_ch(console_ch_t *dest, uint32_t ch);
 
 typedef struct GraphicHwOps {
     void (*invalidate)(void *opaque);

diff -puN ui/console.c~support-curses-border ui/console.c
@@ -333,6 +333,19 @@ void qmp_screendump(const char *filename
     ppm_save(filename, surface, errp);
 }
 
+static hw_text_update_t *hw_text_update_cb;
+
+void register_hw_text_update_cb(hw_text_update_t *cb)
+{
+    hw_text_update_cb = cb;
+}
+
+void console_write_ch(console_ch_t *dest, uint32_t ch)
+{
+    if (hw_text_update_cb)
+        hw_text_update_cb(dest, ch);
+}
+
 void graphic_hw_text_update(QemuConsole *con, console_ch_t *chardata)
 {
     if (!con) {

_

Copying maintainer. You can use scripts/get_maintainer.pl to find maintainers yourself.

Ah, I was missing to use it. Thanks.

OGAWA Hirofumi <[email protected]>