bug#4136: 23.1; delete-pair

classic Classic list List threaded Threaded
33 messages Options
12
Reply | Threaded
Open this post in threaded view
|

bug#4136: 23.1; delete-pair

Stefan Monnier
> ...this is exactly the issue: it is much better if `delete-foo' is
> always an operation that reverts what `insert-foo' does.

I don't think we shojuld make it hard to delete (...] just because it
may be done by mistake.  That's what `undo' is for.

So, I'm in favor of making delete-pair a bit more picky, but not quite
as much as you suggest.  I suggest we try and make sure delete-pair
indeed only eliminates chars like ([" on the left side and )]" on the
right side (including by moving point a little bit in order to find the
intended pair), but let's not impose that ( can only match ).

> The current state of `delete-pair' is so bad that my guess is that
> hardly anyone used it, so adding another command doesn't make much
> sense.

It works fine, tho it indeed does it in a "blind" way that means that
you may get surprise results which require undo.  No big deal, tho.


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#4136: 23.1; delete-pair

martin rudalics
In reply to this post by Juri Linkov
 > We are in violent agreement.  I didn't claim that `delete-pair'
 > shouldn't check `insert-pair-alist'.  On the contrary, I think
 > `delete-pair' should verify if the character pair starting at the
 > current point is part of a pair according to `insert-pair-alist'.
 > "At the current point" - that's my point.  It shouldn't try
 > finding the opening character somewhere else.  So in your original
 > test case it should throw an error when the cursor is on some
 > whitespace that precedes an expression.

Ne hlebom jedinnym.  Why can't we be generous and provide something like
the attached?

martin

*** emacs-lisp/lisp.el.~1.102.~ 2009-07-27 08:10:54.124527300 +0200
--- emacs-lisp/lisp.el 2009-08-18 08:52:42.734375000 +0200
***************
*** 527,537 ****
    (interactive "P")
    (insert-pair arg ?\( ?\)))
 
! (defun delete-pair ()
!   "Delete a pair of characters enclosing the sexp that follows point."
!   (interactive)
!   (save-excursion (forward-sexp 1) (delete-char -1))
!   (delete-char 1))
 
  (defun raise-sexp (&optional arg)
    "Raise ARG sexps higher up the tree."
--- 527,566 ----
    (interactive "P")
    (insert-pair arg ?\( ?\)))
 
! (defun delete-pair (&optional arg)
!   "Delete a pair of characters enclosing ARG sexps that follow point.
! A negative ARG deletes a pair around the preceding ARG sexps instead."
!   (interactive "P")
!
!   (if arg
!       (setq arg (prefix-numeric-value arg))
!     (setq arg 1))
!
!   (if (< arg 0)
!       (save-excursion
! (skip-chars-backward " \t")
! (save-excursion
!  (let ((close-char (char-before)))
!    (forward-sexp arg)
!    (unless (member (list (char-after) close-char)
!    (mapcar (lambda (p)
!      (if (= (length p) 3) (cdr p) p))
!    insert-pair-alist))
!      (error "Not after matching pair"))
!    (delete-char 1)))
! (delete-char -1))
!     (save-excursion
!       (skip-chars-forward " \t")
!       (save-excursion
! (let ((open-char (char-after)))
!  (forward-sexp arg)
!  (unless (member (list open-char (char-before))
!  (mapcar (lambda (p)
!    (if (= (length p) 3) (cdr p) p))
!  insert-pair-alist))
!    (error "Not before matching pair"))
!  (delete-char -1)))
!       (delete-char 1))))
 
  (defun raise-sexp (&optional arg)
    "Raise ARG sexps higher up the tree."
Reply | Threaded
Open this post in threaded view
|

bug#4136: 23.1; delete-pair

Juri Linkov
In reply to this post by Stefan Monnier
>> ...this is exactly the issue: it is much better if `delete-foo' is
>> always an operation that reverts what `insert-foo' does.
>
> I don't think we shojuld make it hard to delete (...] just because it
> may be done by mistake.  That's what `undo' is for.
>
> So, I'm in favor of making delete-pair a bit more picky, but not quite
> as much as you suggest.  I suggest we try and make sure delete-pair
> indeed only eliminates chars like ([" on the left side and )]" on the
> right side (including by moving point a little bit in order to find the
> intended pair), but let's not impose that ( can only match ).

I don't oppose strict checking for only a pair of ( and ) because
a situation with (...] is extremely rare and usually means that
there is some bug.  So in reality there is no problem with
checking against the `insert-pair-alist' list.

What I care about is simplicity.  It should be easy for the user
to understand and remember what the command `delete-pair' does.
Failing on a non-pair character would be natural for a command
that is a counterpart of `insert-pair'.

--
Juri Linkov
http://www.jurta.org/emacs/



Reply | Threaded
Open this post in threaded view
|

bug#4136: 23.1; delete-pair

Juri Linkov
In reply to this post by martin rudalics
>> We are in violent agreement.  I didn't claim that `delete-pair'
>> shouldn't check `insert-pair-alist'.  On the contrary, I think
>> `delete-pair' should verify if the character pair starting at the
>> current point is part of a pair according to `insert-pair-alist'.
>> "At the current point" - that's my point.  It shouldn't try
>> finding the opening character somewhere else.  So in your original
>> test case it should throw an error when the cursor is on some
>> whitespace that precedes an expression.
>
> Ne hlebom jedinnym.  Why can't we be generous and provide something like
> the attached?
>
> martin
> *** emacs-lisp/lisp.el.~1.102.~ 2009-07-27 08:10:54.124527300 +0200
> --- emacs-lisp/lisp.el 2009-08-18 08:52:42.734375000 +0200
> ***************
> *** 527,537 ****
>     (interactive "P")
>     (insert-pair arg ?\( ?\)))
>  
> ! (defun delete-pair ()
> !   "Delete a pair of characters enclosing the sexp that follows point."
> !   (interactive)
> !   (save-excursion (forward-sexp 1) (delete-char -1))
> !   (delete-char 1))
>  
>   (defun raise-sexp (&optional arg)
>     "Raise ARG sexps higher up the tree."
> --- 527,566 ----
>     (interactive "P")
>     (insert-pair arg ?\( ?\)))
>  
> ! (defun delete-pair (&optional arg)
> !   "Delete a pair of characters enclosing ARG sexps that follow point.
> ! A negative ARG deletes a pair around the preceding ARG sexps instead."
> !   (interactive "P")
> !
> !   (if arg
> !       (setq arg (prefix-numeric-value arg))
> !     (setq arg 1))
> !
> !   (if (< arg 0)
> !       (save-excursion
> ! (skip-chars-backward " \t")
> ! (save-excursion
> !  (let ((close-char (char-before)))
> !    (forward-sexp arg)
> !    (unless (member (list (char-after) close-char)
> !    (mapcar (lambda (p)
> !      (if (= (length p) 3) (cdr p) p))
> !    insert-pair-alist))
> !      (error "Not after matching pair"))
> !    (delete-char 1)))
> ! (delete-char -1))
> !     (save-excursion
> !       (skip-chars-forward " \t")
> !       (save-excursion
> ! (let ((open-char (char-after)))
> !  (forward-sexp arg)
> !  (unless (member (list open-char (char-before))
> !  (mapcar (lambda (p)
> !    (if (= (length p) 3) (cdr p) p))
> !  insert-pair-alist))
> !    (error "Not before matching pair"))
> !  (delete-char -1)))
> !       (delete-char 1))))
>  
>   (defun raise-sexp (&optional arg)
>     "Raise ARG sexps higher up the tree."

Hmm, I think this makes sense.

--
Juri Linkov
http://www.jurta.org/emacs/



Reply | Threaded
Open this post in threaded view
|

bug#4136: 23.1; delete-pair

Lars Ingebrigtsen
In reply to this post by martin rudalics
Juri Linkov <[hidden email]> writes:

>> I don't use delete-pair myself regularly, but giving it some testing
>> now, this new version seems to work fine.
>>
>> Any comments?  (The patch is 11 years old.)
>
> I don't know, I use the current version of delete-pair many times every day,
> and happy with it.  If the patch doesn't break the current behavior, that
> would be fine.

Could you test the patch and see whether there's any problems?  :-)

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#4136: 23.1; delete-pair

Juri Linkov
>>> I don't use delete-pair myself regularly, but giving it some testing
>>> now, this new version seems to work fine.
>>>
>>> Any comments?  (The patch is 11 years old.)
>>
>> I don't know, I use the current version of delete-pair many times every day,
>> and happy with it.  If the patch doesn't break the current behavior, that
>> would be fine.
>
> Could you test the patch and see whether there's any problems?  :-)

Now I tested that at least it doesn't cause regression.
Hopefully, its new features work equally well :-)



Reply | Threaded
Open this post in threaded view
|

bug#4136: 23.1; delete-pair

Lars Ingebrigtsen
Juri Linkov <[hidden email]> writes:

> Now I tested that at least it doesn't cause regression.
> Hopefully, its new features work equally well :-)

Thanks for checking.

The new feature is just that it only deletes things that are part of
insert-pair-alist, if I understood Martin correctly...

I've now pushed it to Emacs 28.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#4136: 23.1; delete-pair

Juri Linkov
>> Now I tested that at least it doesn't cause regression.
>> Hopefully, its new features work equally well :-)
>
> Thanks for checking.
>
> The new feature is just that it only deletes things that are part of
> insert-pair-alist, if I understood Martin correctly...
>
> I've now pushed it to Emacs 28.

I see that you reverted it now because it breaks on "foo" in text-mode.
But the previous version is worse: it deletes wrong text on "foo"
in text-mode.



Reply | Threaded
Open this post in threaded view
|

bug#4136: 23.1; delete-pair

Lars Ingebrigtsen
Juri Linkov <[hidden email]> writes:

> I see that you reverted it now because it breaks on "foo" in text-mode.
> But the previous version is worse: it deletes wrong text on "foo"
> in text-mode.

Heh, I was misreading the tests -- I thought that the first one of these
was the one that failed, and not the second, kinda weird one:

(ert-deftest lisp-delete-pair-quotation-marks ()
  "Test \\[delete-pair] with quotation marks."
  (with-temp-buffer
    (insert "\"foo\"")
    (goto-char (point-min))
    (delete-pair)
    (should (string-equal "foo" (buffer-string)))))

(ert-deftest lisp-delete-pair-quotes-in-text-mode ()
  "Test \\[delete-pair] against string in Text Mode for #15014."
  (with-temp-buffer
    (text-mode)
    (insert "\"foo\"")
    (goto-char (point-min))
    (delete-pair)
    (should (string-equal "fo\"" (buffer-string)))))

So in text mode, delete-pair just deletes two characters, and that
doesn't seem helpful.  The patch I reverted made it bug out instead,
which seems better.

However, bug#15014 says:

> I'm not arguing here that we font lock double quoted strings (though
> I'm not convinced either). Without font locking, I can't think of a
> concrete case that yields the "pretty annoying behavior". What do you
> have in mind?
>
> I added
>
>   (modify-syntax-entry ?\" "$")
>
> to my configuration and find that forward-sexp and delete-pair seem to
> work without confusion, even in the presence of odd double quotes. It
> seems forward-sexp just scans forward from point.

And the patch wouldn't allow this, either, so I'm not sure this
behavioural change would be welcome -- the user would have to alter
insert-pair-alist in text mode, too.

So...

I think the current thing delete-pair does in text-mode is bad, so I'm
inclined to re-apply the patch, and announce this as an incompatible
change in NEWS (and explain how to get the behaviour described above
back).

Any opinions?

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#4136: 23.1; delete-pair

Juri Linkov
> (ert-deftest lisp-delete-pair-quotes-in-text-mode ()
>   "Test \\[delete-pair] against string in Text Mode for #15014."
>   (with-temp-buffer
>     (text-mode)
>     (insert "\"foo\"")
>     (goto-char (point-min))
>     (delete-pair)
>     (should (string-equal "fo\"" (buffer-string)))))
>
> So in text mode, delete-pair just deletes two characters, and that
> doesn't seem helpful.  The patch I reverted made it bug out instead,
> which seems better.

Yes, it's better to bug out than to delete text instead of deleting quotes.

> I think the current thing delete-pair does in text-mode is bad, so I'm
> inclined to re-apply the patch, and announce this as an incompatible
> change in NEWS (and explain how to get the behaviour described above
> back).
>
> Any opinions?

If it's really impossible to delete a pair in text-mode, then better
to signal an error.



Reply | Threaded
Open this post in threaded view
|

bug#4136: 23.1; delete-pair

Lars Ingebrigtsen
Juri Linkov <[hidden email]> writes:

> If it's really impossible to delete a pair in text-mode, then better
> to signal an error.

OK; I've reapplied the patch and fixed the test, and I'm now re-closing
this bug report.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#4136: 23.1; delete-pair

Juri Linkov
>> If it's really impossible to delete a pair in text-mode, then better
>> to signal an error.
>
> OK; I've reapplied the patch and fixed the test, and I'm now re-closing
> this bug report.

Thanks.  Every time I delete a pair of quotes in a long string,
or a pair of parens in a long sexp, it makes me feel uneasy since
I worry whether it deleted the intended characters or not.

How about using the same delay blink-matching-delay that is used
after showing a matching paren, but in this case to show
a character at the other end of the pair with a delay
before deleting it?

This is achievable with this patch:

diff --git a/lisp/emacs-lisp/lisp.el b/lisp/emacs-lisp/lisp.el
index ac4ba78897..88e1b26af3 100644
--- a/lisp/emacs-lisp/lisp.el
+++ b/lisp/emacs-lisp/lisp.el
@@ -752,6 +752,8 @@ delete-pair
       (if (= (length p) 3) (cdr p) p))
     insert-pair-alist))
       (error "Not after matching pair"))
+    (when blink-matching-paren
+      (sit-for blink-matching-delay))
     (delete-char 1)))
  (delete-char -1))
     (save-excursion
@@ -764,6 +766,8 @@ delete-pair
     (if (= (length p) 3) (cdr p) p))
   insert-pair-alist))
     (error "Not before matching pair"))
+  (when blink-matching-paren
+    (sit-for blink-matching-delay))
   (delete-char -1)))
       (delete-char 1))))
 



Reply | Threaded
Open this post in threaded view
|

bug#4136: 23.1; delete-pair

Lars Ingebrigtsen
Juri Linkov <[hidden email]> writes:

> How about using the same delay blink-matching-delay that is used
> after showing a matching paren, but in this case to show
> a character at the other end of the pair with a delay
> before deleting it?

Makes sense to me, but I'm not a regular user of delete-pair, so I don't
know whether that'd be annoying or reassuring...

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



12