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#16694: bugs #16694/#16378: Patches Date: Sun, 23 Mar 2014 18:04:47 +0100 Message-ID: <532F142F.6010803@binary-island.eu> References: <52F601AE.5040309@binary-island.eu> <83siqa4ovj.fsf@gnu.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------070805000709090603070505" X-Trace: ger.gmane.org 1395594312 12453 80.91.229.3 (23 Mar 2014 17:05:12 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 23 Mar 2014 17:05:12 +0000 (UTC) Cc: 16694@debbugs.gnu.org To: Eli Zaretskii , Barry OReilly Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Sun Mar 23 18:05:21 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 1WRlpn-00062Y-Tt for geb-bug-gnu-emacs@m.gmane.org; Sun, 23 Mar 2014 18:05:20 +0100 Original-Received: from localhost ([::1]:32772 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WRlpn-00075X-FG for geb-bug-gnu-emacs@m.gmane.org; Sun, 23 Mar 2014 13:05:19 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:50845) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WRlpe-00070Z-Eb for bug-gnu-emacs@gnu.org; Sun, 23 Mar 2014 13:05:16 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WRlpY-0003KA-Da for bug-gnu-emacs@gnu.org; Sun, 23 Mar 2014 13:05:10 -0400 Original-Received: from debbugs.gnu.org ([140.186.70.43]:44878) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WRlpY-0003JJ-Af for bug-gnu-emacs@gnu.org; Sun, 23 Mar 2014 13:05:04 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.80) (envelope-from ) id 1WRlpW-0002jF-C3 for bug-gnu-emacs@gnu.org; Sun, 23 Mar 2014 13: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: Sun, 23 Mar 2014 17:05:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 16694 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 16694-submit@debbugs.gnu.org id=B16694.139559429310469 (code B ref 16694); Sun, 23 Mar 2014 17:05:02 +0000 Original-Received: (at 16694) by debbugs.gnu.org; 23 Mar 2014 17:04:53 +0000 Original-Received: from localhost ([127.0.0.1]:46060 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1WRlpM-0002im-Ot for submit@debbugs.gnu.org; Sun, 23 Mar 2014 13:04:53 -0400 Original-Received: from hemera.binary-island.eu ([97.107.138.233]:54797) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1WRlpJ-0002ib-T1 for 16694@debbugs.gnu.org; Sun, 23 Mar 2014 13:04:51 -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 347E13C0D5; Sun, 23 Mar 2014 13:10:59 -0400 (EDT) User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 In-Reply-To: <83siqa4ovj.fsf@gnu.org> 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:87236 Archived-At: This is a multi-part message in MIME format. --------------070805000709090603070505 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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 --------------070805000709090603070505 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 d20fd8cb26c11248ee0e88f8234c000e6c951638 Mon Sep 17 00:00:00 2001 From: Matthias Dahl 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 --------------070805000709090603070505 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 377e9fa696f74548bb198f3305734017daee523b Mon Sep 17 00:00:00 2001 From: Matthias Dahl 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 --------------070805000709090603070505 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 2f71ebf50944397ea3b4f05abcb3a37e8b309698 Mon Sep 17 00:00:00 2001 From: Matthias Dahl 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)))) @@ -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 --------------070805000709090603070505--