Re: enhanced select-safe-coding-system

Re: enhanced select-safe-coding-system

[Richard Stallman]

Your code was based on an older version did not have some other
changes that are in the sources now.  I merged it, and the changes
you made seem to be these.

Does anyone see a problem with these changes?
Handa, do they look correct to you?

*** mule-cmds.el.~1.190.~	Sat Apr 27 23:22:23 2002
--- mule-cmds.el	Wed May  1 01:33:40 2002
***************
*** 636,641 ****
--- 636,651 ----
  	      (setcar l mime-charset))
  	  (setq l (cdr l))))
  
+       ;; Don't offer variations with locking shift, which you
+       ;; basically never want.
+       (let (l)
+ 	(dolist (elt codings (setq codings (nreverse l)))
+ 	  (unless (or (eq 'coding-category-iso-7-else
+ 			  (coding-system-category elt))
+ 		      (eq 'coding-category-iso-8-else
+ 			  (coding-system-category elt)))
+ 	    (push elt l))))
+ 
        ;; Make sure the offending buffer is displayed.
        (or (stringp from)
  	  (pop-to-buffer bufname))
***************
*** 705,711 ****
  
      (if (eq coding-system t)
  	(setq coding-system buffer-file-coding-system))
!     coding-system))
  
  (setq select-safe-coding-system-function 'select-safe-coding-system)
  
--- 715,737 ----
  
      (if (eq coding-system t)
  	(setq coding-system buffer-file-coding-system))
!     ;; Check we're not inconsistent with what coding cookies &c would
!     ;; give when file is re-read.
!     (unless (stringp from)
!       (let ((auto-cs (save-restriction
! 		       (widen)
! 		       (save-excursion
! 			 (goto-char (point-min))
! 			 (set-auto-coding (or buffer-file-name "")
! 					  (buffer-size))))))
! 	(if (and auto-cs
! 		 (not (coding-system-equal (coding-system-base coding-system)
! 					   (coding-system-base auto-cs))))
! 	    (unless (yes-or-no-p
! 		     (format "Selected encoding %s disagrees with \
! %s specified by file contents.  Really save (else edit coding cookies \
! and try again)? " coding-system auto-cs))
! 	      (error "Save aborted")))))    coding-system))
  
  (setq select-safe-coding-system-function 'select-safe-coding-system)

Re: enhanced select-safe-coding-system

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Date: Wed, 1 May 2002 01:14:52 -0600 (MDT)
> 
> Does anyone see a problem with these changes?

Shouldn't we disallow saving a file whose coding: says something
different from the actual encoding?  Is there any situation when such
a file means anything but trouble when it is visited?

Re: enhanced select-safe-coding-system

[Stefan Monnier]

> > From: Richard Stallman <rms@gnu.org>
> > Date: Wed, 1 May 2002 01:14:52 -0600 (MDT)
> > 
> > Does anyone see a problem with these changes?
> 
> Shouldn't we disallow saving a file whose coding: says something
> different from the actual encoding?  Is there any situation when such
> a file means anything but trouble when it is visited?

I'm not sure what to say.
On the one hand I agree, but on the other I have found myself annoyed
at select-safe-coding-system's insistence that I select a coding-system
that it deemed safe (whereas I wanted another coding-system which I knew
was safe even though Emacs disagreed).

So I think that select-safe-coding-system should be changed to allow
the user to select an unsafe coding-system and that it should
also be possible to save with a different coding-system than the
one specified in the `coding:' tag.
But in both cases, it should only allow it after the user has gone
through a fair bit of warnings and questions (and/or something
like (put 'eval-expression 'disabled nil)).


	Stefan

Re: enhanced select-safe-coding-system

[Richard Stallman]

    > Does anyone see a problem with these changes?

    Shouldn't we disallow saving a file whose coding: says something
    different from the actual encoding?

I don't think so, but in any case that's a different issue.  Whatever
we think about that question, that is NOT a problem "with these
changes."  It is about something that these changes do not alter.

I have not seen any problem *with these changes*, so I think
they should be installed.  I will install them today.

Re: enhanced select-safe-coding-system

[Dave Love]

"Eli Zaretskii" <eliz@is.elta.co.il> writes:

> Shouldn't we disallow saving a file whose coding: says something
> different from the actual encoding?  Is there any situation when such
> a file means anything but trouble when it is visited?

Yes, but anyhow it surely can't be right absolutely to prevent users
saving files.

`coding:' tags aren't the only thing `set-auto-coding' can note.

Re: enhanced select-safe-coding-system

[Eli Zaretskii]

> From: Dave Love <d.love@dl.ac.uk>
> Date: 02 May 2002 23:41:57 +0100
> 
> "Eli Zaretskii" <eliz@is.elta.co.il> writes:
> 
> > Shouldn't we disallow saving a file whose coding: says something
> > different from the actual encoding?  Is there any situation when such
> > a file means anything but trouble when it is visited?
> 
> Yes, but anyhow it surely can't be right absolutely to prevent users
> saving files.

That's true, there's a dilemma here.  I believe Stefan was saying
something similar.

Perhaps we should think about rewriting `coding:', or maybe removing
it, after asking for confirmation.

> `coding:' tags aren't the only thing `set-auto-coding' can note.

Sorry, I'm afraid I don't follow; could you please elaborate?

Re: enhanced select-safe-coding-system

[Dave Love]

"Eli Zaretskii" <eliz@is.elta.co.il> writes:

> Perhaps we should think about rewriting `coding:', or maybe removing
> it, after asking for confirmation.

Sigh.  Apart from the fact that there may not be a `coding:' tag,
please don't take fascist attitudes to people's data.

> > `coding:' tags aren't the only thing `set-auto-coding' can note.
> 
> Sorry, I'm afraid I don't follow; could you please elaborate?

See the documentation and the code.  I hope the facilities haven't
been removed now, though I think they could be better.

Re: enhanced select-safe-coding-system

[Dave Love]

Richard Stallman <rms@gnu.org> writes:

> Your code was based on an older version

Yes, probably over a year old.

> did not have some other changes that are in the sources now.  I
> merged it, and the changes you made seem to be these.

They look like the right sort of changes, but you want to break this
line so that it's obvious that `coding-system' is being returned.

> ! 	      (error "Save aborted")))))    coding-system))

