bug#27569: delete-selection on yank via popup-menu

bug#27569: delete-selection on yank via popup-menu

[Tak Kunihiro]

I want to yank via popup-menu with delete-selection-mode.
Problem is yanking via popup-menu does not delete region.

emacs -Q
(defun popup-edit-menu ()
 (interactive)
 (popup-menu menu-bar-edit-menu))
(define-key global-map [mouse-3] 'popup-edit-menu)
(delete-selection-mode 1)

1. kill something
2. select text (activate region) to be replaced
3. choose yank in popup-menu via mouse-3

Problem: Selected text by (2) is not deleted.

I barely see problem is that on delete-selection-pre-hook,
this-command is `popup-edit-menu' instead `yank'.

Is there a way to solve the problem?

bug#27569: delete-selection on yank via popup-menu

[npostavs]

Tak Kunihiro <tkk@misasa.okayama-u.ac.jp> writes:

> I want to yank via popup-menu with delete-selection-mode.
> Problem is yanking via popup-menu does not delete region.
>
> emacs -Q
> (defun popup-edit-menu ()
>  (interactive)
>  (popup-menu menu-bar-edit-menu))
> (define-key global-map [mouse-3] 'popup-edit-menu)
> (delete-selection-mode 1)
>
> 1. kill something
> 2. select text (activate region) to be replaced
> 3. choose yank in popup-menu via mouse-3
>
> Problem: Selected text by (2) is not deleted.
>
> I barely see problem is that on delete-selection-pre-hook,
> this-command is `popup-edit-menu' instead `yank'.
>
> Is there a way to solve the problem?

This seems to work, though I'm not sure if it's the right way.  Perhaps
`this-command' should be setq instead of let-bound?  Will running
pre-command-hook twice be a problem? etc...

--- i/lisp/menu-bar.el
+++ w/lisp/menu-bar.el
@@ -2360,6 +2360,8 @@ (defun popup-menu (menu &optional position prefix from-menu-bar)
       ;; `setup-specified-language-environment', for instance,
       ;; expects this to be set from a menu keymap.
       (setq last-command-event (car (last event)))
+      (let ((this-command cmd))
+        (run-hook 'pre-command-hook))
       ;; mouse-major-mode-menu was using `command-execute' instead.
       (call-interactively cmd))))

bug#27569: delete-selection on yank via popup-menu

[Tak Kunihiro]

>> I want to yank via popup-menu with delete-selection-mode.
>> Problem is yanking via popup-menu does not delete region.
>>
>> emacs -Q
>> (defun popup-edit-menu ()
>>  (interactive)
>>  (popup-menu menu-bar-edit-menu))
>> (define-key global-map [mouse-3] 'popup-edit-menu)
>> (delete-selection-mode 1)
>>
>> 1. kill something
>> 2. select text (activate region) to be replaced
>> 3. choose yank in popup-menu via mouse-3
>>
>> Problem: Selected text by (2) is not deleted.
>>
>> I barely see problem is that on delete-selection-pre-hook,
>> this-command is `popup-edit-menu' instead `yank'.
>>
>> Is there a way to solve the problem?
> 
> This seems to work, though I'm not sure if it's the right way.  Perhaps
> `this-command' should be setq instead of let-bound?  Will running
> pre-command-hook twice be a problem? etc...
> 
> --- i/lisp/menu-bar.el
> +++ w/lisp/menu-bar.el
> @@ -2360,6 +2360,8 @@ (defun popup-menu (menu &optional position prefix from-menu-bar)
>        ;; `setup-specified-language-environment', for instance,
>        ;; expects this to be set from a menu keymap.
>        (setq last-command-event (car (last event)))
> +      (let ((this-command cmd))
> +        (run-hooks 'pre-command-hook))
>        ;; mouse-major-mode-menu was using `command-execute' instead.
>        (call-interactively cmd))))

Although I cannot comment on what you worry, it works well for me.
Thank you for the fix.

bug#27569: delete-selection on yank via popup-menu

[Tak Kunihiro]

Could you push to the master?

bug#27569: delete-selection on yank via popup-menu

[npostavs]

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

tags 27569 + patch
quit

Tak Kunihiro <tkk@misasa.okayama-u.ac.jp> writes:

> Could you push to the master?

Okay, I'll wait a couple more days and then push the following (I think
doing setq on this-command makes more sense than let-binding after all).


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

From 52c16f83b6781c83676507c165e69aaad0a3b2fd Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Tue, 11 Jul 2017 21:09:10 -0400
Subject: [PATCH v2] Let delete-selection-mode work with popup-menu commands
 (Bug#27569)

* lisp/menu-bar.el (popup-menu): Run `pre-command-hook' with
`this-command' set to the selected command.
---
 lisp/menu-bar.el | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
index 9c7bcffbaa..107043b501 100644
--- a/lisp/menu-bar.el
+++ b/lisp/menu-bar.el
@@ -2360,6 +2360,10 @@ (defun popup-menu (menu &optional position prefix from-menu-bar)
       ;; `setup-specified-language-environment', for instance,
       ;; expects this to be set from a menu keymap.
       (setq last-command-event (car (last event)))
+      ;; Update `this-command' and run `pre-command-hook' so that
+      ;; things like `delete-selection-pre-hook' will work correctly.
+      (setq this-command cmd)
+      (run-hook 'pre-command-hook)
       ;; mouse-major-mode-menu was using `command-execute' instead.
       (call-interactively cmd))))
 
-- 
2.11.1

bug#27569: delete-selection on yank via popup-menu

[Tak Kunihiro]

The patch works well in my computer.  Is it time to push?

> tags 27569 + patch
> quit
>
> Tak Kunihiro <tkk@misasa.okayama-u.ac.jp> writes:
>
>> Could you push to the master?
>
> Okay, I'll wait a couple more days and then push the following (I think
> doing setq on this-command makes more sense than let-binding after all).
>
> * lisp/menu-bar.el (popup-menu): Run `pre-command-hook' with
> `this-command' set to the selected command.
> ---
>  lisp/menu-bar.el | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
> index 9c7bcffbaa..107043b501 100644
> --- a/lisp/menu-bar.el
> +++ b/lisp/menu-bar.el
> @@ -2360,6 +2360,10 @@ (defun popup-menu (menu &optional position prefix from-menu-bar)
>        ;; `setup-specified-language-environment', for instance,
>        ;; expects this to be set from a menu keymap.
>        (setq last-command-event (car (last event)))
> +      ;; Update `this-command' and run `pre-command-hook' so that
> +      ;; things like `delete-selection-pre-hook' will work correctly.
> +      (setq this-command cmd)
> +      (run-hook 'pre-command-hook)
>        ;; mouse-major-mode-menu was using `command-execute' instead.
>        (call-interactively cmd))))
>
> --
> 2.11.1

