bug#19903: 24.4; Emacs fails to save enriched buffer with error message `wrong-type-argument symbolp "bold"'

bug#19903: 24.4; Emacs fails to save enriched buffer with error message `wrong-type-argument symbolp "bold"'

[Jorge]

[-- Attachment #1: Type: text/plain, Size: 3957 bytes --]

Download the attached files to the same directory.  In GNU Bash, with that
directory current, run
emacs -Q --script bug_script.el &> bug_output.txt

I ran the above command for emacs versions 24.4.1 and 24.4.90.  The output
is in `bug_output_24.4.1.txt' and `bug_output_24.4.90.txt' respectively.  The
bug manifests in both tested Emacs versions.
To compose the initial draft of this bug report, I ran
emacs -Q -l bug_script.el
and then called `report-emacs-bug' with the enriched buffer that fails to be
saved as the current buffer.

So, the input file (`enriched_bug_orig.txt') is in enriched mode, and consists
of one `0' followed by newlines.  I expected Emacs to write the buffer to
`enriched_bug_edited.txt', make the `0' bold, and save the buffer.  In
practice, Emacs does make the `0' bold, but then fails to save the buffer.  The
second line of bug_script.el is `(toggle-debug-on-error)' so Emacs should have
provided useful information.

In GNU Emacs 24.4.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.12.2) of
 2015-01-15 on jorge-Notebook-HP-G42
Windowing system distributor `The X.Org Foundation', version 11.0.11600000
System Description: Ubuntu 14.10

Configured using:
 `configure --prefix=/usr/local/emacs'

Important settings:
  value of $LC_MONETARY: pt_BR.UTF-8
  value of $LC_NUMERIC: pt_BR.UTF-8
  value of $LC_TIME: pt_BR.UTF-8
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Text

Minor modes in effect:
  enriched-mode: t
  tooltip-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  use-hard-newlines: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
C-x o C-x o C-x C-f ~ / D e s k <tab> t r a b a l h
o <tab> <return> C-x o M-x r e p o r t - e m <tab>
<return> C-g C-x o C-x 3 C-x o C-x C-f C-g C-x b <return>
C-f C-SPC C-e C-b M-w C-x o M-x r e p o r t - e m <tab>
<return>

Recent messages:
Note: file is write protected
Saving file /home/jorge/tmp/emacs_enriched_bug/enriched_bug_edited.txt...
Enriched: encoding document...
Wrote /home/jorge/tmp/emacs_enriched_bug/enriched_bug_edited.txt
Saving file /home/jorge/tmp/emacs_enriched_bug/enriched_bug_edited.txt...
Enriched: encoding document...
Entering debugger...
Enriched: decoding document...
Indenting...
Quit [2 times]

Load-path shadows:
None found.

Features:
(shadow sort gnus-util mail-extr emacsbug message format-spec rfc822 mml
mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev
gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums mm-util
help-fns mail-prsvr mail-utils help-mode easymenu debug disp-table
enriched cus-start cus-load time-date tooltip electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel x-win x-dnd tool-bar dnd
fontset image regexp-opt fringe tabulated-list newcomment lisp-mode
prog-mode register page menu-bar rfn-eshadow timer select scroll-bar
mouse jit-lock font-lock syntax facemenu font-core frame cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese case-table epa-hook jka-cmpr-hook help simple abbrev
minibuffer nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote make-network-process
dbusbind gfilenotify dynamic-setting system-font-setting
font-render-setting move-toolbar gtk x-toolkit x multi-tty emacs)

Memory information:
((conses 16 83013 6903)
 (symbols 48 18640 0)
 (miscs 40 67 174)
 (strings 32 11108 4144)
 (string-bytes 1 292848)
 (vectors 16 9828)
 (vector-slots 8 391654 14920)
 (floats 8 72 382)
 (intervals 56 263 1)
 (buffers 960 16)
 (heap 1024 41562 939))