Incidentally, I think Emacs should have `remove-if', or whatever
Common Lisp calls the function for filtering lists according to a
predicate, as I needed to filter the coding list.  That operation seems
quite common.

If you want to make select-safe-coding-system more helpful, you could
also use code like this to find at least the first unencodable
character and display that part of the buffer when
select-safe-coding-system needs to prompt for a coding system.  Emacs
20 used to do something similar, but it was only necessary to check
charsets then.

(defun unencodable-char-position (start end coding-system)
  "Return position of first un-encodable character in a region.
START and END specfiy the region and CODING-SYSTEM specifies the
encoding to check.  Return nil if CODING-SYSTEM does encode the region.

CODING-SYSTEM may also be a list of coding systems, in which case return
the first position not encodable by any of them.

This function is fairly slow."
  ;; Use recursive calls in the binary chop below, since we're
  ;; O(logN), and the call overhead shouldn't be a bottleneck.
  (unless enable-multibyte-characters
    (error "unibyte buffer"))
  ;; Recurse if list of coding systems.
  (if (consp coding-system)
      (let ((end end) res)
	(dolist (elt coding-system (and res (>= res 0) res))
	  (let ((pos (unencodable-char-position start end elt)))
	    (if pos
		(setq end pos
		      res pos)))))
    ;; Skip ASCII initially.
    (save-excursion
      (skip-chars-forward "\000-\177" end)
      (setq start (point)))
    (unless (= start end)
      (setq coding-system (coding-system-base coding-system)) ; canonicalize
      (let ((codings (find-coding-systems-region start end)))
	(unless (or (equal codings '(undecided))
		    (memq coding-system
			  (find-coding-systems-region start end)))
	  ;; Binary chop.
	  (if (= start (1- end))
	      start
	    (or (unencodable-char-position start (/ (+ start end) 2)
					   coding-system)
		(unencodable-char-position (/ (+ start end) 2) end
					   coding-system))))))))

Re: enhanced select-safe-coding-system

[Stefan Monnier]

> Incidentally, I think Emacs should have `remove-if', or whatever
> Common Lisp calls the function for filtering lists according to a
> predicate, as I needed to filter the coding list.  That operation seems
> quite common.

