bug#33975: [PATCH] inhibit read-only text properties in comint-interrupt-subjob

bug#33975: [PATCH] inhibit read-only text properties in comint-interrupt-subjob

[Alex Branham]

[-- Attachment #1.1: Type: text/plain, Size: 1266 bytes --]

Hi -

This patch inhibits read-only properties during comint-interrupt-subjob.
I ran across this while using ESS and (setq comint-prompt-read-only t).
There's a little more info (including a reproducible example) on ESS's
bugtracker.[1]

Thanks,
Alex

From 8e3885c5b9747987cacd3b17b9de29975e7691e3 Mon Sep 17 00:00:00 2001
From: Alex Branham <alex.branham@gmail.com>
Date: Fri, 4 Jan 2019 10:28:09 -0600
Subject: [PATCH] * lisp/comint.el (comint-interrupt-subjob): Inhibit read only

Otherwise with comint-prompt-read-only set to t users can be incapable
of interrupting running busy processes. See ESS's issue tracker for
details: https://github.com/emacs-ess/ESS/issues/792
---
 lisp/comint.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/comint.el b/lisp/comint.el
index 0a6aff2e73..2ed65c1c1c 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -2500,8 +2500,9 @@ comint-clear-buffer
 (defun comint-interrupt-subjob ()
   "Interrupt the current subjob."
   (interactive)
-  (comint-skip-input)
-  (interrupt-process nil comint-ptyp)
+  (let ((inhibit-read-only t))
+    (comint-skip-input)
+    (interrupt-process nil comint-ptyp))
   ;; (process-send-string nil "\n")
   )

--
2.19.2



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-lisp-comint.el-comint-interrupt-subjob-Inhibit-read-.patch --]
[-- Type: text/x-patch, Size: 1004 bytes --]

From 8e3885c5b9747987cacd3b17b9de29975e7691e3 Mon Sep 17 00:00:00 2001
From: Alex Branham <alex.branham@gmail.com>
Date: Fri, 4 Jan 2019 10:28:09 -0600
Subject: [PATCH] * lisp/comint.el (comint-interrupt-subjob): Inhibit read only

Otherwise with comint-prompt-read-only set to t users can be incapable
of interrupting running busy processes. See ESS's issue tracker for
details: https://github.com/emacs-ess/ESS/issues/792
---
 lisp/comint.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/comint.el b/lisp/comint.el
index 0a6aff2e73..2ed65c1c1c 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -2500,8 +2500,9 @@ comint-clear-buffer
 (defun comint-interrupt-subjob ()
   "Interrupt the current subjob."
   (interactive)
-  (comint-skip-input)
-  (interrupt-process nil comint-ptyp)
+  (let ((inhibit-read-only t))
+    (comint-skip-input)
+    (interrupt-process nil comint-ptyp))
   ;; (process-send-string nil "\n")
   )
 
-- 
2.19.2