[-- Attachment #2: enriched_bug_orig.txt --]
[-- Type: text/plain, Size: 47 bytes --]

Content-Type: text/enriched
Text-Width: 70

0


[-- Attachment #3: bug_script.el --]
[-- Type: text/x-emacs-lisp, Size: 170 bytes --]

(print (emacs-version))
(toggle-debug-on-error)
(find-file "enriched_bug_orig.txt")
(write-file "enriched_bug_edited.txt")
(facemenu-set-face "bold" 1 2)
(save-buffer 0)

[-- Attachment #4: bug_output_24.4.1.txt --]
[-- Type: text/plain, Size: 4081 bytes --]


Debug on Error enabled globally
Enriched: decoding document...
Indenting...

Saving file /home/jorge/tmp/emacs_enriched_bug/enriched_bug_edited.txt...
Enriched: encoding document...

Wrote /home/jorge/tmp/emacs_enriched_bug/enriched_bug_edited.txt
Saving file /home/jorge/tmp/emacs_enriched_bug/enriched_bug_edited.txt...
Enriched: encoding document...
Debugger entered--Lisp error: (wrong-type-argument symbolp "bold")
  internal-get-lisp-face-attribute("bold" :foreground nil)
  face-attribute("bold" :foreground)
  enriched-face-ans("bold")
  enriched-encode-other-face(nil "bold")
  format-annotate-atomic-property-change(((bold-italic "bold" "italic") (bold "bold") (italic "italic") (underline "underline") (fixed "fixed") (excerpt "excerpt") (default) (nil enriched-encode-other-face)) nil "bold")
  format-annotate-single-property-change(face nil "bold" ((face (bold-italic "bold" "italic") (bold "bold") (italic "italic") (underline "underline") (fixed "fixed") (excerpt "excerpt") (default) (nil enriched-encode-other-face)) (left-margin (4 "indent")) (right-margin (4 "indentright")) (justification (none "nofill") (right "flushright") (left "flushleft") (full "flushboth") (center "center")) (PARAMETER (t "param")) (FUNCTION (enriched-decode-foreground "x-color") (enriched-decode-background "x-bg-color") (enriched-decode-display-prop "x-display")) (read-only (t "x-read-only")) (display (nil enriched-handle-display-prop)) (unknown (nil format-annotate-value))))
  format-annotate-location(1 t (front-sticky rear-nonsticky hard) ((face (bold-italic "bold" "italic") (bold "bold") (italic "italic") (underline "underline") (fixed "fixed") (excerpt "excerpt") (default) (nil enriched-encode-other-face)) (left-margin (4 "indent")) (right-margin (4 "indentright")) (justification (none "nofill") (right "flushright") (left "flushleft") (full "flushboth") (center "center")) (PARAMETER (t "param")) (FUNCTION (enriched-decode-foreground "x-color") (enriched-decode-background "x-bg-color") (enriched-decode-display-prop "x-display")) (read-only (t "x-read-only")) (display (nil enriched-handle-display-prop)) (unknown (nil format-annotate-value))))
  format-annotate-region(1 3 ((face (bold-italic "bold" "italic") (bold "bold") (italic "italic") (underline "underline") (fixed "fixed") (excerpt "excerpt") (default) (nil enriched-encode-other-face)) (left-margin (4 "indent")) (right-margin (4 "indentright")) (justification (none "nofill") (right "flushright") (left "flushleft") (full "flushboth") (center "center")) (PARAMETER (t "param")) (FUNCTION (enriched-decode-foreground "x-color") (enriched-decode-background "x-bg-color") (enriched-decode-display-prop "x-display")) (read-only (t "x-read-only")) (display (nil enriched-handle-display-prop)) (unknown (nil format-annotate-value))) enriched-make-annotation (front-sticky rear-nonsticky hard))
  enriched-encode(1 3 #<buffer enriched_bug_edited.txt>)
  format-encode-run-method(enriched-encode 1 3 #<buffer enriched_bug_edited.txt>)
  format-annotate-function(text/enriched nil nil #<buffer enriched_bug_edited.txt> 0)
  write-region(nil nil "/home/jorge/tmp/emacs_enriched_bug/enriched_bug_edited.txt" nil t "~/tmp/emacs_enriched_bug/enriched_bug_edited.txt")
  basic-save-buffer-2()
  basic-save-buffer-1()
  basic-save-buffer()
  save-buffer(0)
  eval-buffer(#<buffer  *load*> nil "/home/jorge/tmp/emacs_enriched_bug/bug_script.el" nil t)  ; Reading at buffer position 170
  load-with-code-conversion("/home/jorge/tmp/emacs_enriched_bug/bug_script.el" "/home/jorge/tmp/emacs_enriched_bug/bug_script.el" nil t)
  load("/home/jorge/tmp/emacs_enriched_bug/bug_script.el" nil t t)
  command-line-1(("-scriptload" "bug_script.el"))
  command-line()
  normal-top-level()

Enriched: encoding document...
Auto-saving  *Format Temp 0*: Wrong type argument: symbolp, "bold"
Auto-saving  *Format Temp 0*: Wrong type argument: symbolp, "bold"
Auto-saving  *Format Temp 0*: Wrong type argument: symbolp, "bold"

"GNU Emacs 24.4.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.12.2)
 of 2015-01-15 on jorge-Notebook-HP-G42"

[-- Attachment #5: bug_output_24.4.90.txt --]
[-- Type: text/plain, Size: 4072 bytes --]


Debug on Error enabled globally
Enriched: decoding document...
Indenting...

Saving file /home/jorge/tmp/emacs_enriched_bug/enriched_bug_edited.txt...
Enriched: encoding document...

Wrote /home/jorge/tmp/emacs_enriched_bug/enriched_bug_edited.txt
Saving file /home/jorge/tmp/emacs_enriched_bug/enriched_bug_edited.txt...
Enriched: encoding document...
Debugger entered--Lisp error: (wrong-type-argument symbolp "bold")
  internal-get-lisp-face-attribute("bold" :foreground nil)
  face-attribute("bold" :foreground)
  enriched-face-ans("bold")
  enriched-encode-other-face(nil "bold")
  format-annotate-atomic-property-change(((bold-italic "bold" "italic") (bold "bold") (italic "italic") (underline "underline") (fixed "fixed") (excerpt "excerpt") (default) (nil enriched-encode-other-face)) nil "bold")
  format-annotate-single-property-change(face nil "bold" ((face (bold-italic "bold" "italic") (bold "bold") (italic "italic") (underline "underline") (fixed "fixed") (excerpt "excerpt") (default) (nil enriched-encode-other-face)) (left-margin (4 "indent")) (right-margin (4 "indentright")) (justification (none "nofill") (right "flushright") (left "flushleft") (full "flushboth") (center "center")) (PARAMETER (t "param")) (FUNCTION (enriched-decode-foreground "x-color") (enriched-decode-background "x-bg-color") (enriched-decode-display-prop "x-display")) (read-only (t "x-read-only")) (display (nil enriched-handle-display-prop)) (unknown (nil format-annotate-value))))
  format-annotate-location(1 t (front-sticky rear-nonsticky hard) ((face (bold-italic "bold" "italic") (bold "bold") (italic "italic") (underline "underline") (fixed "fixed") (excerpt "excerpt") (default) (nil enriched-encode-other-face)) (left-margin (4 "indent")) (right-margin (4 "indentright")) (justification (none "nofill") (right "flushright") (left "flushleft") (full "flushboth") (center "center")) (PARAMETER (t "param")) (FUNCTION (enriched-decode-foreground "x-color") (enriched-decode-background "x-bg-color") (enriched-decode-display-prop "x-display")) (read-only (t "x-read-only")) (display (nil enriched-handle-display-prop)) (unknown (nil format-annotate-value))))
  format-annotate-region(1 3 ((face (bold-italic "bold" "italic") (bold "bold") (italic "italic") (underline "underline") (fixed "fixed") (excerpt "excerpt") (default) (nil enriched-encode-other-face)) (left-margin (4 "indent")) (right-margin (4 "indentright")) (justification (none "nofill") (right "flushright") (left "flushleft") (full "flushboth") (center "center")) (PARAMETER (t "param")) (FUNCTION (enriched-decode-foreground "x-color") (enriched-decode-background "x-bg-color") (enriched-decode-display-prop "x-display")) (read-only (t "x-read-only")) (display (nil enriched-handle-display-prop)) (unknown (nil format-annotate-value))) enriched-make-annotation (front-sticky rear-nonsticky hard))
  enriched-encode(1 3 #<buffer enriched_bug_edited.txt>)
  format-encode-run-method(enriched-encode 1 3 #<buffer enriched_bug_edited.txt>)
  format-annotate-function(text/enriched nil nil #<buffer enriched_bug_edited.txt> 0)
  write-region(nil nil "/home/jorge/tmp/emacs_enriched_bug/enriched_bug_edited.txt" nil t "~/tmp/emacs_enriched_bug/enriched_bug_edited.txt")
  basic-save-buffer-2()
  basic-save-buffer-1()
  basic-save-buffer()
  save-buffer(0)
  eval-buffer(#<buffer  *load*> nil "/home/jorge/tmp/emacs_enriched_bug/bug_script.el" nil t)  ; Reading at buffer position 170
  load-with-code-conversion("/home/jorge/tmp/emacs_enriched_bug/bug_script.el" "/home/jorge/tmp/emacs_enriched_bug/bug_script.el" nil t)
  load("/home/jorge/tmp/emacs_enriched_bug/bug_script.el" nil t t)
  command-line-1(("-scriptload" "bug_script.el"))
  command-line()
  normal-top-level()

Enriched: encoding document...
Auto-saving  *Format Temp 0*: Wrong type argument: symbolp, "bold"
Auto-saving  *Format Temp 0*: Wrong type argument: symbolp, "bold"
Auto-saving  *Format Temp 0*: Wrong type argument: symbolp, "bold"

"GNU Emacs 24.4.90.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.12.2)
 of 2015-02-19 on SD-271230"

bug#19903: 24.4; wrong-type-argument symbolp "bold" during enriched-encode

[Ivan Shmakov]

>>>>> Jorge  <jorge13515@gmail.com> writes:

[…]

 > (print (emacs-version))
 > (toggle-debug-on-error)
 > (find-file "enriched_bug_orig.txt")
 > (write-file "enriched_bug_edited.txt")
 > (facemenu-set-face "bold" 1 2)
 > (save-buffer 0)

	Could you please try and report if the error is still signalled
	if the code above is changed to use the 'bold symbol instead of
	a "bold" string?

[…]

 > Enriched: encoding document...
 > Debugger entered--Lisp error: (wrong-type-argument symbolp "bold")
 >   internal-get-lisp-face-attribute("bold" :foreground nil)
 >   face-attribute("bold" :foreground)
 >   enriched-face-ans("bold")
 >   enriched-encode-other-face(nil "bold")

	Unless I be mistaken, faces are ought to be symbols, not
	strings, so this error appears entirely reasonable.

[…]

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A

bug#19903: 24.4; Emacs fails to save enriched buffer with error message `wrong-type-argument symbolp "bold"'

[Eli Zaretskii]

> Date: Thu, 19 Feb 2015 15:15:40 -0200
> From: Jorge <jorge13515@gmail.com>
> 
> Download the attached files to the same directory.  In GNU Bash, with that
> directory current, run
> emacs -Q --script bug_script.el &> bug_output.txt
> 
> I ran the above command for emacs versions 24.4.1 and 24.4.90.  The output
> is in `bug_output_24.4.1.txt' and `bug_output_24.4.90.txt' respectively.  The
> bug manifests in both tested Emacs versions.
> To compose the initial draft of this bug report, I ran
> emacs -Q -l bug_script.el
> and then called `report-emacs-bug' with the enriched buffer that fails to be
> saved as the current buffer.
> 
> So, the input file (`enriched_bug_orig.txt') is in enriched mode, and consists
> of one `0' followed by newlines.  I expected Emacs to write the buffer to
> `enriched_bug_edited.txt', make the `0' bold, and save the buffer.  In
> practice, Emacs does make the `0' bold, but then fails to save the buffer.  The
> second line of bug_script.el is `(toggle-debug-on-error)' so Emacs should have
> provided useful information.
> [...]
> (print (emacs-version))
> (toggle-debug-on-error)
> (find-file "enriched_bug_orig.txt")
> (write-file "enriched_bug_edited.txt")
> (facemenu-set-face "bold" 1 2)  <<<<<<<<<<<<<<<<<<<<<<<
> (save-buffer 0)

That's a cockpit error: the marked line should have said

  (facemenu-set-face 'bold 1 2)

instead.  Then this script will work and do what you expect.

A face is represented by its symbol, not by its string name.

I'm therefore closing this bug.

bug#19903: 24.4; Emacs fails to save enriched buffer with error message `wrong-type-argument symbolp "bold"'

[Jorge]

On Fri, Feb 20, 2015 at 6:27 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> That's a cockpit error: the marked line should have said
>
>   (facemenu-set-face 'bold 1 2)
>
> instead.  Then this script will work and do what you expect.
Thank you very much for the clarification.  I just corrected my code.
However, it seems to me that the current behavior is not
user-friendly.  (facemenu-set-face "bold" 1 2) _does_ make the text
bold, without any warning - which makes it appear that the call was
successful - but then we get an error when we try to save the buffer.
Generally, it is best for errors to be detected as close to their
source as possible.  What do you think?

Regards

bug#19903: 24.4; wrong-type-argument symbolp "bold" during enriched-encode

[Ivan Shmakov]

>>>>> Jorge  <jorge13515@gmail.com> writes:

[…]

 > However, it seems to me that the current behavior is not
 > user-friendly.  (facemenu-set-face "bold" 1 2) _does_ make the text
 > bold, without any warning – which makes it appear that the call was
 > successful – but then we get an error when we try to save the buffer.

	The facemenu-set-face command is essentially a call to
	facemenu-add-new-face, followed by a call to facemenu-add-face.
	Curiously enough, the former implements explicit support for
	string arguments:

$ git archive --format=tar  c4e2be4587ec -- lisp/facemenu.el \
      | tar -xO | nl -ba ;  # 2015-02-16 07:22:46 +0000 
…
   785	(defun facemenu-add-new-face (face)
   786	  "Add FACE (a face) to the Face menu if `facemenu-listed-faces' says so.
   787	This is called whenever you create a new face, and at other times."
   788	  (let* (name
   789		 symbol
…
   792		 function menu-val)
   793	    (if (symbolp face)
   794		(setq name (symbol-name face)
   795		      symbol face)
   796	      (setq name face
   797		    symbol (intern name)))

 > Generally, it is best for errors to be detected as close to their
 > source as possible.  What do you think?

	If string faces are rendered just the same as symbol ones,
	I’d rather wonder if either facemenu-add-face should silently
	‘intern’ all the strings it gets before use, /or/ face-attribute
	should do that.  (Or perhaps both.)

-- 
FSF associate member #7257  np. Transformation — David Modica  B6A0 230E 334A

bug#19903: 24.4; Emacs fails to save enriched buffer with error message `wrong-type-argument symbolp "bold"'

[Eli Zaretskii]

> Date: Fri, 20 Feb 2015 06:51:54 -0200
> From: Jorge <jorge13515@gmail.com>
> Cc: 19903-done@debbugs.gnu.org
> 
> On Fri, Feb 20, 2015 at 6:27 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> > That's a cockpit error: the marked line should have said
> >
> >   (facemenu-set-face 'bold 1 2)
> >
> > instead.  Then this script will work and do what you expect.
> Thank you very much for the clarification.  I just corrected my code.
> However, it seems to me that the current behavior is not
> user-friendly.  (facemenu-set-face "bold" 1 2) _does_ make the text
> bold, without any warning - which makes it appear that the call was
> successful - but then we get an error when we try to save the buffer.
> Generally, it is best for errors to be detected as close to their
> source as possible.  What do you think?

facemenu-set-face calls several other functions, some of which accept
face names, some don't.

bug#19903: 24.4; wrong-type-argument symbolp "bold" during enriched-encode

[Eli Zaretskii]

> From: Ivan Shmakov <ivan@siamics.net>
> Date: Fri, 20 Feb 2015 09:12:08 +0000
> 
> 	If string faces are rendered just the same as symbol ones,
> 	I’d rather wonder if either facemenu-add-face should silently
> 	‘intern’ all the strings it gets before use, /or/ face-attribute
> 	should do that.  (Or perhaps both.)

Patches to do that are welcome.

Thanks.

bug#19903: 24.4; wrong-type-argument symbolp "bold" during enriched-encode

[Stefan Monnier]

>> If string faces are rendered just the same as symbol ones,

I think that's a misfeature.

>> I’d rather wonder if either facemenu-add-face should silently
>> ‘intern’ all the strings it gets before use, /or/ face-attribute
>> should do that.  (Or perhaps both.)

I think interning would be good, indeed, tho I would prefer if it
emitted some kind of warning instead of doing it silently, to try and
stop this bad habit.


        Stefan

bug#19903: 24.4; wrong-type-argument symbolp "bold" during enriched-encode

[Ivan Shmakov]

[-- Attachment #1: Type: text/plain, Size: 726 bytes --]

>>>>> Stefan Monnier <monnier@iro.umontreal.ca> writes:

 >> If string faces are rendered just the same as symbol ones,

 > I think that's a misfeature.

	I’m not familiar with the Emacs display code, so I have no
	opinion on this.

 >> I’d rather wonder if either facemenu-add-face should silently
 >> ‘intern’ all the strings it gets before use, /or/ face-attribute
 >> should do that.  (Or perhaps both.)

 > I think interning would be good, indeed, tho I would prefer if it
 > emitted some kind of warning instead of doing it silently, to try and
 > stop this bad habit.

	Please consider the patch MIMEd.

-- 
FSF associate member #7257  np. Night Prowler — AC/DC   … 3013 B6A0 230E 334A

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/diff, Size: 1065 bytes --]

--- a/lisp/facemenu.el
+++ b/lisp/facemenu.el
@@ -702,6 +702,9 @@ defun facemenu-add-face (face &optional start end)
 text property.  Otherwise, selecting the default face would not have any
 effect.  See `facemenu-remove-face-function'."
   (interactive "*xFace: \nr")
+  (when (stringp face)
+    (warn "Face %S is a string; interning" face)
+    (setq face (intern face)))
   (cond
    ((and (eq face 'default)
          (not (eq facemenu-remove-face-function t)))
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -402,6 +402,9 @@ defun face-attribute (face attribute &optional frame inherit)
 value of `default' for INHERIT; this will resolve any unspecified or
 relative values by merging with the `default' face (which is always
 completely specified)."
+  (when (stringp face)
+    (message "Face %S is a string; interning" face)
+    (setq face (intern face)))
   (let ((value (internal-get-lisp-face-attribute face attribute frame)))
     (when (and inherit (face-attribute-relative-p attribute value))
       ;; VALUE is relative, so merge with inherited faces

bug#19903: 24.4; wrong-type-argument symbolp "bold" during enriched-encode

[Stefan Monnier]

> +    (warn "Face %S is a string; interning" face)
[...]
> +    (message "Face %S is a string; interning" face)

Why `warn' in one and `message' in the other?

These could end up spewing a lot of warnings at times, so it's a bit
risky, but if such uses are sufficiently rare it might be OK.


        Stefan

bug#19903: 24.4; wrong-type-argument symbolp "bold" during enriched-encode

[Ivan Shmakov]

>>>>> Stefan Monnier <monnier@iro.umontreal.ca> writes:

 >> + (warn "Face %S is a string; interning" face)

 >> + (message "Face %S is a string; interning" face)

 > Why `warn' in one and `message' in the other?

	By the time we hit this in ‘face-attribute’, the point at which
	the sub-setandard string-named face was introduced is presumably
	long gone, so there’s no good reason to use ‘warn’.

	On the contrary, ‘facemenu-add-face’ is likely to be invoked
	either directly by the user, or as part of some other
	interactive command.  Should the call result in a warning,
	the user would probably be able to gather enough context from
	his or her actions immediately preceding the warning.

 > These could end up spewing a lot of warnings at times, so it's a bit
 > risky, but if such uses are sufficiently rare it might be OK.

	At least so I hope.

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A

bug#19903: 24.4; wrong-type-argument symbolp "bold" during enriched-encode

[Ivan Shmakov]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

>>>>> Ivan Shmakov <ivan@siamics.net> writes:
>>>>> Stefan Monnier <monnier@iro.umontreal.ca> writes:

 >>> + (warn "Face %S is a string; interning" face)

 >>> + (message "Face %S is a string; interning" face)

 >> Why `warn' in one and `message' in the other?

 > By the time we hit this in ‘face-attribute’, the point at which the
 > sub-setandard string-named face was introduced is presumably long
 > gone, so there’s no good reason to use ‘warn’.

	… Or, on a second thought, ‘message’, either.  Given that
	‘face-attribute’ has no idea of where the caller got this face
	from, there’s simply no way for it to provide any helpful
	message at this point.  (Say, “Face "bold", as found at position
	42 in #<buffer foo>, is a string; interning”.)

	Now, given that there’s a number of “internal” functions (such
	as ‘internal-lisp-face-p’, for instance) which accept string
	face names just fine, I wonder if it makes sense to just change
	‘internal-get-lisp-face-attribute’ accordingly?  An untested
	patch is hereby MIMEd.

[…]

-- 
FSF associate member #7257  np. Fear of the Dark — Iron Maiden B6A0 230E 334A

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/diff, Size: 1333 bytes --]

--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -3570,15 +3570,18 @@ the result will be absolute, otherwise it will be relative.  */)
 DEFUN ("internal-get-lisp-face-attribute", Finternal_get_lisp_face_attribute,
        Sinternal_get_lisp_face_attribute,
        2, 3, 0,
-       doc: /* Return face attribute KEYWORD of face SYMBOL.
-If SYMBOL does not name a valid Lisp face or KEYWORD isn't a valid
+       doc: /* Return face attribute KEYWORD of face FACE.
+FACE should be a symbol or string.
+If FACE does not name a valid Lisp face or KEYWORD isn't a valid
 face attribute name, signal an error.
-If the optional argument FRAME is given, report on face SYMBOL in that
-frame.  If FRAME is t, report on the defaults for face SYMBOL (for new
+If the optional argument FRAME is given, report on face FACE in that
+frame.  If FRAME is t, report on the defaults for face FACE (for new
 frames).  If FRAME is omitted or nil, use the selected frame.  */)
-  (Lisp_Object symbol, Lisp_Object keyword, Lisp_Object frame)
+  (Lisp_Object face, Lisp_Object keyword, Lisp_Object frame)
 {
   struct frame *f = EQ (frame, Qt) ? NULL : decode_live_frame (frame);
+  Lisp_Object symbol = (STRINGP (face) ? intern (SSDATA (face))
+			: face);
   Lisp_Object lface = lface_from_face_name (f, symbol, true), value = Qnil;
 
   CHECK_SYMBOL (symbol);

bug#19903: 24.4; wrong-type-argument symbolp "bold" during enriched-encode

[Eli Zaretskii]

> From: Ivan Shmakov <ivan@siamics.net>
> Date: Sat, 21 Feb 2015 11:12:44 +0000
> 
> 	Now, given that there’s a number of “internal” functions (such
> 	as ‘internal-lisp-face-p’, for instance) which accept string
> 	face names just fine, I wonder if it makes sense to just change
> 	‘internal-get-lisp-face-attribute’ accordingly?  An untested
> 	patch is hereby MIMEd.

I don't think internal functions should cater to UI issues, unless
they are themselves interactive.

If we keep this confined to interactive functions, how many such
functions in facemenu.el will have to be changed?  If not too many,
I'm inclined to keep this there.

bug#19912: facemenu-add-face: does not handle 'face being set to a property list

[Ivan Shmakov]

[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]

Package:  emacs
Severity: minor
Tags: patch

	As currently implemented, facemenu-add-face doesn’t handle the
	case of the 'face property value being a property list, like:

(with-temp-buffer
  (insert "Hello, world!")
  (put-text-property 3 11 'face '(:weight bold))
  (facemenu-add-face 'italic 5 7)
  (buffer-string))

	The relevant part of the backtrace is like:

  check-face(:weight)
  facemenu-active-faces((italic :weight bold) #<frame F1 0xb497d0>)
  facemenu-add-face(italic 5 7)

	With the patch MIMEd, the example produces the expected result:

#("Hello, world!"
  2 4 (face (:weight bold))
  4 6 (face (italic (:weight bold)))
  6 10 (face (:weight bold)))

	* lisp/facemenu.el (facemenu-add-face): Follow the (stricter)
	logic of merge_face_ref when determining whether the value of
	the 'face property is a sole face or a list thereof.  (Bug#???)

	(Tested on c4e2be4587ec, 2015-02-16 07:22:46 UTC.)

	Alternatively, a suitable, Lisp-callable predicate may be
	split off ‘merge_face_ref’ (src/faces.c) and be used ine
	‘facemenu-add-face’.

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/diff, Size: 1024 bytes --]

--- a/lisp/facemenu.el
+++ b/lisp/facemenu.el
@@ -732,9 +732,17 @@ defun facemenu-add-face (face &optional start end)
                                  face
                                (facemenu-active-faces
                                 (cons face
-                                      (if (listp prev)
-                                          prev
-                                        (list prev)))
+                                      (if (or (atom prev)
+					      (not (symbolp (car prev)))
+                                              (memq (car prev)
+						    '(foreground-color
+						      background-color))
+                                              (let ((n (symbol-name
+							(car prev))))
+						(and (> (length n)  0)
+						     (eq ?: (aref n 0)))))
+					  (list prev)
+					prev))
                                 ;; Specify the selected frame
                                 ;; because nil would mean to use
                                 ;; the new-frame default settings,

bug#19903: 24.4; wrong-type-argument symbolp "bold" during enriched-encode

[Ivan Shmakov]

>>>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>> From: Ivan Shmakov  Date: Sat, 21 Feb 2015 11:12:44 +0000

 >> Now, given that there’s a number of “internal” functions (such as
 >> ‘internal-lisp-face-p’, for instance) which accept string face names
 >> just fine, I wonder if it makes sense to just change
 >> ‘internal-get-lisp-face-attribute’ accordingly?  An untested patch
 >> is hereby MIMEd.

 > I don't think internal functions should cater to UI issues, unless
 > they are themselves interactive.

	I’m unsure where you see an UI issue here?  The issue, as
	originally reported, is that face-attribute fails to handle
	string-named faces, which are considered perfectly valid by the
	rest of Emacs (including, say, facep and the display engine.)

	That is: the user accidentally sets 'face to a string, and is
	surprised to find that while the result is displayed just fine,
	some part of Emacs (namely, enriched-mode) breaks instantly.

	From there, we can go different ways, including:

	• bury our head in the sand and pretend there’s no issue;

	• patch one or two of the functions which can be used to add
	  such faces – to either silently (or not so) replace them with
	  the respective symbol-named ones, /or/ to signal an error;
	  this won’t prevent, however, the use of put-text-property and
	  the likes of it to achieve that same effect;

	• begin to slowly deprecate string-named faces; I guess we’d
	  rather have the display engine log the cases of their use,
	  at least once per buffer, to highlight the affected code and
	  thus ease the migration – from this undocumented feature to
	  the documented lack thereof;

	• accept string-named faces as valid and make sure that this
	  applies uniformly throughout the code; my latest patch changes
	  internal-get-lisp-face-attribute to achieve this, but I’m fine
	  with applying an earlier one instead, which similarly changes
	  face-attribute.

 > If we keep this confined to interactive functions, how many such
 > functions in facemenu.el will have to be changed?  If not too many,
 > I'm inclined to keep this there.

	I believe that facemenu-add-face is the only function which can
	be used to add a string-named face /interactively/, as it reads
	an arbitrary Lisp form for the face.  (See also #18369.)

	The original report, however, was concerned with the use of
	facemenu-add-face from Lisp, and there’re still a plenty of ways
	to hit this issue apart from using facemenu-add-face with a
	string argument.

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A

bug#19912: facemenu-add-face: does not handle 'face being set to a property list

[Ivan Shmakov]

[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]

>>>>> Ivan Shmakov <ivan@siamics.net> writes:

[…]

 > * lisp/facemenu.el (facemenu-add-face): Follow the (stricter) logic
 > of merge_face_ref when determining whether the value of the 'face
 > property is a sole face or a list thereof.  (Bug#???)

 > (Tested on c4e2be4587ec, 2015-02-16 07:22:46 UTC.)

 > Alternatively, a suitable, Lisp-callable predicate may be
 > split off ‘merge_face_ref’ (src/faces.c) and be used ine
 > ‘facemenu-add-face’.

	… Or should such a predicate be introduced to faces.el instead?
	The merge_face_ref () logic is a bit arcane, so I’d rather keep
	it confined to the inner parts of Emacs (whether faces.c,
	faces.el, or similar.)

	Please consider the revised patch MIMEd.

	* lisp/faces.el (face-list-p): New function.
	* lisp/facemenu.el (facemenu-add-face): Use it.  (Bug#19912)

	Unless there be objections, I hope to push this new change
	to ‘master’ within the next day or two.

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/diff, Size: 1378 bytes --]

--- a/lisp/facemenu.el
+++ b/lisp/facemenu.el
@@ -732,7 +732,7 @@ defun facemenu-add-face (face &optional start end)
                                  face
                                (facemenu-active-faces
                                 (cons face
-                                      (if (listp prev)
+                                      (if (face-list-p prev)
                                           prev
                                         (list prev)))
                                 ;; Specify the selected frame
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -273,6 +273,22 @@
   (not (internal-lisp-face-empty-p face frame)))
 
 
+(defun face-list-p (face-or-list)
+  "True if FACE-OR-LIST is a list of faces.
+Return nil if FACE-OR-LIST is a non-nil atom, or a cons cell whose car
+is either 'foreground-color, 'background-color, or a symbol starting
+with a colon (:)."
+  ;; The logic of merge_face_ref (xfaces.c) is recreated here.
+  (or (null face-or-list)
+      (and (consp face-or-list)
+	   (symbolp (car face-or-list))
+	   (not (memq (car face-or-list)
+		      '(foreground-color background-color)))
+	   (let ((n (symbol-name (car face-or-list))))
+	     (or (zerop (length n))
+		 (not (eq ?: (aref n 0))))))))
+
+
 \f
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;;; Setting face attributes from X resources.

bug#19903: 24.4; wrong-type-argument symbolp "bold" during enriched-encode

[Eli Zaretskii]

> From: Ivan Shmakov <ivan@siamics.net>
> Date: Wed, 25 Feb 2015 06:20:36 +0000
> 
>  > I don't think internal functions should cater to UI issues, unless
>  > they are themselves interactive.
> 
> 	I’m unsure where you see an UI issue here?  The issue, as
> 	originally reported, is that face-attribute fails to handle
> 	string-named faces, which are considered perfectly valid by the
> 	rest of Emacs (including, say, facep and the display engine.)

Accepting strings instead of symbols is a convenience feature for
users, so it's a UI issue.

> 	From there, we can go different ways, including:
> 
> 	• bury our head in the sand and pretend there’s no issue;

I don't think anybody suggested that; I certainly didn't.  If you are
now suggesting it, then no, I don't think we should.

> 	• patch one or two of the functions which can be used to add
> 	  such faces – to either silently (or not so) replace them with
> 	  the respective symbol-named ones, /or/ to signal an error;
> 	  this won’t prevent, however, the use of put-text-property and
> 	  the likes of it to achieve that same effect;

I don't see why we need to spread this so wide.  Making facemenu.el
behave consistently sounds good enough, and will solve the original
report.

>  > If we keep this confined to interactive functions, how many such
>  > functions in facemenu.el will have to be changed?  If not too many,
>  > I'm inclined to keep this there.
> 
> 	I believe that facemenu-add-face is the only function which can
> 	be used to add a string-named face /interactively/, as it reads
> 	an arbitrary Lisp form for the face.  (See also #18369.)

Yes, but how many don't?

> 
> 	The original report, however, was concerned with the use of
> 	facemenu-add-face from Lisp, and there’re still a plenty of ways
> 	to hit this issue apart from using facemenu-add-face with a
> 	string argument.

I don't think we need to consider them.

bug#19903: 24.4; wrong-type-argument symbolp "bold" during enriched-encode

[Ivan Shmakov]

>>>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>> From: Ivan Shmakov  Date: Wed, 25 Feb 2015 06:20:36 +0000

 >>> I don't think internal functions should cater to UI issues, unless
 >>> they are themselves interactive.

 >> I’m unsure where you see an UI issue here?  The issue, as originally
 >> reported, is that face-attribute fails to handle string-named faces,
 >> which are considered perfectly valid by the rest of Emacs
 >> (including, say, facep and the display engine.)

 > Accepting strings instead of symbols is a convenience feature
 > for users, so it's a UI issue.

	Could you please elaborate on this?  Specifically, does this
	apply to the interactive or non-interactive use (or both) of
	facemenu-add-face?

[…]

 >>> If we keep this confined to interactive functions, how many such
 >>> functions in facemenu.el will have to be changed?  If not too many,
 >>> I'm inclined to keep this there.

 >> I believe that facemenu-add-face is the only function which can be
 >> used to add a string-named face /interactively/, as it reads an
 >> arbitrary Lisp form for the face.  (See also #18369.)

 > Yes, but how many don't?

	One another (facemenu-set-face) uses read-face-name, which in
	turn explicitly passes user input through ‘intern’.

	Then, facemenu-set-face-from-menu uses last-command-event (when
	called interactively), assumes it’s a symbol, and uses it either
	as a face directly /or/ (should its name begin with fg: or bg:)
	as the cdr for a cons cell face.  (The facemenu-set-foreground
	and facemenu-set-background commands rely on this.)

	Per my reading of the code, no other command there accepts
	user-specified faces when used interactively.

[…]

-- 
FSF associate member #7257  np. Coming Home — Iron Maiden  B6A0 230E 334A

bug#19912: facemenu-add-face: does not handle 'face being set to a property list

[Ivan Shmakov]

[-- Attachment #1: Type: text/plain, Size: 408 bytes --]

	Please consider the (once again) revised patch MIMEd.

	* lisp/faces.el (face-list-p): Split from face-at-point.
	(face-at-point): Use it.
	* lisp/facemenu.el (facemenu-add-face): Likewise.  (Bug#19912)

	Unless there be objections, I hope to push this new change
	to ‘master’ within the next day or two.

-- 
FSF associate member #7257  np. Isle of Avalon — Iron Maiden … B6A0 230E 334A

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/diff, Size: 1891 bytes --]

--- a/lisp/facemenu.el
+++ b/lisp/facemenu.el
@@ -732,7 +732,7 @@ defun facemenu-add-face (face &optional start end)
                                  face
                                (facemenu-active-faces
                                 (cons face
-                                      (if (listp prev)
+                                      (if (face-list-p prev)
                                           prev
                                         (list prev)))
                                 ;; Specify the selected frame
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -273,6 +273,17 @@ defun face-nontrivial-p (face &optional frame)
   (not (internal-lisp-face-empty-p face frame)))
 
 
+(defun face-list-p (face-or-list)
+  "True if FACE-OR-LIST is a list of faces.
+Return nil if FACE-OR-LIST is a non-nil atom, or a cons cell whose car
+is either 'foreground-color, 'background-color, or a keyword."
+  ;; The logic of merge_face_ref (xfaces.c) is recreated here.
+  (and (listp face-or-list)
+       (not (memq (car face-or-list)
+		  '(foreground-color background-color)))
+       (not (keywordp (car face-or-list)))))
+
+
 \f
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;;; Setting face attributes from X resources.
@@ -1922,11 +1933,7 @@ defun face-at-point (&optional thing multiple)
                         (get-char-property (point) 'face))))
       (cond ((facep faceprop)
              (push faceprop faces))
-            ((and (listp faceprop)
-                  ;; Don't treat an attribute spec as a list of faces.
-                  (not (keywordp (car faceprop)))
-                  (not (memq (car faceprop)
-                             '(foreground-color background-color))))
+            ((face-list-p faceprop)
              (dolist (face faceprop)
                (if (facep face)
                    (push face faces))))))

bug#19903: 24.4; wrong-type-argument symbolp "bold" during enriched-encode

[Eli Zaretskii]

> From: Ivan Shmakov <ivan@siamics.net>
> Date: Wed, 25 Feb 2015 17:10:13 +0000
> 
> >>>>> Eli Zaretskii <eliz@gnu.org> writes:
> >>>>> From: Ivan Shmakov  Date: Wed, 25 Feb 2015 06:20:36 +0000
> 
>  >>> I don't think internal functions should cater to UI issues, unless
>  >>> they are themselves interactive.
> 
>  >> I’m unsure where you see an UI issue here?  The issue, as originally
>  >> reported, is that face-attribute fails to handle string-named faces,
>  >> which are considered perfectly valid by the rest of Emacs
>  >> (including, say, facep and the display engine.)
> 
>  > Accepting strings instead of symbols is a convenience feature
>  > for users, so it's a UI issue.
> 
> 	Could you please elaborate on this?  Specifically, does this
> 	apply to the interactive or non-interactive use (or both) of
> 	facemenu-add-face?

Both.

>  >>> If we keep this confined to interactive functions, how many such
>  >>> functions in facemenu.el will have to be changed?  If not too many,
>  >>> I'm inclined to keep this there.
> 
>  >> I believe that facemenu-add-face is the only function which can be
>  >> used to add a string-named face /interactively/, as it reads an
>  >> arbitrary Lisp form for the face.  (See also #18369.)
> 
>  > Yes, but how many don't?
> 
> 	One another (facemenu-set-face) uses read-face-name, which in
> 	turn explicitly passes user input through ‘intern’.
> 
> 	Then, facemenu-set-face-from-menu uses last-command-event (when
> 	called interactively), assumes it’s a symbol, and uses it either
> 	as a face directly /or/ (should its name begin with fg: or bg:)
> 	as the cdr for a cons cell face.  (The facemenu-set-foreground
> 	and facemenu-set-background commands rely on this.)
> 
> 	Per my reading of the code, no other command there accepts
> 	user-specified faces when used interactively.

So only one function needs a change?  If so, I think that's what we
should do.

bug#19903: 24.4; wrong-type-argument symbolp "bold" during enriched-encode

[Ivan Shmakov]

>>>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>> From: Ivan Shmakov  Date: Wed, 25 Feb 2015 17:10:13 +0000
>>>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>> From: Ivan Shmakov  Date: Wed, 25 Feb 2015 06:20:36 +0000

[…]

 >>>> I’m unsure where you see an UI issue here?  The issue, as
 >>>> originally reported, is that face-attribute fails to handle
 >>>> string-named faces, which are considered perfectly valid by the
 >>>> rest of Emacs (including, say, facep and the display engine.)

 >>> Accepting strings instead of symbols is a convenience feature
 >>> for users, so it's a UI issue.

 >> Could you please elaborate on this?  Specifically, does this apply
 >> to the interactive or non-interactive use (or both) of
 >> facemenu-add-face?

 > Both.

	Frankly, I fail to see how allowing the first of the following
	provides for any additional convenience over the second:

    M-x facemenu-add-face RET "bold" RET  (adds 'bold face);
    M-x facemenu-add-face RET bold RET    (adds "bold" one.)

	And, similarly:

   (facemenu-add-face "bold" here there)
   (facemenu-add-face 'bold  here there)

[…]

-- 
FSF associate member #7257  When the Wild Wind Blows — Iron Maiden  230E 334A

bug#19912: facemenu-add-face: does not handle 'face being set to a property list

[Ivan Shmakov]

>>>>> Ivan Shmakov <ivan@siamics.net> writes:

 > * lisp/faces.el (face-list-p): Split from face-at-point.
 > (face-at-point): Use it.
 > * lisp/facemenu.el (facemenu-add-face): Likewise.  (Bug#19912)

 > Unless there be objections, I hope to push this new change to
 > ‘master’ within the next day or two.

	Pushed, closing.

commit 619fc5c197ebef5444aed24fe30657989fc2a839
CommitDate: 2015-02-26 18:09:48 +0000

    Fix 'face property handling in facemenu-add-face.
    
    * lisp/faces.el (face-list-p): Split from face-at-point.
    (face-at-point): Use it.
    * lisp/facemenu.el (facemenu-add-face): Likewise.
    
    Fixes: debbugs:19912

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A