bug#27569: delete-selection on yank via popup-menu

[npostavs]

tags 27569 fixed
close 27569 26.1
quit

Tak Kunihiro <tkk@misasa.okayama-u.ac.jp> writes:

> The patch works well in my computer.  Is it time to push?

Yes, thanks for the reminder, pushed [1: a2ee81911b].

>> +      (run-hook 'pre-command-hook)

After fixing this to run-hookS, that is.

[1: a2ee81911b]: 2017-07-18 06:31:39 -0400
  Let delete-selection-mode work with popup-menu commands (Bug#27569)
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=a2ee81911bdf0f37b992989a9d36bb4d2ba14052

bug#27569: delete-selection on yank via popup-menu

[npostavs]

tags 27569 =
notfixed 27569 26.1
quit

npostavs@users.sourceforge.net writes:
>
> Yes, thanks for the reminder, pushed [1: a2ee81911b].

I've reverted this following discussion in emacs-devel.

https://lists.gnu.org/archive/html/emacs-devel/2017-07/msg00757.html
https://lists.gnu.org/archive/html/emacs-devel/2017-07/msg00840.html

[2: 37954f3916]: 2017-07-22 08:20:13 -0400
  Revert "Let delete-selection-mode work with popup-menu commands (Bug#27569)"
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=37954f39168e0dbfe3a82feb9b58fecfc5f1f318