[PATCH 2/2] Some CJK glyphs are wide, which occupy two columns. If the glyph is wide, then use two columns instead of one.

[David Herrmann]

Hi

On Thu, Jun 6, 2013 at 9:32 AM, Peng Wu <peng.e.wu at gmail.com> wrote:
> ---

Please cut the headline and instead provide a proper commit message.

>  clients/Makefile.am |  2 +-
>  clients/terminal.c  | 18 ++++++++++++++++--
>  2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/clients/Makefile.am b/clients/Makefile.am
> index cad0d40..d37d66a 100644
> --- a/clients/Makefile.am
> +++ b/clients/Makefile.am
> @@ -104,7 +104,7 @@ weston_screenshooter_SOURCES =                      \
>  weston_screenshooter_LDADD = libtoytoolkit.la
>
>  weston_terminal_SOURCES = terminal.c
> -weston_terminal_LDADD = libtoytoolkit.la -lutil
> +weston_terminal_LDADD = libtoytoolkit.la -lutil $(PANGO_LIBS)
>
>  image_SOURCES = image.c
>  image_LDADD = libtoytoolkit.la
> diff --git a/clients/terminal.c b/clients/terminal.c
> index 0d4f726..4495530 100644
> --- a/clients/terminal.c
> +++ b/clients/terminal.c
> @@ -33,6 +33,7 @@
>  #include <ctype.h>
>  #include <cairo.h>
>  #include <sys/epoll.h>
> +#include <glib.h>
>
>  #include <wayland-client.h>
>
> @@ -942,6 +943,9 @@ redraw_handler(struct widget *widget, void *data)
>         struct glyph_run run;
>         cairo_font_extents_t extents;
>         double average_width;
> +       gunichar unichar;
> +       gboolean iswide;
> +       int extracol;
>
>         surface = window_get_surface(terminal->window);
>         widget_get_allocation(terminal->widget, &allocation);
> @@ -991,22 +995,32 @@ redraw_handler(struct widget *widget, void *data)
>         glyph_run_init(&run, terminal, cr);
>         for (row = 0; row < terminal->height; row++) {
>                 p_row = terminal_get_row(terminal, row);
> +               extracol = 0;
>                 for (col = 0; col < terminal->width; col++) {
>                         /* get the attributes for this character cell */
>                         terminal_decode_attr(terminal, row, col, &attr);
>
>                         glyph_run_flush(&run, attr);
>
> -                       text_x = col * average_width;
> +                       /* check dual width unicode character */
> +                       unichar = g_utf8_get_char((const char*) p_row[col].byte);
> +                       iswide = g_unichar_iswide(unichar);
> +
> +                       text_x = (col + extracol) * average_width;
>                         text_y = extents.ascent + row * extents.height;
>                         if (attr.attr.a & ATTRMASK_UNDERLINE) {
>                                 terminal_set_color(terminal, cr, attr.attr.fg);
>                                 cairo_move_to(cr, text_x, (double)text_y + 1.5);
> -                               cairo_line_to(cr, text_x + average_width, (double) text_y + 1.5);
> +                               if (iswide)
> +                                       cairo_line_to(cr, text_x + average_width * 2, (double) text_y + 1.5);
> +                               else
> +                                       cairo_line_to(cr, text_x + average_width, (double) text_y + 1.5);
>                                 cairo_stroke(cr);
>                         }
>
>                         glyph_run_add(&run, text_x, text_y, &p_row[col]);
> +                       if (iswide)
> +                               extracol++;

This patch is actually wrong. multi-width character support requires
more than just drawing the glyphs with multiple columns. In fact, if a
client sends a multi-width character, the terminal emulation is
supposed to jump two columns so the next character will be inserted at
the correct position (this includes automatic newline, tabs, etc..).

What you currently do is just dropping the glyph which is inserted
after the double-col glyph. Are you sure this works as expected?

And for a wcwidth() implementation, see here:
  https://github.com/dvdhrm/kmscon/blob/master/external/wcwidth.c
Just copy it verbatim and use wcwidth() instead of glib.

I recommend just leaving multi-width support out of terminal.c. It
adds complexity (including wcwidth()) and seems misplaced in a debug
helper like weston-terminal. But feel free to resend and I will try to
review it.

Cheers
David

>                 }
>         }
>
> --
> 1.8.1.4
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel