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: Wed, 05 Mar 2014 17:14:34 +0100 Message-ID: <53174D6A.7010104@binary-island.eu> References: <52F601AE.5040309@binary-island.eu> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------080604090709030908070103" X-Trace: ger.gmane.org 1394036114 31321 80.91.229.3 (5 Mar 2014 16:15:14 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Wed, 5 Mar 2014 16:15:14 +0000 (UTC) Cc: 16378@debbugs.gnu.org To: 16694@debbugs.gnu.org Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Wed Mar 05 17:15:22 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 1WLETa-0003zV-1u for geb-bug-gnu-emacs@m.gmane.org; Wed, 05 Mar 2014 17:15:22 +0100 Original-Received: from localhost ([::1]:53135 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLETY-000457-VY for geb-bug-gnu-emacs@m.gmane.org; Wed, 05 Mar 2014 11:15:21 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:53892) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLETP-0003w8-I9 for bug-gnu-emacs@gnu.org; Wed, 05 Mar 2014 11:15:17 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WLETJ-0002Up-A7 for bug-gnu-emacs@gnu.org; Wed, 05 Mar 2014 11:15:11 -0500 Original-Received: from debbugs.gnu.org ([140.186.70.43]:50693) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLETJ-0002UZ-7p for bug-gnu-emacs@gnu.org; Wed, 05 Mar 2014 11:15:05 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.80) (envelope-from ) id 1WLETI-0002kn-ED for bug-gnu-emacs@gnu.org; Wed, 05 Mar 2014 11:15:04 -0500 X-Loop: help-debbugs@gnu.org In-Reply-To: <52F601AE.5040309@binary-island.eu> Resent-From: Matthias Dahl Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 05 Mar 2014 16:15:04 +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.139403608510528 (code B ref 16694); Wed, 05 Mar 2014 16:15:04 +0000 Original-Received: (at 16694) by debbugs.gnu.org; 5 Mar 2014 16:14:45 +0000 Original-Received: from localhost ([127.0.0.1]:51874 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1WLESy-0002je-VQ for submit@debbugs.gnu.org; Wed, 05 Mar 2014 11:14:45 -0500 Original-Received: from hemera.binary-island.eu ([97.107.138.233]:34297) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1WLESr-0002jI-0m; Wed, 05 Mar 2014 11:14:42 -0500 Original-Received: from [10.0.0.20] (95-90-43-186-dynip.superkabel.de [95.90.43.186]) (using TLSv1 with cipher ECDHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) by hemera.binary-island.eu (Postfix) with ESMTPSA id 972253C0D5; Wed, 5 Mar 2014 11:20:21 -0500 (EST) User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 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:86576 Archived-At: This is a multi-part message in MIME format. --------------080604090709030908070103 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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 --------------080604090709030908070103 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 fc76e8f48a8298c3093df353e048b9a1e996089d 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..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 --------------080604090709030908070103 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 f598a7d85e683c3d99e889bf764435aac3decb80 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 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 --------------080604090709030908070103 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 bc9280d9cab6bb59ad79225f729bfd0ef0d394fa 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 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)))) @@ -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 --------------080604090709030908070103--