[-- Attachment #1.3: Type: text/plain, Size: 61 bytes --]


Footnotes:
[1]  https://github.com/emacs-ess/ESS/issues/792

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

bug#33975: [PATCH] inhibit read-only text properties in comint-interrupt-subjob

[Alex Branham]

Hi all - 

Is this patch OK for Emacs? Or should we figure it out downstream?

Thanks,
Alex

On Fri 04 Jan 2019 at 10:35, Alex Branham <alex.branham@gmail.com> wrote:

> Hi -
>
> This patch inhibits read-only properties during comint-interrupt-subjob.
> I ran across this while using ESS and (setq comint-prompt-read-only t).
> There's a little more info (including a reproducible example) on ESS's
> bugtracker.[1]
>
> Thanks,
> Alex
>
> From 8e3885c5b9747987cacd3b17b9de29975e7691e3 Mon Sep 17 00:00:00 2001
> From: Alex Branham <alex.branham@gmail.com>
> Date: Fri, 4 Jan 2019 10:28:09 -0600
> Subject: [PATCH] * lisp/comint.el (comint-interrupt-subjob): Inhibit read only
>
> Otherwise with comint-prompt-read-only set to t users can be incapable
> of interrupting running busy processes. See ESS's issue tracker for
> details: https://github.com/emacs-ess/ESS/issues/792
> ---
>  lisp/comint.el | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/lisp/comint.el b/lisp/comint.el
> index 0a6aff2e73..2ed65c1c1c 100644
> --- a/lisp/comint.el
> +++ b/lisp/comint.el
> @@ -2500,8 +2500,9 @@ comint-clear-buffer
>  (defun comint-interrupt-subjob ()
>    "Interrupt the current subjob."
>    (interactive)
> -  (comint-skip-input)
> -  (interrupt-process nil comint-ptyp)
> +  (let ((inhibit-read-only t))
> +    (comint-skip-input)
> +    (interrupt-process nil comint-ptyp))
>    ;; (process-send-string nil "\n")
>    )
>
> --
> 2.19.2
>
>
> From 8e3885c5b9747987cacd3b17b9de29975e7691e3 Mon Sep 17 00:00:00 2001
> From: Alex Branham <alex.branham@gmail.com>
> Date: Fri, 4 Jan 2019 10:28:09 -0600
> Subject: [PATCH] * lisp/comint.el (comint-interrupt-subjob): Inhibit read only
>
> Otherwise with comint-prompt-read-only set to t users can be incapable
> of interrupting running busy processes. See ESS's issue tracker for
> details: https://github.com/emacs-ess/ESS/issues/792
> ---
>  lisp/comint.el | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/lisp/comint.el b/lisp/comint.el
> index 0a6aff2e73..2ed65c1c1c 100644
> --- a/lisp/comint.el
> +++ b/lisp/comint.el
> @@ -2500,8 +2500,9 @@ comint-clear-buffer
>  (defun comint-interrupt-subjob ()
>    "Interrupt the current subjob."
>    (interactive)
> -  (comint-skip-input)
> -  (interrupt-process nil comint-ptyp)
> +  (let ((inhibit-read-only t))
> +    (comint-skip-input)
> +    (interrupt-process nil comint-ptyp))
>    ;; (process-send-string nil "\n")
>    )

bug#33975: [PATCH] inhibit read-only text properties in comint-interrupt-subjob

[Eli Zaretskii]

> From: Alex Branham <alex.branham@gmail.com>
> Date: Thu, 21 Feb 2019 10:32:16 -0600
> 
> Is this patch OK for Emacs? Or should we figure it out downstream?

Can you explain how having comint-prompt-read-only interferes with
interrupt-process in this case?  I don't think I understand the
connection, and neither this bug report nor the ESS issue do, AFAICT.

Thanks.

bug#33975: [PATCH] inhibit read-only text properties in comint-interrupt-subjob

[Alex Branham]

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


On Fri 22 Feb 2019 at 02:37, Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Alex Branham <alex.branham@gmail.com>
>> Date: Thu, 21 Feb 2019 10:32:16 -0600
>>
>> Is this patch OK for Emacs? Or should we figure it out downstream?
>
> Can you explain how having comint-prompt-read-only interferes with
> interrupt-process in this case?  I don't think I understand the
> connection, and neither this bug report nor the ESS issue do, AFAICT.

Sure thing. `comint-interrupt-subjob' calls `comint-skip-input', which
tries to do this:

(insert "  " (key-description (this-command-keys)))

Which fails if the text at point is read only.

Thanks,
Alex

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

bug#33975: [PATCH] inhibit read-only text properties in comint-interrupt-subjob

[Eli Zaretskii]

> From: Alex Branham <alex.branham@gmail.com>
> Cc: 33975@debbugs.gnu.org
> Date: Fri, 22 Feb 2019 15:26:00 -0600
> 
> > Can you explain how having comint-prompt-read-only interferes with
> > interrupt-process in this case?  I don't think I understand the
> > connection, and neither this bug report nor the ESS issue do, AFAICT.
> 
> Sure thing. `comint-interrupt-subjob' calls `comint-skip-input', which
> tries to do this:
> 
> (insert "  " (key-description (this-command-keys)))
> 
> Which fails if the text at point is read only.

Then shouldn't the change be inside comint-skip-input instead?  I
mean, the same problem will happen also in all other callers of
comint-skip-input, no?

(I'm not sure I understand why that function inserts the description
of this-command-keys -- is that to insert "C-c C-c" into the buffer?
This is not in the doc string, perhaps we should add that.)

Thanks.

bug#33975: [PATCH] inhibit read-only text properties in comint-interrupt-subjob

[Alex Branham]

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


On Sat 23 Feb 2019 at 02:51, Eli Zaretskii <eliz@gnu.org> wrote:

> Then shouldn't the change be inside comint-skip-input instead?  I
> mean, the same problem will happen also in all other callers of
> comint-skip-input, no?

Probably, yes. Changed in the attached patch.

> (I'm not sure I understand why that function inserts the description
> of this-command-keys -- is that to insert "C-c C-c" into the buffer?
> This is not in the doc string, perhaps we should add that.)

The attached patch adds it. WDYT?

From 0eba90f40c4ffdd020b86ffa2e19815c0edfe2f0 Mon Sep 17 00:00:00 2001
From: Alex Branham <alex.branham@gmail.com>
Date: Sat, 23 Feb 2019 07:35:01 -0600
Subject: [PATCH] * lisp/comint.el (comint-skip-input): Set inhibit-read-only
 to t

Bug#33975
---
 lisp/comint.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/comint.el b/lisp/comint.el
index 0a6aff2e73..ab6297a709 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -2536,13 +2536,16 @@ Useful if you accidentally suspend the top-level process."
 
 (defun comint-skip-input ()
   "Skip all pending input, from last stuff output by interpreter to point.
-This means mark it as if it had been sent as input, without sending it."
+This means mark it as if it had been sent as input, without
+sending it.  The command keys used to trigger this command are
+inserted into the buffer."
   (let ((comint-input-sender 'ignore)
 	(comint-input-filter-functions nil))
     (comint-send-input t t))
   (end-of-line)
   (let ((pos (point))
-	(marker (process-mark (get-buffer-process (current-buffer)))))
+	(marker (process-mark (get-buffer-process (current-buffer))))
+        (inhibit-read-only t))
     (insert "  " (key-description (this-command-keys)))
     (if (= marker pos)
 	(set-marker marker (point)))))
-- 
2.19.2




[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-comint.el-comint-skip-input-Set-inhibit-read-on.patch --]
[-- Type: text/x-patch, Size: 1304 bytes --]

From 0eba90f40c4ffdd020b86ffa2e19815c0edfe2f0 Mon Sep 17 00:00:00 2001
From: Alex Branham <alex.branham@gmail.com>
Date: Sat, 23 Feb 2019 07:35:01 -0600
Subject: [PATCH] * lisp/comint.el (comint-skip-input): Set inhibit-read-only
 to t

Bug#33975
---
 lisp/comint.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/comint.el b/lisp/comint.el
index 0a6aff2e73..ab6297a709 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -2536,13 +2536,16 @@ Useful if you accidentally suspend the top-level process."
 
 (defun comint-skip-input ()
   "Skip all pending input, from last stuff output by interpreter to point.
-This means mark it as if it had been sent as input, without sending it."
+This means mark it as if it had been sent as input, without
+sending it.  The command keys used to trigger this command are
+inserted into the buffer."
   (let ((comint-input-sender 'ignore)
 	(comint-input-filter-functions nil))
     (comint-send-input t t))
   (end-of-line)
   (let ((pos (point))
-	(marker (process-mark (get-buffer-process (current-buffer)))))
+	(marker (process-mark (get-buffer-process (current-buffer))))
+        (inhibit-read-only t))
     (insert "  " (key-description (this-command-keys)))
     (if (= marker pos)
 	(set-marker marker (point)))))
-- 
2.19.2

bug#33975: [PATCH] inhibit read-only text properties in comint-interrupt-subjob

[Eli Zaretskii]

> From: Alex Branham <alex.branham@gmail.com>
> Cc: 33975@debbugs.gnu.org
> Date: Sat, 23 Feb 2019 07:36:43 -0600
> 
> > Then shouldn't the change be inside comint-skip-input instead?  I
> > mean, the same problem will happen also in all other callers of
> > comint-skip-input, no?
> 
> Probably, yes. Changed in the attached patch.
> 
> > (I'm not sure I understand why that function inserts the description
> > of this-command-keys -- is that to insert "C-c C-c" into the buffer?
> > This is not in the doc string, perhaps we should add that.)
> 
> The attached patch adds it. WDYT?
> 
>  (defun comint-skip-input ()
>    "Skip all pending input, from last stuff output by interpreter to point.
> -This means mark it as if it had been sent as input, without sending it."
> +This means mark it as if it had been sent as input, without
> +sending it.  The command keys used to trigger this command are
> +inserted into the buffer."

"this command" is inaccurate here, as comint-skip-input is not a
command.  I'd say "the command which called this function" instead.

Other than that, LGTM.  Thanks.

bug#33975: [PATCH] inhibit read-only text properties in comint-interrupt-subjob

[Alex Branham]

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


On Sat 23 Feb 2019 at 11:01, Eli Zaretskii <eliz@gnu.org> wrote:

>>  (defun comint-skip-input ()
>>    "Skip all pending input, from last stuff output by interpreter to point.
>> -This means mark it as if it had been sent as input, without sending it."
>> +This means mark it as if it had been sent as input, without
>> +sending it.  The command keys used to trigger this command are
>> +inserted into the buffer."
>
> "this command" is inaccurate here, as comint-skip-input is not a
> command.  I'd say "the command which called this function" instead.
>
> Other than that, LGTM.  Thanks.

Great, thanks I'll make that change and push to master later today or
tomorrow, unless I hear back otherwise.

Alex

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

bug#33975: [PATCH] inhibit read-only text properties in comint-interrupt-subjob

[Alex Branham]

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


On Sat 23 Feb 2019 at 11:01, Eli Zaretskii <eliz@gnu.org> wrote:

> "this command" is inaccurate here, as comint-skip-input is not a
> command.  I'd say "the command which called this function" instead.
>
> Other than that, LGTM.  Thanks.

Thanks, fixed and pushed to master as
6a3b1aaa066dac28355ca5d09550947250108950 to appear in Emacs 27

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]