bug#31376: 26.0.50; print-charset-text-property not honored

bug#31376: 26.0.50; print-charset-text-property not honored

[Helmut Eller]

This example 

(let ((print-charset-text-property nil)
      (print-escape-multibyte t))
   (prin1 #("\x00f6\ " 0 1 (charset iso-8859-1))
           (current-buffer)))

when executed in the *scratch* buffer inserts
#("\x00f6\ " 0 1 (charset iso-8859-1))

I would expect "\x00f6\ " without the #(...) as
print-charset-text-property is bound to nil.


In GNU Emacs 26.0.50 (build 10, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2017-08-15 built on caladan
Repository revision: 66b75d3f2002459edccd241af57c63b380b192d3
Windowing system distributor 'The X.Org Foundation', version 11.0.11902000
System Description:	Debian GNU/Linux 9.0 (stretch)

Configured using:
 'configure --with-xpm=no --with-gif=no --with-tiff=no --with-jpeg=no
 --without-pop'

Configured features:
PNG SOUND DBUS GSETTINGS NOTIFY GNUTLS LIBXML2 FREETYPE XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 LIBSYSTEMD

Important settings:
  value of $LANG: C.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  diff-auto-refine-mode: t
  whitespace-mode: t
  outline-minor-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

bug#31376: 26.0.50; print-charset-text-property not honored

[Andreas Schwab]

On Mai 07 2018, Helmut Eller <eller.helmut@gmail.com> wrote:

> This example 
>
> (let ((print-charset-text-property nil)
>       (print-escape-multibyte t))
>    (prin1 #("\x00f6\ " 0 1 (charset iso-8859-1))
>            (current-buffer)))
>
> when executed in the *scratch* buffer inserts
> #("\x00f6\ " 0 1 (charset iso-8859-1))
>
> I would expect "\x00f6\ " without the #(...) as
> print-charset-text-property is bound to nil.
>
>
> In GNU Emacs 26.0.50 (build 10, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
>  of 2017-08-15 built on caladan

Worksforme.  Have you tried a current version?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#31376: 26.0.50; print-charset-text-property not honored

[Helmut Eller]

On Mon, May 07 2018, Andreas Schwab wrote:

>> In GNU Emacs 26.0.50 (build 10, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
>>  of 2017-08-15 built on caladan
>
> Worksforme.  Have you tried a current version?

Same (wrong) result with the current version.  Maybe it's an issue with
environment settings?

Helmut

In GNU Emacs 27.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2018-05-07 built on caladan
Repository revision: 6e362a32bc9d21f73a0f29ca6f45481edeea6f29
Windowing system distributor 'The X.Org Foundation', version 11.0.11902000
System Description: Debian GNU/Linux 9 (stretch)

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --with-xpm=no --with-gif=no --with-tiff=no --with-jpeg=no
 --without-pop'

Configured features:
PNG SOUND DBUS GSETTINGS NOTIFY GNUTLS LIBXML2 FREETYPE XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 THREADS LIBSYSTEMD

Important settings:
  value of $LANG: C.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

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

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny seq byte-opt gv
bytecomp byte-compile cconv dired dired-loaddefs format-spec rfc822 mml
easymenu mml-sec password-cache epa derived epg epg-config gnus-util
rmail rmail-loaddefs mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader cl-loaddefs cl-lib sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils time-date elec-pair
mule-util tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/x-win x-win term/common-win x-dnd tool-bar
dnd fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core term/tty-colors frame cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray minibuffer
cl-preloaded 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 dbusbind inotify
dynamic-setting system-font-setting font-render-setting move-toolbar gtk
x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 96485 9179)
 (symbols 48 20209 1)
 (miscs 40 37 70)
 (strings 32 29094 2061)
 (string-bytes 1 762890)
 (vectors 16 14810)
 (vector-slots 8 501612 12200)
 (floats 8 48 69)
 (intervals 56 204 0)
 (buffers 992 11))

bug#31376: 26.0.50; print-charset-text-property not honored

[Noam Postavsky]

Helmut Eller <eller.helmut@gmail.com> writes:

> On Mon, May 07 2018, Andreas Schwab wrote:
>
>>> In GNU Emacs 26.0.50 (build 10, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
>>>  of 2017-08-15 built on caladan
>>
>> Worksforme.  Have you tried a current version?
>
> Same (wrong) result with the current version.  Maybe it's an issue with
> environment settings?

I can reproduce this.  I see something about
PRINT_STRING_UNSAFE_CHARSET_FOUND in print.c, so I guess iso-8859-1 is
considered "unsafe"?

bug#31376: 26.0.50; print-charset-text-property not honored

[Helmut Eller]

On Mon, May 07 2018, Noam Postavsky wrote:

> Helmut Eller <eller.helmut@gmail.com> writes:
>
>> On Mon, May 07 2018, Andreas Schwab wrote:
>>
>>>> In GNU Emacs 26.0.50 (build 10, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
>>>>  of 2017-08-15 built on caladan
>>>
>>> Worksforme.  Have you tried a current version?
>>
>> Same (wrong) result with the current version.  Maybe it's an issue with
>> environment settings?
>
> I can reproduce this.  I see something about
> PRINT_STRING_UNSAFE_CHARSET_FOUND in print.c, so I guess iso-8859-1 is
> considered "unsafe"?

Apparently which charsets are unsafe depends on the current locale.

If I start Emacs with LANG=it_IT.UTF-8 emacs -Q, then the charset
property is not printed.  Even though that locale is not installed and
this warning is printed at startup:

(process:23336): Gtk-WARNING **: Locale not supported by C library.
        Using the fallback 'C' locale.

With LANG=en_US.UTF-8 emacs -Q the problem is the same as for
LANG=C.UTF-8.

Maybe this dependency on the current locale is useful, but the docstring
of print-charset-text-property should probably mention it.

Helmut

bug#31376: 26.0.50; print-charset-text-property not honored

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Date: Mon, 07 May 2018 20:08:19 +0200
> Cc: 31376@debbugs.gnu.org, Andreas Schwab <schwab@linux-m68k.org>
> 
> On Mon, May 07 2018, Noam Postavsky wrote:
> 
> > Helmut Eller <eller.helmut@gmail.com> writes:
> >
> >> On Mon, May 07 2018, Andreas Schwab wrote:
> >>
> >>>> In GNU Emacs 26.0.50 (build 10, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
> >>>>  of 2017-08-15 built on caladan
> >>>
> >>> Worksforme.  Have you tried a current version?
> >>
> >> Same (wrong) result with the current version.  Maybe it's an issue with
> >> environment settings?
> >
> > I can reproduce this.  I see something about
> > PRINT_STRING_UNSAFE_CHARSET_FOUND in print.c, so I guess iso-8859-1 is
> > considered "unsafe"?
> 
> Apparently which charsets are unsafe depends on the current locale.
> 
> If I start Emacs with LANG=it_IT.UTF-8 emacs -Q, then the charset
> property is not printed.  Even though that locale is not installed and
> this warning is printed at startup:
> 
> (process:23336): Gtk-WARNING **: Locale not supported by C library.
>         Using the fallback 'C' locale.
> 
> With LANG=en_US.UTF-8 emacs -Q the problem is the same as for
> LANG=C.UTF-8.

Can someone see any difference between the value t and 'default', wrt
when/how the 'charset' property of strings is printed?

I think the behavior under the value of nil is actually intended for
'default', and the value of nil is not implemented.  So I think we
should have a change in print_check_string_charset_prop that sets the
PRINT_STRING_UNSAFE_CHARSET_FOUND flag whenever it finds _any_
'charset' property on the string.

Comments?

bug#31376: 26.0.50; print-charset-text-property not honored

[Noam Postavsky]

On 11 May 2018 at 09:31, Eli Zaretskii <eliz@gnu.org> wrote:

> Can someone see any difference between the value t and 'default', wrt
                                                   ^
                                                   nil
> when/how the 'charset' property of strings is printed?

Assuming you meant nil, then no, I can't see any difference. As far as
I can tell, setting to 'default' just makes Emacs do pointless extra
checking for "unsafe" charsets even after it has found one.

> I think the behavior under the value of nil is actually intended for
> 'default', and the value of nil is not implemented.  So I think we
> should have a change in print_check_string_charset_prop that sets the
> PRINT_STRING_UNSAFE_CHARSET_FOUND flag whenever it finds _any_
> 'charset' property on the string.

It looks like the NILP (Vprint_charset_text_property) check is just in
the wrong place, if I move it to print_prune_string_charset then it
seems to work as documented.

--- i/src/print.c
+++ w/src/print.c
@@ -1317,8 +1317,7 @@ print_check_string_charset_prop (INTERVAL interval,
          || CONSP (XCDR (XCDR (val))))
        print_check_string_result |= PRINT_STRING_NON_CHARSET_FOUND;
     }
-  if (NILP (Vprint_charset_text_property)
-      || ! (print_check_string_result & PRINT_STRING_UNSAFE_CHARSET_FOUND))
+  if (! (print_check_string_result & PRINT_STRING_UNSAFE_CHARSET_FOUND))
     {
       int i, c;
       ptrdiff_t charpos = interval->position;
@@ -1348,7 +1347,8 @@ print_prune_string_charset (Lisp_Object string)
   print_check_string_result = 0;
   traverse_intervals (string_intervals (string), 0,
                      print_check_string_charset_prop, string);
-  if (! (print_check_string_result & PRINT_STRING_UNSAFE_CHARSET_FOUND))
+  if (NILP (Vprint_charset_text_property)
+      || ! (print_check_string_result & PRINT_STRING_UNSAFE_CHARSET_FOUND))
     {
       string = Fcopy_sequence (string);
       if (print_check_string_result & PRINT_STRING_NON_CHARSET_FOUND)

bug#31376: 26.0.50; print-charset-text-property not honored

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Fri, 11 May 2018 13:44:46 -0400
> Cc: Helmut Eller <eller.helmut@gmail.com>, Kenichi Handa <handa@gnu.org>, 31376@debbugs.gnu.org, 
> 	Andreas Schwab <schwab@linux-m68k.org>
> 
> On 11 May 2018 at 09:31, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > Can someone see any difference between the value t and 'default', wrt
>                                                    ^
>                                                    nil
> > when/how the 'charset' property of strings is printed?
> 
> Assuming you meant nil, then no, I can't see any difference.

Mmm... yes, nil.

> As far as
> I can tell, setting to 'default' just makes Emacs do pointless extra
> checking for "unsafe" charsets even after it has found one.

You mean, setting it to nil makes it do pointless extra work, yes? ;-)

> It looks like the NILP (Vprint_charset_text_property) check is just in
> the wrong place, if I move it to print_prune_string_charset then it
> seems to work as documented.
> 
> --- i/src/print.c
> +++ w/src/print.c
> @@ -1317,8 +1317,7 @@ print_check_string_charset_prop (INTERVAL interval,
>           || CONSP (XCDR (XCDR (val))))
>         print_check_string_result |= PRINT_STRING_NON_CHARSET_FOUND;
>      }
> -  if (NILP (Vprint_charset_text_property)
> -      || ! (print_check_string_result & PRINT_STRING_UNSAFE_CHARSET_FOUND))
> +  if (! (print_check_string_result & PRINT_STRING_UNSAFE_CHARSET_FOUND))
>      {
>        int i, c;
>        ptrdiff_t charpos = interval->position;
> @@ -1348,7 +1347,8 @@ print_prune_string_charset (Lisp_Object string)
>    print_check_string_result = 0;
>    traverse_intervals (string_intervals (string), 0,
>                       print_check_string_charset_prop, string);
> -  if (! (print_check_string_result & PRINT_STRING_UNSAFE_CHARSET_FOUND))
> +  if (NILP (Vprint_charset_text_property)
> +      || ! (print_check_string_result & PRINT_STRING_UNSAFE_CHARSET_FOUND))

Yes, that sounds right, thanks.  We should also mention in the doc
string that any non-nil, non-t value is treated as 'default'.

And this variable should be mentioned in the ELisp manual, in the node
"Output Variables".

bug#31376: 26.0.50; print-charset-text-property not honored

[Noam Postavsky]

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

tags 31376 + patch
quit

Eli Zaretskii <eliz@gnu.org> writes:

>> As far as I can tell, setting to 'default' just makes Emacs do
>> pointless extra checking for "unsafe" charsets even after it has
>> found one.
>
> You mean, setting it to nil makes it do pointless extra work, yes? ;-)

Hah!  I guess I really can't tell the difference :)

> Yes, that sounds right, thanks.  We should also mention in the doc
> string that any non-nil, non-t value is treated as 'default'.

> And this variable should be mentioned in the ELisp manual, in the node
> "Output Variables".

Okay, how about this:


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

From eed29c8e7164cbc13df4d7b4e3974ae90d9ecb51 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Fri, 11 May 2018 13:44:46 -0400
Subject: [PATCH] Honor print-charset-text-property value of nil (Bug#31376)

* src/print.c (print_check_string_charset_prop): Move check
for nil Vprint_charset_text_property from here...
(print_prune_string_charset): ... to here.
(syms_of_print) <print-charset-text-property>: Clarify that any
non-boolean values are treated the same as `default'.
* doc/lispref/streams.texi (Output Variables): Add
print-prune-string-charset.
---
 doc/lispref/streams.texi | 13 +++++++++++++
 src/print.c              | 11 ++++++-----
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/doc/lispref/streams.texi b/doc/lispref/streams.texi
index ebd806601e..ae3b982080 100644
--- a/doc/lispref/streams.texi
+++ b/doc/lispref/streams.texi
@@ -809,6 +809,19 @@ Output Variables
 one.
 @end defvar
 
+@defvar print-charset-text-property
+This variable controls printing of `charset' text property on printing
+a string.  The value should be @code{nil}, @code{t}, or
+@code{default}.
+
+If the value is @code{nil}, @code{charset} text properties are never
+printed.  If @code{t}, they are always printed.  If the value is
+@code{default}, print the text property @code{charset} only when the
+value is different from what is guessed in the current charset
+priorities.  Values other than @code{nil} or @code{t} are treated the
+same as @code{default}.
+@end defvar
+
 @defvar print-length
 @cindex printing limits
 The value of this variable is the maximum number of elements to print in
diff --git a/src/print.c b/src/print.c
index 15177759cf..427ab7d158 100644
--- a/src/print.c
+++ b/src/print.c
@@ -1317,8 +1317,7 @@ print_check_string_charset_prop (INTERVAL interval, Lisp_Object string)
 	  || CONSP (XCDR (XCDR (val))))
 	print_check_string_result |= PRINT_STRING_NON_CHARSET_FOUND;
     }
-  if (NILP (Vprint_charset_text_property)
-      || ! (print_check_string_result & PRINT_STRING_UNSAFE_CHARSET_FOUND))
+  if (! (print_check_string_result & PRINT_STRING_UNSAFE_CHARSET_FOUND))
     {
       int i, c;
       ptrdiff_t charpos = interval->position;
@@ -1348,7 +1347,8 @@ print_prune_string_charset (Lisp_Object string)
   print_check_string_result = 0;
   traverse_intervals (string_intervals (string), 0,
 		      print_check_string_charset_prop, string);
-  if (! (print_check_string_result & PRINT_STRING_UNSAFE_CHARSET_FOUND))
+  if (NILP (Vprint_charset_text_property)
+      || ! (print_check_string_result & PRINT_STRING_UNSAFE_CHARSET_FOUND))
     {
       string = Fcopy_sequence (string);
       if (print_check_string_result & PRINT_STRING_NON_CHARSET_FOUND)
@@ -2422,7 +2422,7 @@ representation) and `#N#' in place of each subsequent occurrence,
 
   DEFVAR_LISP ("print-charset-text-property", Vprint_charset_text_property,
 	       doc: /* A flag to control printing of `charset' text property on printing a string.
-The value must be nil, t, or `default'.
+The value should be nil, t, or `default'.
 
 If the value is nil, don't print the text property `charset'.
 
@@ -2430,7 +2430,8 @@ representation) and `#N#' in place of each subsequent occurrence,
 
 If the value is `default', print the text property `charset' only when
 the value is different from what is guessed in the current charset
-priorities.  */);
+priorities.  Values other than nil or t are also treated as
+`default'.  */);
   Vprint_charset_text_property = Qdefault;
 
   /* prin1_to_string_buffer initialized in init_buffer_once in buffer.c */
-- 
2.11.0

bug#31376: 26.0.50; print-charset-text-property not honored

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: handa@gnu.org,  schwab@linux-m68k.org,  31376@debbugs.gnu.org,  eller.helmut@gmail.com
> Date: Sat, 12 May 2018 16:02:01 -0400
> 
> Okay, how about this:
> 
> >From eed29c8e7164cbc13df4d7b4e3974ae90d9ecb51 Mon Sep 17 00:00:00 2001
> From: Noam Postavsky <npostavs@gmail.com>
> Date: Fri, 11 May 2018 13:44:46 -0400
> Subject: [PATCH] Honor print-charset-text-property value of nil (Bug#31376)
> 
> * src/print.c (print_check_string_charset_prop): Move check
> for nil Vprint_charset_text_property from here...
> (print_prune_string_charset): ... to here.
> (syms_of_print) <print-charset-text-property>: Clarify that any
> non-boolean values are treated the same as `default'.
> * doc/lispref/streams.texi (Output Variables): Add
> print-prune-string-charset.

Could we also have a couple of tests, including the original snippet
from Helmut that started this?

> +@defvar print-charset-text-property
> +This variable controls printing of `charset' text property on printing
> +a string.  The value should be @code{nil}, @code{t}, or
> +@code{default}.
> +
> +If the value is @code{nil}, @code{charset} text properties are never
> +printed.  If @code{t}, they are always printed.  If the value is
> +@code{default}, print the text property @code{charset} only when the
> +value is different from what is guessed in the current charset
> +priorities.  Values other than @code{nil} or @code{t} are treated the
> +same as @code{default}.
> +@end defvar

This LGTM, but the description of 'default' IMO is too
implementation-centric, which doesn't help the user in understanding
what to expect from that value.  How about the variant below?

  If the value is @code{default}, print only those @code{charset} text
  properties that are ``unusual'' for the respective characters under
  the current language environment.  The @code{charset} property of a
  character is considered unusual if its value is different from what
  @code{char-charset} returns for that character.

Thanks.

bug#31376: 26.0.50; print-charset-text-property not honored

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

> This LGTM, but the description of 'default' IMO is too
> implementation-centric, which doesn't help the user in understanding
> what to expect from that value.  How about the variant below?

Yeah, I only understand it from the implementation myself.

>   If the value is @code{default}, print only those @code{charset} text
>   properties that are ``unusual'' for the respective characters under
>   the current language environment.  The @code{charset} property of a
>   character is considered unusual if its value is different from what
>   @code{char-charset} returns for that character.

Okay, when writing up some tests I may have found another minor bug, or
at least something which contradicts the text above.  Is there supposed
to be an exception for ascii characters?

    (char-charset ?a) ;=> ascii
    (prin1-to-string
     (propertize (string ?a) 'charset 'chinese-cns11643-15))
    ;=> "\"a\""

vs

    (char-charset ?\xf6) ;=> unicode
    (prin1-to-string
     (propertize (string ?\xf6) 'charset 'chinese-cns11643-15))
    ;=> "#(\"ö\" 0 1 (charset chinese-cns11643-15))"

(Nothing special about chinese-cns11643-15, by the way, it's just the
first element in charset-list.)

bug#31376: 26.0.50; print-charset-text-property not honored

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: handa@gnu.org,  31376@debbugs.gnu.org,  schwab@linux-m68k.org,  eller.helmut@gmail.com
> Date: Sun, 13 May 2018 14:29:58 -0400
> 
> >   If the value is @code{default}, print only those @code{charset} text
> >   properties that are ``unusual'' for the respective characters under
> >   the current language environment.  The @code{charset} property of a
> >   character is considered unusual if its value is different from what
> >   @code{char-charset} returns for that character.
> 
> Okay, when writing up some tests I may have found another minor bug, or
> at least something which contradicts the text above.  Is there supposed
> to be an exception for ascii characters?

Yes, you can see that in print_check_string_charset_prop:

      charset = XCAR (XCDR (val));
      for (i = 0; i < LENGTH (interval); i++)
	{
	  FETCH_STRING_CHAR_ADVANCE (c, string, charpos, bytepos);
	  if (! ASCII_CHAR_P (c)  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
	      && ! EQ (CHARSET_NAME (CHAR_CHARSET (c)), charset))
	    {
	      print_check_string_result |= PRINT_STRING_UNSAFE_CHARSET_FOUND;
	      break;
	    }
	}

CHAR_CHARSET always returns 'ascii' for ASCII characters, while
showing 'charset' for ASCII characters probably makes littles sense.
Of course, that's only my guess, the default behavior wrt this is pure
heuristics, so YMMV.

We could mention this exception in the manual.

bug#31376: 26.0.50; print-charset-text-property not honored

[Noam Postavsky]

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

Eli Zaretskii <eliz@gnu.org> writes:

> CHAR_CHARSET always returns 'ascii' for ASCII characters, while
> showing 'charset' for ASCII characters probably makes littles sense.
> Of course, that's only my guess, the default behavior wrt this is pure
> heuristics, so YMMV.
>
> We could mention this exception in the manual.

Yeah, I think we should.  Here's a new patch with updated manual and
tests.


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

From b2ff6ca64fbde643867f4ddada1d8939c7e1ba1f Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Fri, 11 May 2018 13:44:46 -0400
Subject: [PATCH v2] Honor print-charset-text-property value of nil (Bug#31376)

* src/print.c (print_check_string_charset_prop): Move check
for nil Vprint_charset_text_property from here...
(print_prune_string_charset): ... to here.
(syms_of_print) <print-charset-text-property>: Clarify that any
non-boolean values are treated the same as `default'.
* doc/lispref/streams.texi (Output Variables): Add
print-prune-string-charset.
* test/src/print-tests.el (print-charset-text-property-nil)
(print-charset-text-property-default)
(print-charset-text-property-t): New tests.
(print-tests--prints-with-charset-p): New helper function.
---
 doc/lispref/streams.texi | 15 +++++++++++++++
 src/print.c              | 11 ++++++-----
 test/src/print-tests.el  | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/doc/lispref/streams.texi b/doc/lispref/streams.texi
index ebd806601e..032669cb10 100644
--- a/doc/lispref/streams.texi
+++ b/doc/lispref/streams.texi
@@ -809,6 +809,21 @@ Output Variables
 one.
 @end defvar
 
+@defvar print-charset-text-property
+This variable controls printing of `charset' text property on printing
+a string.  The value should be @code{nil}, @code{t}, or
+@code{default}.
+
+If the value is @code{nil}, @code{charset} text properties are never
+printed.  If @code{t}, they are always printed.
+
+If the value is @code{default}, only print @code{charset} text
+properties if there is an ``unexpected'' @code{charset} property.  For
+ascii characters, all charsets are considered ``expected''.
+Otherwise, the expected @code{charset} property of a character is
+given by @code{char-charset}.
+@end defvar
+
 @defvar print-length
 @cindex printing limits
 The value of this variable is the maximum number of elements to print in
diff --git a/src/print.c b/src/print.c
index 15177759cf..427ab7d158 100644
--- a/src/print.c
+++ b/src/print.c
@@ -1317,8 +1317,7 @@ print_check_string_charset_prop (INTERVAL interval, Lisp_Object string)
 	  || CONSP (XCDR (XCDR (val))))
 	print_check_string_result |= PRINT_STRING_NON_CHARSET_FOUND;
     }
-  if (NILP (Vprint_charset_text_property)
-      || ! (print_check_string_result & PRINT_STRING_UNSAFE_CHARSET_FOUND))
+  if (! (print_check_string_result & PRINT_STRING_UNSAFE_CHARSET_FOUND))
     {
       int i, c;
       ptrdiff_t charpos = interval->position;
@@ -1348,7 +1347,8 @@ print_prune_string_charset (Lisp_Object string)
   print_check_string_result = 0;
   traverse_intervals (string_intervals (string), 0,
 		      print_check_string_charset_prop, string);
-  if (! (print_check_string_result & PRINT_STRING_UNSAFE_CHARSET_FOUND))
+  if (NILP (Vprint_charset_text_property)
+      || ! (print_check_string_result & PRINT_STRING_UNSAFE_CHARSET_FOUND))
     {
       string = Fcopy_sequence (string);
       if (print_check_string_result & PRINT_STRING_NON_CHARSET_FOUND)
@@ -2422,7 +2422,7 @@ representation) and `#N#' in place of each subsequent occurrence,
 
   DEFVAR_LISP ("print-charset-text-property", Vprint_charset_text_property,
 	       doc: /* A flag to control printing of `charset' text property on printing a string.
-The value must be nil, t, or `default'.
+The value should be nil, t, or `default'.
 
 If the value is nil, don't print the text property `charset'.
 
@@ -2430,7 +2430,8 @@ representation) and `#N#' in place of each subsequent occurrence,
 
 If the value is `default', print the text property `charset' only when
 the value is different from what is guessed in the current charset
-priorities.  */);
+priorities.  Values other than nil or t are also treated as
+`default'.  */);
   Vprint_charset_text_property = Qdefault;
 
   /* prin1_to_string_buffer initialized in init_buffer_once in buffer.c */
diff --git a/test/src/print-tests.el b/test/src/print-tests.el
index 01e65028bc..a1825b79d0 100644
--- a/test/src/print-tests.el
+++ b/test/src/print-tests.el
@@ -27,6 +27,44 @@
                      (prin1-to-string "\u00A2\ff"))
                    "\"\\x00a2\\ff\"")))
 
+(defun print-tests--prints-with-charset-p (ch odd-charset)
+  "Return t if `prin1-to-string' prints CH with the `charset' property.
+CH is propertized with a `charset' value according to
+ODD-CHARSET: if nil, then use the one returned by `char-charset',
+otherwise, use a different charset."
+  (integerp
+   (string-match
+    "charset"
+    (prin1-to-string
+     (propertize (string ch)
+                 'charset
+                 (if odd-charset
+                     (car (if (eq (char-charset ch) (car charset-list))
+                              charset-list
+                            (cdr charset-list)))
+                   (char-charset ch)))))))
+
+(ert-deftest print-charset-text-property-nil ()
+  (let ((print-charset-text-property nil))
+    (should-not (print-tests--prints-with-charset-p ?\xf6 t)) ; Bug#31376.
+    (should-not (print-tests--prints-with-charset-p ?a t))
+    (should-not (print-tests--prints-with-charset-p ?\xf6 nil))
+    (should-not (print-tests--prints-with-charset-p ?a nil))))
+
+(ert-deftest print-charset-text-property-default ()
+  (let ((print-charset-text-property 'default))
+    (should (print-tests--prints-with-charset-p ?\xf6 t))
+    (should-not (print-tests--prints-with-charset-p ?a t))
+    (should-not (print-tests--prints-with-charset-p ?\xf6 nil))
+    (should-not (print-tests--prints-with-charset-p ?a nil))))
+
+(ert-deftest print-charset-text-property-t ()
+  (let ((print-charset-text-property t))
+    (should (print-tests--prints-with-charset-p ?\xf6 t))
+    (should (print-tests--prints-with-charset-p ?a t))
+    (should (print-tests--prints-with-charset-p ?\xf6 nil))
+    (should (print-tests--prints-with-charset-p ?a nil))))
+
 (ert-deftest terpri ()
   (should (string= (with-output-to-string
                      (princ 'abc)
-- 
2.11.0

bug#31376: 26.0.50; print-charset-text-property not honored

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: handa@gnu.org,  31376@debbugs.gnu.org,  schwab@linux-m68k.org,  eller.helmut@gmail.com
> Date: Sun, 13 May 2018 15:42:02 -0400
> 
> > We could mention this exception in the manual.
> 
> Yeah, I think we should.  Here's a new patch with updated manual and
> tests.

Thanks.

One minor comment about the tests:

> +(defun print-tests--prints-with-charset-p (ch odd-charset)
> +  "Return t if `prin1-to-string' prints CH with the `charset' property.
> +CH is propertized with a `charset' value according to
> +ODD-CHARSET: if nil, then use the one returned by `char-charset',
> +otherwise, use a different charset."
> +  (integerp
> +   (string-match
> +    "charset"
> +    (prin1-to-string
> +     (propertize (string ch)
> +                 'charset
> +                 (if odd-charset
> +                     (car (if (eq (char-charset ch) (car charset-list))
> +                              charset-list
> +                            (cdr charset-list)))
> +                   (char-charset ch)))))))

This seems to rely on some relationship between what char-charset
returns and the car of charset-list, is that right?  AFAIK, when
ODD-CHARSET is non-nil, you want to propertize with a charset that is
_different_ from what char-charset returns, but taking the first or
the second member of charset-list doesn't guarantee that.  It could
work with the particular characters you used for the tests, but if
this function is limited to such characters, we should document that
in the doc string or comments, lest someone will try to expand the
tests and will see strange failures.

Thanks, I have no other comments.

bug#31376: 26.0.50; print-charset-text-property not honored

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

> This seems to rely on some relationship between what char-charset
> returns and the car of charset-list, is that right?

The idea is to take the first if it's not eq to what char-charset
returns, or the second otherwise (on the assumption that it's different
from the first).  Perhaps clearer if I use cl-find:

    (defun print-tests--prints-with-charset-p (ch odd-charset)
      "Return t if `prin1-to-string' prints CH with the `charset' property.
    CH is propertized with a `charset' value according to
    ODD-CHARSET: if nil, then use the one returned by `char-charset',
    otherwise, use a different charset."
      (integerp
       (string-match
        "charset"
        (prin1-to-string
         (propertize (string ch)
                     'charset
                     (if odd-charset
                         (cl-find (char-charset ch) charset-list :test-not #'eq)
                       (char-charset ch)))))))

bug#31376: 26.0.50; print-charset-text-property not honored

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: handa@gnu.org,  31376@debbugs.gnu.org,  schwab@linux-m68k.org,  eller.helmut@gmail.com
> Date: Mon, 14 May 2018 19:15:30 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > This seems to rely on some relationship between what char-charset
> > returns and the car of charset-list, is that right?
> 
> The idea is to take the first if it's not eq to what char-charset
> returns, or the second otherwise (on the assumption that it's different
> from the first).  Perhaps clearer if I use cl-find:
> 
>     (defun print-tests--prints-with-charset-p (ch odd-charset)
>       "Return t if `prin1-to-string' prints CH with the `charset' property.
>     CH is propertized with a `charset' value according to
>     ODD-CHARSET: if nil, then use the one returned by `char-charset',
>     otherwise, use a different charset."
>       (integerp
>        (string-match
>         "charset"
>         (prin1-to-string
>          (propertize (string ch)
>                      'charset
>                      (if odd-charset
>                          (cl-find (char-charset ch) charset-list :test-not #'eq)
>                        (char-charset ch)))))))

Maybe I'm missing something, but the negation of the test seemed to be
missing from the original code.  But it was a hard day, so maybe my
mind is foggy...

Thanks.

bug#31376: 26.0.50; print-charset-text-property not honored

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

>> The idea is to take the first if it's not eq to what char-charset
>> returns, or the second otherwise (on the assumption that it's different
>> from the first).  Perhaps clearer if I use cl-find:
>> 
> Maybe I'm missing something, but the negation of the test seemed to be
> missing from the original code.  But it was a hard day, so maybe my
> mind is foggy...

Oh, you're right, it was missing.  I didn't notice because both the
first two elements of charset-list were different from (char-charset
?\0xf6) which let the test pass regardless.

bug#31376: 26.0.50; print-charset-text-property not honored

[Noam Postavsky]

tags 31376 fixed
close 31376 27.1
quit

Noam Postavsky <npostavs@gmail.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> The idea is to take the first if it's not eq to what char-charset
>>> returns, or the second otherwise (on the assumption that it's different
>>> from the first).  Perhaps clearer if I use cl-find:
>>> 
>> Maybe I'm missing something, but the negation of the test seemed to be
>> missing from the original code.  But it was a hard day, so maybe my
>> mind is foggy...
>
> Oh, you're right, it was missing.  I didn't notice because both the
> first two elements of charset-list were different from (char-charset
> ?\0xf6) which let the test pass regardless.

Pushed the one with cl-find to master.

[1: 6f037f427a]: 2018-05-23 07:53:58 -0400
  Honor print-charset-text-property value of nil (Bug#31376)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=6f037f427a25160168e842bff0d12b816d69067d

bug#31376: 26.0.50; print-charset-text-property not honored

[Drew Adams]

> We should also mention in the doc
> string that any non-nil, non-t value is treated as 'default'.

That doesn't sound great - not very conventional.
Instead of `t' meaning something special and other
non-nil meaning something non-special we usually
have a mnemonic symbol for the special case and
other non-nil for the non-special case. 

It's no doubt too late to change this, since the
mistake seems to have been made way back in Emacs 23.

The doc should even point out that this is maybe not
what you might expect.  IOW, stress that `t' is a
special case.

bug#31376: 26.0.50; print-charset-text-property not honored

[Eli Zaretskii]

> Date: Fri, 11 May 2018 12:58:11 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 31376@debbugs.gnu.org, schwab@linux-m68k.org, eller.helmut@gmail.com
> 
> > We should also mention in the doc
> > string that any non-nil, non-t value is treated as 'default'.
> 
> That doesn't sound great - not very conventional.
> Instead of `t' meaning something special and other
> non-nil meaning something non-special we usually
> have a mnemonic symbol for the special case and
> other non-nil for the non-special case. 

Usually, but not always.  We have examples of the other behavior as
well.