bug#13312: 24.3.50; delete selection mode not killing on overwrite

bug#13312: 24.3.50; delete selection mode not killing on overwrite

[Tony Day]

recipe:

Emacs -Q

(delete-selection-mode)

Selected text gets deleted on overwriting, rather than killed.


In GNU Emacs 24.3.50.1 (x86_64-apple-darwin, NS apple-appkit-1038.36)
 of 2012-12-30 on bob.porkrind.org
Bzr revision: 111377 yamaoka@jpl.org-20121229231805-uxaoiydm94ttem4b
Windowing system distributor `Apple', version 10.3.1187
Configured using:
 `configure --host=x86_64-apple-darwin --build=i686-apple-darwin
 --with-ns'

Important settings:
  value of $LC_ALL: en_US.UTF-8
  value of $LANG: en_US
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Major mode: Fundamental

Minor modes in effect:
  delete-selection-mode: t
  tooltip-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-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-h C-a <down> <up> M-x d e l e t <tab> s e l <tab> 
<return> C-x b f o o <return> K i l l SPC m e <left> 
<left> <left> <left> <left> <left> <left> <down> <return> 
<return> <return> <return> C-y M-x k i l l <tab> r 
i n <tab> <backspace> <backspace> <backspace> <backspace> 
c l e a <tab> <backspace> <backspace> <backspace> <backspace> 
<backspace> <backspace> <backspace> <backspace> <backspace> 
<backspace> <backspace> <backspace> <backspace> <backspace> 
C-g <down> <return> <return> <return> <up> <up> <up> 
<up> <up> <up> <up> <up> <S-down> <S-left> o v e r 
w r i t e <down> <down> <down> <down> <down> <down> 
C-y M-y M-y <help-echo> <down-mouse-1> <mouse-1> <menu-bar> 
<help-menu> <send-emacs-bug-report>

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

Load-path shadows:
None found.

Features:
(shadow sort gnus-util mail-extr emacsbug message format-spec rfc822 mml
easymenu mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util mail-prsvr mail-utils cus-start cus-load delsel time-date
tooltip ediff-hook vc-hooks lisp-float-type mwheel ns-win tool-bar dnd
fontset image regexp-opt fringe tabulated-list newcomment lisp-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 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 ns multi-tty
emacs)

bug#13312: 24.3.50; delete selection mode not killing on overwrite

[Glenn Morris]

Tony Day wrote:

> Emacs -Q
>
> (delete-selection-mode)
>
> Selected text gets deleted on overwriting, rather than killed.

Why did you expect `delete-selection-mode' to kill rather than delete?

bug#13312: 24.3.50; delete selection mode not killing on overwrite

[Tony Day]

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

On Mon, Dec 31, 2012 at 12:29 PM, Glenn Morris <rgm@gnu.org> wrote:

> Why did you expect `delete-selection-mode' to kill rather than delete?


I asked on stack exchange for how to change the behaviour and someone
suggested it was a bug.  I figured delete-selection-mode was less of a
mouthful than maybe-delete-or-kill-selection-mode ;)

The critical code lines were was also ambiguous and beyond my powers of
comprehension:

