Inclusion of naquadah-theme

Inclusion of naquadah-theme

[Julien Danjou]

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

Hi,

I'd like to distribute my color theme more widely and therefore include
it in Emacs distribution. I don't know what the inclusion constraints
are, so I'm sending this here for review.

   http://git.naquadah.org/?p=naquadah-theme.git;a=blob;f=naquadah-theme.el;h=ea9e1b1aba1bac6631e7577e5fc4217ad699d779;hb=HEAD

If that's ok I'll merge in trunk.

-- 
           Julien

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: Inclusion of naquadah-theme

[Chong Yidong]

Julien Danjou <julien@danjou.info> writes:

> I'd like to distribute my color theme more widely and therefore include
> it in Emacs distribution. I don't know what the inclusion constraints
> are, so I'm sending this here for review.
>
>    http://git.naquadah.org/?p=naquadah-theme.git;a=blob;f=naquadah-theme.el;h=ea9e1b1aba1bac6631e7577e5fc4217ad699d779;hb=HEAD

Please have a better description in the first line than "A color theme",
since this serves as the accompanying text in the M-x customize-themes
buffer.  Likewise the docstring to deftheme, which is shown with the
describe-theme command.

Do not set ansi-term-color-vector directly with setq.  If you find the
need to customize it, please fix it right: submit a patch to term.el
that converts this to a defcustom, then use custom-theme-set-variables.

Having defuns in a theme file is obnoxious.  Could you explain what
these functions are needed for?  If there is missing functionality, we
should incorporate that directly into Emacs; they should not be
naquadah-specific.

Re: Inclusion of naquadah-theme

[Julien Danjou]

[-- Attachment #1.1: Type: text/plain, Size: 628 bytes --]

On Mon, Jun 25 2012, Chong Yidong wrote:

> Please have a better description in the first line than "A color theme",
> since this serves as the accompanying text in the M-x customize-themes
> buffer.  Likewise the docstring to deftheme, which is shown with the
> describe-theme command.

Sure, I fixed this two. But it's kinda hard to describe a theme
actually. :)

> Do not set ansi-term-color-vector directly with setq.  If you find the
> need to customize it, please fix it right: submit a patch to term.el
> that converts this to a defcustom, then use custom-theme-set-variables.

What do you think of the following patch?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-term-use-faces-for-color-handling.patch --]
[-- Type: text/x-diff, Size: 8319 bytes --]

From 15acb6ecb90cca322109344521379d843ce53c15 Mon Sep 17 00:00:00 2001
From: Julien Danjou <julien@danjou.info>
Date: Tue, 26 Jun 2012 12:39:41 +0200
Subject: [PATCH] term: use faces for color handling

Signed-off-by: Julien Danjou <julien@danjou.info>
---
 lisp/ChangeLog |    5 +++
 lisp/term.el   |  135 +++++++++++++++++++++++++++++++-------------------------
 2 files changed, 81 insertions(+), 59 deletions(-)

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index 69fee94..9680fb3 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,8 @@
+2012-06-26  Julien Danjou  <julien@danjou.info>
+
+	* term.el (term-handle-colors-array): Use a set of new faces to
+	color the terminal. Also uses :inverse-video property.
+
 2012-06-26  Martin Rudalics  <rudalics@gmx.at>
 
 	* calendar/calendar.el (calendar-exit): Don't try to delete or
diff --git a/lisp/term.el b/lisp/term.el
index 7461d74..d11fd57 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -108,11 +108,6 @@
 ;;
 ;;  Blink, is not supported.  Currently it's mapped as bold.
 ;;
-;; Important caveat:
-;; -----------------
-;;   if you want custom colors in term.el redefine term-default-fg-color
-;;  and term-default-bg-color BEFORE loading it.
-;;
 ;;             ----------------------------------------
 ;;
 ;;  If you'd like to check out my complete configuration, you can download
@@ -459,7 +454,7 @@ state 4: term-terminal-parameter contains pending output.")
   "A queue of strings whose echo we want suppressed.")
 (defvar term-terminal-parameter)
 (defvar term-terminal-previous-parameter)
