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

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

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

Hello @all...

Like promised, attached revised versions of my first three patches which
had a nasty and very embarrassing bug/brain-fart as well as a potential
fix for bug #16434.

I'd appreciate testing and any feedback, so that we (as in who ever is
in charge and can do this) commit this for the pretest. :)

Keeping my fingers crossed...

So long,
Matthias


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

From a209fe3499b5c25b409539deb6b395de098411c1 Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Wed, 2 Apr 2014 15:26:43 +0200
Subject: [PATCH 1/4] 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 49b5928..7d04938 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1639,11 +1639,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 inhibit-x-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))
@@ -1661,8 +1669,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 4155005861d68e2bc6a4bf7ec21d308e090e8e39 Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Wed, 2 Apr 2014 15:30:01 +0200
Subject: [PATCH 2/4] 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 7d04938..8536c08 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1532,13 +1532,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))
@@ -1558,9 +1560,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)
@@ -1655,11 +1666,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: 3544 bytes --]

From 00e322ce045ce2b678b28dd71eeb5df3085a4a45 Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Wed, 2 Apr 2014 15:32:40 +0200
Subject: [PATCH 3/4] lisp/faces.el: Centralize no-init-from-x-resources check
 logic

Centralize the check for inhibit-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 8536c08..28205d2 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -149,13 +149,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)
@@ -166,8 +163,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)
@@ -175,7 +171,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.
@@ -354,11 +350,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 `inhibit-x-resources' is t, this will do nothing."
+  (unless inhibit-x-resources
+    (if (null frame)
+	(dolist (frame (frame-list))
+	  (set-face-attributes-from-resources face frame))
+      (set-face-attributes-from-resources face frame))))
 
 
 \f
@@ -1661,8 +1660,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 inhibit-x-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


[-- Attachment #5: 0004-lisp-faces.el-Fix-reverse-video-for-X-window-system.patch --]
[-- Type: text/x-patch, Size: 2409 bytes --]

From b2d03ac8f2daf84130378d8dcb36ee1d26b368b0 Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Wed, 2 Apr 2014 15:50:42 +0200
Subject: [PATCH 4/4] lisp/faces.el: Fix reverse-video for X window system

During frame creation, the initial values for the default face
are set-- including swapped fg/bg colors in the reverse-video case.

Commit 15e14b165dcbc6566a0459b0d5e66f89080f569e introduced a bug
that overwrote those defaults by accident.

Previously: If reverse-video was active, the default face was no
longer synced with any X resources. The aforementioned commit placed
make-face-x-resource-internal in face-spec-recalc and called it
unconditionally there, which overwrote, amongst other things, the
proper set defaults.

To fix this, make-face-x-resource-internal itself has to make sure
that fg/bg colors for the default face are swapped appropriately
while still syncing with all of the X resources.

Fixes bug #16434.
---
 lisp/faces.el | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/lisp/faces.el b/lisp/faces.el
index 28205d2..187702a 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -354,10 +354,13 @@ FRAME nil or not specified means do it for all frames.
 
 If `inhibit-x-resources' is t, this will do nothing."
   (unless inhibit-x-resources
-    (if (null frame)
-	(dolist (frame (frame-list))
-	  (set-face-attributes-from-resources face frame))
-      (set-face-attributes-from-resources face frame))))
+    (dolist (frame (if (null frame) (frame-list) (list frame)))
+      (set-face-attributes-from-resources face frame)
+      (when (and (eq face 'default)
+		 (frame-parameter frame 'reverse))
+        (let ((fg (face-attribute face :foreground frame))
+	      (bg (face-attribute face :background frame)))
+	  (set-face-attribute face frame :foreground bg :background fg))))))
 
 
 \f
@@ -2063,10 +2066,6 @@ frame parameters in PARAMETERS."
 	  (progn
 	    ;; Initialize faces from face spec and custom theme.
 	    (face-spec-recalc face frame)
-	    ;; X resources for the default face are applied during
-	    ;; `x-create-frame'.
-	    (and (not (eq face 'default)) window-system-p
-		 (make-face-x-resource-internal face frame))
 	    ;; Apply attributes specified by face-new-frame-defaults
 	    (internal-merge-in-global-face face frame))
 	;; Don't let invalid specs prevent frame creation.
-- 
1.9.1