(put 'self-insert-command 'delete-selection
     (lambda ()
       (not (run-hook-with-args-until-success
             'self-insert-uses-region-functions))))

Thanks for confirming the default behaviour.  On changing to kill rather
than delete with:

(put 'self-insert-command 'delete-selection 'kill)

I get no self-inserted key, and an extra blank item in the kill ring.

Regardless, why don't we call this a hack and not a bug.

Tony

[-- Attachment #2: Type: text/html, Size: 1423 bytes --]

bug#13312: 24.3.50; delete selection mode not killing on overwrite

[Stefan Monnier]

severity 13312 wishlist
thanks

>> Emacs -Q
>> (delete-selection-mode)
>> Selected text gets deleted on overwriting, rather than killed.

It's the expected behavior, but we could accept patches to optionally
provide the other behavior (tho usually the deleted text is still available
from the "PRIMARY" selection, so the need to put it on the kill-ring is
not that high and could even be reduced further if we provided more
commands to access the last "PRIMARY" selection).


        Stefan

bug#13312: 24.3.50; delete selection mode not killing on overwrite

[Drew Adams]

I don't think this should be on the wishlist.  It's a clear bug.

(put 'ANY-COMMAND 'delete-selection 'kill) should kill the
text in the active region.  It does not do so in the case
where ANY-COMMAND is `self-insert-command'.  A bug.

The fact that "usually the delete text is still available from the
"PRIMARY" selection" is irrelevant.  (Not to mention that there is
no "PRIMARY" selection on some platforms.)

Putting `kill' as the property value should kill the text, pure and
simple.
ss

bug#13312: 24.3.50; delete selection mode not killing on overwrite

[Juri Linkov]

> I don't think this should be on the wishlist.  It's a clear bug.
>
> (put 'ANY-COMMAND 'delete-selection 'kill) should kill the
> text in the active region.  It does not do so in the case
> where ANY-COMMAND is `self-insert-command'.  A bug.
>
> The fact that "usually the delete text is still available from the
> "PRIMARY" selection" is irrelevant.  (Not to mention that there is
> no "PRIMARY" selection on some platforms.)
>
> Putting `kill' as the property value should kill the text, pure and
> simple.

There is no bug.  In delete-selection-mode with overwrite-mode
selecting a region and typing a self-inserting key deletes the
selected region and replaces it with the typed key.

bug#13312: 24.3.50; delete selection mode not killing on overwrite

[Drew Adams]

> > I don't think this should be on the wishlist.  It's a clear bug.
> >
> > (put 'ANY-COMMAND 'delete-selection 'kill) should kill the
> > text in the active region.  It does not do so in the case
> > where ANY-COMMAND is `self-insert-command'.  A bug.
> >
> > The fact that "usually the delete text is still available from the
> > "PRIMARY" selection" is irrelevant.  (Not to mention that there is
> > no "PRIMARY" selection on some platforms.)
> >
> > Putting `kill' as the property value should kill the text, pure
> > and simple.
> 
> There is no bug.  In delete-selection-mode with overwrite-mode
> selecting a region and typing a self-inserting key deletes the
> selected region and replaces it with the typed key.

Did you read the bug description and the associate StackOverflow
question?  The bug is that when you put `kill' as the
`delete-selection' property value on `self-insert-command' the
region is not killed - it is not added to the `kill-ring'.  And
the first char to be self-inserted is not inserted.

This is not about the out-of-the-box behavior of self-inserting
in delete-selection mode, i.e., with the property value having
its default value.

bug#13312: 24.3.50; delete selection mode not killing on overwrite

[Juri Linkov]

> Did you read the bug description and the associate StackOverflow
> question?  The bug is that when you put `kill' as the
> `delete-selection' property value on `self-insert-command' the
> region is not killed - it is not added to the `kill-ring'.

Ah, I thought this is a new bug report from you.  But I still
can't find a link to the associate StackOverflow question.

What I wanted to suggest is to add more self-inserting commands
to delsel like `insert-char' and `quoted-insert'.  I guess
they don't need the same `self-insert-uses-region-functions'
like the currently existing `self-insert-iso' doesn't use that.

But when you want put `kill' as the `delete-selection' property
then it's better to use this code:

(put 'self-insert-command 'delete-selection
     (lambda ()
       (and (not (run-hook-with-args-until-success
		  'self-insert-uses-region-functions))
	    'kill)))

It currently doesn't work properly because `kill-region' overwrites
the value of `this-command'.  This is a bug that can be fixed
by this patch that also fixes overwrite-mode for `kill' and
puts `delete-selection' on more commands:

=== modified file 'lisp/delsel.el'
--- lisp/delsel.el	2013-01-01 09:11:05 +0000
+++ lisp/delsel.el	2013-10-19 16:24:16 +0000
@@ -78,7 +78,9 @@ (defun delete-active-region (&optional k
   "Delete the active region.
 If KILLP in not-nil, the active region is killed instead of deleted."
   (if killp
-      (kill-region (point) (mark))
+      ;; Prevent `kill-region' from changing the value of `this-command'.
+      (let (this-command)
+	(kill-region (point) (mark)))
     (delete-region (point) (mark)))
   t)
 
@@ -102,7 +104,13 @@ (defun delete-selection-helper (type)
      FUNCTION should take no argument and return one of the above values or nil."
   (condition-case data
       (cond ((eq type 'kill)
-	     (delete-active-region t))
+	     (delete-active-region t)
+	     (if (and overwrite-mode
+		      (eq this-command 'self-insert-command))
+		 (let ((overwrite-mode nil))
+		   (self-insert-command
+		    (prefix-numeric-value current-prefix-arg))
+		   (setq this-command 'ignore))))
 	    ((eq type 'yank)
 	     ;; Before a yank command, make sure we don't yank the
 	     ;; head of the kill-ring that really comes from the
@@ -166,6 +174,8 @@ (put 'self-insert-command 'delete-select
              'self-insert-uses-region-functions))))
 
 (put 'self-insert-iso 'delete-selection t)
+(put 'insert-char 'delete-selection t)
+(put 'quoted-insert 'delete-selection t)
 
 (put 'yank 'delete-selection 'yank)
 (put 'clipboard-yank 'delete-selection 'yank)
@@ -175,6 +185,7 @@ (put 'delete-backward-char 'delete-selec
 (put 'backward-delete-char-untabify 'delete-selection 'supersede)
 (put 'delete-char 'delete-selection 'supersede)
 
+(put 'reindent-then-newline-and-indent 'delete-selection t)
 (put 'newline-and-indent 'delete-selection t)
 (put 'newline 'delete-selection t)
 (put 'open-line 'delete-selection 'kill)
@@ -203,9 +214,9 @@ (defun delsel-unload-function ()
   (define-key minibuffer-local-completion-map "\C-g" 'abort-recursive-edit)
   (define-key minibuffer-local-must-match-map "\C-g" 'abort-recursive-edit)
   (define-key minibuffer-local-isearch-map "\C-g" 'abort-recursive-edit)
-  (dolist (sym '(self-insert-command self-insert-iso yank clipboard-yank
+  (dolist (sym '(self-insert-command self-insert-iso insert-char quoted-insert yank clipboard-yank
 		 insert-register delete-backward-char backward-delete-char-untabify
-		 delete-char newline-and-indent newline open-line))
+		 delete-char reindent-then-newline-and-indent newline-and-indent newline open-line))
     (put sym 'delete-selection nil))
   ;; continue standard unloading
   nil)

bug#13312: 24.3.50; delete selection mode not killing on overwrite

[Drew Adams]

> Ah, I thought this is a new bug report from you.

;-)

> But I still can't find a link to the associate StackOverflow question.

http://stackoverflow.com/questions/14076480/how-to-kill-text-when-overwriting-in-delete-selection-mode

> What I wanted to suggest is to add more self-inserting commands
> to delsel like `insert-char' and `quoted-insert'.  I guess
> they don't need the same `self-insert-uses-region-functions'
> like the currently existing `self-insert-iso' doesn't use that.

Sounds like a more general discussion.

Please consider moving that and the rest of the discussion to a
separate bug report or emacs-devel.  It doesn't seem like it is
limited to this bug.

My request was to move this bug back to Normal from Wish List.
If someone would also look into fixing it, that would be great.

> But when you want put `kill' as the `delete-selection' property
> then it's better to use this code:
> (put 'self-insert-command 'delete-selection
>      (lambda ()
>        (and (not (run-hook-with-args-until-success
> 		  'self-insert-uses-region-functions))
> 	    'kill)))

Maybe so, but that is not at all what the `delete-selection-mode'
doc & comments tell users.  If this is a new requirement/guideline,
then it needs to be documented.

But I wonder why this must now be so.  In the past, a user could
just put `kill' as the property.  The code does not seem so clean now.

One of the benefits of the `delsel.el' design (and yes, along with
those benefits come also some disadvantages) is its simplicity for
users.

This seems to go against that.  Is it really necessary?  Isn't
there another way to accomplish the same thing (whatever that is),
so we can keep the simple and clean design for users?

What was the reason for introducing
`self-insert-uses-region-functions'?  It seems it was only for
`electric-pair-mode'.  IIRC, I wasn't too happy with that hack when
it was done.  Now it seems to be dirtying (complicating) `delsel.el'.
Isn't there a better way?

But again, we should probably be discussing this elsewhere, since
it does not seem to be only about this bug.

> It currently doesn't work properly because `kill-region' overwrites
> the value of `this-command'.  This is a bug that can be fixed
> by this patch that also fixes overwrite-mode for `kill' and
> puts `delete-selection' on more commands:

Sorry, I cannot judge now whether this DTRT.  But again, this seems
to reach beyond this bug.  (Let me know if I'm missing something
and this is really specific to this bug.)

I would like to see, if possible, a simpler `delsel.el', rather than
a more complicated one - from the user's perspective, at least, if
not necessarily the implementation.  Users should be able to `put'
a single, understandable symbol as the `delete-selection' property
value.  They should not need to fiddle with obscure lambda forms (or
symbols whose names are not simple to understand).  Symbol `kill' is
simple - it says that you want the region to be killed.

This simplicity was the case before (`delsel.el' is old and simple).
Someone introduced `electric-pair-mode', and then someone else
complained about its interaction with `delete-selection-mode'.  The
fix for that should not have involved screwing `delete-selection-mode',
as seems to be the case so far.

bug#13312: 24.3.50; delete selection mode not killing on overwrite

[Juri Linkov]

> What was the reason for introducing
> `self-insert-uses-region-functions'?  It seems it was only for
> `electric-pair-mode'.  IIRC, I wasn't too happy with that hack when
> it was done.  Now it seems to be dirtying (complicating) `delsel.el'.
> Isn't there a better way?

I think (run-hook-with-args-until-success 'self-insert-uses-region-functions)
could be moved to the body of `delete-selection-helper'.
Then the users again will enjoy the simplicity of

  (put 'self-insert-command 'delete-selection t)

and

  (put 'self-insert-command 'delete-selection 'kill)

> Sorry, I cannot judge now whether this DTRT.  But again, this seems
> to reach beyond this bug.  (Let me know if I'm missing something
> and this is really specific to this bug.)

The first hunk of my patch fixes the reported bug.

bug#13312: 24.3.50; delete selection mode not killing on overwrite

[Drew Adams]

> > Isn't there a better way?
> 
> I think (run-hook-with-args-until-success 'self-insert-uses-region-
> functions) could be moved to the body of `delete-selection-helper'.
> Then the users again will enjoy the simplicity of
>     (put 'self-insert-command 'delete-selection t)
> and (put 'self-insert-command 'delete-selection 'kill)

That sounds good.  Thank you, Juri.  I didn't check that things work
well with that change, but the description sounds right.

> The first hunk of my patch fixes the reported bug.

Please install that part, at least.  Not sure what the rest does,
but why not handle it as a separate bug fix?

bug#13312: 24.3.50; delete selection mode not killing on overwrite

[Juri Linkov]

Version: 24.4

> It currently doesn't work properly because `kill-region' overwrites
> the value of `this-command'.  This is a bug that can be fixed
> by this patch that also fixes overwrite-mode for `kill' and
> puts `delete-selection' on more commands:

This is fixed now.