* [bug?] org-copy-face doesn’t add faces to org-faces customize group
@ 2014-08-27 1:41 Aaron Ecay
2014-08-29 11:45 ` Sebastien Vauban
0 siblings, 1 reply; 8+ messages in thread
From: Aaron Ecay @ 2014-08-27 1:41 UTC (permalink / raw)
To: orgmode list
Hello all,
I’ve noticed that the faces defined by org-copy-face are not added to
the org-faces customize group. This is in accordance with the docstring
of ‘copy-face’, which says (in part) “This function does not copy face
customization data, so NEW-FACE will not be made customizable. Most
Lisp code should not call this function; use `defface' with :inherit
instead.”
I think it’s at best an odd surprise and at worst a bug that all org’s
faces are not accessible from the org-faces customize group. Would
there be any objection to replacing all uses of this function with
:inherit as recommended by the docstring, and removing the org-copy-face
function?
For reference, here are the uses of the function, as returned by rgrep:
./lisp/org-faces.el:431:(org-copy-face 'org-todo 'org-checkbox-statistics-todo
./lisp/org-faces.el:434:(org-copy-face 'org-done 'org-checkbox-statistics-done
./lisp/org-faces.el:540:(org-copy-face 'org-meta-line 'org-block-begin-line
./lisp/org-faces.el:543:(org-copy-face 'org-meta-line 'org-block-end-line
./lisp/org-faces.el:560:(org-copy-face 'org-block 'org-quote
./lisp/org-faces.el:562:(org-copy-face 'org-block 'org-verse
./lisp/org-faces.el:600:(org-copy-face 'org-agenda-structure 'org-agenda-date
./lisp/org-faces.el:603:(org-copy-face 'org-agenda-date 'org-agenda-date-today
./lisp/org-faces.el:607:(org-copy-face 'secondary-selection 'org-agenda-clocking
./lisp/org-faces.el:610:(org-copy-face 'org-agenda-date 'org-agenda-date-weekend
./lisp/org-faces.el:719:(org-copy-face 'org-time-grid 'org-agenda-current-time
./lisp/org-faces.el:791:(org-copy-face 'mode-line 'org-mode-line-clock
./lisp/org-faces.el:793:(org-copy-face 'mode-line 'org-mode-line-clock-overrun
Thanks,
--
Aaron Ecay
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug?] org-copy-face doesn’t add faces to org-faces customize group
2014-08-27 1:41 [bug?] org-copy-face doesn’t add faces to org-faces customize group Aaron Ecay
@ 2014-08-29 11:45 ` Sebastien Vauban
2014-08-30 19:03 ` Aaron Ecay
0 siblings, 1 reply; 8+ messages in thread
From: Sebastien Vauban @ 2014-08-29 11:45 UTC (permalink / raw)
To: emacs-orgmode-mXXj517/zsQ
Aaron Ecay wrote:
> I’ve noticed that the faces defined by org-copy-face are not added to
> the org-faces customize group. This is in accordance with the docstring
> of ‘copy-face’, which says (in part) “This function does not copy face
> customization data, so NEW-FACE will not be made customizable. Most
> Lisp code should not call this function; use `defface' with :inherit
> instead.”
>
> I think it’s at best an odd surprise and at worst a bug that all org’s
> faces are not accessible from the org-faces customize group. Would
> there be any objection to replacing all uses of this function with
> :inherit as recommended by the docstring, and removing the org-copy-face
> function?
>
> For reference, here are the uses of the function, as returned by rgrep:
>
> ./lisp/org-faces.el:431:(org-copy-face 'org-todo 'org-checkbox-statistics-todo
> ./lisp/org-faces.el:434:(org-copy-face 'org-done 'org-checkbox-statistics-done
> ./lisp/org-faces.el:540:(org-copy-face 'org-meta-line 'org-block-begin-line
> ./lisp/org-faces.el:543:(org-copy-face 'org-meta-line 'org-block-end-line
> ./lisp/org-faces.el:560:(org-copy-face 'org-block 'org-quote
> ./lisp/org-faces.el:562:(org-copy-face 'org-block 'org-verse
> ./lisp/org-faces.el:600:(org-copy-face 'org-agenda-structure 'org-agenda-date
> ./lisp/org-faces.el:603:(org-copy-face 'org-agenda-date 'org-agenda-date-today
> ./lisp/org-faces.el:607:(org-copy-face 'secondary-selection 'org-agenda-clocking
> ./lisp/org-faces.el:610:(org-copy-face 'org-agenda-date 'org-agenda-date-weekend
> ./lisp/org-faces.el:719:(org-copy-face 'org-time-grid 'org-agenda-current-time
> ./lisp/org-faces.el:791:(org-copy-face 'mode-line 'org-mode-line-clock
> ./lisp/org-faces.el:793:(org-copy-face 'mode-line 'org-mode-line-clock-overrun
I think it's related to an Emacs bug (#16440) which I reported on the
Org mailing list in February.
As stated by Eli (in http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16440+):
╭────
│ Org uses org-copy-face to define the faces that you show in your
│ screencast, and org-copy-face assumes the face it inherits from
│ already exists. But loading a theme now doesn't create the faces, it
│ only prepares the data for when the face will be created. So :inherit
│ in org-copy-face doesn't do what you expect.
│
│ I guess either some change is needed in how themes are handled, or
│ org-copy-face needs to change to follow suit. (CC to Bastien for
│ that.)
╰────
In the same functional area, there is also the bug #15298 still pending
(http://debbugs.gnu.org/cgi/bugreport.cgi?bug=15298) about "Background
color lost when highlighting a string".
Best regards,
Seb
--
Sebastien Vauban
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug?] org-copy-face doesn’t add faces to org-faces customize group
2014-08-29 11:45 ` Sebastien Vauban
@ 2014-08-30 19:03 ` Aaron Ecay
[not found] ` <87wq9pssuo.fsf-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 8+ messages in thread
From: Aaron Ecay @ 2014-08-30 19:03 UTC (permalink / raw)
To: Sebastien Vauban, emacs-orgmode
Hi Seb,
2014ko abuztuak 29an, Sebastien Vauban-ek idatzi zuen:
> I think it's related to an Emacs bug (#16440) which I reported on the
> Org mailing list in February.
I don’t completely understand what is going on in that bug report. My
proposal is to convert org-copy-face to defface. I can’t tell if this
would fix your problem or not, but if it’s exclusive to the faces I
listed in my previous email the answer is probably “yes.”
--
Aaron Ecay
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug?] org-copy-face doesn’t add faces to org-faces customize group
[not found] ` <87wq9pssuo.fsf-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2014-09-23 9:40 ` Sebastien Vauban
2014-09-23 17:27 ` Aaron Ecay
0 siblings, 1 reply; 8+ messages in thread
From: Sebastien Vauban @ 2014-09-23 9:40 UTC (permalink / raw)
To: emacs-orgmode-mXXj517/zsQ
Hi Aaron,
Aaron Ecay wrote:
> 2014ko abuztuak 29an, Sebastien Vauban-ek idatzi zuen:
>> I think it's related to an Emacs bug (#16440) which I reported on the
>> Org mailing list in February.
>
> I don’t completely understand what is going on in that bug report. My
> proposal is to convert org-copy-face to defface.
I think this is "the right thing" (TM).
> I can’t tell if this would fix your problem or not, but if it’s
> exclusive to the faces I listed in my previous email the answer is
> probably “yes.”
From what I can see, yes, the faces which are wrongly displayed (before
re-applying the theme) match the ones defined by `org-copy-face'. So,
this seems the right way to go.
Best regards,
Seb
--
Sebastien Vauban
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug?] org-copy-face doesn’t add faces to org-faces customize group
2014-09-23 9:40 ` Sebastien Vauban
@ 2014-09-23 17:27 ` Aaron Ecay
2014-09-23 18:00 ` Sebastien Vauban
0 siblings, 1 reply; 8+ messages in thread
From: Aaron Ecay @ 2014-09-23 17:27 UTC (permalink / raw)
To: Sebastien Vauban, emacs-orgmode
[-- Attachment #1: Type: text/plain, Size: 977 bytes --]
Hi Seb,
2014ko irailak 23an, Sebastien Vauban-ek idatzi zuen:
>
> Hi Aaron,
>
> Aaron Ecay wrote:
>> 2014ko abuztuak 29an, Sebastien Vauban-ek idatzi zuen:
>>> I think it's related to an Emacs bug (#16440) which I reported on the
>>> Org mailing list in February.
>>
>> I don’t completely understand what is going on in that bug report. My
>> proposal is to convert org-copy-face to defface.
>
> I think this is "the right thing" (TM).
>
>> I can’t tell if this would fix your problem or not, but if it’s
>> exclusive to the faces I listed in my previous email the answer is
>> probably “yes.”
>
> From what I can see, yes, the faces which are wrongly displayed (before
> re-applying the theme) match the ones defined by `org-copy-face'. So,
> this seems the right way to go.
OK, thanks for the followup. I’m attaching the patch to this email. If
I don’t hear any further feedback, I’ll commit it to master in a few
days.
[-- Attachment #2: 0001-org-faces-remove-org-copy-face.patch --]
[-- Type: text/x-diff, Size: 6400 bytes --]
From 128726a88ca2ec2232071cd9a6d7869df01fd953 Mon Sep 17 00:00:00 2001
From: Aaron Ecay <aaronecay@gmail.com>
Date: Tue, 23 Sep 2014 13:21:22 -0400
Subject: [PATCH] org-faces: remove org-copy-face
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* lisp/org-faces.el (org-copy-face): Remove function.
(org-checkbox-statistics-todo, org-checkbox-statistics-done)
(org-block-begin-line, org-block-end-line, org-quote)
(org-verse, org-agenda-date, org-agenda-date-today)
(org-agenda-clocking, org-agenda-date-weekend)
(org-agenda-current-time, org-mode-line-clock)
(org-mode-line-clock-overrun): Convert to `defface' from
`org-copy-face'.
The ‘org-copy-face’ function didn’t properly deal with face
customizations and color themes.
---
lisp/org-faces.el | 96 +++++++++++++++++++++++++++++++------------------------
1 file changed, 54 insertions(+), 42 deletions(-)
diff --git a/lisp/org-faces.el b/lisp/org-faces.el
index 6e62ad0..36f810e 100644
--- a/lisp/org-faces.el
+++ b/lisp/org-faces.el
@@ -31,19 +31,6 @@
(require 'org-macs)
(require 'org-compat)
-(defun org-copy-face (old-face new-face docstring &rest attributes)
- (unless (facep new-face)
- (if (fboundp 'set-face-attribute)
- (progn
- (make-face new-face)
- (set-face-attribute new-face nil :inherit old-face)
- (apply 'set-face-attribute new-face nil attributes)
- (set-face-doc-string new-face docstring))
- (copy-face old-face new-face)
- (if (fboundp 'set-face-doc-string)
- (set-face-doc-string new-face docstring)))))
-(put 'org-copy-face 'lisp-indent-function 2)
-
(when (featurep 'xemacs)
(put 'mode-line 'face-alias 'modeline))
@@ -427,12 +414,15 @@ determines if it is a foreground or a background color."
"Face for checkboxes."
:group 'org-faces)
+(defface org-checkbox-statistics-todo
+ '((t (:inherit org-todo)))
+ "Face used for unfinished checkbox statistics."
+ :group 'org-faces)
-(org-copy-face 'org-todo 'org-checkbox-statistics-todo
- "Face used for unfinished checkbox statistics.")
-
-(org-copy-face 'org-done 'org-checkbox-statistics-done
- "Face used for finished checkbox statistics.")
+(defface org-checkbox-statistics-done
+ '((t (:inherit org-done)))
+ "Face used for finished checkbox statistics."
+ :group 'org-faces)
(defcustom org-tag-faces nil
"Faces for specific tags.
@@ -537,11 +527,15 @@ follows a #+DATE:, #+AUTHOR: or #+EMAIL: keyword."
:group 'org-faces
:version "22.1")
-(org-copy-face 'org-meta-line 'org-block-begin-line
- "Face used for the line delimiting the begin of source blocks.")
+(defface org-block-begin-line
+ '((t (:inherit org-meta-line)))
+ "Face used for the line delimiting the begin of source blocks."
+ :group 'org-faces)
-(org-copy-face 'org-meta-line 'org-block-end-line
- "Face used for the line delimiting the end of source blocks.")
+(defface org-block-end-line
+ '((t (:inherit org-block-begin-line)))
+ "Face used for the line delimiting the end of source blocks."
+ :group 'org-faces)
(defface org-verbatim
(org-compatible-face 'shadow
@@ -557,10 +551,15 @@ follows a #+DATE:, #+AUTHOR: or #+EMAIL: keyword."
:group 'org-faces
:version "22.1")
-(org-copy-face 'org-block 'org-quote
- "Face for #+BEGIN_QUOTE ... #+END_QUOTE blocks.")
-(org-copy-face 'org-block 'org-verse
- "Face for #+BEGIN_VERSE ... #+END_VERSE blocks.")
+(defface org-quote
+ '((t (:inherit org-block)))
+ "Face for #+BEGIN_QUOTE ... #+END_QUOTE blocks."
+ :group 'org-faces)
+
+(defface org-verse
+ '((t (:inherit org-block)))
+ "Face for #+BEGIN_VERSE ... #+END_VERSE blocks."
+ :group 'org-faces)
(defcustom org-fontify-quote-and-verse-blocks nil
"Non-nil means, add a special face to #+begin_quote and #+begin_verse block.
@@ -597,21 +596,28 @@ content of these blocks will still be treated as Org syntax."
"Face used in agenda for captions and dates."
:group 'org-faces)
-(org-copy-face 'org-agenda-structure 'org-agenda-date
- "Face used in agenda for normal days.")
+(defface org-agenda-date
+ '((t (:inherit org-agenda-structure)))
+ "Face used in agenda for normal days."
+ :group 'org-faces)
-(org-copy-face 'org-agenda-date 'org-agenda-date-today
+(defface org-agenda-date-today
+ '((t (:inherit org-agenda-date :weight bold :italic t)))
"Face used in agenda for today."
- :weight 'bold :italic 't)
+ :group 'org-faces)
-(org-copy-face 'secondary-selection 'org-agenda-clocking
- "Face marking the current clock item in the agenda.")
+(defface org-agenda-clocking
+ '((t (:inherit secondary-selection)))
+ "Face marking the current clock item in the agenda."
+ :group 'org-faces)
-(org-copy-face 'org-agenda-date 'org-agenda-date-weekend
+(defface org-agenda-date-weekend
+ '((t (:inherit org-agenda-date :weight bold)))
"Face used in agenda for weekend days.
-See the variable `org-agenda-weekend-days' for a definition of which days
-belong to the weekend."
- :weight 'bold)
+
+See the variable `org-agenda-weekend-days' for a definition of
+which days belong to the weekend."
+ :group 'org-faces)
(defface org-scheduled
(org-compatible-face nil
@@ -716,8 +722,10 @@ month and 365.24 days for a year)."
"Face used for time grids."
:group 'org-faces)
-(org-copy-face 'org-time-grid 'org-agenda-current-time
- "Face used to show the current time in the time grid.")
+(defface org-agenda-current-time
+ '((t (:inherit org-time-grid)))
+ "Face used to show the current time in the time grid."
+ :group 'org-faces)
(defface org-agenda-diary
(org-compatible-face 'default nil)
@@ -788,11 +796,15 @@ level org-n-level-faces"
:version "24.4"
:package-version '(Org . "8.0"))
-(org-copy-face 'mode-line 'org-mode-line-clock
- "Face used for clock display in mode line.")
-(org-copy-face 'mode-line 'org-mode-line-clock-overrun
+(defface org-mode-line-clock
+ '((t (:inherit mode-line)))
+ "Face used for clock display in mode line."
+ :group 'org-faces)
+
+(defface org-mode-line-clock-overrun
+ '((t (:inherit mode-line :background "red")))
"Face used for clock display for overrun tasks in mode line."
- :background "red")
+ :group 'org-faces)
(provide 'org-faces)
--
2.1.0
[-- Attachment #3: Type: text/plain, Size: 15 bytes --]
--
Aaron Ecay
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [bug?] org-copy-face doesn’t add faces to org-faces customize group
2014-09-23 17:27 ` Aaron Ecay
@ 2014-09-23 18:00 ` Sebastien Vauban
2014-09-28 3:38 ` Aaron Ecay
0 siblings, 1 reply; 8+ messages in thread
From: Sebastien Vauban @ 2014-09-23 18:00 UTC (permalink / raw)
To: emacs-orgmode-mXXj517/zsQ
Hi Aaron,
Aaron Ecay wrote:
> 2014ko irailak 23an, Sebastien Vauban-ek idatzi zuen:
>> Aaron Ecay wrote:
>>> 2014ko abuztuak 29an, Sebastien Vauban-ek idatzi zuen:
>>>> I think it's related to an Emacs bug (#16440) which I reported on the
>>>> Org mailing list in February.
>>>
>>> I don’t completely understand what is going on in that bug report. My
>>> proposal is to convert org-copy-face to defface.
>>
>> I think this is "the right thing" (TM).
>>
>>> I can’t tell if this would fix your problem or not, but if it’s
>>> exclusive to the faces I listed in my previous email the answer is
>>> probably “yes.”
>>
>> From what I can see, yes, the faces which are wrongly displayed (before
>> re-applying the theme) match the ones defined by `org-copy-face'. So,
>> this seems the right way to go.
>
> OK, thanks for the followup. I’m attaching the patch to this email. If
> I don’t hear any further feedback, I’ll commit it to master in a few
> days.
I'd say: commit it now, so that it really gets to a broad audience.
Anyway, would there be problems, they would be very, very limited: face
problems, that's all!
Best regards,
Seb
--
Sebastien Vauban
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug?] org-copy-face doesn’t add faces to org-faces customize group
2014-09-23 18:00 ` Sebastien Vauban
@ 2014-09-28 3:38 ` Aaron Ecay
2014-09-29 10:06 ` Sebastien Vauban
0 siblings, 1 reply; 8+ messages in thread
From: Aaron Ecay @ 2014-09-28 3:38 UTC (permalink / raw)
To: Sebastien Vauban, emacs-orgmode
Hi Seb,
2014ko irailak 23an, Sebastien Vauban-ek idatzi zuen:
>> OK, thanks for the followup. I’m attaching the patch to this email. If
>> I don’t hear any further feedback, I’ll commit it to master in a few
>> days.
>
> I'd say: commit it now, so that it really gets to a broad audience.
>
> Anyway, would there be problems, they would be very, very limited: face
> problems, that's all!
The patch is now pushed to master.
Thanks,
--
Aaron Ecay
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug?] org-copy-face doesn’t add faces to org-faces customize group
2014-09-28 3:38 ` Aaron Ecay
@ 2014-09-29 10:06 ` Sebastien Vauban
0 siblings, 0 replies; 8+ messages in thread
From: Sebastien Vauban @ 2014-09-29 10:06 UTC (permalink / raw)
To: emacs-orgmode-mXXj517/zsQ
Hi Aaron,
Aaron Ecay wrote:
> 2014ko irailak 23an, Sebastien Vauban-ek idatzi zuen:
>>> OK, thanks for the followup. I’m attaching the patch to this email. If
>>> I don’t hear any further feedback, I’ll commit it to master in a few
>>> days.
>>
>> I'd say: commit it now, so that it really gets to a broad audience.
>>
>> Anyway, would there be problems, they would be very, very limited: face
>> problems, that's all!
>
> The patch is now pushed to master.
That works great. Thanks.
Best regards,
Seb
--
Sebastien Vauban
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-09-29 10:06 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-27 1:41 [bug?] org-copy-face doesn’t add faces to org-faces customize group Aaron Ecay
2014-08-29 11:45 ` Sebastien Vauban
2014-08-30 19:03 ` Aaron Ecay
[not found] ` <87wq9pssuo.fsf-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2014-09-23 9:40 ` Sebastien Vauban
2014-09-23 17:27 ` Aaron Ecay
2014-09-23 18:00 ` Sebastien Vauban
2014-09-28 3:38 ` Aaron Ecay
2014-09-29 10:06 ` Sebastien Vauban
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).