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

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

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

Hello @all.

Since bugs #16694 and #16378 are somewhat loosely connected, I am
posting this to both.

The attached patches have been tested quite a bit locally and especially
the fix for #16378 has gone through several revisions to cover all
corner cases and side-effects.

The first two patches should be quite safe to apply. Naturally I would
appreciate any testing and feedback. :)

The last patch is not required to fix any bugs (though it might), it
just cleans up a bit but introduces a backwards incompatible change on
the way which should (imho) not be a problem. That one is more a RFC but
works just fine on my system as well and I think applying it to the tree
shouldn't be trouble.

If there are any questions or problems, please let me know.

So long,
Matthias

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

From fc76e8f48a8298c3093df353e048b9a1e996089d 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..145ac39 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 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.0


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

From f598a7d85e683c3d99e889bf764435aac3decb80 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 145ac39..8225048 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.0


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

From bc9280d9cab6bb59ad79225f729bfd0ef0d394fa 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 8225048..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 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.0