bug#16694: Regression by commit 115663 (bzr): Theme face attributes no longer take precedence over X resources

bug#16694: Regression by commit 115663 (bzr): Theme face attributes no longer take precedence over X resources

[Matthias Dahl]

Hello @all...

If a theme changes attributes from the default face for example (like
:background), those will shortly flash and then revert back to the
default of the current GTK theme, thus leaving the user with a mixed
state of the Emacs theme and GTK theme, which is most certainly not
intended.

This regression was introduced by bzr commit 115663: imho in function
face-spec-recalc from faces.el, make-face-x-resource-internal should not
be called last, which overrides parts of the theme again with the
defaults from the GTK theme.

Either commenting that line, or starting emacs with inhibit-x-resources
set to true, will work again. With the later though, you naturally loose
some other options as well, which is not desirable.

(As a side note: It is not properly documented either in the command
line help, nor the man pages, that starting emacs with -Q also sets
inhibit-x-resources to true.)

If more information or help is required, please let me know and I will
happily lend a hand or two. :-)

INFO from a stripped down Emacs checked out and built moments ago, which
(as expected) still has the problem:

In GNU Emacs 24.3.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.10.7)
 of 2014-02-08 on dreamgate
Repository revision: larsi@gnus.org-20140208065815-ie7d8pg17yeaagla
Windowing system distributor `The X.Org Foundation', version 11.0.11500000
System Description:	NAME=Gentoo

Configured using:
 `configure --prefix=/usr --build=x86_64-pc-linux-gnu
 --host=x86_64-pc-linux-gnu --mandir=/usr/share/man
 --infodir=/usr/share/info --datadir=/usr/share --sysconfdir=/etc
 --localstatedir=/var/lib --libdir=/usr/lib64 --disable-silent-rules
 --disable-dependency-tracking --program-suffix=-emacs-24-vcs
 --infodir=/usr/share/info/emacs-24-vcs
 --enable-locallisppath=/etc/emacs:/usr/share/emacs/site-lisp
 --with-gameuser=games --without-compress-install
 --with-file-notification=inotify --enable-acl --with-dbus
 --without-gnutls --with-gpm --without-hesiod --without-kerberos
 --without-kerberos5 --with-xml2 --without-selinux --with-wide-int
 --with-zlib --with-sound=alsa --with-x --without-ns --without-gconf
 --without-gsettings --without-toolkit-scroll-bars --with-gif
 --with-jpeg --with-png --with-rsvg --with-tiff --with-xpm
 --with-imagemagick --with-xft --without-libotf --without-m17n-flt
 --with-x-toolkit=gtk3 GENTOO_PACKAGE=app-editors/emacs-vcs-24.3.9999
 EBZR_BRANCH=trunk EBZR_REVNO=116339 'CFLAGS=-march=native -O2 -pipe'
 CPPFLAGS= 'LDFLAGS=-Wl,-O1 -Wl,--as-needed -Wl,-O1 -Wl,-z,combreloc
 -Wl,-z,now''

Important settings:
  value of $LC_COLLATE: de_DE.utf8
  value of $LC_CTYPE: de_DE.utf8
  value of $LC_MESSAGES: en_US.utf8
  value of $LC_MONETARY: de_DE.utf8
  value of $LC_NUMERIC: de_DE.utf8
  value of $LC_TIME: en_GB.utf8
  value of $LANG: en_US.utf8
  locale-coding-system: utf-8-unix

Major mode: Fundamental

Minor modes in effect:
  tooltip-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
M-x s e n d - b u g <tab> <backspace> <backspace> <backspace>
<backspace> <backspace> <backspace> <backspace> <backspace>
r e p o <tab> r t - e <tab> <return>

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
Making completion list...
call-interactively: End of buffer [6 times]

Load-path shadows:
None found.

Features:
(shadow sort gnus-util mail-extr emacsbug message idna format-spec
rfc822 mml mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util help-fns mail-prsvr mail-utils help-mode easymenu package
site-gentoo time-date tooltip electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel x-win x-dnd tool-bar dnd fontset image regexp-opt
fringe tabulated-list newcomment lisp-mode prog-mode register page
menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core frame cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
case-table epa-hook jka-cmpr-hook help simple abbrev minibuffer nadvice
loaddefs button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process dbusbind inotify
dynamic-setting font-render-setting move-toolbar gtk x-toolkit x
multi-tty emacs)

bug#16694: Regression by commit 115663 (bzr): Theme face attributes no longer take precedence over X resources

[Glenn Morris]

Matthias Dahl wrote:

> This regression was introduced by bzr commit 115663:

Then this is probably the same root issue as
http://debbugs.gnu.org/16434 (and 16440, and 16443, and maybe 16378).

There is clearly something going on there that needs to be investigated
before 24.4.

bug#16694: Regression by commit 115663 (bzr): Theme face attributes no longer take precedence over X resources

[Matthias Dahl]

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

On 12/02/14 23:21, Glenn Morris wrote:

> Then this is probably the same root issue as
> http://debbugs.gnu.org/16434 (and 16440, and 16443, and maybe 16378).

More than likely.

> There is clearly something going on there that needs to be investigated
> before 24.4.

The attached patch is what I am using locally and it fixes all problems
for me. I also tested bug #16434 and it works fine w/ the patch applied.
Basically it puts everything from X resources back to be applied first
and everything else on-top. Thus everything else is taking priority.

Consider the patch a raw version and a RFC-- missing proper changes to
the comments and I'd probably pull the conditional
(no-init-from-x-resources) up into make-face-x-resource-internal and
change the usage throughout the rest of the file.

But I'm not too well versed with Emacs' deep inner workings, and even
though it makes sense to me :), I might be completely off and just
fixing the symptoms. If so, I'd gladly help in finding the proper root
cause and learning a bit more in the process...

Hope that helps...
Matthias

