bug#16694: bugs #16694/#16378: Patches

[Matthias Dahl <ml_emacs-lists@binary-island.eu>]

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

Hello Eli and Barry,

sorry for my brain fart on this one. It was actually a rather late edit
which, even though I tested it and deemed harmless but useful, slipped
through my fingers.

By the way: no-init-from-resources is a global variable set by Emacs
during startup if (and only if) "-Q" is given, thus X resources should
not be applied. In patch 3 I accounted for that but simply forgot to do
the same in patch 1. Oh well.

Attached are the updated patches which should all work just fine now.
Thanks for the feedback and testing btw. Very much appreciated.

So long,
Matthias

[-- Attachment #2: 0001-lisp-faces.el-Fix-application-of-X-resource-attribut.patch --]
[-- Type: text/x-patch, Size: 2244 bytes --]

From d20fd8cb26c11248ee0e88f8234c000e6c951638 Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Wed, 5 Mar 2014 10:44:41 +0100
Subject: [PATCH 1/3] lisp/faces.el: Fix application of X resource attributes

Attributes from X resources have to applied first after the face spec has
been reset, to give Emacs' own face spec theming, customization and defface
defaults a higher priority over it. Otherwise those will be overriden and
the user sees a broken (= as in unintended and mixed up) user interface.

Commit 15e14b165dcbc6566a0459b0d5e66f89080f569e inadvertently changed that
priority.

This fixes bug #16694.
---
 lisp/faces.el | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/lisp/faces.el b/lisp/faces.el
index e008993..bffc787 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1619,11 +1619,19 @@ function for its other effects."
 
 (defun face-spec-recalc (face frame)
   "Reset the face attributes of FACE on FRAME according to its specs.
-This applies the defface/custom spec first, then the custom theme specs,
-then the override spec."
+After the reset, the specs are applied from the following sources in this order:
+  X resources (if applicable)
+   |
+  (theme and user customization) 
+    or, if nonexistent or does not match the current frame, 
+  (defface default spec)
+   |
+  defface override spec"
   (while (get face 'face-alias)
     (setq face (get face 'face-alias)))
   (face-spec-reset-face face frame)
+  (unless (and (boundp 'no-init-from-resources) (no-init-from-resources))
+    (make-face-x-resource-internal face frame))
   ;; If FACE is customized or themed, set the custom spec from
   ;; `theme-face' records.
   (let ((theme-faces (get face 'theme-face))
@@ -1641,8 +1649,7 @@ then the override spec."
       (setq spec (face-spec-choose (face-default-spec face) frame))
       (face-spec-set-2 face frame spec))
     (setq spec (face-spec-choose (get face 'face-override-spec) frame))
-    (face-spec-set-2 face frame spec))
-  (make-face-x-resource-internal face frame))
+    (face-spec-set-2 face frame spec)))
 
 (defun face-spec-set-2 (face frame spec)
   "Set the face attributes of FACE on FRAME according to SPEC."
-- 
1.9.1


[-- Attachment #3: 0002-lisp-faces.el-Fix-empty-face-handling.patch --]
[-- Type: text/x-patch, Size: 3519 bytes --]

From 377e9fa696f74548bb198f3305734017daee523b Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Wed, 5 Mar 2014 15:11:32 +0100
Subject: [PATCH 2/3] lisp/faces.el: Fix empty face handling

Commit 57becb6238dd612e3bf56bef1608bba9486dcec8 inadvertently broke the
handling of empty face specs. If face-spec-choose returned an empty
spec (which is valid to do), this was mishandled as a no match found
condition and as such, the defface default spec was applied.

The bug is twofold:
  * face-spec-choose maps two valid states onto a single
    return value (no match and empty face both return as nil).
  * face-spec-recalc does not differentiate between both cases
    appropriately.

This patch fixes the former by introducing a new optional parameter to
face-spec-choose which, if given, is returned if no match was found. Thus
the function is completely backwards compatible.

The latter is fixed by adapting face-spec-recalc to make use of this new
functionality and thus reliably detect if no match was found and only then
apply the defface default spec.

Fixes bug #16378.
---
 lisp/faces.el | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/lisp/faces.el b/lisp/faces.el
index bffc787..699f2af 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1512,13 +1512,15 @@ If FRAME is nil, the current FRAME is used."
     match))
 
 
-(defun face-spec-choose (spec &optional frame)
-  "Choose the proper attributes for FRAME, out of SPEC.
-If SPEC is nil, return nil."
+(defun face-spec-choose (spec &optional frame no-match-retval)
+  "Return the proper attributes for FRAME, out of SPEC.
+
+If no match is found or SPEC is nil, nil is returned unless NO-MATCH-RETVAL
+is given which would then be returned instead."
   (unless frame
     (setq frame (selected-frame)))
   (let ((tail spec)
-	result defaults)
+	result defaults match-found)
     (while tail
       (let* ((entry (pop tail))
 	     (display (car entry))
@@ -1538,9 +1540,18 @@ If SPEC is nil, return nil."
 	    (setq defaults thisval)
 	  ;; Otherwise, if it matches, use it.
 	  (when (face-spec-set-match-display display frame)
-	    (setq result thisval)
-	    (setq tail nil)))))
-    (if defaults (append result defaults) result)))
+	    (setq result thisval
+	          tail nil
+		  match-found t)))))
+    ;; If defaults have been found, it's safe to just append those to the result
+    ;; list (which at this point will be either nil or contain actual specs) and
+    ;; return it to the caller. Since there will most definitely be something to
+    ;; return in this case, there's no need to know/check if a match was found.
+    (if defaults
+	(append result defaults)
+      (if match-found
+	  result
+	no-match-retval))))
 
 
 (defun face-spec-reset-face (face &optional frame)
@@ -1635,11 +1646,12 @@ After the reset, the specs are applied from the following sources in this order:
   ;; If FACE is customized or themed, set the custom spec from
   ;; `theme-face' records.
   (let ((theme-faces (get face 'theme-face))
+	(no-match-found 0)
 	spec theme-face-applied)
     (if theme-faces
 	(dolist (elt (reverse theme-faces))
-	  (setq spec (face-spec-choose (cadr elt) frame))
-	  (when spec
+	  (setq spec (face-spec-choose (cadr elt) frame no-match-found))
+	  (unless (eq spec no-match-found)
 	    (face-spec-set-2 face frame spec)
 	    (setq theme-face-applied t))))
     ;; If there was a spec applicable to FRAME, that overrides the
-- 
1.9.1


[-- Attachment #4: 0003-lisp-faces.el-Centralize-no-init-from-x-resources-ch.patch --]
[-- Type: text/x-patch, Size: 3640 bytes --]

From 2f71ebf50944397ea3b4f05abcb3a37e8b309698 Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Wed, 5 Mar 2014 15:50:01 +0100
Subject: [PATCH 3/3] lisp/faces.el: Centralize no-init-from-x-resources check
 logic

Centralize the check for no-init-from-x-resources directly in
make-face-x-resource-internal and remove all redundant checks.

Backwards incompatible change: make-face previously accepted
no-init-from-resources as an optional parameter which has now
been removed. There were no other users within Emacs itself. And this
parameter shouldn't have been there in the first place, imho.
---
 lisp/faces.el | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/lisp/faces.el b/lisp/faces.el
index 699f2af..bcb8665 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -129,13 +129,10 @@ REGISTRY, ALTERNATIVE1, ALTERNATIVE2, and etc."
   "Return a list of all defined faces."
   (mapcar #'car face-new-frame-defaults))
 
-(defun make-face (face &optional no-init-from-resources)
+(defun make-face (face)
   "Define a new face with name FACE, a symbol.
 Do not call this directly from Lisp code; use `defface' instead.
-
-If NO-INIT-FROM-RESOURCES is non-nil, don't initialize face
-attributes from X resources.  If FACE is already known as a face,
-leave it unmodified.  Return FACE."
+If FACE is already known as a face, leave it unmodified.  Return FACE."
   (interactive (list (read-from-minibuffer
 		      "Make face: " nil nil t 'face-name-history)))
   (unless (facep face)
@@ -146,8 +143,7 @@ leave it unmodified.  Return FACE."
     (when (fboundp 'facemenu-add-new-face)
       (facemenu-add-new-face face))
     ;; Define frame-local faces for all frames from X resources.
-    (unless no-init-from-resources
-      (make-face-x-resource-internal face)))
+    (make-face-x-resource-internal face))
   face)
 
 (defun make-empty-face (face)
@@ -155,7 +151,7 @@ leave it unmodified.  Return FACE."
 Do not call this directly from Lisp code; use `defface' instead."
   (interactive (list (read-from-minibuffer
 		      "Make empty face: " nil nil t 'face-name-history)))
-  (make-face face 'no-init-from-resources))
+  (make-face face))
 
 (defun copy-face (old-face new-face &optional frame new-frame)
   "Define a face named NEW-FACE, which is a copy of OLD-FACE.
@@ -334,11 +330,14 @@ specifies an invalid attribute."
 
 (defun make-face-x-resource-internal (face &optional frame)
   "Fill frame-local FACE on FRAME from X resources.
-FRAME nil or not specified means do it for all frames."
-  (if (null frame)
-      (dolist (frame (frame-list))
-	(set-face-attributes-from-resources face frame))
-    (set-face-attributes-from-resources face frame)))
+FRAME nil or not specified means do it for all frames.
+
+If `no-init-from-resources' is t, this will do nothing."
+  (unless (and (boundp 'no-init-from-resources) (no-init-from-resources))
+    (if (null frame)
+	(dolist (frame (frame-list))
+	  (set-face-attributes-from-resources face frame))
+      (set-face-attributes-from-resources face frame))))
 
 
 \f
@@ -1641,8 +1640,7 @@ After the reset, the specs are applied from the following sources in this order:
   (while (get face 'face-alias)
     (setq face (get face 'face-alias)))
   (face-spec-reset-face face frame)
-  (unless (and (boundp 'no-init-from-resources) (no-init-from-resources))
-    (make-face-x-resource-internal face frame))
+  (make-face-x-resource-internal face frame)
   ;; If FACE is customized or themed, set the custom spec from
   ;; `theme-face' records.
   (let ((theme-faces (get face 'theme-face))
-- 
1.9.1