I would agree (I actually use a `partition' function sometimes which
returns two lists: one for elements that matched the predicate and one
for those that didn't, but a CL-style function is just as good).

> If you want to make select-safe-coding-system more helpful, you could
> also use code like this to find at least the first unencodable
> character and display that part of the buffer when
> select-safe-coding-system needs to prompt for a coding system.  Emacs
> 20 used to do something similar, but it was only necessary to check
> charsets then.

I have recently posted a fairly simple patch that does that by slightly
modifying the C code, so it's fast.  The only question is if such
a functionality is good enough to efficiently implement Emacs-20's behavior
(which was to highlight the offending chars, rather than just jump to the
first one)


	Stefan

Re: enhanced select-safe-coding-system

[Richard Stallman]

    I have recently posted a fairly simple patch that does that by slightly
    modifying the C code, so it's fast.  The only question is if such
    a functionality is good enough to efficiently implement Emacs-20's behavior
    (which was to highlight the offending chars, rather than just jump to the
    first one)

Showing the first one is better than nothing, so how about if you
install your patch now?  Then people can look at modifying it to
report more than just the first one.

Re: enhanced select-safe-coding-system

[Stefan Monnier]

>     I have recently posted a fairly simple patch that does that by slightly
>     modifying the C code, so it's fast.  The only question is if such
>     a functionality is good enough to efficiently implement Emacs-20's behavior
>     (which was to highlight the offending chars, rather than just jump to the
>     first one)
> 
> Showing the first one is better than nothing, so how about if you
> install your patch now?  Then people can look at modifying it to
> report more than just the first one.

Will do.


	Stefan

Re: enhanced select-safe-coding-system

[Dave Love]

"Stefan Monnier" <monnier+gnu/emacs@RUM.cs.yale.edu> writes:

> I have recently posted a fairly simple patch that does that by slightly
> modifying the C code, so it's fast.

How is that?  I think the Lisp is actually fast enough in that
application on a P100.  I included the warning in case anyone used it
without checking.

Anyway, be careful.  As far as I can tell, find-coding-systems-region
should remain the basic operation for this sort of thing and you need
to beware of confusion and maintenance issues from providing
alternative functionality.  (Look at Gnus as an example of confusion
in this area.)

> The only question is if such
> a functionality is good enough to efficiently implement Emacs-20's behavior
> (which was to highlight the offending chars, rather than just jump to the
> first one)

I think what's most important is to be able to get information on the
first one (at point) more than to have them all highlighted.  That's
usually enough to figure out what the encoding problem is, not that
extra information would hurt, obviously.

Re: enhanced select-safe-coding-system

[Stefan Monnier]

> "Stefan Monnier" <monnier+gnu/emacs@RUM.cs.yale.edu> writes:
> 
> > I have recently posted a fairly simple patch that does that by slightly
> > modifying the C code, so it's fast.
> 
> How is that?  I think the Lisp is actually fast enough in that
> application on a P100.  I included the warning in case anyone used it
> without checking.

You might be right that the elisp code is fast enough, but my
patch (attached) makes fewer changes and avoids re-implementing
the same functionality in elisp.


	Stefan


Index: coding.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/coding.c,v
retrieving revision 1.241
diff -u -r1.241 coding.c
--- coding.c	13 Apr 2002 17:49:00 -0000	1.241
+++ coding.c	5 May 2002 23:23:51 -0000
@@ -6366,8 +6361,6 @@
 	continue;
       if (SINGLE_BYTE_CHAR_P (c))
 	*single_byte_char_found = 1;
-      if (NILP (safe_codings))
-	continue;
       /* Check the safe coding systems for C.  */
       val = char_table_ref_and_index (work_table, c, &idx);
       if (EQ (val, Qt))
@@ -6382,6 +6375,8 @@
       if (!EQ (safe_codings, Qt) && !NILP (val))
 	val = intersection (safe_codings, val);
       safe_codings = val;
+      if (NILP (safe_codings))
+	return make_number (pend - p);
     }
   return safe_codings;
 }
@@ -6393,20 +6388,24 @@
 
 DEFUN ("find-coding-systems-region-internal",
        Ffind_coding_systems_region_internal,
-       Sfind_coding_systems_region_internal, 2, 2, 0,
+       Sfind_coding_systems_region_internal, 2, 3, 0,
        doc: /* Internal use only.  */)
-     (start, end)
-     Lisp_Object start, end;
+     (start, end, safe_codings)
+     Lisp_Object start, end, safe_codings;
 {
-  Lisp_Object work_table, safe_codings;
+  Lisp_Object work_table;
   int non_ascii_p = 0;
   int single_byte_char_found = 0;
   unsigned char *p1, *p1end, *p2, *p2end, *p;
+  int from, to, stop;
+
+  if (NILP (safe_codings))
+    safe_codings = Qt;
 
   if (STRINGP (start))
     {
       if (!STRING_MULTIBYTE (start))
-	return Qt;
+	return safe_codings;
       p1 = XSTRING (start)->data, p1end = p1 + STRING_BYTES (XSTRING (start));
       p2 = p2end = p1end;
       if (XSTRING (start)->size != STRING_BYTES (XSTRING (start)))
@@ -6414,14 +6413,12 @@
     }
   else
     {
-      int from, to, stop;
-
       CHECK_NUMBER_COERCE_MARKER (start);
       CHECK_NUMBER_COERCE_MARKER (end);
       if (XINT (start) < BEG || XINT (end) > Z || XINT (start) > XINT (end))
 	args_out_of_range (start, end);
       if (NILP (current_buffer->enable_multibyte_characters))
-	return Qt;
+	return safe_codings;
       from = CHAR_TO_BYTE (XINT (start));
       to = CHAR_TO_BYTE (XINT (end));
       stop = from < GPT_BYTE && GPT_BYTE < to ? GPT_BYTE : to;
@@ -6444,17 +6441,27 @@
 	{
 	  for (p = p2; p < p2end && ASCII_BYTE_P (*p); p++);
 	  if (p == p2end)
-	    return Qt;
+	    return safe_codings;
 	}
     }
 
   /* The text contains non-ASCII characters.  */
   work_table = Fcopy_sequence (Vchar_coding_system_table);
-  safe_codings = find_safe_codings (p1, p1end, Qt, work_table,
+  safe_codings = find_safe_codings (p1, p1end, safe_codings, work_table,
 				    &single_byte_char_found);
+  if (INTEGERP (safe_codings))
+    {
+      int byte = p1end - p1 - XINT (safe_codings);
+      return (STRINGP (start)
+	      ? string_byte_to_char (start, byte)
+	      : bytepos_to_charpos (byte + from)) - 1;
+    }
   if (p2 < p2end)
     safe_codings = find_safe_codings (p2, p2end, safe_codings, work_table,
 				      &single_byte_char_found);
+  if (INTEGERP (safe_codings))
+    return bytepos_to_charpos (p2end - p2 - XINT (safe_codings) + stop) - 1;
+
 
   if (EQ (safe_codings, Qt))
     ; /* Nothing to be done.  */

Re: enhanced select-safe-coding-system

[Dave Love]

"Stefan Monnier" <monnier+gnu/emacs@RUM.cs.yale.edu> writes:

> You might be right that the elisp code is fast enough, but my
> patch (attached) makes fewer changes and avoids re-implementing
> the same functionality in elisp.

I'm afraid I can't see how that works out of context, and I'm not sure
it's the right thing to do.  Make sure handa approves if he hasn't
done already.

Re: enhanced select-safe-coding-system

[Richard Stallman]

Would you please install your patch?  I'm surprised it isn't installed yet.

Re: enhanced select-safe-coding-system

[Stefan Monnier]

> Would you please install your patch?  I'm surprised it isn't installed yet.

It turns out it's not correct and I haven't found the time to fix it yet.
The problem is that find-coding-systems-region-internal handles
"generic" coding-systems specially, which my patch does not
take into account.
By "generic" I mean those who `safe-chars' property is t.

Maybe we should go ahead and install Dave's code instead.  It's
O(n log n) rather than O(n), but it has the advantage of being
"obviously correct".


	Stefan

Re: enhanced select-safe-coding-system

[Richard Stallman]

    Maybe we should go ahead and install Dave's code instead.  It's
    O(n log n) rather than O(n), but it has the advantage of being
    "obviously correct".

Ok, let's do that, and see how people like it.

Re: enhanced select-safe-coding-system

[Richard Stallman]

    They look like the right sort of changes, but you want to break this
    line so that it's obvious that `coding-system' is being returned.

    > ! 	      (error "Save aborted")))))    coding-system))

Thanks.

    If you want to make select-safe-coding-system more helpful, you could
    also use code like this to find at least the first unencodable
    character and display that part of the buffer when
    select-safe-coding-system needs to prompt for a coding system.  Emacs
    20 used to do something similar, but it was only necessary to check
    charsets then.

This is a feature we want very much.  I would install this, except
that Stefan already seems to have something faster.

Re: enhanced select-safe-coding-system

[Dave Love]

Richard Stallman <rms@gnu.org> writes:

> This is a feature we want very much.

I don't understand why it's that important, but no matter.

> I would install this, except that Stefan already seems to have
> something faster.

I haven't implemented anything specifically in
select-safe-coding-system (because I didn't feel the need).  Finding
coding system regions is relevant more generally, e.g. for finding
charset regions in MIME parts.