Re: [dwm] [dvtm] scrollback and 256color support

On Mon, Aug 25, 2008 at 11:58:45PM -0700, Donald Chai wrote:
> Hi, the following two patches add support for a scrollback buffer and
> enhanced color in dvtm.
>
> 256color:
> - Allows the use of nice VIM color schemes
> - Ncurses doesn't allow more than 256 fg/bg color pairs on screen at
> the same time.
> - 8 or 256 colors only. If your terminal supports 88 colors, you need to
> set up your own variant of rxvt, e.g. rxvt-88color

Ok, i finally found the time to take a *quick* look at the color patch,
see some remarks below.

>diff --git a/config.h b/config.h
>index b2884e3..9c1387d 100644
>--- a/config.h
>+++ b/config.h
>@@ -10,24 +10,21 @@
> * A_BOLD Extra bright or bold
> * A_PROTECT Protected mode
> * A_INVIS Invisible or blank mode
>- * COLOR(fg,bg) Color where fg and bg are one of:
> *
>- * COLOR_BLACK
>- * COLOR_RED
>- * COLOR_GREEN
>- * COLOR_YELLOW
>- * COLOR_BLUE
>- * COLOR_MAGENTA
>- * COLOR_CYAN
>- * COLOR_WHITE
> */

Aren't there some macros which could be used instead of raw color numbers?
Maybe something like RGB(r, g, b) which would fall back to the most
resembling basic color.

>-#define ATTR_SELECTED COLOR(COLOR_RED,COLOR_BLACK)
>+#define ATTR_SELECTED A_NORMAL
>+#define FG_SELECTED COLORS==256 ? 68 : COLOR_BLUE
>+#define BG_SELECTED -1
> /* curses attributes for normal (not selected) windows */
> #define ATTR_NORMAL A_NORMAL
>+#define FG_NORMAL -1
>+#define BG_NORMAL -1
> /* status bar (command line option -s) position */
> #define BARPOS BarTop /* BarBot, BarOff */
> /* curses attributes for the status bar */
>-#define BAR_ATTR COLOR(COLOR_RED,COLOR_BLACK)
>+#define BAR_ATTR A_NORMAL
>+#define FG_BAR COLOR_RED
>+#define BG_BAR -1

Just a minor detail, i will probably rename those so that they share a
common prefix BAR_{ATTR,FG,BG}, NORMAL_{ATTR,FG,BG} etc.

>@@ -459,9 +470,11 @@ static void interpret_csi_ICH(madtty_t *t, int param[], int pcount)
> for (i = t->cols - 1; i >= t->curs_col + n; i--) {
> row->text[i] = row->text[i - n];
> row->attr[i] = row->attr[i - n];
>+ row->bg [i] = row->fg [i - n];

Is this a typo or not?

>+ row->fg [i] = row->fg [i - n];
> }
 
 
>@@ -478,16 +491,18 @@ static void interpret_csi_DCH(madtty_t *t, int param[], int pcount)
> for (i = t->curs_col; i < t->cols - n; i++) {
> row->text[i] = row->text[i + n];
> row->attr[i] = row->attr[i + n];
>+ row->bg [i] = row->fg [i + n];

Same here.

>+ row->fg [i] = row->fg [i + n];
> }
 
> void madtty_init_colors(void)
> {
>- if (COLOR_PAIRS > 64) {
>+ use_palette = 0;
>+
>+ if (1 && COLORS >= 256 && COLOR_PAIRS >= 256) {
          ^^^^^
            ` not needed

>+ use_palette = 1;
>+ use_default_colors();
>+ has_default = 1;
>+
 
@@ -1231,22 +1337,6 @@ void madtty_init_colors(void)
     }
 }
 
>-int madtty_color_pair(int fg, int bg)
>-{
>- if (has_default) {
>- if (fg < -1)
>- fg = -1;
>- if (bg < -1)
>- bg = -1;
>- return COLOR_PAIR((fg + 1) * 16 + bg + 1);
>- }
>- if (fg < 0)
>- fg = COLOR_WHITE;
>- if (bg < 0)
>- bg = COLOR_BLACK;
>- return COLOR_PAIR((7 - fg) * 8 + bg);
>-}
>-
> void madtty_set_handler(madtty_t *t, madtty_handler_t handler)
> {
> t->handler = handler;
>diff --git a/madtty.h b/madtty.h
>index 9445526..a30acee 100644
>--- a/madtty.h
>+++ b/madtty.h
>@@ -71,6 +71,7 @@ void madtty_keypress(madtty_t *, int keycode);
> void madtty_keypress_sequence(madtty_t *, const char *seq);
> void madtty_dirty(madtty_t *t);
> void madtty_draw(madtty_t *, WINDOW *win, int startrow, int startcol);
>+void madtty_color_set(WINDOW *win, short fg, short bg);

You didn't remove madtty_color_pair from the header file.

I tested it with script from[1] and it seems to work. The other
script[2] from that page triggers an assertion / results in a segfault.
This also happens without your patch but it would neverthless be good to
fix it.

Thanks,
Marc

[1] http://www.frexx.de/xterm-256-notes/data/xterm-colortest
[2] http://www.frexx.de/xterm-256-notes/data/256colors2.pl

-- 
 Marc Andre Tanner >< http://www.brain-dump.org/ >< GPG key: CF7D56C0