Re: tty-color-mode

[Stefan Monnier]

> I expected this to disable colors on the current frame:
>   (set-frame-parameter nil 'tty-color-mode -1)
> But it disables colors on all tty frames.

Indeed the code that handles this is wrong.  It tries to handle it
inside `select-frame' but does it at a time where it has no effect (it
always ends up deciding that nothing has changed because it compares
the new selected window with what it thinks is the old one, but is
already the new one as well).

Furthermore it's lucky that it doesn't work: if you fix the text to make
it work, you then discover that it creates havoc, because you end up
calling set_tty_color_mode (which runs Elisp code) directly from
select-frame, which leads to breakage (and slow down because we end up
recomputing faces even when we do a `with-selected-frame' so that the
displayed frame actually never changes).

I believe the patch below gets it right, but since I'm not familiar with
this part of the code, I'd like other people to take a look at it: is it
a good idea to set "FRAME_TTY (f)->previous_frame = NULL;" to force
redisplay of the whole frame or should I use some other mechanism?


        Stefan


Index: src/term.c
===================================================================
RCS file: /sources/emacs/emacs/src/term.c,v
retrieving revision 1.214
diff -u -r1.214 term.c
--- src/term.c  27 Feb 2008 22:49:29 -0000      1.214
+++ src/term.c  4 Apr 2008 19:50:55 -0000
@@ -2161,56 +2161,40 @@
 }
 
 void
-set_tty_color_mode (f, val)
+set_tty_color_mode (tty, f)
+     struct tty_display_info *tty;
      struct frame *f;
-     Lisp_Object val;
 {
-  Lisp_Object color_mode_spec, current_mode_spec;
-  Lisp_Object color_mode, current_mode;
-  int mode, old_mode;
+  Lisp_Object tem, val, color_mode_spec;
+  Lisp_Object color_mode;
+  int mode;
   extern Lisp_Object Qtty_color_mode;
-  Lisp_Object tty_color_mode_alist;
+  Lisp_Object tty_color_mode_alist
+    = Fintern_soft (build_string ("tty-color-mode-alist"), Qnil);
 
-  tty_color_mode_alist = Fintern_soft (build_string ("tty-color-mode-alist"),
-                                      Qnil);
+  eassert (FRAME_TERMCAP_P (f));
+  eassert (FRAME_TTY (t) == tty);
+  tem = assq_no_quit (Qtty_color_mode, XFRAME (val)->param_alist);
+  val = CONSP (tem) ? XCDR (tem) : Qnil;
 
   if (INTEGERP (val))
     color_mode = val;
   else
     {
-      if (NILP (tty_color_mode_alist))
-       color_mode_spec = Qnil;
-      else
-       color_mode_spec = Fassq (val, XSYMBOL (tty_color_mode_alist)->value);
-
-      if (CONSP (color_mode_spec))
-       color_mode = XCDR (color_mode_spec);
-      else
-       color_mode = Qnil;
+      tem = (NILP (tty_color_mode_alist) ? Qnil
+            : Fassq (val, XSYMBOL (tty_color_mode_alist)->value));
+      color_mode = CONSP (tem) ? XCDR (tem) : Qnil;
     }
 
-  current_mode_spec = assq_no_quit (Qtty_color_mode, f->param_alist);
-
-  if (CONSP (current_mode_spec))
-    current_mode = XCDR (current_mode_spec);
-  else
-    current_mode = Qnil;
-  if (INTEGERP (color_mode))
-    mode = XINT (color_mode);
-  else
-    mode = 0;  /* meaning default */
-  if (INTEGERP (current_mode))
-    old_mode = XINT (current_mode);
-  else
-    old_mode = 0;
+  mode = INTEGERP (color_mode) ? XINT (color_mode) : 0;
 
-  if (mode != old_mode)
+  if (mode != tty->previous_color_mode)
     {
-      tty_setup_colors (FRAME_TTY (f), mode);
-      /*  This recomputes all the faces given the new color
-         definitions.  */
-      call0 (intern ("tty-set-up-initial-frame-faces"));
-      redraw_frame (f);
+      Lisp_Object funsym = intern ("tty-set-up-initial-frame-faces");
+      tty->previous_color_mode = mode;
+      tty_setup_colors (tty , mode);
+      /*  This recomputes all the faces given the new color definitions.  */
+      safe_call (1, &funsym);
     }
 }
 
Index: src/frame.c
===================================================================
RCS file: /sources/emacs/emacs/src/frame.c,v
retrieving revision 1.369
diff -u -r1.369 frame.c
--- src/frame.c 29 Mar 2008 01:46:08 -0000      1.369
+++ src/frame.c 4 Apr 2008 19:50:56 -0000
@@ -884,23 +884,6 @@
 
   Fselect_window (XFRAME (frame)->selected_window, Qnil);
 
-#ifndef WINDOWSNT
-  /* Make sure to switch the tty color mode to that of the newly
-     selected frame.  */
-  sf = SELECTED_FRAME ();
-  if (FRAME_TERMCAP_P (sf))
-    {
-      Lisp_Object color_mode_spec, color_mode;
-
-      color_mode_spec = assq_no_quit (Qtty_color_mode, sf->param_alist);
-      if (CONSP (color_mode_spec))
-       color_mode = XCDR (color_mode_spec);
-      else
-       color_mode = make_number (0);
-      set_tty_color_mode (sf, color_mode);
-    }
-#endif /* !WINDOWSNT */
-
   /* We want to make sure that the next event generates a frame-switch
      event to the appropriate frame.  This seems kludgy to me, but
      before you take it out, make sure that evaluating something like
@@ -2302,11 +2285,13 @@
     }
 
 #ifndef WINDOWSNT
-  /* The tty color mode needs to be set before the frame's parameter
-     alist is updated with the new value, because set_tty_color_mode
-     wants to look at the old mode.  */
-  if (FRAME_TERMCAP_P (f) && EQ (prop, Qtty_color_mode))
-    set_tty_color_mode (f, val);
+  /* The tty color needed to be set before the frame's parameter
+     alist was updated with the new value.  This is not true any more,
+     but we still do this test early on.  */
+  if (FRAME_TERMCAP_P (f) && EQ (prop, Qtty_color_mode)
+      && f == FRAME_TTY (f)->previous_frame)
+    /* Force redisplay of this tty.  */
+    FRAME_TTY (f)->previous_frame = NULL;
 #endif
 
   /* Update the frame parameter alist.  */
Index: src/dispextern.h
===================================================================
RCS file: /sources/emacs/emacs/src/dispextern.h,v
retrieving revision 1.241
diff -u -r1.241 dispextern.h
--- src/dispextern.h    1 Mar 2008 22:30:51 -0000       1.241
+++ src/dispextern.h    4 Apr 2008 19:50:56 -0000
@@ -3057,7 +3057,7 @@
 extern void produce_glyphs P_ ((struct it *));
 extern void produce_special_glyphs P_ ((struct it *, enum 
display_element_type));
 extern int tty_capable_p P_ ((struct tty_display_info *, unsigned, unsigned 
long, unsigned long));
-extern void set_tty_color_mode P_ ((struct frame *, Lisp_Object));
+extern void set_tty_color_mode (struct tty_display_info *, struct frame *);
 extern struct terminal *get_tty_terminal P_ ((Lisp_Object, int));
 extern struct terminal *get_named_tty P_ ((char *));
 EXFUN (Ftty_type, 1);