-(defvar term-current-face 'default)
+(defvar term-current-face 'term-face)
 (defvar term-scroll-start 0 "Top-most line (inclusive) of scrolling region.")
 (defvar term-scroll-end) ; Number of line (zero-based) after scrolling region.
 (defvar term-pager-count nil
@@ -796,27 +791,71 @@ Buffer local variable.")
 (defvar term-terminal-previous-parameter-4 -1)
 
 ;;; faces -mm
+(defvar ansi-term-color-vector
+  [term-face
+   term-color-black
+   term-color-red
+   term-color-green
+   term-color-yellow
+   term-color-blue
+   term-color-magenta
+   term-color-cyan
+   term-color-white])
+
+(defface term-face
+  '((t :inherit default))
+  "Default face to use in term."
+  :group 'term)
 
-(defcustom term-default-fg-color
-  ;; FIXME: This depends on the current frame, so depending on when
-  ;; it's loaded, the result may be different.
-  (face-foreground term-current-face)
-  "Default color for foreground in `term'."
-  :group 'term
-  :type 'string)
+(defface term-bold
+  '((t :bold t))
+  "Default face to use for bold text."
+  :group 'term)
 
-(defcustom term-default-bg-color
-  ;; FIXME: This depends on the current frame, so depending on when
-  ;; it's loaded, the result may be different.
-  (face-background term-current-face)
-  "Default color for background in `term'."
-  :group 'term
-  :type 'string)
+(defface term-underline
+  '((t :underline t))
+  "Default face to use for underlined text."
+  :group 'term)
 
-;; Use the same colors that xterm uses, see `xterm-standard-colors'.
-(defvar ansi-term-color-vector
-  [unspecified "black" "red3" "green3" "yellow3" "blue2"
-   "magenta3" "cyan3" "white"])
+(defface term-color-black
+  '((t :foreground "black" :background "black"))
+  "Face used to render black color code."
+  :group 'term)
+
+(defface term-color-red
+  '((t :foreground "red3" :background "red3"))
+  "Face used to render red color code."
+  :group 'term)
+
+(defface term-color-green
+  '((t :foreground "green3" :background "green3"))
+  "Face used to render green color code."
+  :group 'term)
+
+(defface term-color-yellow
+  '((t :foreground "yellow3" :background "yellow3"))
+  "Face used to render yellow color code."
+  :group 'term)
+
+(defface term-color-blue
+  '((t :foreground "blue2" :background "blue2"))
+  "Face used to render blue color code."
+  :group 'term)
+
+(defface term-color-magenta
+  '((t :foreground "magenta3" :background "magenta3"))
+  "Face used to render magenta color code."
+  :group 'term)
+
+(defface term-color-cyan
+  '((t :foreground "cyan3" :background "cyan3"))
+  "Face used to render cyan color code."
+  :group 'term)
+
+(defface term-color-white
+  '((t :foreground "white" :background "white"))
+  "Face used to render white color code."
+  :group 'term)
 
 ;; Inspiration came from comint.el -mm
 (defcustom term-buffer-maximum-size 2048
@@ -951,11 +990,7 @@ is buffer-local."
     dt))
 
 (defun term-ansi-reset ()
-  (setq term-current-face (nconc
-                           (if term-default-bg-color
-                               (list :background term-default-bg-color))
-                           (if term-default-fg-color
-                               (list :foreground term-default-fg-color))))
+  (setq term-current-face 'term-face)
   (setq term-ansi-current-underline nil)
   (setq term-ansi-current-bold nil)
   (setq term-ansi-current-reverse nil)
@@ -3088,10 +3123,6 @@ See `term-prompt-regexp'."
 ;; New function to deal with ansi colorized output, as you can see you can
 ;; have any bold/underline/fg/bg/reverse combination. -mm
 
-(defvar term-bold-attribute '(:weight bold)
-  "Attribute to use for the bold terminal attribute.
-Set it to nil to disable bold.")
-
 (defun term-handle-colors-array (parameter)
   (cond
 
@@ -3153,46 +3184,32 @@ Set it to nil to disable bold.")
   ;;          term-ansi-current-color
   ;;          term-ansi-current-bg-color)
 
-
   (unless term-ansi-face-already-done
     (if term-ansi-current-invisible
         (let ((color
                (if term-ansi-current-reverse
-                   (if (= term-ansi-current-color 0)
-                       term-default-fg-color
-                     (elt ansi-term-color-vector term-ansi-current-color))
-                 (if (= term-ansi-current-bg-color 0)
-                     term-default-bg-color
-                   (elt ansi-term-color-vector term-ansi-current-bg-color)))))
+                   (face-foreground
+                    (elt ansi-term-color-vector term-ansi-current-color))
+                 (face-background
+                  (elt ansi-term-color-vector term-ansi-current-bg-color)))))
           (setq term-current-face
                 (list :background color
                       :foreground color))
           ) ;; No need to bother with anything else if it's invisible.
-
       (setq term-current-face
-            (if term-ansi-current-reverse
-                (if (= term-ansi-current-color 0)
-                    (list :background term-default-fg-color
-                          :foreground term-default-bg-color)
-                  (list :background
-                        (elt ansi-term-color-vector term-ansi-current-color)
-                        :foreground
-                        (elt ansi-term-color-vector term-ansi-current-bg-color)))
-
-              (if (= term-ansi-current-color 0)
-                  (list :foreground term-default-fg-color
-                        :background term-default-bg-color)
-                (list :foreground
-                      (elt ansi-term-color-vector term-ansi-current-color)
-                      :background
-                      (elt ansi-term-color-vector term-ansi-current-bg-color)))))
+            (list :foreground
+                  (face-foreground (elt ansi-term-color-vector term-ansi-current-color))
+                  :background
+                  (face-background (elt ansi-term-color-vector term-ansi-current-bg-color))
+                  :inverse-video term-ansi-current-reverse))
 
       (when term-ansi-current-bold
         (setq term-current-face
-              (append term-bold-attribute term-current-face)))
+              (list* term-current-face :inherit 'term-bold)))
+
       (when term-ansi-current-underline
         (setq term-current-face
-              (list* :underline t term-current-face)))))
+              (list* term-current-face :inherit 'term-underline)))))
 
   ;;	(message "Debug %S" term-current-face)
   ;; FIXME: shouldn't we set term-ansi-face-already-done to t here?  --Stef
-- 
1.7.10


[-- Attachment #1.3: Type: text/plain, Size: 1150 bytes --]


> Having defuns in a theme file is obnoxious.  Could you explain what
> these functions are needed for?  If there is missing functionality, we
> should incorporate that directly into Emacs; they should not be
> naquadah-specific.

It's a system we built to define faces with symbols and then expand into
a real color value, depending on the terminal capability. This allows to
define a face once with generic color and expand it:

(naquadah-simple-face-to-multiple
 '(default (:background background :foreground aluminium-1))))

=> (default
    ((((class color)
       (min-colors 65535))
      (:background "#252A2B" :foreground "#eeeeec"))
     (((class color)
       (min-colors 256))
      (:background "color-234" :foreground "color-255"))
     (((class color)
       (min-colors 88))
      (:background "color-80" :foreground "color-87"))
     (t
      (:background "black" :foreground "white"))))

This way we just have to define the color "aluminium-1" to be a
different color depending on the numbre of color available, and then
define the font. The rest is expanded by this function.

-- 
           Julien

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: Inclusion of naquadah-theme

[Chong Yidong]

Julien Danjou <julien@danjou.info> writes:

> What do you think of the following patch?

> +	* term.el (term-handle-colors-array): Use a set of new faces to
> +	color the terminal. Also uses :inverse-video property.

For consistency with the rest of Emacs, please use two spaces after each
sentence.

Please mark term-default-fg-color and term-default-bg-color as
deprecated, instead of deleting them outright.  Something like this:

(defcustom term-default-fg-color
  (face-foreground term-current-face)
  "If non-nil, default color for foreground in Term mode."
  :group 'term
  :type 'string)

(defface term-face
  (if term-default-fg-color
      `((t :foreground ,term-default-fg-color :inherit default))
    '((t :inherit default)))
  "Default face to use in Term mode."
  :group 'term)

and similarly for term-default-bg-color.  It is not necessary to handle
term-bold-attribute this way since it is just a defvar.

Please add a ChangeLog entry for each changed variable,
e.g. term-bold-attribute should say "Variable deleted".  Also, write a
NEWS item noting the deprecation of term-default-*-color and the
introduction of the new faces.

The rest of the patch looks fine.  Go ahead and commit once you have
made the above changes, and thanks.


>> Having defuns in a theme file is obnoxious.  Could you explain what
>> these functions are needed for?  If there is missing functionality, we
>> should incorporate that directly into Emacs; they should not be
>> naquadah-specific.
>
> It's a system we built to define faces with symbols and then expand
> into a real color value, depending on the terminal capability. This
> allows to define a face once with generic color and expand it:

I'm not sure I understand.  If you specify a face with a color like
"#252A2B", doesn't Emacs display the closest matching color on terminals
with limited colors?

Re: Inclusion of naquadah-theme

[Julien Danjou]

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

On Thu, Jun 28 2012, Chong Yidong wrote:

[…]

> The rest of the patch looks fine.  Go ahead and commit once you have
> made the above changes, and thanks.

Thanks for your review! I've commited with the requested changes. If
anything bad comes up with this, don't hesitate to beat me, I'll fix. :)

> I'm not sure I understand.  If you specify a face with a color like
> "#252A2B", doesn't Emacs display the closest matching color on terminals
> with limited colors?

It does try, but the result is not always good. Some colors are
correctly picked up as closest/best choice, others aren't fine. We're
doing a better job manually.

-- 
           Julien

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: Inclusion of naquadah-theme

[Chong Yidong]

Julien Danjou <julien@danjou.info> writes:

>> I'm not sure I understand.  If you specify a face with a color like
>> "#252A2B", doesn't Emacs display the closest matching color on terminals
>> with limited colors?
>
> It does try, but the result is not always good. Some colors are
> correctly picked up as closest/best choice, others aren't fine. We're
> doing a better job manually.

Well, first of all there are really only two cases that need to be
handled: terminals that can handle #rrggbb, and 16-color terminals.  In
my experience, 256-color xterms give a rather good approximation of the
desired colors automatically, without manual color placement.

As for matching color names to faces, the approach that we've used in
tango-theme.el and others is to do something like this:

(let ((class '((class color) (min-colors 89)))
      ;; Tango palette colors.
      (butter-1 "#fce94f")
      ...)

  (custom-theme-set-faces
   'tango-dark
   ...
   `(cursor ((,class (:background ,butter-1))))

Apart from handling multiple terminals, naquadah-simple-face-to-multiple
aims to do the same thing, right?  I'm guessing the reason is that you
feel that having explicit functions is more structured/less ad-hoc than
the above let-form.  If that's the case, maybe the solution is to add a
new macro to custom.el, which does something like this:

(custom-theme-set-faces-with-color-names THEME TERMINALS COLORS FACES...)

(custom-theme-set-faces-with-color-names
  'tango-dark
  ((term-1 ((class color) (min-colors 4096)))
   (term-2 ((class color) (min-colors 16))))
  ((butter (term-1 "#fce94f") (term-2 "yellow")))
  (cursor :background butter)
  ...)

WDYT?

Re: Inclusion of naquadah-theme

[Julien Danjou]

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

On Sat, Jun 30 2012, Chong Yidong wrote:

> Well, first of all there are really only two cases that need to be
> handled: terminals that can handle #rrggbb, and 16-color terminals.  In
> my experience, 256-color xterms give a rather good approximation of the
> desired colors automatically, without manual color placement.

Clearly that fails with naquadah-theme. I wish I had more time to try to
fix this.

> As for matching color names to faces, the approach that we've used in
> tango-theme.el and others is to do something like this:
>
> (let ((class '((class color) (min-colors 89)))
>       ;; Tango palette colors.
>       (butter-1 "#fce94f")
>       ...)
>
>   (custom-theme-set-faces
>    'tango-dark
>    ...
>    `(cursor ((,class (:background ,butter-1))))
>
> Apart from handling multiple terminals, naquadah-simple-face-to-multiple
> aims to do the same thing, right? 

Yes, that's exactly what we're doing.

> I'm guessing the reason is that you feel that having explicit
> functions is more structured/less ad-hoc than the above let-form.

Not, that's because I ignored the syntax you used in tango actually.

> If that's the case, maybe the solution is to add a new macro to
> custom.el, which does something like this:
>
> (custom-theme-set-faces-with-color-names THEME TERMINALS COLORS FACES...)
>
> (custom-theme-set-faces-with-color-names
>   'tango-dark
>   ((term-1 ((class color) (min-colors 4096)))
>    (term-2 ((class color) (min-colors 16))))
>   ((butter (term-1 "#fce94f") (term-2 "yellow")))
>   (cursor :background butter)
>   ...)
>
> WDYT?

That would be really handy, indeed. Do you propose to do the patch
yourself, or should it?

-- 
/*  Julien Danjou
   ╭ Free Software hacker & freelance
   ╰ http://julien.danjou.info          */

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]