all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
@ 2017-07-10  3:58 Paul Rankin
  2017-07-10  6:33 ` Tino Calancha
  2017-07-21  3:47 ` Allen Li
  0 siblings, 2 replies; 18+ messages in thread
From: Paul Rankin @ 2017-07-10  3:58 UTC (permalink / raw
  To: 27634

When saving to a register C-g does not quit but instead saves to the register "C-g".

To reproduce:

1. $ emacs -Q
2. M-x point-to-register [C-x r SPC]
3. C-g

Expected results:

C-g should call keyboard-quit and escape point-to-register

Actual results:

Point marker is saved to register "C-g".

This runs counter to expected behaviour with both quitting from saving a register, and in quitting from recalling a register, e.g. by attempting to quit from saving a point to register in buffer-A with C-g, when the user then attempts to keyboard-quit from jump-to-register [C-x r j C-g] he/she will be returned to point in buffer-A.

The root of the problem appears to be that register-read-with-preview only tests input with characterp:

(characterp ?^G) -> t

This appears to conflict with the Emacs manual entry on registers, which states that registers can be a letters or numbers:

> Each register has a name that consists of a single character, which
> we will denote by R; R can be a letter (such as ‘a’) or a number (such
> as ‘1’); case matters, so register ‘a’ is not the same as register ‘A’.

(info "(emacs) Registers")

I have worked around this with the following patch to register.el.gz:

@@ -164,7 +164,7 @@
 		       help-chars)
 	    (unless (get-buffer-window buffer)
 	      (register-preview buffer 'show-empty)))
-	  (if (characterp last-input-event) last-input-event
+	  (if (< 31 last-input-event 127) last-input-event
 	    (error "Non-character input-event")))
       (and (timerp timer) (cancel-timer timer))
       (let ((w (get-buffer-window buffer)))

However, I think this will not work on international keyboards and so is probably a poor fix.

Configuration:

GNU Emacs 25.2.1 (x86_64-apple-darwin16.6.0, NS appkit-1504.83 Version 10.12.5 (Build 16F73)) of 2017-06-06
macOS 10.12.5 (16F73)

MacBook Pro (Retina, 15-inch, Mid 2014)
2.2 GHz Intel Core i7
16 GB 1600 MHz DDR3
Intel Iris Pro 1536 MB

--
www.paulwrankin.com





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
  2017-07-10  3:58 bug#27634: 25.2.1; C-g does not quit register-read-with-preview Paul Rankin
@ 2017-07-10  6:33 ` Tino Calancha
  2017-07-10  7:20   ` Paul Rankin
  2017-07-10 19:01   ` Andreas Schwab
  2017-07-21  3:47 ` Allen Li
  1 sibling, 2 replies; 18+ messages in thread
From: Tino Calancha @ 2017-07-10  6:33 UTC (permalink / raw
  To: Paul Rankin; +Cc: 27634

Paul Rankin <hello@paulwrankin.com> writes:

> When saving to a register C-g does not quit but instead saves to the register "C-g".
>
> To reproduce:
>
> 1. $ emacs -Q
> 2. M-x point-to-register [C-x r SPC]
> 3. C-g
>
> Expected results:
>
> C-g should call keyboard-quit and escape point-to-register
>
> Actual results:
>
> Point marker is saved to register "C-g".

> I have worked around this with the following patch to register.el.gz:
>
> @@ -164,7 +164,7 @@
>  		       help-chars)
>  	    (unless (get-buffer-window buffer)
>  	      (register-preview buffer 'show-empty)))
> -	  (if (characterp last-input-event) last-input-event
> +	  (if (< 31 last-input-event 127) last-input-event
>  	    (error "Non-character input-event")))
>        (and (timerp timer) (cancel-timer timer))
>        (let ((w (get-buffer-window buffer)))
>
> However, I think this will not work on international keyboards and so is probably a poor fix.
How about the following?

@@ -164,6 +164,8 @@ register-read-with-preview
 		       help-chars)
 	    (unless (get-buffer-window buffer)
 	      (register-preview buffer 'show-empty)))
+          (when (eq (string-to-char "\C-g") last-input-event)
+            (keyboard-quit))
 	  (if (characterp last-input-event) last-input-event
 	    (error "Non-character input-event")))
       (and (timerp timer) (cancel-timer timer))


--8<-----------------------------cut here---------------start------------->8---
commit 51de600be1351704d4e435a4977d63b3d4e04eaf
Author: Tino Calancha <tino.calancha@gmail.com>
Date:   Mon Jul 10 15:22:32 2017 +0900

    Don't set registers in 'C-g'
    
    That key is reserved to abort commands (Bug#27634).
    * lisp/register-tests.el (register-read-with-preview):
    Abort when user inputs 'C-g'.
    * test/lisp/register-tests.el (register-test-bug27634): Add test.

diff --git a/lisp/register.el b/lisp/register.el
index 7cc3ccd870..19b7dee4b9 100644
--- a/lisp/register.el
+++ b/lisp/register.el
@@ -164,6 +164,8 @@ register-read-with-preview
 		       help-chars)
 	    (unless (get-buffer-window buffer)
 	      (register-preview buffer 'show-empty)))
+          (when (eq (string-to-char "\C-g") last-input-event)
+            (keyboard-quit))
 	  (if (characterp last-input-event) last-input-event
 	    (error "Non-character input-event")))
       (and (timerp timer) (cancel-timer timer))
diff --git a/test/lisp/register-tests.el b/test/lisp/register-tests.el
new file mode 100644
index 0000000000..30c1468e25
--- /dev/null
+++ b/test/lisp/register-tests.el
@@ -0,0 +1,40 @@
+;;; register-tests.el --- tests for electric.el
+
+;; Copyright (C) 2017 Free Software Foundation, Inc.
+
+;; Author: Tino Calacha <tino.calancha@gmail.com>
+;; Keywords:
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+\f
+;;; Code:
+(require 'ert)
+(require 'cl-lib)
+
+(ert-deftest register-test-bug27634 ()
+  "Test for http://debbugs.gnu.org/27634 ."
+  (cl-letf (((symbol-function 'read-key) #'ignore)
+            (last-input-event 7)
+            (register-alist nil))
+    (should (equal 'quit
+                   (condition-case err
+                       (call-interactively 'point-to-register)
+                     (quit (car err)))))
+    (should-not register-alist)))
+
+(provide 'register-tests)
+;;; register-tests.el ends here

--8<-----------------------------cut here---------------end--------------->8---
In GNU Emacs 26.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2017-07-10
Repository revision: 273f4bde39af5d87f10fd58f35b666dfa8a996a3





^ permalink raw reply related	[flat|nested] 18+ messages in thread

* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
  2017-07-10  6:33 ` Tino Calancha
@ 2017-07-10  7:20   ` Paul Rankin
  2017-07-10  7:59     ` Tino Calancha
  2017-07-10 17:06     ` Eli Zaretskii
  2017-07-10 19:01   ` Andreas Schwab
  1 sibling, 2 replies; 18+ messages in thread
From: Paul Rankin @ 2017-07-10  7:20 UTC (permalink / raw
  To: Tino Calancha; +Cc: 27634

On Mon, 10 Jul 2017, at 04:33 PM, Tino Calancha wrote:
> How about the following?
> 
> @@ -164,6 +164,8 @@ register-read-with-preview
>  		       help-chars)
>  	    (unless (get-buffer-window buffer)
>  	      (register-preview buffer 'show-empty)))
> +          (when (eq (string-to-char "\C-g") last-input-event)
> +            (keyboard-quit))
>  	  (if (characterp last-input-event) last-input-event
>  	    (error "Non-character input-event")))
>        (and (timerp timer) (cancel-timer timer))

I think that's more a bandaid than fixing the root of the problem, which is that the manual tells the user that register names are alphanumeric characters (and I assume 99% of users only use alphanumeric characters) but the function doesn't test for this. e.g. testing for C-g doesn't catch for ^L or ^M, etc.

If we want to be strict about it, this might work:

@@ -164,8 +164,8 @@
 		       help-chars)
 	    (unless (get-buffer-window buffer)
 	      (register-preview buffer 'show-empty)))
-	  (if (characterp last-input-event) last-input-event
-	    (error "Non-character input-event")))
+	  (if (= (char-syntax last-input-event) 119) last-input-event
+	    (error "Register name must be alphanumeric")))
       (and (timerp timer) (cancel-timer timer))
       (let ((w (get-buffer-window buffer)))
         (and (window-live-p w) (delete-window w)))

That prohibits anything except "a-zA-Z0-9", although users may want to save registers to "$" or "*".





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
  2017-07-10  7:20   ` Paul Rankin
@ 2017-07-10  7:59     ` Tino Calancha
  2017-07-10 17:06     ` Eli Zaretskii
  1 sibling, 0 replies; 18+ messages in thread
From: Tino Calancha @ 2017-07-10  7:59 UTC (permalink / raw
  To: Paul Rankin; +Cc: Tino Calancha, 27634



On Mon, 10 Jul 2017, Paul Rankin wrote:

> On Mon, 10 Jul 2017, at 04:33 PM, Tino Calancha wrote:
>> How about the following?
>>
>> @@ -164,6 +164,8 @@ register-read-with-preview
>>  		       help-chars)
>>  	    (unless (get-buffer-window buffer)
>>  	      (register-preview buffer 'show-empty)))
>> +          (when (eq (string-to-char "\C-g") last-input-event)
>> +            (keyboard-quit))
>>  	  (if (characterp last-input-event) last-input-event
>>  	    (error "Non-character input-event")))
>>        (and (timerp timer) (cancel-timer timer))
>
> I think that's more a bandaid than fixing the root of the problem, which is that the manual tells the user that register names are alphanumeric characters (and I assume 99% of users only use alphanumeric characters) but the function doesn't test for this. e.g. testing for C-g doesn't catch for ^L or ^M, etc.
Opps, you are right.
>
> If we want to be strict about it, this might work:
>
> @@ -164,8 +164,8 @@
> 		       help-chars)
> 	    (unless (get-buffer-window buffer)
> 	      (register-preview buffer 'show-empty)))
> -	  (if (characterp last-input-event) last-input-event
> -	    (error "Non-character input-event")))
> +	  (if (= (char-syntax last-input-event) 119) last-input-event
> +	    (error "Register name must be alphanumeric")))
>       (and (timerp timer) (cancel-timer timer))
>       (let ((w (get-buffer-window buffer)))
>         (and (window-live-p w) (delete-window w)))
>
> That prohibits anything except "a-zA-Z0-9", although users may want to save registers to "$" or "*".
I cannot assure that i've never used "@" or "," as a register.  Probably 
i did it several times.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
  2017-07-10  7:20   ` Paul Rankin
  2017-07-10  7:59     ` Tino Calancha
@ 2017-07-10 17:06     ` Eli Zaretskii
  2017-07-11  4:14       ` Paul Rankin
  1 sibling, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2017-07-10 17:06 UTC (permalink / raw
  To: Paul Rankin; +Cc: tino.calancha, 27634

> From: Paul Rankin <hello@paulwrankin.com>
> Cc: 27634@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
> Date: Mon, 10 Jul 2017 17:20:55 +1000
> 
> On Mon, 10 Jul 2017, at 04:33 PM, Tino Calancha wrote:
> > How about the following?
> > 
> > @@ -164,6 +164,8 @@ register-read-with-preview
> >  		       help-chars)
> >  	    (unless (get-buffer-window buffer)
> >  	      (register-preview buffer 'show-empty)))
> > +          (when (eq (string-to-char "\C-g") last-input-event)
> > +            (keyboard-quit))
> >  	  (if (characterp last-input-event) last-input-event
> >  	    (error "Non-character input-event")))
> >        (and (timerp timer) (cancel-timer timer))
> 
> I think that's more a bandaid than fixing the root of the problem, which is that the manual tells the user that register names are alphanumeric characters (and I assume 99% of users only use alphanumeric characters) but the function doesn't test for this. e.g. testing for C-g doesn't catch for ^L or ^M, etc.

FWIW, I actually agree with Tino's solution, and was about to propose
something similar.  It's true that control characters are not
alphanumeric, but we could fix the documentation to be more accurate
if we care about that.  OTOH, we've supported control characters as
register names for many years, and by now it should be quite clear it
didn't bother anyone yet.

C-g needs indeed to generate keyboard quit, and perhaps ESC ESC as
well.  I'd look at read-char-choice for inspiration.

> If we want to be strict about it, this might work:
> 
> @@ -164,8 +164,8 @@
>  		       help-chars)
>  	    (unless (get-buffer-window buffer)
>  	      (register-preview buffer 'show-empty)))
> -	  (if (characterp last-input-event) last-input-event
> -	    (error "Non-character input-event")))
> +	  (if (= (char-syntax last-input-event) 119) last-input-event
> +	    (error "Register name must be alphanumeric")))
>        (and (timerp timer) (cancel-timer timer))
>        (let ((w (get-buffer-window buffer)))
>          (and (window-live-p w) (delete-window w)))
> 
> That prohibits anything except "a-zA-Z0-9", although users may want to save registers to "$" or "*".

Why would we want to be so strict when the only real problem is that
C-g doesn't quit?





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
  2017-07-10  6:33 ` Tino Calancha
  2017-07-10  7:20   ` Paul Rankin
@ 2017-07-10 19:01   ` Andreas Schwab
  1 sibling, 0 replies; 18+ messages in thread
From: Andreas Schwab @ 2017-07-10 19:01 UTC (permalink / raw
  To: Tino Calancha; +Cc: Paul Rankin, 27634

On Jul 10 2017, Tino Calancha <tino.calancha@gmail.com> wrote:

> +          (when (eq (string-to-char "\C-g") last-input-event)
                       ?\C-g

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
  2017-07-10 17:06     ` Eli Zaretskii
@ 2017-07-11  4:14       ` Paul Rankin
  2017-07-11  4:48         ` Tino Calancha
  2017-07-11 14:36         ` Eli Zaretskii
  0 siblings, 2 replies; 18+ messages in thread
From: Paul Rankin @ 2017-07-11  4:14 UTC (permalink / raw
  To: Eli Zaretskii; +Cc: tino.calancha, 27634

On Tue, 11 Jul 2017, at 03:06 AM, Eli Zaretskii wrote:
> FWIW, I actually agree with Tino's solution, and was about to propose
> something similar. It's true that control characters are not
> alphanumeric, but we could fix the documentation to be more accurate
> if we care about that.  OTOH, we've supported control characters as
> register names for many years, and by now it should be quite clear it
> didn't bother anyone yet.

Hmm, it bothers me?

If no one had reported an issue before now, it doesn't then follow that the issue didn't bother anyone, or, wouldn't have bothered them if they knew about it.

But the question is moot I think, since this is an opportunity to improve the code.. why waste time arguing for poorer code when we can make it better?
 
> > That prohibits anything except "a-zA-Z0-9", although users may want to save registers to "$" or "*".
> 
> Why would we want to be so strict when the only real problem is that
> C-g doesn't quit?

I think there are two good options for good UX: make the code reflect the documentation (this is the strict option), or update both the documentation and the code to reflect what we believe is user expectation, i.e. that the user may save registers to any character key on their keyboard (this is my preference).

One overlooked thing about Tino's solution is that C-g is a keystroke and keyboard-quit is a function, which obviously aren't necessarily equivalent. What if the user remaps keyboard quit to "7"?





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
  2017-07-11  4:14       ` Paul Rankin
@ 2017-07-11  4:48         ` Tino Calancha
  2017-07-11  5:07           ` Paul Rankin
  2017-07-11 14:36         ` Eli Zaretskii
  1 sibling, 1 reply; 18+ messages in thread
From: Tino Calancha @ 2017-07-11  4:48 UTC (permalink / raw
  To: Paul Rankin; +Cc: tino.calancha, 27634



On Tue, 11 Jul 2017, Paul Rankin wrote:

> One overlooked thing about Tino's solution is that C-g is a keystroke and keyboard-quit is a function, which obviously aren't necessarily equivalent. What if the user remaps keyboard quit to "7"?
I thought about that, but i discarded because i think binding something
other that `keyboard-quit' to `C-g' is a misuse.  The Emacs manual is full
of mentions to `C-g' as `keyboard-quit'.
There is even the following remark in the tips section:

"don't bind a key sequence ending in @key{C-g}, since that
is commonly used to cancel a key sequence."

If a user want to ignore such kind of advice he/she should
not expect everything will work the same.

Maybe we can fix this so that `register-read-with-preview'
will work with `C-g' bound to `my-cool-foo-command'; but we
cannot assure that no other Emacs part is affected because such
misguided `C-g' binding.  We must encourage users to follow
good practices.

Sure, it would be great if the entire Emacs code is robust against
any kind of user abuse/misuse, but that's not realistic.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
  2017-07-11  4:48         ` Tino Calancha
@ 2017-07-11  5:07           ` Paul Rankin
  2017-07-11  5:50             ` Tino Calancha
  0 siblings, 1 reply; 18+ messages in thread
From: Paul Rankin @ 2017-07-11  5:07 UTC (permalink / raw
  To: Tino Calancha; +Cc: 27634



> On 11 Jul 2017, at 2:48 pm, Tino Calancha <tino.calancha@gmail.com> wrote:
> 
> 
> 
>> On Tue, 11 Jul 2017, Paul Rankin wrote:
>> 
>> One overlooked thing about Tino's solution is that C-g is a keystroke and keyboard-quit is a function, which obviously aren't necessarily equivalent. What if the user remaps keyboard quit to "7"?
> I thought about that, but i discarded because i think binding something
> other that `keyboard-quit' to `C-g' is a misuse.  The Emacs manual is full
> of mentions to `C-g' as `keyboard-quit'.
> There is even the following remark in the tips section:
> 
> "don't bind a key sequence ending in @key{C-g}, since that
> is commonly used to cancel a key sequence."
> 
> If a user want to ignore such kind of advice he/she should
> not expect everything will work the same.

I'm gonna do this just to mess with you 😉

> Maybe we can fix this so that `register-read-with-preview'
> will work with `C-g' bound to `my-cool-foo-command'; but we
> cannot assure that no other Emacs part is affected because such
> misguided `C-g' binding.  We must encourage users to follow
> good practices.

While I think encouragement and enforcement are different things, the point about C-g is more what if the user *also* binds keyboard-quit to "7". In this case the user expects 7 to call keyboard-quit, not just C-g.

Also as Eli said ESC ESC is supposed to keyboard quit I think.

But main thing, as in life, better to look for what what you want than control for what you don't want.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
  2017-07-11  5:07           ` Paul Rankin
@ 2017-07-11  5:50             ` Tino Calancha
  2017-07-11  7:20               ` Andreas Schwab
  0 siblings, 1 reply; 18+ messages in thread
From: Tino Calancha @ 2017-07-11  5:50 UTC (permalink / raw
  To: Paul Rankin; +Cc: Tino Calancha, 27634

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



On Tue, 11 Jul 2017, Paul Rankin wrote:

>>>
>>> One overlooked thing about Tino's solution is that C-g is a keystroke and keyboard-quit is a function, which obviously aren't necessarily equivalent. What if the user remaps keyboard quit to "7"?
>> I thought about that, but i discarded because i think binding something
>> other that `keyboard-quit' to `C-g' is a misuse.  The Emacs manual is full
>> of mentions to `C-g' as `keyboard-quit'.
>> There is even the following remark in the tips section:
>>
>> "don't bind a key sequence ending in @key{C-g}, since that
>> is commonly used to cancel a key sequence."
>>
>> If a user want to ignore such kind of advice he/she should
>> not expect everything will work the same.
>
> I'm gonna do this just to mess with you 😉
Thank you.  Actually i feel quite boring now, so it's OK :-)

>> Maybe we can fix this so that `register-read-with-preview'
>> will work with `C-g' bound to `my-cool-foo-command'; but we
>> cannot assure that no other Emacs part is affected because such
>> misguided `C-g' binding.  We must encourage users to follow
>> good practices.
>
>the point about C-g is more what if the user *also* binds keyboard-quit 
>to "7". In this case the user expects 7 to call keyboard-quit, not just 
>C-g.
I see.  Good point!
Paul, what do you think about this?
--- a/lisp/register.el
+++ b/lisp/register.el
@@ -164,6 +164,9 @@ register-read-with-preview
  		       help-chars)
  	    (unless (get-buffer-window buffer)
  	      (register-preview buffer 'show-empty)))
+          (when (and (characterp last-input-event)
+                     (eq 'keyboard-quit (key-binding (string last-input-event))))
+            (keyboard-quit))
  	  (if (characterp last-input-event) last-input-event
  	    (error "Non-character input-event")))

I) Note, that my patch won't work in case our fearless user
    bind "7" to ...:
(lambda () (message "What the hell are you doing?")
 		      (keyboard-quit))

... But i don't think we must protect about things like I).

^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
  2017-07-11  5:50             ` Tino Calancha
@ 2017-07-11  7:20               ` Andreas Schwab
  0 siblings, 0 replies; 18+ messages in thread
From: Andreas Schwab @ 2017-07-11  7:20 UTC (permalink / raw
  To: Tino Calancha; +Cc: Paul Rankin, 27634

On Jul 11 2017, Tino Calancha <tino.calancha@gmail.com> wrote:

>> the point about C-g is more what if the user *also* binds keyboard-quit
>> to "7". In this case the user expects 7 to call keyboard-quit, not just
>> C-g.

For quit it is irrelevant which key is bound to keyboard-quit, only the
quit char it used.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
  2017-07-11  4:14       ` Paul Rankin
  2017-07-11  4:48         ` Tino Calancha
@ 2017-07-11 14:36         ` Eli Zaretskii
  2017-07-12  2:12           ` Paul Rankin
  1 sibling, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2017-07-11 14:36 UTC (permalink / raw
  To: Paul Rankin; +Cc: tino.calancha, 27634

> From: Paul Rankin <hello@paulwrankin.com>
> Cc: tino.calancha@gmail.com, 27634@debbugs.gnu.org
> Date: Tue, 11 Jul 2017 14:14:22 +1000
> 
> On Tue, 11 Jul 2017, at 03:06 AM, Eli Zaretskii wrote:
> > FWIW, I actually agree with Tino's solution, and was about to propose
> > something similar. It's true that control characters are not
> > alphanumeric, but we could fix the documentation to be more accurate
> > if we care about that.  OTOH, we've supported control characters as
> > register names for many years, and by now it should be quite clear it
> > didn't bother anyone yet.
> 
> Hmm, it bothers me?

OK, not "anyone", just "one". ;-)

> But the question is moot I think, since this is an opportunity to improve the code.. why waste time arguing for poorer code when we can make it better?

We are not arguing _whether_ to make it better, we are arguing _how_
to make it better.

> > > That prohibits anything except "a-zA-Z0-9", although users may want to save registers to "$" or "*".
> > 
> > Why would we want to be so strict when the only real problem is that
> > C-g doesn't quit?
> 
> I think there are two good options for good UX: make the code reflect the documentation (this is the strict option), or update both the documentation and the code to reflect what we believe is user expectation, i.e. that the user may save registers to any character key on their keyboard (this is my preference).

I believe the user expectations by now are that any character should
do.  We want to exempt C-g and ESC ESC in order to allow the user to
bail out, but other than that, I see no reason to add restrictions
where they aren't needed.  "Alphanumeric" these days means much more
than just ASCII, and we have no reasons I can see to restrict users to
ASCII, certainly not after so many years of the current behavior.

> One overlooked thing about Tino's solution is that C-g is a keystroke and keyboard-quit is a function, which obviously aren't necessarily equivalent. What if the user remaps keyboard quit to "7"?

As Andreas points out, only quit char is important.  If we want to be
holier than the Pope, we could look at its value by calling
current-input-method.  I don't object to such an extension.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
  2017-07-11 14:36         ` Eli Zaretskii
@ 2017-07-12  2:12           ` Paul Rankin
  0 siblings, 0 replies; 18+ messages in thread
From: Paul Rankin @ 2017-07-12  2:12 UTC (permalink / raw
  To: Eli Zaretskii; +Cc: tino.calancha, 27634

On Wed, 12 Jul 2017, at 12:36 AM, Eli Zaretskii wrote:
> I believe the user expectations by now are that any character should
> do.  We want to exempt C-g and ESC ESC in order to allow the user to
> bail out, but other than that, I see no reason to add restrictions
> where they aren't needed.  "Alphanumeric" these days means much more
> than just ASCII, and we have no reasons I can see to restrict users to
> ASCII, certainly not after so many years of the current behavior.
> 
> > One overlooked thing about Tino's solution is that C-g is a keystroke and keyboard-quit is a function, which obviously aren't necessarily equivalent. What if the user remaps keyboard quit to "7"?
> 
> As Andreas points out, only quit char is important.  If we want to be
> holier than the Pope, we could look at its value by calling
> current-input-method.  I don't object to such an extension.

Looks like you guys have this under control 👍





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
  2017-07-10  3:58 bug#27634: 25.2.1; C-g does not quit register-read-with-preview Paul Rankin
  2017-07-10  6:33 ` Tino Calancha
@ 2017-07-21  3:47 ` Allen Li
  2017-07-21  6:19   ` Tino Calancha
  1 sibling, 1 reply; 18+ messages in thread
From: Allen Li @ 2017-07-21  3:47 UTC (permalink / raw
  To: tino.calancha; +Cc: 27634

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

It sounds like there's a general consensus on how to fix this.  Tino, would
you care to post a finalized patch?

Note: bug#25370 is a duplicate of this, it should probably be marked
closed, wontfix, etc.

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
  2017-07-21  3:47 ` Allen Li
@ 2017-07-21  6:19   ` Tino Calancha
  2017-07-21  8:40     ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Tino Calancha @ 2017-07-21  6:19 UTC (permalink / raw
  To: Allen Li; +Cc: Andreas Schwab, Paul Rankin, 27634, Tino Calancha

Allen Li <vianchielfaura@gmail.com> writes:

> It sounds like there's a general consensus on how to fix this.  Tino, would you care to post a finalized patch?
OK.
> Note: bug#25370 is a duplicate of this, it should probably be marked closed, wontfix, etc.
Thanks, i merged this thread with your bug report.

Let's discuss following patch:

*) Call `keyboard-quit' whenever last-input-event is
    ?\C-g or 'escape or ?\C-\[.

**) Updated the manual to point out that non-alphanumeric
    keys are valid to store registers as well.

Let me know if that is OK or if we need a fine-grained control.

--8<-----------------------------cut here---------------start------------->8---
commit 484faa217fe5a76d12ce266bc3f84737da73f0ae
Author: Tino Calancha <tino.calancha@gmail.com>
Date:   Fri Jul 21 15:08:06 2017 +0900

    register-read-with-preview: Quit if user input C-g or ESC
    
    * lisp/register.el (register-read-with-preview):
    Quit if user input C-g or ESC (bug#27634).
    * doc/emacs/regs.texi (Registers): Update manual.

diff --git a/doc/emacs/regs.texi b/doc/emacs/regs.texi
index 7369f6b05b..40e3e2c1c3 100644
--- a/doc/emacs/regs.texi
+++ b/doc/emacs/regs.texi
@@ -15,7 +15,10 @@ Registers
   Each register has a name that consists of a single character, which
 we will denote by @var{r}; @var{r} can be a letter (such as @samp{a})
 or a number (such as @samp{1}); case matters, so register @samp{a} is
-not the same as register @samp{A}.
+not the same as register @samp{A}.  You can also set a register in
+non-alphanumeric characters, for instance @samp{*} or @samp{C-d}.
+Note, it's not possible to set a register in @samp{C-g} or @samp{ESC},
+because these keys are reserved to terminate interactive commands.
 
 @findex view-register
   A register can store a position, a piece of text, a rectangle, a
diff --git a/lisp/register.el b/lisp/register.el
index 7cc3ccd870..e395963f56 100644
--- a/lisp/register.el
+++ b/lisp/register.el
@@ -164,6 +164,10 @@ register-read-with-preview
 		       help-chars)
 	    (unless (get-buffer-window buffer)
 	      (register-preview buffer 'show-empty)))
+          (when (or (eq ?\C-g last-input-event)
+                    (eq 'escape last-input-event)
+                    (eq ?\C-\[ last-input-event))
+            (keyboard-quit))
 	  (if (characterp last-input-event) last-input-event
 	    (error "Non-character input-event")))
       (and (timerp timer) (cancel-timer timer))
--8<-----------------------------cut here---------------end--------------->8---
In GNU Emacs 26.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2017-07-21
Repository revision: 1d559e384b467b3f74e8b78695f124b561c884d9





^ permalink raw reply related	[flat|nested] 18+ messages in thread

* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
  2017-07-21  6:19   ` Tino Calancha
@ 2017-07-21  8:40     ` Eli Zaretskii
  2017-07-21  8:55       ` Tino Calancha
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2017-07-21  8:40 UTC (permalink / raw
  To: Tino Calancha; +Cc: vianchielfaura, hello, 27634, schwab

> From: Tino Calancha <tino.calancha@gmail.com>
> Cc: 27634@debbugs.gnu.org, Paul Rankin <hello@paulwrankin.com>, Andreas Schwab <schwab@suse.de>, Eli Zaretskii <eliz@gnu.org>, Tino Calancha <tino.calancha@gmail.com>
> Date: Fri, 21 Jul 2017 15:19:03 +0900
> 
> Allen Li <vianchielfaura@gmail.com> writes:
> 
> > It sounds like there's a general consensus on how to fix this.  Tino, would you care to post a finalized patch?
> OK.
> > Note: bug#25370 is a duplicate of this, it should probably be marked closed, wontfix, etc.
> Thanks, i merged this thread with your bug report.
> 
> Let's discuss following patch:
> 
> *) Call `keyboard-quit' whenever last-input-event is
>     ?\C-g or 'escape or ?\C-\[.
> 
> **) Updated the manual to point out that non-alphanumeric
>     keys are valid to store registers as well.

Looks good, thanks.  (I didn't test the code, but I assume you did.)





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
  2017-07-21  8:40     ` Eli Zaretskii
@ 2017-07-21  8:55       ` Tino Calancha
  2017-07-25  2:45         ` Tino Calancha
  0 siblings, 1 reply; 18+ messages in thread
From: Tino Calancha @ 2017-07-21  8:55 UTC (permalink / raw
  To: Eli Zaretskii; +Cc: vianchielfaura, hello, Tino Calancha, schwab, 27634



On Fri, 21 Jul 2017, Eli Zaretskii wrote:

>> Let's discuss following patch:
>>
>> *) Call `keyboard-quit' whenever last-input-event is
>>     ?\C-g or 'escape or ?\C-\[.
>>
>> **) Updated the manual to point out that non-alphanumeric
>>     keys are valid to store registers as well.
>
> Looks good, thanks.  (I didn't test the code, but I assume you did.)
Yes i tested it.
Thank you.  I will push it next week if i don't get more communications.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#27634: 25.2.1; C-g does not quit register-read-with-preview
  2017-07-21  8:55       ` Tino Calancha
@ 2017-07-25  2:45         ` Tino Calancha
  0 siblings, 0 replies; 18+ messages in thread
From: Tino Calancha @ 2017-07-25  2:45 UTC (permalink / raw
  To: 27634-done

Tino Calancha <tino.calancha@gmail.com> writes:

> On Fri, 21 Jul 2017, Eli Zaretskii wrote:
>
>> Looks good, thanks.  (I didn't test the code, but I assume you did.)
> Yes i tested it.
> Thank you.  I will push it next week if i don't get more communications.
Fixed in master branch as commit 35954cb92b8cd4ad093756d171688343bab02c2e





^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2017-07-25  2:45 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-10  3:58 bug#27634: 25.2.1; C-g does not quit register-read-with-preview Paul Rankin
2017-07-10  6:33 ` Tino Calancha
2017-07-10  7:20   ` Paul Rankin
2017-07-10  7:59     ` Tino Calancha
2017-07-10 17:06     ` Eli Zaretskii
2017-07-11  4:14       ` Paul Rankin
2017-07-11  4:48         ` Tino Calancha
2017-07-11  5:07           ` Paul Rankin
2017-07-11  5:50             ` Tino Calancha
2017-07-11  7:20               ` Andreas Schwab
2017-07-11 14:36         ` Eli Zaretskii
2017-07-12  2:12           ` Paul Rankin
2017-07-10 19:01   ` Andreas Schwab
2017-07-21  3:47 ` Allen Li
2017-07-21  6:19   ` Tino Calancha
2017-07-21  8:40     ` Eli Zaretskii
2017-07-21  8:55       ` Tino Calancha
2017-07-25  2:45         ` Tino Calancha

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.