[-- Attachment #2: faces.el.diff --]
[-- Type: text/plain, Size: 910 bytes --]

--- a/lisp/faces.el	2014-02-11 15:36:01.122479000 +0100
+++ b/lisp/faces.el	2014-02-11 15:35:34.305796014 +0100
@@ -1624,6 +1624,8 @@
   (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 +1643,7 @@
       (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."

bug#16694: Regression by commit 115663 (bzr): Theme face attributes no longer take precedence over X resources

[Eli Zaretskii]

> Date: Fri, 14 Feb 2014 20:17:42 +0100
> From: Matthias Dahl <ml_emacs-lists@binary-island.eu>
> Cc: 16694@debbugs.gnu.org
> 
> The attached patch is what I am using locally and it fixes all problems
> for me. I also tested bug #16434 and it works fine w/ the patch applied.
> Basically it puts everything from X resources back to be applied first
> and everything else on-top. Thus everything else is taking priority.
> 
> Consider the patch a raw version and a RFC-- missing proper changes to
> the comments and I'd probably pull the conditional
> (no-init-from-x-resources) up into make-face-x-resource-internal and
> change the usage throughout the rest of the file.
> 
> But I'm not too well versed with Emacs' deep inner workings, and even
> though it makes sense to me :), I might be completely off and just
> fixing the symptoms. If so, I'd gladly help in finding the proper root
> cause and learning a bit more in the process...

Any reasons not to install this?  Anybody?

bug#16694: Regression by commit 115663 (bzr): Theme face attributes no longer take precedence over X resources

[Stefan Monnier]

> Any reasons not to install this?  Anybody?

I don't understand this code nor the change that seems to have broken
it, so I neither support nor oppose the change.
Does it fix bug#16378 as well?


        Stefan

bug#16694: Regression by commit 115663 (bzr): Theme face attributes no longer take precedence over X resources

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Matthias Dahl <ml_emacs-lists@binary-island.eu>,  16694@debbugs.gnu.org
> Date: Thu, 20 Feb 2014 16:47:07 -0500
> 
> > Any reasons not to install this?  Anybody?
> 
> I don't understand this code nor the change that seems to have broken
> it, so I neither support nor oppose the change.

If no one objects in a couple of days, I think we should install it.

> Does it fix bug#16378 as well?

No.

bug#16694: Regression by commit 115663 (bzr): Theme face attributes no longer take precedence over X resources

[Stefan Monnier]

>> Does it fix bug#16378 as well?
> No.

Bummer!


        Stefan "who experiences bug#16378 all the time"

bug#16694: Regression by commit 115663 (bzr): Theme face attributes no longer take precedence over X resources

[Matthias Dahl]

Hello...

Sorry for my late reply. :(

> If no one objects in a couple of days, I think we should install it.

Good to hear. If you don't mind, I'd like to take a closer look again
and probably revise the original comments to match the new behavior.
Also, maybe add a second patch to pull the check up into the utility
function itself and refactor the few bits and pieces where it is used
accordingly. I'll try to get this done within the next few days.

>        Stefan "who experiences bug#16378 all the time"

I had a look at the description of bug #16378 but unfortunately I
couldn't make head or tail of it. If you don't mind filling me in, I'd
try to track this one down as well.

So long,
Matthias

bug#16694: Regression by commit 115663 (bzr): Theme face attributes no longer take precedence over X resources

[Eli Zaretskii]

> Date: Sun, 23 Feb 2014 17:46:39 +0100
> From: Matthias Dahl <ml_emacs-lists@binary-island.eu>
> CC: Stefan Monnier <monnier@iro.umontreal.ca>, 16694@debbugs.gnu.org
> 
> > If no one objects in a couple of days, I think we should install it.
> 
> Good to hear. If you don't mind, I'd like to take a closer look again
> and probably revise the original comments to match the new behavior.
> Also, maybe add a second patch to pull the check up into the utility
> function itself and refactor the few bits and pieces where it is used
> accordingly. I'll try to get this done within the next few days.

Sure, please do, and thanks.

> >        Stefan "who experiences bug#16378 all the time"
> 
> I had a look at the description of bug #16378 but unfortunately I
> couldn't make head or tail of it. If you don't mind filling me in, I'd
> try to track this one down as well.

You mean, you couldn't reproduce the problem?  Or do you mean
something else?

bug#16694: Regression by commit 115663 (bzr): Theme face attributes no longer take precedence over X resources

[Matthias Dahl]

> You mean, you couldn't reproduce the problem?  Or do you mean
> something else?

Both, actually. :) What I meant was: Since I am still "new" to emacs, I
don't know all of its dark corners and functionality. Thus, when I read
the bug report done by Stefan, I tried to figure out what was the
desired and what was the buggy behavior, all in the context of some
functionality that was rather foggy to me. When I tried to reproduce it
with my current installation (which has the patch applied), I could
not... but I suspect that I just didn't know what to look for or
messed up along the road.

To make a long story short: If someone could give me a bit more details
and background, I'd be more than happy to look into that bug and try to
track it down.

bug#16378: Empty face settings ignored (was bug#16694: Regression by commit 115663 (bzr): Theme face attributes no longer take precedence over X resources)

[Stefan Monnier]

> don't know all of its dark corners and functionality. Thus, when I read
> the bug report done by Stefan, I tried to figure out what was the
> desired and what was the buggy behavior, all in the context of some
> functionality that was rather foggy to me.

The desired behavior is that after middle clicking "State => Set for
current session", the "sample" (and all code highlighted with that
font-lock-comment-delimiter-face) should be displayed with the default
color used for all non-highlighted text (i.e. a black foreground,
typically), whereas it's still displayed using the reddish color of
font-lock-comment-face.
IOW, even though I specifically asked to remove the inheritance between 
font-lock-comment-delimiter-face and font-lock-comment-face, this
inheritance is still present.

Note that this has nothing to do with inheritance.  The same recipe
works for other properties:

   src/emacs -Q lisp/minibuffer.el
   C-e
   M-x customize-face RET RET
   [ So this time we get font-lock-comment-face. ]
   click on the "Foreground" button so as to disable the reddish color:
   the result should be a face that does not specify any particular
   appearance.  Middle click "State => Set for current session" and
   notice that the face is still using the same reddish foreground.


-- Stefan

bug#16378: Empty face settings ignored (was bug#16694: Regression by commit 115663 (bzr): Theme face attributes no longer take precedence over X resources)

[Matthias Dahl]

Thanks Stefan for the explanation, that was quite helpful. I'll have a
look and see if I can find the cause. I'll keep you posted.

bug#16378: Empty face settings ignored (was bug#16694: Regression by commit 115663 (bzr): Theme face attributes no longer take precedence over X resources)

[Matthias Dahl]

FYI: I found the cause and I've a patch in the pipeline to correct this
which needs further work at this point. Since I'm quite pressed for time
today, it might take another day or two.

bug#16378: Empty face settings ignored (was bug#16694: Regression by commit 115663 (bzr): Theme face attributes no longer take precedence over X resources)

[Eli Zaretskii]

> Date: Sun, 02 Mar 2014 15:26:18 +0100
> From: Matthias Dahl <ml_emacs-lists@binary-island.eu>
> Cc: 16378@debbugs.gnu.org
> 
> FYI: I found the cause and I've a patch in the pipeline to correct this
> which needs further work at this point. Since I'm quite pressed for time
> today, it might take another day or two.

Take your time, and thanks.

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

[Matthias Dahl]

[-- 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

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

[Matthias Dahl]

Hello @all.

It's been almost two weeks since I sent the patches with absolute and
rather discouraging silence in-between. :-) So I quickly wanted to ask
if there is any feedback (whether good or very bad) or something that
needs improving? And if there is a chance of getting those fixes applied
to the tree?

Thanks in advance,
Matthias

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

[Eli Zaretskii]

> Date: Sun, 16 Mar 2014 20:13:53 +0100
> From: Matthias Dahl <ml_emacs-lists@binary-island.eu>
> CC: 16378@debbugs.gnu.org, Stefan Monnier <monnier@IRO.UMontreal.CA>, 
>  Eli Zaretskii <eliz@gnu.org>
> 
> Hello @all.
> 
> It's been almost two weeks since I sent the patches with absolute and
> rather discouraging silence in-between. :-) So I quickly wanted to ask
> if there is any feedback (whether good or very bad) or something that
> needs improving? And if there is a chance of getting those fixes applied
> to the tree?

Barring any objections, I will commit this in a few days.

Thanks.

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

[Barry OReilly]

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

I tried 0001-lisp-faces.el-Fix-application-of-X-resource-attribut.patch and
got:

Loading /home/boreilly/l/emacs/x-resources-16434/lisp/faces.el (source)...
Symbol's value as variable is void: no-init-from-resources
make[2]: *** [bootstrap-emacs] Error 1

