[bug?] org-copy-face doesn’t add faces to org-faces customize group

[bug?] org-copy-face doesn’t add faces to org-faces customize group

[Aaron Ecay]

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

Re: [bug?] org-copy-face doesn’t add faces to org-faces customize group

[Sebastien Vauban]

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

Re: [bug?] org-copy-face doesn’t add faces to org-faces customize group

[Aaron Ecay]

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

Re: [bug?] org-copy-face doesn’t add faces to org-faces customize group

[Sebastien Vauban]

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

Re: [bug?] org-copy-face doesn’t add faces to org-faces customize group

[Aaron Ecay]

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

Re: [bug?] org-copy-face doesn’t add faces to org-faces customize group

[Sebastien Vauban]

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

Re: [bug?] org-copy-face doesn’t add faces to org-faces customize group

[Aaron Ecay]

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

Re: [bug?] org-copy-face doesn’t add faces to org-faces customize group

[Sebastien Vauban]

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