bug#73499: 31.0.50; sql-interactive-mode problem with package caps-lock-mode

bug#73499: 31.0.50; sql-interactive-mode problem with package caps-lock-mode

[Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

0. Install the GNU ELPA package caps-lock.
1. emacs -Q
2. M-x package-initialize
3. Start an SQL inferior process, e.g. with `M-x sql-sqlite RET test RET'.
4. In the *SQL: SQLite* buffer type `M-x caps-lock-mode' and at sqlite>
   prompt type "select * from".
=> The typed text appears as this: "SELECT * FRoM", with a lowercase "o"
   instead of an uppercase "O".

This is because sql-interactive-mode-map binds both "o" and "O" to the
command `sql-magic-go', but caps-lock-mode changes case only if the last
key event either invoked `self-insert-command' or
`isearch-printing-char' or if the command it invoked remaps
self-insert-command, and since neither is the case here, no case change
occurs.  (You can of course enter "O" by typing S-o, but caps-lock-mode
is supposed to obviate the need to do that.)  This problem also happens
with PostgreSQL and presumably other SQL interpreters supported by
sql.el (but I have only tested SQLite and PostgreSQL).

I have found two ways to fix this problem.  One is simply to add
`sql-magic-go' to the list `caps-lock-commands'.  In caps-lock.el this
is a defvar with the value '(self-insert-command isearch-printing-char).
Instead of just changing this value in the source code, I think it would
be better to make the variable a defcustom, since many packages bind
letter keys to commands other than `self-insert-command' but may
nevertheless want to change the case of these letters when
caps-lock-mode is enabled.  So it would be up to the user to add
`sql-magic-go' to the value.  Here's a patch that allows this (I don't
have a local clone of the GNU ELPA repo, so it's just a plain diff):


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

diff -c /home/steve/.emacs.d/elpa/caps-lock-1.0/caps-lock.el_orig /home/steve/.emacs.d/elpa/caps-lock-1.0/caps-lock.el
*** /home/steve/.emacs.d/elpa/caps-lock-1.0/caps-lock.el_orig	2024-06-27 11:21:19.635674501 +0200
--- /home/steve/.emacs.d/elpa/caps-lock-1.0/caps-lock.el	2024-09-26 11:10:54.443573430 +0200
***************
*** 22,35 ****

  ;;; Code:

! (defvar caps-lock-commands
!   '(self-insert-command isearch-printing-char)
!   "List of commands that are subject to `caps-lock-mode'.")

  ;;;###autoload
  (define-minor-mode caps-lock-mode
    "Make self-inserting keys invert the capitalization."
!   :global t
    (if caps-lock-mode
        (add-hook 'pre-command-hook #'caps-lock--pch)
      (remove-hook 'pre-command-hook #'caps-lock--pch)))
--- 22,38 ----

  ;;; Code:

! (defcustom caps-lock-commands '(self-insert-command isearch-printing-char)
!   "List of commands that are subject to `caps-lock-mode'."
!   :type '(repeat (restricted-sexp :match-alternatives (commandp null)))
!   ;; FIXME: which group, or both or neither?  (Fix below too?)
!   ;; :group 'editing
!   :group 'convenience)

  ;;;###autoload
  (define-minor-mode caps-lock-mode
    "Make self-inserting keys invert the capitalization."
!   :global t :group 'convenience
    (if caps-lock-mode
        (add-hook 'pre-command-hook #'caps-lock--pch)
      (remove-hook 'pre-command-hook #'caps-lock--pch)))

Diff finished.  Thu Sep 26 12:05:47 2024

[-- Attachment #3: Type: text/plain, Size: 712 bytes --]


The alternative fix is to change the implemention of `sql-magic-go' so
it doesn't interfere with caps-lock-mode; this would absolve the user
from the responsability of customizing which commands trigger case
change, but requires more invasive changes to sql.el (at least I haven't
come up with a simpler change), specifically, (i) removing the bindings
of "o" and "O", (ii) making `sql-magic-go' a function instead of a
command, which checks if the last keyboard input was either of these
letters and if so, does what `sql-magic-go' is supposed to do, and (iii)
adding `sql-magic-go' to `post-self-insert-hook' if `sql-electric-stuff'
is customized to give the string "go" special handling.  Here's the
patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: sql-magic-go patch --]
[-- Type: text/x-patch, Size: 1834 bytes --]

diff --git a/lisp/progmodes/sql.el b/lisp/progmodes/sql.el
index 5273ba2bee1..f39eee1fb77 100644
--- a/lisp/progmodes/sql.el
+++ b/lisp/progmodes/sql.el
@@ -869,7 +869,13 @@ sql-electric-stuff
   :type '(choice (const :tag "Nothing" nil)
 		 (const :tag "The semicolon `;'" semicolon)
 		 (const :tag "The string `go' by itself" go))
-  :version "20.8")
+  :initialize #'custom-initialize-default
+  :set (lambda (symbol value)
+         (custom-set-default symbol value)
+         (if (eq value 'go)
+             (add-hook 'post-self-insert-hook 'sql-magic-go)
+           (remove-hook 'post-self-insert-hook 'sql-magic-go)))
+  :version "31.1")

 (defcustom sql-send-terminator nil
   "When non-nil, add a terminator to text sent to the SQL interpreter.
@@ -1359,8 +1365,6 @@ sql-interactive-mode-map
   :parent comint-mode-map
   "C-j"       #'sql-accumulate-and-indent
   "C-c C-w"   #'sql-copy-column
-  "O"         #'sql-magic-go
-  "o"         #'sql-magic-go
   ";"         #'sql-magic-semicolon
   "C-c C-l a" #'sql-list-all
   "C-c C-l t" #'sql-list-table)
@@ -3067,16 +3071,15 @@ sql-end-of-statement

 ;;; Small functions

-(defun sql-magic-go (arg)
+(defun sql-magic-go ()
   "Insert \"o\" and call `comint-send-input'.
 `sql-electric-stuff' must be the symbol `go'."
-  (interactive "P")
-  (self-insert-command (prefix-numeric-value arg))
-  (if (and (equal sql-electric-stuff 'go)
-	   (save-excursion
-	     (comint-bol nil)
-	     (looking-at "go\\b")))
-      (comint-send-input)))
+  (and (equal sql-electric-stuff 'go)
+       (or (eq last-command-event ?o) (eq last-command-event ?O))
+       (save-excursion
+	 (comint-bol nil)
+	 (looking-at "go\\b"))
+       (comint-send-input)))
 (put 'sql-magic-go 'delete-selection t)

 (defun sql-magic-semicolon (arg)

[-- Attachment #5: Type: text/plain, Size: 721 bytes --]



In GNU Emacs 31.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.43, cairo version 1.18.2) of 2024-09-22 built on strobelfssd
Repository revision: 3fb966dc6392e1908304a1b6fe481da9f670cfbb
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101013
System Description: Linux From Scratch r12.2-5-systemd

Configured using:
 'configure -C 'CFLAGS=-Og -g3' PKG_CONFIG_PATH=/opt/qt6/lib/pkgconfig'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
LCMS2 LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG
RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS TREE_SITTER
WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

bug#73499: 31.0.50; sql-interactive-mode problem with package caps-lock-mode

[Eli Zaretskii]

> Date: Thu, 26 Sep 2024 14:44:12 +0200
> From:  Stephen Berman via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> 0. Install the GNU ELPA package caps-lock.
> 1. emacs -Q
> 2. M-x package-initialize
> 3. Start an SQL inferior process, e.g. with `M-x sql-sqlite RET test RET'.
> 4. In the *SQL: SQLite* buffer type `M-x caps-lock-mode' and at sqlite>
>    prompt type "select * from".
> => The typed text appears as this: "SELECT * FRoM", with a lowercase "o"
>    instead of an uppercase "O".
> 
> This is because sql-interactive-mode-map binds both "o" and "O" to the
> command `sql-magic-go', but caps-lock-mode changes case only if the last
> key event either invoked `self-insert-command' or
> `isearch-printing-char' or if the command it invoked remaps
> self-insert-command, and since neither is the case here, no case change
> occurs.  (You can of course enter "O" by typing S-o, but caps-lock-mode
> is supposed to obviate the need to do that.)  This problem also happens
> with PostgreSQL and presumably other SQL interpreters supported by
> sql.el (but I have only tested SQLite and PostgreSQL).
> 
> I have found two ways to fix this problem.  One is simply to add
> `sql-magic-go' to the list `caps-lock-commands'.  In caps-lock.el this
> is a defvar with the value '(self-insert-command isearch-printing-char).
> Instead of just changing this value in the source code, I think it would
> be better to make the variable a defcustom, since many packages bind
> letter keys to commands other than `self-insert-command' but may
> nevertheless want to change the case of these letters when
> caps-lock-mode is enabled.  So it would be up to the user to add
> `sql-magic-go' to the value.  Here's a patch that allows this (I don't
> have a local clone of the GNU ELPA repo, so it's just a plain diff):
> 
> 
> The alternative fix is to change the implemention of `sql-magic-go' so
> it doesn't interfere with caps-lock-mode; this would absolve the user
> from the responsability of customizing which commands trigger case
> change, but requires more invasive changes to sql.el (at least I haven't
> come up with a simpler change), specifically, (i) removing the bindings
> of "o" and "O", (ii) making `sql-magic-go' a function instead of a
> command, which checks if the last keyboard input was either of these
> letters and if so, does what `sql-magic-go' is supposed to do, and (iii)
> adding `sql-magic-go' to `post-self-insert-hook' if `sql-electric-stuff'
> is customized to give the string "go" special handling.  Here's the
> patch:

Stefan, Andrea, Stefan: any opinions on which way is better?

Thanks.

bug#73499: 31.0.50; sql-interactive-mode problem with package caps-lock-mode

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Stefan, Andrea, Stefan: any opinions on which way is better?

My vote is clearly for `post-self-insert-hook`.
I added this hook specifically to solve these kinds of problems.


        Stefan

bug#73499: 31.0.50; sql-interactive-mode problem with package caps-lock-mode

[Stefan Kangas]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Stefan, Andrea, Stefan: any opinions on which way is better?
>
> My vote is clearly for `post-self-insert-hook`.

+1, it seems like the cleaner option, and will fix this not only for
`caps-lock-mode' but other similar modes as well.

bug#73499: 31.0.50; sql-interactive-mode problem with package caps-lock-mode

[Andrea Corallo]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Stefan, Andrea, Stefan: any opinions on which way is better?
>
> My vote is clearly for `post-self-insert-hook`.

Same

  Andrea

bug#73499: 31.0.50; sql-interactive-mode problem with package caps-lock-mode

[Eli Zaretskii]

> From: Andrea Corallo <acorallo@gnu.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Stephen Berman <stephen.berman@gmx.net>,
>   Stefan Kangas <stefankangas@gmail.com>,  73499@debbugs.gnu.org
> Date: Tue, 01 Oct 2024 14:44:13 -0400
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> >> Stefan, Andrea, Stefan: any opinions on which way is better?
> >
> > My vote is clearly for `post-self-insert-hook`.
> 
> Same

OK, so Stephen, please do it that way, and thanks.

bug#73499: 31.0.50; sql-interactive-mode problem with package caps-lock-mode

[Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

On Sat, 05 Oct 2024 13:22:02 +0300 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Andrea Corallo <acorallo@gnu.org>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  Stephen Berman <stephen.berman@gmx.net>,
>>   Stefan Kangas <stefankangas@gmail.com>,  73499@debbugs.gnu.org
>> Date: Tue, 01 Oct 2024 14:44:13 -0400
>>
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>
>> >> Stefan, Andrea, Stefan: any opinions on which way is better?
>> >
>> > My vote is clearly for `post-self-insert-hook`.
>>
>> Same
>
> OK, so Stephen, please do it that way, and thanks.

Sure; however, my testing of the patch before posting it was inadequate,
and the patch needs to be amended.  As is it now, if you customize
sql-electric-stuff to use sql-magic-go and then type "go" at the prompt
in a comint-derived mode other than sql-interactive-mode, that wrongly
calls comint-send-input.  E.g. in shell-mode, this results in the shell
output "bash: go: command not found".  And in an arbitary buffer not
derived from comint-mode, typing "go" at BOB causes a ding and shows the
message "Current buffer has no process".  These problems are because the
patch adds sql-magic-go to post-self-insert-hook globally; since this is
done when customizing sql-electric-stuff, the current buffer need not
(and probably won't) be in sql-interactive-mode, so the hook can't be
added locally.  AFAICS the simplest fix is to check (eq major-mode
'sql-interactive-mode) in sql-magic-go, as in the attached patch.  If
there's no objection to this, then I'll go ahead and commit the amended
patch to master.

Steve Berman


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

diff --git a/lisp/progmodes/sql.el b/lisp/progmodes/sql.el
index a0b350ce54f..13bf5e874b0 100644
--- a/lisp/progmodes/sql.el
+++ b/lisp/progmodes/sql.el
@@ -869,7 +869,13 @@ sql-electric-stuff
   :type '(choice (const :tag "Nothing" nil)
 		 (const :tag "The semicolon `;'" semicolon)
 		 (const :tag "The string `go' by itself" go))
-  :version "20.8")
+  :initialize #'custom-initialize-default
+  :set (lambda (symbol value)
+         (custom-set-default symbol value)
+         (if (eq value 'go)
+             (add-hook 'post-self-insert-hook 'sql-magic-go)
+           (remove-hook 'post-self-insert-hook 'sql-magic-go)))
+  :version "31.1")

 (defcustom sql-send-terminator nil
   "When non-nil, add a terminator to text sent to the SQL interpreter.
@@ -1359,8 +1365,6 @@ sql-interactive-mode-map
   :parent comint-mode-map
   "C-j"       #'sql-accumulate-and-indent
   "C-c C-w"   #'sql-copy-column
-  "O"         #'sql-magic-go
-  "o"         #'sql-magic-go
   ";"         #'sql-magic-semicolon
   "C-c C-l a" #'sql-list-all
   "C-c C-l t" #'sql-list-table)
@@ -3067,16 +3071,16 @@ sql-end-of-statement

 ;;; Small functions

-(defun sql-magic-go (arg)
+(defun sql-magic-go ()
   "Insert \"o\" and call `comint-send-input'.
 `sql-electric-stuff' must be the symbol `go'."
-  (interactive "P")
-  (self-insert-command (prefix-numeric-value arg))
-  (if (and (equal sql-electric-stuff 'go)
-	   (save-excursion
-	     (comint-bol nil)
-	     (looking-at "go\\b")))
-      (comint-send-input)))
+  (and (eq major-mode 'sql-interactive-mode)
+       (equal sql-electric-stuff 'go)
+       (or (eq last-command-event ?o) (eq last-command-event ?O))
+       (save-excursion
+	 (comint-bol nil)
+	 (looking-at "go\\b"))
+       (comint-send-input)))
 (put 'sql-magic-go 'delete-selection t)

 (defun sql-magic-semicolon (arg)

bug#73499: 31.0.50; sql-interactive-mode problem with package caps-lock-mode

[Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Sat, 05 Oct 2024 22:03:59 +0200 Stephen Berman <stephen.berman@gmx.net> wrote:

> On Sat, 05 Oct 2024 13:22:02 +0300 Eli Zaretskii <eliz@gnu.org> wrote:
>
>>> From: Andrea Corallo <acorallo@gnu.org>
>>> Cc: Eli Zaretskii <eliz@gnu.org>,  Stephen Berman <stephen.berman@gmx.net>,
>>>   Stefan Kangas <stefankangas@gmail.com>,  73499@debbugs.gnu.org
>>> Date: Tue, 01 Oct 2024 14:44:13 -0400
>>>
>>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>>
>>> >> Stefan, Andrea, Stefan: any opinions on which way is better?
>>> >
>>> > My vote is clearly for `post-self-insert-hook`.
>>>
>>> Same
>>
>> OK, so Stephen, please do it that way, and thanks.
>
> Sure; however, my testing of the patch before posting it was inadequate,
> and the patch needs to be amended.  As is it now, if you customize
> sql-electric-stuff to use sql-magic-go and then type "go" at the prompt
> in a comint-derived mode other than sql-interactive-mode, that wrongly
> calls comint-send-input.  E.g. in shell-mode, this results in the shell
> output "bash: go: command not found".  And in an arbitary buffer not
> derived from comint-mode, typing "go" at BOB causes a ding and shows the
> message "Current buffer has no process".  These problems are because the
> patch adds sql-magic-go to post-self-insert-hook globally; since this is
> done when customizing sql-electric-stuff, the current buffer need not
> (and probably won't) be in sql-interactive-mode, so the hook can't be
> added locally.  AFAICS the simplest fix is to check (eq major-mode
> 'sql-interactive-mode) in sql-magic-go, as in the attached patch.  If
> there's no objection to this, then I'll go ahead and commit the amended
> patch to master.

It's been over a week with no objection, so I pushed the amended patch
to master as commit da048c69270 and am closing the bug.

Steve Berman