From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Matthias Dahl Newsgroups: gmane.emacs.bugs Subject: bug#16434: bug#16694: bugs #16694/#16378: Patches Date: Wed, 02 Apr 2014 17:04:19 +0200 Message-ID: <533C26F3.4040600@binary-island.eu> References: <52F601AE.5040309@binary-island.eu> <87k3bj40nu.fsf@cougar.home.aneadesign.com> <83wqfiz36v.fsf@gnu.org> <5331D45B.7090704@binary-island.eu> <5335920F.4030008@binary-island.eu> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------070800060109060102040303" X-Trace: ger.gmane.org 1396520898 2252 80.91.229.3 (3 Apr 2014 10:28:18 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 3 Apr 2014 10:28:18 +0000 (UTC) Cc: 16694@debbugs.gnu.org, 16434@debbugs.gnu.org, Clemens =?UTF-8?Q?Sch=C3=BCller?= To: Barry OReilly Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Thu Apr 03 12:28:10 2014 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1WVeKb-0001eo-E7 for geb-bug-gnu-emacs@m.gmane.org; Thu, 03 Apr 2014 11:53:09 +0200 Original-Received: from localhost ([::1]:39115 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WVMjB-0003Ub-Ac for geb-bug-gnu-emacs@m.gmane.org; Wed, 02 Apr 2014 11:05:21 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:41022) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WVMiy-00035K-V3 for bug-gnu-emacs@gnu.org; Wed, 02 Apr 2014 11:05:15 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WVMis-00037o-QZ for bug-gnu-emacs@gnu.org; Wed, 02 Apr 2014 11:05:08 -0400 Original-Received: from debbugs.gnu.org ([140.186.70.43]:60122) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WVMis-00037C-JJ for bug-gnu-emacs@gnu.org; Wed, 02 Apr 2014 11:05:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.80) (envelope-from ) id 1WVMir-0002vM-NB for bug-gnu-emacs@gnu.org; Wed, 02 Apr 2014 11:05:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Matthias Dahl Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 02 Apr 2014 15:05:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 16434 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 16434-submit@debbugs.gnu.org id=B16434.139645108111197 (code B ref 16434); Wed, 02 Apr 2014 15:05:01 +0000 Original-Received: (at 16434) by debbugs.gnu.org; 2 Apr 2014 15:04:41 +0000 Original-Received: from localhost ([127.0.0.1]:33069 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1WVMiW-0002uV-4I for submit@debbugs.gnu.org; Wed, 02 Apr 2014 11:04:41 -0400 Original-Received: from hemera.binary-island.eu ([97.107.138.233]:53070) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1WVMiE-0002tx-E7; Wed, 02 Apr 2014 11:04:37 -0400 Original-Received: from [10.0.0.20] (95-90-43-88-dynip.superkabel.de [95.90.43.88]) (using TLSv1 with cipher ECDHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) by hemera.binary-island.eu (Postfix) with ESMTPSA id 7C99B3C0D5; Wed, 2 Apr 2014 11:10:44 -0400 (EDT) User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 In-Reply-To: X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.15 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 140.186.70.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:87624 Archived-At: This is a multi-part message in MIME format. --------------070800060109060102040303 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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 --------------070800060109060102040303 Content-Type: text/x-patch; name="0001-lisp-faces.el-Fix-application-of-X-resource-attribut.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0001-lisp-faces.el-Fix-application-of-X-resource-attribut.pa"; filename*1="tch" >From a209fe3499b5c25b409539deb6b395de098411c1 Mon Sep 17 00:00:00 2001 From: Matthias Dahl 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 --------------070800060109060102040303 Content-Type: text/x-patch; name="0002-lisp-faces.el-Fix-empty-face-handling.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0002-lisp-faces.el-Fix-empty-face-handling.patch" >From 4155005861d68e2bc6a4bf7ec21d308e090e8e39 Mon Sep 17 00:00:00 2001 From: Matthias Dahl 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 --------------070800060109060102040303 Content-Type: text/x-patch; name="0003-lisp-faces.el-Centralize-no-init-from-x-resources-ch.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0003-lisp-faces.el-Centralize-no-init-from-x-resources-ch.pa"; filename*1="tch" >From 00e322ce045ce2b678b28dd71eeb5df3085a4a45 Mon Sep 17 00:00:00 2001 From: Matthias Dahl 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)))) @@ -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 --------------070800060109060102040303 Content-Type: text/x-patch; name="0004-lisp-faces.el-Fix-reverse-video-for-X-window-system.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0004-lisp-faces.el-Fix-reverse-video-for-X-window-system.pat"; filename*1="ch" >From b2d03ac8f2daf84130378d8dcb36ee1d26b368b0 Mon Sep 17 00:00:00 2001 From: Matthias Dahl 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)))))) @@ -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 --------------070800060109060102040303--