unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#39353: [PATCH] Use current-prefix-arg instead of prefix-arg
@ 2020-01-30  3:35 Kyle Hubert
  2020-01-30  3:53 ` Kyle Hubert
  2020-01-31 14:29 ` bug#39353: [PATCH] Ediff fix for ediff-scroll-horizontally Kyle Hubert
  0 siblings, 2 replies; 7+ messages in thread
From: Kyle Hubert @ 2020-01-30  3:35 UTC (permalink / raw)
  To: 39353; +Cc: Kyle Hubert

Forwarding arguments through to children function invocations should
be handled by current-prefix-arg. This was first uncovered when ediff
used full window width scrolling in ediff-scroll-horizontally, when
the call to scroll-left or scroll-right should have been half-window
size. Through further grepping, there was another usage in
simple.el. Note, execute-extended-command was visually inspected that
it was in interactive mode.

---
 lisp/simple.el        | 2 +-
 lisp/vc/ediff-util.el | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 2ec3da680f..53afa7b138 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -1888,7 +1888,7 @@ invoking, give a prefix argument to `execute-extended-command'."
     ;; `function' and not `execute-extended-command'.  The difference is
     ;; visible in cases such as M-x <cmd> RET and then C-x z (bug#11506).
     (setq real-this-command function)
-    (let ((prefix-arg prefixarg))
+    (let ((current-prefix-arg prefixarg))
       (command-execute function 'record))
     ;; If enabled, show which key runs this command.
     ;; But first wait, and skip the message if there is input.
diff --git a/lisp/vc/ediff-util.el b/lisp/vc/ediff-util.el
index a8af9ba37a..5f8a4a86b1 100644
--- a/lisp/vc/ediff-util.el
+++ b/lisp/vc/ediff-util.el
@@ -1540,10 +1540,10 @@ the width of the A/B/C windows."
    ;; hscrolling.
    (if (= last-command-event ?<)
        (lambda (arg)
-	 (let ((prefix-arg arg))
+	 (let ((current-prefix-arg arg))
 	   (call-interactively #'scroll-left)))
      (lambda (arg)
-       (let ((prefix-arg arg))
+       (let ((current-prefix-arg arg))
 	 (call-interactively #'scroll-right))))
    ;; calculate argument to scroll-left/right
    ;; if there is an explicit argument
-- 
2.25.0






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

* bug#39353: [PATCH] Use current-prefix-arg instead of prefix-arg
  2020-01-30  3:35 bug#39353: [PATCH] Use current-prefix-arg instead of prefix-arg Kyle Hubert
@ 2020-01-30  3:53 ` Kyle Hubert
  2020-01-30 13:12   ` Noam Postavsky
  2020-01-31 14:29 ` bug#39353: [PATCH] Ediff fix for ediff-scroll-horizontally Kyle Hubert
  1 sibling, 1 reply; 7+ messages in thread
From: Kyle Hubert @ 2020-01-30  3:53 UTC (permalink / raw)
  To: 39353

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

I have a hard time testing the change to simple.el, as I don't understand
execute-extended-command. Can anyone help here? I'm worried since it isn't
using (interactive "P") that this is incorrect. I admit I'm deeper in the
guts of emacs than typical.

Thanks,
-Kyle


On Wed, Jan 29, 2020 at 10:35 PM Kyle Hubert <khubert@gmail.com> wrote:

> Forwarding arguments through to children function invocations should
> be handled by current-prefix-arg. This was first uncovered when ediff
> used full window width scrolling in ediff-scroll-horizontally, when
> the call to scroll-left or scroll-right should have been half-window
> size. Through further grepping, there was another usage in
> simple.el. Note, execute-extended-command was visually inspected that
> it was in interactive mode.
>
> ---
>  lisp/simple.el        | 2 +-
>  lisp/vc/ediff-util.el | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lisp/simple.el b/lisp/simple.el
> index 2ec3da680f..53afa7b138 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -1888,7 +1888,7 @@ invoking, give a prefix argument to
> `execute-extended-command'."
>      ;; `function' and not `execute-extended-command'.  The difference is
>      ;; visible in cases such as M-x <cmd> RET and then C-x z (bug#11506).
>      (setq real-this-command function)
> -    (let ((prefix-arg prefixarg))
> +    (let ((current-prefix-arg prefixarg))
>        (command-execute function 'record))
>      ;; If enabled, show which key runs this command.
>      ;; But first wait, and skip the message if there is input.
> diff --git a/lisp/vc/ediff-util.el b/lisp/vc/ediff-util.el
> index a8af9ba37a..5f8a4a86b1 100644
> --- a/lisp/vc/ediff-util.el
> +++ b/lisp/vc/ediff-util.el
> @@ -1540,10 +1540,10 @@ the width of the A/B/C windows."
>     ;; hscrolling.
>     (if (= last-command-event ?<)
>         (lambda (arg)
> -        (let ((prefix-arg arg))
> +        (let ((current-prefix-arg arg))
>            (call-interactively #'scroll-left)))
>       (lambda (arg)
> -       (let ((prefix-arg arg))
> +       (let ((current-prefix-arg arg))
>          (call-interactively #'scroll-right))))
>     ;; calculate argument to scroll-left/right
>     ;; if there is an explicit argument
> --
> 2.25.0
>
>

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

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

* bug#39353: [PATCH] Use current-prefix-arg instead of prefix-arg
  2020-01-30  3:53 ` Kyle Hubert
@ 2020-01-30 13:12   ` Noam Postavsky
  2020-01-30 16:35     ` Kyle Hubert
  0 siblings, 1 reply; 7+ messages in thread
From: Noam Postavsky @ 2020-01-30 13:12 UTC (permalink / raw)
  To: Kyle Hubert; +Cc: 39353

Kyle Hubert <khubert@gmail.com> writes:

> I have a hard time testing the change to simple.el, as I don't understand
> execute-extended-command. Can anyone help here? I'm worried since it isn't
> using (interactive "P") that this is incorrect. I admit I'm deeper in the
> guts of emacs than typical.

The simple.el let-binding is around command-execute, not
execute-extended-command.  command-execute does specifically read
prefix-arg, so I think that part of your patch should be dropped (I
haven't looked in detail at the ediff part, but it sounds right).

>> --- a/lisp/simple.el
>> +++ b/lisp/simple.el
>> @@ -1888,7 +1888,7 @@ invoking, give a prefix argument to
>> `execute-extended-command'."
>>      ;; `function' and not `execute-extended-command'.  The difference is
>>      ;; visible in cases such as M-x <cmd> RET and then C-x z (bug#11506).
>>      (setq real-this-command function)
>> -    (let ((prefix-arg prefixarg))
>> +    (let ((current-prefix-arg prefixarg))
>>        (command-execute function 'record))





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

* bug#39353: [PATCH] Use current-prefix-arg instead of prefix-arg
  2020-01-30 13:12   ` Noam Postavsky
@ 2020-01-30 16:35     ` Kyle Hubert
  2020-01-30 18:39       ` Eli Zaretskii
  0 siblings, 1 reply; 7+ messages in thread
From: Kyle Hubert @ 2020-01-30 16:35 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: 39353

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

Thank you for the review. I had a feeling that
execute-extended-command's calling of prefix-arg was the correct
usage for command-execute. If I drop that from the PATCH, do I
repost to this bug ID email? I'm unfamiliar with the project.

-Kyle

On Thu, Jan 30, 2020 at 8:12 AM Noam Postavsky <npostavs@gmail.com> wrote:

> Kyle Hubert <khubert@gmail.com> writes:
>
> > I have a hard time testing the change to simple.el, as I don't understand
> > execute-extended-command. Can anyone help here? I'm worried since it
> isn't
> > using (interactive "P") that this is incorrect. I admit I'm deeper in the
> > guts of emacs than typical.
>
> The simple.el let-binding is around command-execute, not
> execute-extended-command.  command-execute does specifically read
> prefix-arg, so I think that part of your patch should be dropped (I
> haven't looked in detail at the ediff part, but it sounds right).
>
> >> --- a/lisp/simple.el
> >> +++ b/lisp/simple.el
> >> @@ -1888,7 +1888,7 @@ invoking, give a prefix argument to
> >> `execute-extended-command'."
> >>      ;; `function' and not `execute-extended-command'.  The difference
> is
> >>      ;; visible in cases such as M-x <cmd> RET and then C-x z
> (bug#11506).
> >>      (setq real-this-command function)
> >> -    (let ((prefix-arg prefixarg))
> >> +    (let ((current-prefix-arg prefixarg))
> >>        (command-execute function 'record))
>

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

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

* bug#39353: [PATCH] Use current-prefix-arg instead of prefix-arg
  2020-01-30 16:35     ` Kyle Hubert
@ 2020-01-30 18:39       ` Eli Zaretskii
  0 siblings, 0 replies; 7+ messages in thread
From: Eli Zaretskii @ 2020-01-30 18:39 UTC (permalink / raw)
  To: Kyle Hubert; +Cc: 39353, npostavs

> From: Kyle Hubert <khubert@gmail.com>
> Date: Thu, 30 Jan 2020 11:35:01 -0500
> Cc: 39353@debbugs.gnu.org
> 
> If I drop that from the PATCH, do I repost to this bug ID email?

Yes, please.





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

* bug#39353: [PATCH] Ediff fix for ediff-scroll-horizontally
  2020-01-30  3:35 bug#39353: [PATCH] Use current-prefix-arg instead of prefix-arg Kyle Hubert
  2020-01-30  3:53 ` Kyle Hubert
@ 2020-01-31 14:29 ` Kyle Hubert
  2020-02-08 10:01   ` Eli Zaretskii
  1 sibling, 1 reply; 7+ messages in thread
From: Kyle Hubert @ 2020-01-31 14:29 UTC (permalink / raw)
  To: 39353; +Cc: Kyle Hubert

When ediff-scroll-horizontally interactively calls scroll-left and
scroll-right, current-prefix-arg should be used. This allows for the
case that when no args or '-' are passed to ediff-scroll-horizontally,
it will determine the default amount (half the window width or
half-again, respectively).
---
 lisp/vc/ediff-util.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/ediff-util.el b/lisp/vc/ediff-util.el
index a8af9ba37a..5f8a4a86b1 100644
--- a/lisp/vc/ediff-util.el
+++ b/lisp/vc/ediff-util.el
@@ -1540,10 +1540,10 @@ the width of the A/B/C windows."
    ;; hscrolling.
    (if (= last-command-event ?<)
        (lambda (arg)
-	 (let ((prefix-arg arg))
+	 (let ((current-prefix-arg arg))
 	   (call-interactively #'scroll-left)))
      (lambda (arg)
-       (let ((prefix-arg arg))
+       (let ((current-prefix-arg arg))
 	 (call-interactively #'scroll-right))))
    ;; calculate argument to scroll-left/right
    ;; if there is an explicit argument
-- 
2.25.0






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

* bug#39353: [PATCH] Ediff fix for ediff-scroll-horizontally
  2020-01-31 14:29 ` bug#39353: [PATCH] Ediff fix for ediff-scroll-horizontally Kyle Hubert
@ 2020-02-08 10:01   ` Eli Zaretskii
  0 siblings, 0 replies; 7+ messages in thread
From: Eli Zaretskii @ 2020-02-08 10:01 UTC (permalink / raw)
  To: Kyle Hubert; +Cc: 39353-done

> From: Kyle Hubert <khubert@gmail.com>
> Date: Fri, 31 Jan 2020 09:29:43 -0500
> Cc: Kyle Hubert <khubert@gmail.com>
> 
> When ediff-scroll-horizontally interactively calls scroll-left and
> scroll-right, current-prefix-arg should be used. This allows for the
> case that when no args or '-' are passed to ediff-scroll-horizontally,
> it will determine the default amount (half the window width or
> half-again, respectively).
> ---
>  lisp/vc/ediff-util.el | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks, pushed to the master branch.





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

end of thread, other threads:[~2020-02-08 10:01 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-01-30  3:35 bug#39353: [PATCH] Use current-prefix-arg instead of prefix-arg Kyle Hubert
2020-01-30  3:53 ` Kyle Hubert
2020-01-30 13:12   ` Noam Postavsky
2020-01-30 16:35     ` Kyle Hubert
2020-01-30 18:39       ` Eli Zaretskii
2020-01-31 14:29 ` bug#39353: [PATCH] Ediff fix for ediff-scroll-horizontally Kyle Hubert
2020-02-08 10:01   ` Eli Zaretskii

Code repositories for project(s) associated with this public inbox

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).