[-- Attachment #2: Type: text/html, Size: 305 bytes --]

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

[Eli Zaretskii]

> Date: Fri, 21 Mar 2014 14:05:22 -0400
> From: Barry OReilly <gundaetiapo@gmail.com>
> 
> I tried 0001-lisp-faces.el-Fix-application-of-X-resource-attribut.patch and
> got:
> 
> Loading /home/boreilly/l/emacs/x-resources-16434/lisp/faces.el (source)...
> Symbol's value as variable is void: no-init-from-resources
> make[2]: *** [bootstrap-emacs] Error 1

Mathias, it looks something is indeed missing in your patches.  Should
face-spec-recalc somehow accept an additional argument?

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

[Matthias Dahl]

[-- 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

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

[Barry OReilly]

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

> 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.

I built fine with the
0001-lisp-faces.el-Fix-application-of-X-resource-attribut.patch .

  ./src/emacs -Q -r

works as expected. However, commenting out my entire init.el,

  ./src/emacs -r

still starts Emacs with a white background, contrary to expectation.
So the patch doesn't fix 16434.

[-- Attachment #2: Type: text/html, Size: 554 bytes --]

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

[Clemens Schüller]

Hello!

>> 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.
>
> I built fine with the
> 0001-lisp-faces.el-Fix-application-of-X-resource-attribut.patch .
>
> ./Src/emacs -Q -r
>
> Works as expected. However, commenting out my entire init.el,
>
> ./Src/emacs -r
>
> Still starts Emacs with a white background, contrary to expectation.
> So the patch doesn't fix 16434.

I had the same problem with the white background:

After the integration of _all_ three patches the problem was gone.

-- 
Best Regards, Clemens Schüller

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

[Barry OReilly]

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

> After the integration of _all_ three patches the problem was gone.

I applied all three patches, but './src/emacs -r' with commented out
init.el still starts Emacs with unexpected white background.

[-- Attachment #2: Type: text/html, Size: 250 bytes --]

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

[Eli Zaretskii]

> Date: Tue, 25 Mar 2014 10:17:58 -0400
> From: Barry OReilly <gundaetiapo@gmail.com>
> Cc: ml_emacs-lists@binary-island.eu, 16694@debbugs.gnu.org
> 
> > After the integration of _all_ three patches the problem was gone.
> 
> I applied all three patches, but './src/emacs -r' with commented out
> init.el still starts Emacs with unexpected white background.

The reverse-video option was misbehaving for a long time; are you sure
that issue is at all related to the bugs discussed here?  When did you
last see -r DTRT in this scenario?

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

[Barry OReilly]

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

> When did you last see -r DTRT in this scenario?

I Git bisected it in bug 16434.

http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16434#5

[-- Attachment #2: Type: text/html, Size: 249 bytes --]

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

[Matthias Dahl]

Hello all.

I'll have a look at everything later this week and report back. I am
rather busy at the moment, unfortunately. :(

So long,
Matthias

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

[Barry OReilly]

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

In face-set-after-frame-default, a function untouched in Chong's
changes, there is:

  (let ((window-system-p (memq (window-system frame) '(x w32))))
[...]
        ;; 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))

But the

  (and (not (eq face 'default)) window-system-p ...)

is completely pointless, since Chong's changes have face-spec-recalc
call make-face-x-resource-internal unconditionally. On a hunch, I
tried:

diff --git a/lisp/faces.el b/lisp/faces.el
index e008993..1150d8f 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1642,7 +1642,9 @@ then the override spec."
       (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))
+  (and (not (eq face 'default))
+       (memq (window-system frame) '(x w32))
+       (make-face-x-resource-internal face frame)))

 (defun face-spec-set-2 (face frame spec)
   "Set the face attributes of FACE on FRAME according to SPEC."

with no other patches, and './src/emacs -r' worked as expected.

Should face-spec-recalc take responsibility for calling
make-face-x-resource-internal, or leave it to the caller as before the
offending changes? A patch along those lines also allows './src/emacs
-r' to work:

diff --git a/lisp/faces.el b/lisp/faces.el
index e008993..2f8560a 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1615,7 +1615,8 @@ function for its other effects."
   ;; Initialize the face if it does not exist, then recalculate.
   (make-empty-face face)
   (dolist (frame (frame-list))
-    (face-spec-recalc face frame)))
+    (face-spec-recalc face frame)
+    (make-face-x-resource-internal face frame)))

 (defun face-spec-recalc (face frame)
   "Reset the face attributes of FACE on FRAME according to its specs.
@@ -1641,8 +1642,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."

Someone who knows this code better than me should decide what's The
Right Thing.

[-- Attachment #2: Type: text/html, Size: 2977 bytes --]

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

[Stefan Monnier]

> Someone who knows this code better than me should decide what's The
> Right Thing.

Not sure if such a person exists, and she does, whether she is reading
this list :-(


        Stefan

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

[Barry OReilly]

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

The last patch I indicated is my best guess safe patch that solves the
--reverse-video symptom I care about. To summarize my rationale: an
unconditional call to make-face-x-resource-internal was moved down into
face-spec-recalc, but at least one other caller of face-spec-recalc clearly
doesn't want an unconditional call to make-face-x-resource-internal. Moving
the make-face-x-resource-internal call back up one call level to the caller
which the offending patch touched thus seems right.

Matthias, in your patch, that same make-face-x-resource-internal call was
moved to an earlier line in the same face-spec-recalc function, so it's not
obvious how to reconcile your patch with mine. The concern I raised applies
equally to your patch. Maybe you could give my patch a go to see if it has
any effect on your ill symptom. If not, then perhaps you have an idea about
reconciling the patches?

[-- Attachment #2: Type: text/html, Size: 942 bytes --]

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

[Matthias Dahl]

Hello @all.

Sorry for my late reply. I was _really_ busy this week. :(

@Barry: Thanks for all your investigative work. Unfortunately I fear you
are somewhat barking up the wrong tree (= function). :)

I am pretty sure I've found the real culprit but the fix is a bit more
involved, if I am right. I still need to do a few more tests and think
about a solution that works without side-effects and is as non-invasive
as possible.

If nobody minds, give me a few more days (still a bit busy here) and I
will see if I can get a patch out for testing asap.

Thanks for the patience in advance. :-)

So long,
Matthias

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

[Barry OReilly]

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

> I am pretty sure I've found the real culprit but the fix is a bit
> more involved, if I am right.

It would be nice to get a fix into the upcoming pretest, announced at:

  http://lists.gnu.org/archive/html/emacs-devel/2014-03/msg01243.html

Could you explain how the patch I proposed would be wrong to install,
even if it doesn't solve all ill symptoms? (No one has reported
whether or not it solves theme problems.)

[-- Attachment #2: Type: text/html, Size: 575 bytes --]

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

[Matthias Dahl]

Hello Barry...

> It would be nice to get a fix into the upcoming pretest, announced at:

I agree-- that goes for all the mentioned bugs in the subject btw. ;-)
So I hope we can delay the pretest until those patches are applied.

> Could you explain how the patch I proposed would be wrong to install,
> even if it doesn't solve all ill symptoms? (No one has reported
> whether or not it solves theme problems.)

That whole part of Emacs is not as easy to modify as it might look and
things do tend to have rather unexpected side-effects there-- speaking
from experience. :)

So I honestly would like to keep as much untouched (with my other
patches applied) as possible to fix this bug as well.

The problem is: The inverse video logic is not handled in faces.el but
outside of it. And for X, after the frame has been created, the default
face needs to be left alone, otherwise the inverse video is lost. I've a
few ideas that I just need to test.

I've set aside some time tomorrow for this, so I'll get back to everyone
later that day after I have either cooked up something that works fine
or I've given up and banged my head against the wall. ;)

Sorry for the delay...
Matthias

-- 
Dipl.-Inf. (FH) Matthias Dahl | Software Engineer | binary-island.eu
 services: custom software [desktop, mobile, web], server administration

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

[Matthias Dahl]

[-- 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

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

[Barry OReilly]

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

Hi Matthias, thank you for taking the time to update the patch series.

Patch 3 undoes something patch 1 introduced, perhaps because you
intended a subset of the patches for the emacs-24 branch and the rest
for trunk? Could you clarify that?

I applied all 4 patches and find './src/emacs -r' works correctly.
However, the behavior of './src/emacs -nw -r' is different from
emacs-24.3. I have my gnome-terminal configured to white on black, so
24.3's './src/emacs -nw -r' starts with white background. With your
patch, './src/emacs -nw -r' starts with black background. Not that I
mind the -nw behavior, but I suspect the behavioral difference is
unintended.

+      (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))))))

Wouldn't there already be a place in code responsible for the swap?
(x-handle-reverse-video and tty-handle-reverse-video?) I'm unsure why
the fix would entail a new place in code responsible for it.

For my information, could you confirm the effect my patch has on your
theme problem?

[-- Attachment #2: Type: text/html, Size: 1385 bytes --]

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

[Matthias Dahl]

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

Hello Barry,

I'm on the run but I wanted to answer you this very day, so please
excuse my brief mail. :(

> Patch 3 undoes something patch 1 introduced, perhaps because you
> intended a subset of the patches for the emacs-24 branch and the rest
> for trunk? Could you clarify that?

Thanks for taking a closer look. :) I usually write small contained
patches which work iteratively together which makes it a lot easier to
track down bugs at a later stage with git bisect or whatever.

So sometimes patch x introduces something that is needed for problem x
which patch x+1 slightly modifies or revises to solve problem x+1. :)

> For my information, could you confirm the effect my patch has on your
> theme problem?

I'm sorry but you lost me there. Could you please elaborate more what it
is you want me to comment on? Sorry... and thanks.

Last but not least: Attached you find a new patch for the problem which
is still the fourth in the series but since the last 3 haven't changed,
those are omitted from this mail but still required.

The patch basically restores the behavior (wrt reverse video) to what
Emacs 24.3 did without undoing any of the other work and fixes. I've
tested everything as far as I could and all your test cases work just
fine on my machine.

I hope this one is a keeper.

Thanks for testing and your patience. Again, sorry for the short mail.

So long,
Matthias

-- 
Dipl.-Inf. (FH) Matthias Dahl | Software Engineer | binary-island.eu
 services: custom software [desktop, mobile, web], server administration

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

From 921a907b9ccf990bb0885160fde37a173d237c22 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 now makes sure that it
doesn't touch the default face if reversed video is given-- as it
was done previously.

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

diff --git a/lisp/faces.el b/lisp/faces.el
index 28205d2..f14ffc7 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -354,10 +354,12 @@ 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)))
+      ;; `x-create-frame' already took care of correctly handling
+      ;; the reverse video case-- do _not_ touch the default face
+      (unless (and (eq face 'default)
+		   (frame-parameter frame 'reverse))
+        (set-face-attributes-from-resources face frame)))))
 
 
 \f
@@ -2063,10 +2065,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

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

[Barry OReilly]

>> For my information, could you confirm the effect my patch has on
>> your theme problem?

> I'm sorry but you lost me there. Could you please elaborate more
> what it is you want me to comment on? Sorry... and thanks.

I wanted to know if the following happened to fix the problem
described at http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16694#5 :

diff --git a/lisp/faces.el b/lisp/faces.el
index e008993..2f8560a 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1615,7 +1615,8 @@ function for its other effects."
   ;; Initialize the face if it does not exist, then recalculate.
   (make-empty-face face)
   (dolist (frame (frame-list))
-    (face-spec-recalc face frame)))
+    (face-spec-recalc face frame)
+    (make-face-x-resource-internal face frame)))

 (defun face-spec-recalc (face frame)
   "Reset the face attributes of FACE on FRAME according to its specs.
@@ -1641,8 +1642,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."

> The patch basically restores the behavior (wrt reverse video) to
> what Emacs 24.3 did without undoing any of the other work and fixes.
> I've tested everything as far as I could and all your test cases
> work just fine on my machine.

Confirmed, thanks. I have no further objections to the patch series.
Thank you for the work you put into it.

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

[Eli Zaretskii]

> Date: Wed, 02 Apr 2014 17:04:19 +0200
> From: Matthias Dahl <ml_emacs-lists@binary-island.eu>
> CC: Clemens Schüller
>  <cs.mlists+bug-gnu-emacs@mailbox.org>, 16694@debbugs.gnu.org, 
>  Stefan Monnier <monnier@iro.umontreal.ca>,
>  Eli Zaretskii <eliz@gnu.org>, 16434@debbugs.gnu.org
> 
> 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. :)

Thanks.  I applied to the emacs-24 branch the first 2 patches, the
part of the 3rd patch that does not introduce backward
incompatibilities, and the 4th patch you sent in a later message.
Please confirm that the result will DTRT.  Meanwhile, I'm marking
those bugs as "done".

In the future, please also provide ChangeLog entries for the changes
that could be dropped into the respective ChangeLog files.  TIA.

The incompatible part of the 3rd patch, reproduced below, remains
uncommitted.  I understand the motivation for it, but the emacs-24
branch shouldn't introduce incompatible changes at this time.  If
Stefan agrees with applying this part to the trunk, I will do that.

============================================================
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.

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.

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

[Stefan Monnier]

> 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.

It's kind of late for 24.4, but it looks like a good API cleanup, so
I think it's OK, tho in the 24.4 branch, please keep the optional
parameter and use it to signal a warning.


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: Matthias Dahl <ml_emacs-lists@binary-island.eu>, gundaetiapo@gmail.com,
>         cs.mlists+bug-gnu-emacs@mailbox.org, 16694-done@debbugs.gnu.org,
>         16434-done@debbugs.gnu.org, 16378-done@debbugs.gnu.org
> Date: Sat, 05 Apr 2014 11:48:50 -0400
> 
> > 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.
> 
> It's kind of late for 24.4, but it looks like a good API cleanup, so
> I think it's OK, tho in the 24.4 branch, please keep the optional
> parameter and use it to signal a warning.

Mathias, could you please prepare 2 patches along these lines, one for
the trunk, the other for the emacs-24 branch?  Thanks.

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

[Matthias Dahl]

Hi...

> Mathias, could you please prepare 2 patches along these lines, one for
> the trunk, the other for the emacs-24 branch?  Thanks.
> 

Sure. I'll prepare something tomorrow or Wednesday at the latest. Thanks
for applying the patches, btw. :)

So long,
Matthias

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

[Matthias Dahl]

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

Hello...

Attached the promised patches. Deprecation goes to emacs-24, removal to
master. And the ChangeLog fix, if nobody minds, to both. :)

Hope everything is ok.

Thanks for the patience. :) If there is anything else, please let me know.

So long,
Matthias


[-- Attachment #2: 0001-lisp-ChangeLog-Fix-mail-address-for-entry.patch --]
[-- Type: text/x-patch, Size: 748 bytes --]

From 95dd6855bf10edc770a956031c756de4d2aab0e7 Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Wed, 9 Apr 2014 11:44:43 +0200
Subject: [PATCH] lisp/ChangeLog: Fix mail address for entry

---
 lisp/ChangeLog | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index 5b8e0e9..2dd3e54 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -100,7 +100,7 @@
 
 	* help.el (view-lossage): Doc tweak.
 
-2014-04-07  Matthias Dahl  <ml_emacs-lists@binary-island.eu>
+2014-04-07  Matthias Dahl  <matthias.dahl@binary-island.eu>
 
 	* faces.el (face-spec-recalc): Call make-face-x-resource-internal
 	only when inhibit-x-resources is nil, and do that earlier in the
-- 
1.9.1


[-- Attachment #3: 0001-lisp-faces.el-Deprecate-optional-argument-of-make-fa.patch --]
[-- Type: text/x-patch, Size: 3006 bytes --]

From 28fd5ca2188a80b18762d4b378e7fef96963615c Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Wed, 9 Apr 2014 11:17:37 +0200
Subject: [PATCH] lisp/faces.el: Deprecate optional argument of make-face

The conditional application of X resources has been pushed down to
make-face-x-resource-internal itself and thus the optional argument
is no longer needed nor evaluated.
---
 lisp/ChangeLog |  7 +++++++
 lisp/faces.el  | 18 ++++++++++++------
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index 54ac144..58e488c 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,10 @@
+2014-04-09  Matthias Dahl  <matthias.dahl@binary-island.eu>
+
+	* faces.el (make-face): Deprecate optional argument as it is no
+	longer needed/used since the conditional X resources handling
+	has been pushed down to make-face-x-resource-internal itself.
+	(make-empty-face): Don't pass optional argument to make-face.
+
 2014-04-09  Dmitry Gutov  <dgutov@yandex.ru>
 
 	* progmodes/ruby-mode.el (ruby-font-lock-keywords): Highlight more
diff --git a/lisp/faces.el b/lisp/faces.el
index e4d8a35..cf571af 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -133,9 +133,11 @@ REGISTRY, ALTERNATIVE1, ALTERNATIVE2, and etc."
   "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.
+
+NO-INIT-FROM-RESOURCES has been deprecated and is no longer used
+and will go away. Handling of conditional X resources application
+has been pushed down to make-x-resource-internal itself."
   (interactive (list (read-from-minibuffer
 		      "Make face: " nil nil t 'face-name-history)))
   (unless (facep face)
@@ -146,16 +148,20 @@ 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)
 
+;; Handling of whether to apply X resources or not, has been pushed down
+;; to make-face-x-resource-internal itself, thus the optional arg is no
+;; longer evaluated at all and going away.
+(set-advertised-calling-convention 'make-face '(face) "24.4")
+
 (defun make-empty-face (face)
   "Define a new, empty face with name 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.
-- 
1.9.1


[-- Attachment #4: 0001-lisp-faces.el-Remove-deprecated-optional-argument-of.patch --]
[-- Type: text/x-patch, Size: 2761 bytes --]

From c8848a3c73f3773bdfcc35d177b0ec0421cfbdfa Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Wed, 9 Apr 2014 11:35:22 +0200
Subject: [PATCH] lisp/faces.el: Remove deprecated optional argument of
 make-face

The conditional application of X resources is handled directly by
make-face-x-resource-internal since Emacs 24.4 and thus hasn't been
evaluated since.

Fix make-empty-face to not pass an optional argument to make-face.
---
 lisp/ChangeLog |  7 +++++++
 lisp/faces.el  | 11 ++++-------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index 5b8e0e9..f926d30 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,10 @@
+2014-04-07  Matthias Dahl  <matthias.dahl@binary-island.eu>
+
+	* faces.el (make-face): Remove deprecated optional argument. The
+	conditional application of X resources is handled directly by
+	make-face-x-resource-internal since Emacs 24.4.
+	(make-empty-face): Don't pass optional argument to make-face.
+
 2014-04-09  Daniel Colascione  <dancol@dancol.org>
 
 	* emacs-lisp/cl-indent.el: Add comment claiming
diff --git a/lisp/faces.el b/lisp/faces.el
index b2f353d..c2ef62f 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -149,13 +149,11 @@ 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 +164,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 +172,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.
-- 
1.9.1

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

[Matthias Dahl]

Hello Eli...

Since the pretest is due today, if you get the time, could you apply
those pending patches? Thanks a lot in advance.

Have a nice weekend,
Matthias

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

[Eli Zaretskii]

> Date: Sat, 12 Apr 2014 13:37:11 +0200
> From: Matthias Dahl <matthias.dahl@binary-island.eu>
> CC: gundaetiapo@gmail.com, cs.mlists+bug-gnu-emacs@mailbox.org, 
>  16694-done@debbugs.gnu.org, 16434-done@debbugs.gnu.org, 
>  16378-done@debbugs.gnu.org
> 
> Hello Eli...
> 
> Since the pretest is due today, if you get the time, could you apply
> those pending patches? Thanks a lot in advance.

Done.

(Your changes to the trunk didn't compile, so I fixed them.)

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

[Matthias Dahl]

Hello Eli...

> Done.

Thanks.

> (Your changes to the trunk didn't compile, so I fixed them.)

Sorry about that. I missed a parenthesis. I see that now. I should have
made the compile test before sending it but it was such a simple patch
which was exactly alike what I used locally against 24.4 and I
double-checked it... yeah. :( I appreciate it. And sorry for the
screw-up. It won't happen again.

Have a nice Sunday,
Matthias

-- 
Dipl.-Inf. (FH) Matthias Dahl | Software Engineer | binary-island.eu
 services: custom software [desktop, mobile, web], server administration

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

[Eli Zaretskii]

> Date: Sat, 12 Apr 2014 16:57:46 +0200
> From: Matthias Dahl <matthias.dahl@binary-island.eu>
> Cc: 16434-done@debbugs.gnu.org, gundaetiapo@gmail.com, monnier@IRO.UMontreal.CA,
>  16378-done@debbugs.gnu.org, cs.mlists+bug-gnu-emacs@mailbox.org,
>  16694-done@debbugs.gnu.org
> 
> Hello Eli...
> 
> > Done.
> 
> Thanks.

I'm sorry, but now I see that the fix of this bug caused an adverse
side effect: face attributes that are defined in the X resources are
now overridden by the face defaults.  At least that's what happens on
MS-Windows, where we simulate the X resources in w32reg.c.

Specifically, in Emacs 24.3, the tool bar has its foreground and
background colors set to SystemButtonText and SystemButtonFace,
accordingly, as specified in SYSTEM_DEFAULT_RESOURCES defined by
w32reg.c.  By contrast, in the current pretest, this face has the
default foreground and background colors defined by faces.el:

  (defface tool-bar
    '((default
       :box (:line-width 1 :style released-button)
       :foreground "black")  <<<<<<<<<<<<<<<<<<<<<<<<<<
      (((type x w32 ns) (class color))
       :background "grey75") <<<<<<<<<<<<<<<<<<<<<<<<<<

This is clearly seen if one tries to customize this face in an Emacs
that was started without -Q.

I looked at the code, and it seems that the problem is in
face-spec-recalc, and the doc string explicitly says that it is the
intended behavior:

  (defun face-spec-recalc (face frame)
    "Reset the face attributes of FACE on FRAME according to its specs.
  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"

The code indeed follows the doc string: it first resets the face, then
applies the X resources, and then applies either the theme or the
default face spec:

    (face-spec-reset-face 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))
	  (no-match-found 0)
	  spec theme-face-applied)
      (if theme-faces
	  (dolist (elt (reverse theme-faces))
	    (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
      ;; defface spec entirely (rather than inheriting from it).  If
      ;; there was no spec applicable to FRAME, apply the defface spec.
      (unless theme-face-applied
	(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)))

What happens on Windows is that make-face-x-resource-internal indeed
picks up the colors specified bu w32reg.c, but then face-spec-set-2
resets the colors to what the defface spec says.

Can you or someone else see if setting the colors of the tool-bar face
in the X resources on Unix is similarly overridden?

I don't understand this logic: resources are a kind of customization,
so they should override the default face spec, not the other way
around.  Am I missing something?

This change was done because --reverse-video didn't work, but what
does --reverse-video have to do with X resources and their priority in
determining the face attributes?

Thanks.

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

[Matthias Dahl]

Hello Eli...

If no one else has taken care of this by the weekend, I'll happily take
a look then. Right now, I'm really short on time. I'm very sorry. :(

I apologize for the short answer in advance...

You are right. I looked over the 24.3 sources and also based on what you
described, the priority order needs fixing. If I remember correctly
though, this is very delicate and simply switching positions will cause
other bad side effects. :(

> I don't understand this logic: resources are a kind of customization,
> so they should override the default face spec, not the other way
> around.  Am I missing something?

When I initially set out to fix those bugs, I researched and, if I do
remember correctly, found an old thread where this was discussed and I
believe it was settled that Emacs defaults should always prevail. But in
hindsight, this makes no sense and I might have gotten things wrong.

I would like to remind you that the original and fundamental changes to
those functions where done by someone else. IMHO, they cleared a few
things up in contrast to 24.3... but introduced several bugs-- like the
ones I fixed and the one you are seeing now... unfortunately.

> This change was done because --reverse-video didn't work,

If you refer to my patches and explicitly moving the X resources stuff
at the beginning of face-spec-recalc: Those changes were done because
themes were broken. :) The changes I made for the reverse-video stuff
should not have caused this side effects and actually brought everything
more in line w/ 24.3... if anything else.

Like I said, if no one has taken care of this by the weekend, I will
gladly have a look and try to fix this as well. Sorry I couldn't be of
more help at this time, though.

So long,
Matthias

-- 
Dipl.-Inf. (FH) Matthias Dahl | Software Engineer | binary-island.eu
 services: custom software [desktop, mobile, web], server administration

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

[Stefan Monnier]

> If you refer to my patches and explicitly moving the X resources stuff
> at the beginning of face-spec-recalc: Those changes were done because
> themes were broken. :) The changes I made for the reverse-video stuff
> should not have caused this side effects and actually brought everything
> more in line w/ 24.3... if anything else.

Would it help to turn the X resource settings into a theme which can
then be maybe more easily stacked at the right place?


        Stefan

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

[Matthias Dahl]

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

Hello @all...

> Would it help to turn the X resource settings into a theme which can
> then be maybe more easily stacked at the right place?

Stefan, thanks for the idea. With the way things are currently wired,
this would most certainly mess the logic up. :(

Eli, could you please test the attached patches? Everything is rather
self-explanatory and the fix as simplistic as possible. I tested it for
all known possible regressions, but everything works just fine here. It
looks like it is the right thing to do (tm). Unfortunately, I was not
able to test it on win32 (yeah, I know, sorry) for obvious reasons :)
but based on what you said, it should fix the toolbar coloring just fine.

If there are no regressions reported from other parties and if it fixes
the issues on win32, from my side just go ahead and apply it to master
and emacs-24.

Hoping very much for positive feedback. ;-)

So long,
Matthias

-- 
Dipl.-Inf. (FH) Matthias Dahl | Software Engineer | binary-island.eu
 services: custom software [desktop, mobile, web], server administration

[-- Attachment #2: emacs24-0001-PATCH-lisp-faces.el-Apply-X-resources-after-defface-.patch --]
[-- Type: text/x-patch, Size: 3355 bytes --]

From 829b21a2e9cbcdab8d59f9b7f3b87c112088e729 Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Sun, 27 Apr 2014 10:03:40 +0200
Subject: [PATCH 4/4] [PATCH] lisp/faces.el: Apply X resources after defface
 spec

Apply X resources only after the the defface spec has been applied. Thus,
X resources are no longer overriden by the defface spec which also fixes
issues on win32 where the toolbar coloring was wrong because it is set
through X resources and was (wrongfully) overriden.
---
 lisp/ChangeLog |  8 ++++++++
 lisp/faces.el  | 18 ++++++++++++------
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index bcb649f..cfe61ba 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,11 @@
+2014-04-27  Matthias Dahl  <matthias.dahl@binary-island.eu>
+
+	* faces.el (face-spec-recalc): Apply X resources only after the
+	the defface spec has been applied. Thus, X resources are no longer
+	overriden by the defface spec which also fixes issues on win32 where
+	the toolbar coloring was wrong because it is set through X resources
+	and was (wrongfully) overriden.
+
 2014-04-25  Nicolas Richard  <theonewiththeevillook@yahoo.fr>
 
 	* battery.el (battery-update): Handle the case where battery
diff --git a/lisp/faces.el b/lisp/faces.el
index 88b8748..df31e0d 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1641,18 +1641,22 @@ function for its other effects."
 
 (defun face-spec-recalc (face frame)
   "Reset the face attributes of FACE on FRAME according to its specs.
-After the reset, the specs are applied from the following sources in this order:
-  X resources (if applicable)
+The following sources are applied in this order:
+
+  face reset to default values if it's the default face, otherwise set
+  to unspecifed (through `face-spec-reset-face`)
    |
   (theme and user customization)
-    or, if nonexistent or does not match the current frame,
+    or: if none of the above exist, do not match the current frame or
+        did inherit from the defface spec instead of overwriting it
+        entirely, the following is applied instead:
   (defface default spec)
+  (X resources (if applicable))
    |
   defface override spec"
   (while (get face 'face-alias)
     (setq face (get face 'face-alias)))
   (face-spec-reset-face 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))
@@ -1666,10 +1670,12 @@ After the reset, the specs are applied from the following sources in this order:
 	    (setq theme-face-applied t))))
     ;; If there was a spec applicable to FRAME, that overrides the
     ;; defface spec entirely (rather than inheriting from it).  If
-    ;; there was no spec applicable to FRAME, apply the defface spec.
+    ;; there was no spec applicable to FRAME, apply the defface spec
+    ;; as well as any applicable X resources.
     (unless theme-face-applied
       (setq spec (face-spec-choose (face-default-spec face) frame))
-      (face-spec-set-2 face frame spec))
+      (face-spec-set-2 face frame spec)
+      (make-face-x-resource-internal face frame))
     (setq spec (face-spec-choose (get face 'face-override-spec) frame))
     (face-spec-set-2 face frame spec)))
 
-- 
1.9.2


[-- Attachment #3: master-0001-lisp-faces.el-Apply-X-resources-after-defface-spec.patch --]
[-- Type: text/x-patch, Size: 3341 bytes --]

From 093207c4c79012615377695eb8eba31283aded5d Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Sun, 27 Apr 2014 09:55:51 +0200
Subject: [PATCH] lisp/faces.el: Apply X resources after defface spec

Apply X resources only after the the defface spec has been applied. Thus,
X resources are no longer overriden by the defface spec which also fixes
issues on win32 where the toolbar coloring was wrong because it is set
through X resources and was (wrongfully) overriden.
---
 lisp/ChangeLog |  8 ++++++++
 lisp/faces.el  | 18 ++++++++++++------
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index c81f99e..c982540 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,11 @@
+2014-04-27  Matthias Dahl  <matthias.dahl@binary-island.eu>
+
+	* faces.el (face-spec-recalc): Apply X resources only after the
+	the defface spec has been applied. Thus, X resources are no longer
+	overriden by the defface spec which also fixes issues on win32 where
+	the toolbar coloring was wrong because it is set through X resources
+	and was (wrongfully) overriden.
+
 2014-04-25  Stefan Monnier  <monnier@iro.umontreal.ca>
 
 	* progmodes/perl-mode.el (perl--syntax-exp-intro-regexp): New var.
diff --git a/lisp/faces.el b/lisp/faces.el
index 9c11547..b58575d 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1652,18 +1652,22 @@ function for its other effects."
 
 (defun face-spec-recalc (face frame)
   "Reset the face attributes of FACE on FRAME according to its specs.
-After the reset, the specs are applied from the following sources in this order:
-  X resources (if applicable)
+The following sources are applied in this order:
+
+  face reset to default values if it's the default face, otherwise set
+  to unspecifed (through `face-spec-reset-face`)
    |
   (theme and user customization)
-    or, if nonexistent or does not match the current frame,
+    or: if none of the above exist, do not match the current frame or
+        did inherit from the defface spec instead of overwriting it
+        entirely, the following is applied instead:
   (defface default spec)
+  (X resources (if applicable))
    |
   defface override spec"
   (while (get face 'face-alias)
     (setq face (get face 'face-alias)))
   (face-spec-reset-face 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))
@@ -1677,10 +1681,12 @@ After the reset, the specs are applied from the following sources in this order:
 	    (setq theme-face-applied t))))
     ;; If there was a spec applicable to FRAME, that overrides the
     ;; defface spec entirely (rather than inheriting from it).  If
-    ;; there was no spec applicable to FRAME, apply the defface spec.
+    ;; there was no spec applicable to FRAME, apply the defface spec
+    ;; as well as any applicable X resources.
     (unless theme-face-applied
       (setq spec (face-spec-choose (face-default-spec face) frame))
-      (face-spec-set-2 face frame spec))
+      (face-spec-set-2 face frame spec)
+      (make-face-x-resource-internal face frame))
     (setq spec (face-spec-choose (get face 'face-override-spec) frame))
     (face-spec-set-2 face frame spec)))
 
-- 
1.9.2

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

[Eli Zaretskii]

> Date: Sun, 27 Apr 2014 10:22:27 +0200
> From: Matthias Dahl <matthias.dahl@binary-island.eu>
> CC: Eli Zaretskii <eliz@gnu.org>, 16434@debbugs.gnu.org, 
>  gundaetiapo@gmail.com
> 
> Eli, could you please test the attached patches? Everything is rather
> self-explanatory and the fix as simplistic as possible. I tested it for
> all known possible regressions, but everything works just fine here. It
> looks like it is the right thing to do (tm). Unfortunately, I was not
> able to test it on win32 (yeah, I know, sorry) for obvious reasons :)
> but based on what you said, it should fix the toolbar coloring just fine.

Unfortunately, it doesn't fix the problem.  It looks like the problem
is that when make-face-x-resource-internal is called near the end of
face-spec-recalc, inhibit-x-resources is already set non-nil, and so
make-face-x-resource-internal does nothing.

Don't you see the same problem on X if you set
emacs.tool-bar.attributeBackground in the X resources?  That would
allow you to try the change on your system.

> If there are no regressions reported from other parties and if it fixes
> the issues on win32, from my side just go ahead and apply it to master
> and emacs-24.

Btw, in the future, you don't need to submit 2 identical patches, just
one to the release branch is enough: it will get merged onto the trunk
soon enough after being committed to the branch.

Thanks.

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

[Matthias Dahl]

On 28/04/14 18:38, Eli Zaretskii wrote:

> Unfortunately, it doesn't fix the problem.  It looks like the problem
> is that when make-face-x-resource-internal is called near the end of
> face-spec-recalc, inhibit-x-resources is already set non-nil, and so
> make-face-x-resource-internal does nothing.

Ah, that little bit of information I missed. In that case, everything's
actually working as intended. If you start Emacs and force it to ignore
X resources, it does so entirely and as expected... even more so in 24.4
now. Programmatically "setting" an X resource behind the scenes and
expecting it to work even though that very system has been asked to do
nothing, is bound to cause trouble.

Don't get me wrong, it is still a bug, nevertheless... only it is imho
debatable what the root cause really is in this case. The fact that the
X resource is not being applied (despite inhibit-x-resources == t) or
that the toolbar coloring is changed this way and expected to work at
all times, even if inhibit-x-resources == t.

> Don't you see the same problem on X if you set
> emacs.tool-bar.attributeBackground in the X resources?  That would
> allow you to try the change on your system.

If that would work, I would consider it a bug. If an X resource was
applied even though I started Emacs w/ -Q or otherwise set inhibit-...
that would very much be a bug. At least imho that is.

> Btw, in the future, you don't need to submit 2 identical patches, just
> one to the release branch is enough: it will get merged onto the trunk
> soon enough after being committed to the branch.

Ah, Ok. I'll keep that in mind. Thanks...

I'll have a look at this issue sometime later this week or weekend and
see what is going on exactly and if there is a way to fix this without
actually implementing any exceptions into make-face-x-... or anything
alike. Naturally, if someone else wants to take a stab at it who knows
more about all those tightly intervened and sneaky parts of the code
than I do (that one shouldn't be hard :P), I absolutely won't mind. :-)

So long,
Matthias

-- 
Dipl.-Inf. (FH) Matthias Dahl | Software Engineer | binary-island.eu
 services: custom software [desktop, mobile, web], server administration

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

[Eli Zaretskii]

> Date: Mon, 28 Apr 2014 20:36:26 +0200
> From: Matthias Dahl <matthias.dahl@binary-island.eu>
> CC: monnier@IRO.UMontreal.CA, 16434@debbugs.gnu.org, 
>  gundaetiapo@gmail.com
> 
> On 28/04/14 18:38, Eli Zaretskii wrote:
> 
> > Unfortunately, it doesn't fix the problem.  It looks like the problem
> > is that when make-face-x-resource-internal is called near the end of
> > face-spec-recalc, inhibit-x-resources is already set non-nil, and so
> > make-face-x-resource-internal does nothing.
> 
> Ah, that little bit of information I missed. In that case, everything's
> actually working as intended. If you start Emacs and force it to ignore
> X resources, it does so entirely and as expected... even more so in 24.4
> now. Programmatically "setting" an X resource behind the scenes and
> expecting it to work even though that very system has been asked to do
> nothing, is bound to cause trouble.

I'm terribly sorry, it turns out I tested your change incorrectly.  I
did it correctly this time, and of course the problem is solved.

I committed your changes to the emacs-24 branch.  Thanks.

> I'll have a look at this issue sometime later this week or weekend and
> see what is going on exactly and if there is a way to fix this without
> actually implementing any exceptions into make-face-x-... or anything
> alike.

No need, the problem is solved.  It was my fault.

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

[Matthias Dahl]

Hi Eli...

> I'm terribly sorry, it turns out I tested your change incorrectly.

Absolutely no need to feel sorry. :) The most important part is...

> the problem is solved.

... which is a relieve since I won't have to dig deeper into this. ;-)

> I committed your changes to the emacs-24 branch.

Thanks.

Hopefully we finally caught all the fallout from those changes that went
in earlier this year. Keeping my fingers crossed.

Have a nice evening,
Matthias

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

[Eli Zaretskii]

> Date: Tue, 25 Mar 2014 12:17:45 -0400
> From: Barry OReilly <gundaetiapo@gmail.com>
> Cc: Clemens Schüller <cs.mlists+bug-gnu-emacs@mailbox.org>, 
> 	ml_emacs-lists@binary-island.eu, 16694@debbugs.gnu.org
> 
> > When did you last see -r DTRT in this scenario?
> 
> I Git bisected it in bug 16434.
> 
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16434#5

Thanks.  So this is yet another bug to take care of.

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

[Glenn Morris]

Eli Zaretskii wrote:

>> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16434#5
>
> Thanks.  So this is yet another bug to take care of.

Perhaps I merged too many bugs there. It seemed like the same commit
caused all the issues. Hopefully someone has time to review the various
reports and see if any should be unmerged.

bug#16694: Strange background color problem in gentoo Linux

[Barry OReilly]

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

> I am using Gnome2, KDE and MATE on a fresh Gentoo system. In my
> .emacs I have:
>
> (add-to-list 'default-frame-alist '(background-color . "#333333"))
> (add-to-list 'default-frame-alist '(foreground-color . "White"))
> (add-to-list 'default-frame-alist '(cursor-color . "White"))
>
> Whenever I use MATE or KDE, emacs fails to set the background color
> to Green(#333333), it becomes white instead and I end up with White
> one White on White :( If I open a new Frame I get my Green
> background though.
>
> This happens on 24.3.50_pre20140228 and 24.3.9999-r1 (top of repo
> tree) Tried both gtk2 and gtk3 USE flags

Sounds similar to several other bug reports since the regression was
introduced in December. Most discussion is at
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16694 .

Chong, it would be nice if you could opine about the problem and
Matthias's patches. The offending commit was:

15e14b165dcbc6566a0459b0d5e66f89080f569e is the first bad commit
commit 15e14b165dcbc6566a0459b0d5e66f89080f569e
Author: Chong Yidong <cyd <at> gnu.org>
Date:   Sat Dec 21 23:31:09 2013 +0800

    Don't make faces when loading Custom themes.

    * custom.el (custom-theme-recalc-face): Do nothing if the face is
    undefined.  Thus, theme settings for undefined faces do not take
    effect until the faces are defined with defface, the same as with
    theme variables.

    * faces.el (face-spec-set): Use face-spec-recalc in all cases.
    (face-spec-reset-face): Don't assign extra properties in temacs.
    (face-spec-recalc): Apply X resources too.

[-- Attachment #2: Type: text/html, Size: 1926 bytes --]

bug#16694: Strange background color problem in gentoo Linux

[Joakim Tjernlund]

Barry OReilly <gundaetiapo@gmail.com> wrote on 2014/03/26 14:39:51:
> 
> > I am using Gnome2, KDE and MATE on a fresh Gentoo system. In my
> > .emacs I have:
> >
> > (add-to-list 'default-frame-alist '(background-color . "#333333"))
> > (add-to-list 'default-frame-alist '(foreground-color . "White"))
> > (add-to-list 'default-frame-alist '(cursor-color . "White"))
> >
> > Whenever I use MATE or KDE, emacs fails to set the background color
> > to Green(#333333), it becomes white instead and I end up with White
> > one White on White :( If I open a new Frame I get my Green
> > background though.
> >
> > This happens on 24.3.50_pre20140228 and 24.3.9999-r1 (top of repo
> > tree) Tried both gtk2 and gtk3 USE flags
> 
> Sounds similar to several other bug reports since the regression was
> introduced in December. Most discussion is at
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16694 .

Sure looks similar, thank you

One more data point, if I add 
  (setq initial-frame-alist default-frame-alist)
as in
  (add-to-list 'default-frame-alist '(background-color . "#333333"))
  (add-to-list 'default-frame-alist '(foreground-color . "White"))
  (add-to-list 'default-frame-alist '(cursor-color . "White")) 
  (setq initial-frame-alist default-frame-alist)
I get the correct green background in my initial Frame.

May I also ask, in another Gentoo GNU/Linux system running Gnome 2 I get
this after updating that install:

jocke@gentoo64 ~ $ emacs

(emacs:2436): GLib-GObject-WARNING **: Attempt to add property 
GtkSettings::gtk-button-images after class was initialised

(emacs:2436): GLib-GObject-WARNING **: Attempt to add property 
GtkSettings::gtk-label-select-on-focus after class was initialised

(emacs:2436): GLib-GObject-WARNING **: Attempt to add property 
GtkSettings::gtk-menu-bar-popup-delay after class was initialised

(emacs:2436): GLib-GObject-WARNING **: Attempt to add property 
GtkSettings::gtk-can-change-accels after class was initialised

(emacs:2436): GLib-GObject-WARNING **: Attempt to add property 
GtkSettings::gtk-menu-popup-delay after class was initialised

(emacs:2436): GLib-GObject-WARNING **: Attempt to add property 
GtkSettings::gtk-menu-popdown-delay after class was initialised

and I cannot figure out where the GLib warnings come from, any ideas?

 Jocke

bug#16694: Strange background color problem in gentoo Linux

[Barry OReilly]

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

> (emacs:2436): GLib-GObject-WARNING **: Attempt to add property
> GtkSettings::gtk-menu-popdown-delay after class was initialised

Maybe http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16522 , though the
warning is a little different.

[-- Attachment #2: Type: text/html, Size: 345 bytes --]

bug#16694: Strange background color problem in gentoo GNU/Linux

[Joakim Tjernlund]

Barry OReilly <gundaetiapo@gmail.com> wrote on 2014/03/26 17:49:39:
> 
> > (emacs:2436): GLib-GObject-WARNING **: Attempt to add property
> > GtkSettings::gtk-menu-popdown-delay after class was initialised
> 
> Maybe http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16522 , though the 
warning is a little different.

Ahh, that seems to be it. On this system gtk+ was at an older version(
2.24.17).
I am rebuilding with 2.24.22 now, thanks :)

 Jocke

bug#16694: Strange background color problem in gentoo Linux

[Clemens Schüller]

Hello!


> Sounds similar to several other bug reports since the regression was
> introduced in December. Most discussion is at
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16694 .
>
> Chong, it would be nice if you could opine about the problem and
> Matthias's patches. The offending commit was:
>
> 15e14b165dcbc6566a0459b0d5e66f89080f569e is the first bad commit
> commit 15e14b165dcbc6566a0459b0d5e66f89080f569e
> Author: Chong Yidong <cyd <at> gnu.org>
> Date: Sat Dec 21 23:31:09 2013 +0800
>
> Don't make faces when loading Custom themes.
>
> * custom.el (custom-theme-recalc-face): Do nothing if the face is
> undefined. Thus, theme settings for undefined faces do not take
> effect until the faces are defined with defface, the same as with
> theme variables.
>
> * faces.el (face-spec-set): Use face-spec-recalc in all cases.
> (face-spec-reset-face): Don't assign extra properties in temacs.
> (face-spec-recalc): Apply X resources too.

Here is my patch, after I copied the ebuild file in my local overlay:

--- /usr/portage/app-editors/emacs-vcs/emacs-vcs-24.4.9999.ebuild       2014-03-21 15:22:30.000000000 +0100
+++ emacs-vcs-24.4.9999.ebuild  2014-03-24 21:20:49.947795118 +0100
@@ -115,6 +115,9 @@
        fi
 
        epatch_user
+       epatch "${FILESDIR}/0001-lisp-faces.el-Fix-application-of-X-resource-attribut.patch"
+       epatch "${FILESDIR}/0002-lisp-faces.el-Fix-empty-face-handling.patch"
+       epatch "${FILESDIR}/0003-lisp-faces.el-Centralize-no-init-from-x-resources-ch.patch"
 
        # Fix filename reference in redirected man page
        sed -i -e "/^\\.so/s/etags/&-${EMACS_SUFFIX}/" doc/man/ctags.1 \


-- 
Best Regards, Clemens Schüller

bug#16694: Strange background color problem in gentoo Linux

[Joakim Tjernlund]

Clemens Schüller <cs.mlists+bug-gnu-emacs@mailbox.org> wrote on 2014/03/26 
16:58:11:
> 
> Hello!
> 
> 
> > Sounds similar to several other bug reports since the regression was
> > introduced in December. Most discussion is at
> > http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16694 .
> >
> > Chong, it would be nice if you could opine about the problem and
> > Matthias's patches. The offending commit was:
> >
> > 15e14b165dcbc6566a0459b0d5e66f89080f569e is the first bad commit
> > commit 15e14b165dcbc6566a0459b0d5e66f89080f569e
> > Author: Chong Yidong <cyd <at> gnu.org>
> > Date: Sat Dec 21 23:31:09 2013 +0800
> >
> > Don't make faces when loading Custom themes.
> >
> > * custom.el (custom-theme-recalc-face): Do nothing if the face is
> > undefined. Thus, theme settings for undefined faces do not take
> > effect until the faces are defined with defface, the same as with
> > theme variables.
> >
> > * faces.el (face-spec-set): Use face-spec-recalc in all cases.
> > (face-spec-reset-face): Don't assign extra properties in temacs.
> > (face-spec-recalc): Apply X resources too.
> 
> Here is my patch, after I copied the ebuild file in my local overlay:
> 
> --- /usr/portage/app-editors/emacs-vcs/emacs-vcs-24.4.9999.ebuild 
2014-03-21 15:22:30.000000000 +0100
> +++ emacs-vcs-24.4.9999.ebuild  2014-03-24 21:20:49.947795118 +0100
> @@ -115,6 +115,9 @@
>         fi
> 
>         epatch_user
> +       epatch 
"${FILESDIR}/0001-lisp-faces.el-Fix-application-of-X-resource-attribut.patch"
> +       epatch 
"${FILESDIR}/0002-lisp-faces.el-Fix-empty-face-handling.patch"
> +       epatch 
"${FILESDIR}/0003-lisp-faces.el-Centralize-no-init-from-x-resources-ch.patch"
> 
>         # Fix filename reference in redirected man page
>         sed -i -e "/^\\.so/s/etags/&-${EMACS_SUFFIX}/" doc/man/ctags.1 \

If this is a local fix you are better off using epatch_user than modifying 
the ebuild.
You could also try the easier:
 (setq initial-frame-alist default-frame-alist)

 Jocke