bug#62250: 29.0.60; Allow context menu from text properties to not override everything

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Augusto Stoffel]

It would be nice if the context menu defined by a text property had the
option of adding to instead of overriding the menu specified by
`context-menu-functions'.

I think this can be done in a backwards-compatible way as follows:

--8<---------------cut here---------------start------------->8---
(defun context-menu-map (&optional click)
  "Return menu map constructed for context near mouse CLICK.
The menu is populated by calling functions from
`context-menu-functions'.  Each function receives the menu and the mouse
click event and returns the same menu after adding own menu items to the
composite menu.  Alternatively, it can return a completely new menu.  In
this case, no further functions from `context-menu-functions' are
called.

When there is a text property `context-menu-function' at CLICK, it is
handled as if it were the first entry of `context-menu-functions'.

At the end, it's possible to modify the final menu by specifying the
function `context-menu-filter-function'."
--8<---------------cut here---------------end--------------->8---

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Eli Zaretskii]

> Cc: Juri Linkov <juri@linkov.net>
> From: Augusto Stoffel <arstoffel@gmail.com>
> Date: Sat, 18 Mar 2023 08:51:21 +0100
> 
> It would be nice if the context menu defined by a text property had the
> option of adding to instead of overriding the menu specified by
> `context-menu-functions'.
> 
> I think this can be done in a backwards-compatible way as follows:
> 
> --8<---------------cut here---------------start------------->8---
> (defun context-menu-map (&optional click)
>   "Return menu map constructed for context near mouse CLICK.
> The menu is populated by calling functions from
> `context-menu-functions'.  Each function receives the menu and the mouse
> click event and returns the same menu after adding own menu items to the
> composite menu.  Alternatively, it can return a completely new menu.  In
> this case, no further functions from `context-menu-functions' are
> called.
> 
> When there is a text property `context-menu-function' at CLICK, it is
> handled as if it were the first entry of `context-menu-functions'.
> 
> At the end, it's possible to modify the final menu by specifying the
> function `context-menu-filter-function'."
> --8<---------------cut here---------------end--------------->8---

What if a Lisp program _does_ want to override the menu specified by
context-menu-functions? or what if it wants the menu specified by the
text property to be the _last_ entry, not the first?

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Augusto Stoffel]

On Sat, 18 Mar 2023 at 10:31, Eli Zaretskii wrote:

>> Alternatively, it can return a completely new menu.  In
>> this case, no further functions from `context-menu-functions' are
>> called.

> What if a Lisp program _does_ want to override the menu specified by
> context-menu-functions? or what if it wants the menu specified by the
> text property to be the _last_ entry, not the first?

It's explained in the bit I kept: you ignore the MENU argument and
return a totally new keymap / not `eq'.  Adding things at the beginning
or end of a menu is an option that easy-menu-add-item etc already offer,
right?

Note that currently a member of context-menu-functions cannot override
the menu, and a context-menu-function cannot _not_ override it.  My
suggestions allows all possibilities.

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Cc: 62250@debbugs.gnu.org,  juri@linkov.net
> Date: Sat, 18 Mar 2023 12:19:56 +0100
> 
> On Sat, 18 Mar 2023 at 10:31, Eli Zaretskii wrote:
> 
> >> Alternatively, it can return a completely new menu.  In
> >> this case, no further functions from `context-menu-functions' are
> >> called.
> 
> > What if a Lisp program _does_ want to override the menu specified by
> > context-menu-functions? or what if it wants the menu specified by the
> > text property to be the _last_ entry, not the first?
> 
> It's explained in the bit I kept: you ignore the MENU argument and
> return a totally new keymap / not `eq'.

What MENU argument?  I see only a CLICK argument.

> Adding things at the beginning or end of a menu is an option that
> easy-menu-add-item etc already offer, right?

I admit I don't see how easy-menu-add-item is related to this.

> Note that currently a member of context-menu-functions cannot override
> the menu, and a context-menu-function cannot _not_ override it.  My
> suggestions allows all possibilities.

That part I did understand, but it looked to me that your proposal
stops short of covering all the possible situations.

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Augusto Stoffel]

On Sat, 18 Mar 2023 at 13:24, Eli Zaretskii wrote:

> What MENU argument?  I see only a CLICK argument.

--8<---------------cut here---------------start------------->8---
(defun context-menu-map (&optional click)
  "Return menu map constructed for context near mouse CLICK.
The menu is populated by calling functions from `context-menu-functions'.
Each function receives the menu and the mouse click event
                           ^^^^
and returns the same menu after adding own menu items to the composite menu.
When there is a text property `context-menu-function' at CLICK,
it overrides all functions from `context-menu-functions'.
At the end, it's possible to modify the final menu by specifying
the function `context-menu-filter-function'."
--8<---------------cut here---------------end--------------->8---

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Cc: 62250@debbugs.gnu.org,  juri@linkov.net
> Date: Sat, 18 Mar 2023 12:29:19 +0100
> 
> On Sat, 18 Mar 2023 at 13:24, Eli Zaretskii wrote:
> 
> > What MENU argument?  I see only a CLICK argument.
> 
> --8<---------------cut here---------------start------------->8---
> (defun context-menu-map (&optional click)
>   "Return menu map constructed for context near mouse CLICK.
> The menu is populated by calling functions from `context-menu-functions'.
> Each function receives the menu and the mouse click event
>                            ^^^^
> and returns the same menu after adding own menu items to the composite menu.
> When there is a text property `context-menu-function' at CLICK,
> it overrides all functions from `context-menu-functions'.
> At the end, it's possible to modify the final menu by specifying
> the function `context-menu-filter-function'."
> --8<---------------cut here---------------end--------------->8---

Now I'm completely confused regarding your proposal.

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Juri Linkov]

> It would be nice if the context menu defined by a text property had the
> option of adding to instead of overriding the menu specified by
> `context-menu-functions'.
>
> I think this can be done in a backwards-compatible way as follows:
>
> (defun context-menu-map (&optional click)
> ...
> When there is a text property `context-menu-function' at CLICK, it is
> handled as if it were the first entry of `context-menu-functions'.

I already envisioned such request, so the text property is named
`context-menu-function' (singular).  As an improvement we could add
another text property `context-menu-functions' (plural) that will
play more nicely with the variable `context-menu-functions'.
For example, we could append the list from the text property
to the value of the variable before running `run-hook-wrapped'
on the composite list.

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Juri Linkov]

> Note that currently a member of context-menu-functions cannot override
> the menu,

Actually, a member of context-menu-functions can override the
previous menu it receives as an argument.  And the last member
always wins by overriding the whole menu.  So it's just a question
of ordering the members of context-menu-functions.

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Augusto Stoffel]

On Sat, 18 Mar 2023 at 20:08, Juri Linkov wrote:

> I already envisioned such request

All right.  Just for the record, this would be useful to create a
context menu for images.  Also, I could take advantage of it to slightly
simplify the context menu of jit-spell.

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Augusto Stoffel]

On Sat, 18 Mar 2023 at 20:10, Juri Linkov wrote:

>> Note that currently a member of context-menu-functions cannot override
>> the menu,
>
> Actually, a member of context-menu-functions can override the
> previous menu it receives as an argument.  And the last member
> always wins by overriding the whole menu.  So it's just a question
> of ordering the members of context-menu-functions.

Right.  What I suggested would make the overriding independent of the
ordering of the members, which also (typically) influences the order of
the menu items.  But maybe overriding the menu is not such a common
use-case anyway, and either mechanism is sufficient.

The really interesting feature would be to be able to add to the context
menu with a text property.

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Juri Linkov]

>> Actually, a member of context-menu-functions can override the
>> previous menu it receives as an argument.  And the last member
>> always wins by overriding the whole menu.  So it's just a question
>> of ordering the members of context-menu-functions.
>
> Right.  What I suggested would make the overriding independent of the
> ordering of the members, which also (typically) influences the order of
> the menu items.  But maybe overriding the menu is not such a common
> use-case anyway, and either mechanism is sufficient.

What I still don't understand is why a function in the middle of
context-menu-functions should be able to say "I'm the boss" and
override everything that comes later.  Maybe this could be
implemented as well if needed, but the current implementation already
allows adding such a function to the end by specifying quite a large
value for the argument DEPTH of `add-hook'.

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Juri Linkov]

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

>> I already envisioned such request
>
> All right.  Just for the record, this would be useful to create a
> context menu for images.

The patch below will allow using the text property like

  'context-menu-functions '(image-context-menu)

> Also, I could take advantage of it to slightly
> simplify the context menu of jit-spell.

Actually, flyspell was the reason why the text property
`context-menu-function' overrides everything:

1. `make-flyspell-overlay' puts an overlay property

   (overlay-put overlay 'context-menu-function 'flyspell-context-menu)

2. `flyspell-context-menu' returns `flyspell-correct-word'

3. `context-menu-map' should return this symbol unmodified

4. at the end, the symbol `flyspell-correct-word' is executed
   as a command from the menu binding in `context-menu-entry'
   or from `context-menu-open':

    (if (commandp map)
        (call-interactively map)


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

diff --git a/lisp/mouse.el b/lisp/mouse.el
index 6b9accd6758..2cd2840bad9 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -368,9 +368,10 @@ context-menu-map
 the function `context-menu-filter-function'."
   (let* ((menu (make-sparse-keymap (propertize "Context Menu" 'hide t)))
          (click (or click last-input-event))
-         (window (posn-window (event-start click)))
-         (fun (mouse-posn-property (event-start click)
-                                   'context-menu-function)))
+         (start (event-start click))
+         (window (posn-window start))
+         (fun (mouse-posn-property start 'context-menu-function))
+         (funs (mouse-posn-property start 'context-menu-functions)))
 
     (unless (eq (selected-window) window)
       (select-window window))
@@ -380,7 +381,9 @@ context-menu-map
       (run-hook-wrapped 'context-menu-functions
                         (lambda (fun)
                           (setq menu (funcall fun menu click))
-                          nil)))
+                          nil))
+      (dolist (fun funs)
+        (setq menu (funcall fun menu click))))
 
     ;; Remove duplicate separators as well as ones at the beginning or
     ;; end of the menu.

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Augusto Stoffel]

On Sun, 19 Mar 2023 at 19:40, Juri Linkov wrote:

> What I still don't understand is why a function in the middle of
> context-menu-functions should be able to say "I'm the boss" and
> override everything that comes later.

Right, I actually have the same question: why should the singular
context-menu-function override everything else?  Flyspell could just put
the suggestion list inside a submenu.  It would be more usable.

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Augusto Stoffel]

On Sun, 19 Mar 2023 at 19:43, Juri Linkov wrote:

> The patch below will allow using the text property like
>
>   'context-menu-functions '(image-context-menu)

I didn't test the patch (I don't understand click events very well), but
does it find the context-menu-functions property of overlays as well?
And would those override the context-menu-functions text property or get
merged?

>        (run-hook-wrapped 'context-menu-functions
>                          (lambda (fun)
>                            (setq menu (funcall fun menu click))
> -                          nil)))
> +                          nil))
> +      (dolist (fun funs)
> +        (setq menu (funcall fun menu click))))
>  
>      ;; Remove duplicate separators as well as ones at the beginning or
>      ;; end of the menu.

I believe this makes the menu entries coming from a text property appear
after the regular hook ones, while I would expect the opposite (stuff
specific to the point is more "urgent").

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Juri Linkov]

> I actually have the same question: why should the singular
> context-menu-function override everything else?  Flyspell could just put
> the suggestion list inside a submenu.  It would be more usable.

The problem is that flyspell requires too much refactoring to
be able to put the suggestion list inside a submenu.  No one
volunteered to undertake such an endeavor.

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Juri Linkov]

>> The patch below will allow using the text property like
>>
>>   'context-menu-functions '(image-context-menu)
>
> I didn't test the patch (I don't understand click events very well), but
> does it find the context-menu-functions property of overlays as well?

It should be able to find the property in overlays as well.

> And would those override the context-menu-functions text property or get
> merged?

This needs confirming, but I guess overlays take priority over text properties.

>>        (run-hook-wrapped 'context-menu-functions
>>                          (lambda (fun)
>>                            (setq menu (funcall fun menu click))
>> -                          nil)))
>> +                          nil))
>> +      (dolist (fun funs)
>> +        (setq menu (funcall fun menu click))))
>>  
>>      ;; Remove duplicate separators as well as ones at the beginning or
>>      ;; end of the menu.
>
> I believe this makes the menu entries coming from a text property appear
> after the regular hook ones, while I would expect the opposite (stuff
> specific to the point is more "urgent").

When text properties are processed later, they have a chance to decide
where they add their menu items: at the top of the context menu, at the
bottom, or anywhere in the middle.

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Juri Linkov]

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

>> Just for the record, this would be useful to create a
>> context menu for images.
>
> The patch below will allow using the text property like
>
>   'context-menu-functions '(image-context-menu)

Here is a complete patch that uses the new text property
for create a context menu for images:


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

diff --git a/lisp/iimage.el b/lisp/iimage.el
index b4c175a7b63..8235c4a6fdb 100644
--- a/lisp/iimage.el
+++ b/lisp/iimage.el
@@ -134,6 +134,7 @@ iimage-mode-buffer
                                     :max-width (- (nth 2 edges) (nth 0 edges))
 				    :max-height (- (nth 3 edges) (nth 1 edges)))
                      keymap ,image-map
+                     context-menu-functions (image-context-menu)
                      modification-hooks
                      (iimage-modification-hook)))
                 (remove-list-of-text-properties
diff --git a/lisp/image.el b/lisp/image.el
index 2372fd1ce09..11bf46c80cc 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -188,6 +188,29 @@ image-map
   "C-<wheel-up>"   #'image-mouse-increase-size
   "C-<mouse-4>"    #'image-mouse-increase-size)
 
+(defun image-context-menu (menu click)
+  "Populate MENU with image-related commands at CLICK."
+  (when (mouse-posn-property (event-start click) 'display)
+    (define-key menu [image-separator] menu-bar-separator)
+    (let ((easy-menu (make-sparse-keymap "Image")))
+      (easy-menu-define nil easy-menu nil
+        '("Image"
+          ["Zoom In" image-increase-size
+           :help "Enlarge the image"]
+          ["Zoom Out" image-decrease-size
+           :help "Shrink the image"]
+          ["Rotate Clockwise" image-rotate
+           :help "Rotate the image"]
+          ["Flip horizontally" image-flip-horizontally
+           :help "Flip horizontally"]
+          ["Flip vertically" image-flip-vertically
+           :help "Flip vertically"]))
+      (dolist (item (reverse (lookup-key easy-menu [menu-bar image])))
+        (when (consp item)
+          (define-key menu (vector (car item)) (cdr item))))))
+
+  menu)
+
 (defun image-load-path-for-library (library image &optional path no-error)
   "Return a suitable search path for images used by LIBRARY.
 
@@ -615,6 +638,7 @@ put-image
       (overlay-put overlay 'put-image t)
       (overlay-put overlay 'before-string string)
       (overlay-put overlay 'keymap image-map)
+      (overlay-put overlay 'context-menu-functions '(image-context-menu))
       overlay)))
 
 
@@ -665,7 +689,9 @@ insert-image
 				      image)
                                    rear-nonsticky t
 				   inhibit-isearch ,inhibit-isearch
-                                   keymap ,image-map))))
+                                   keymap ,image-map
+                                   context-menu-functions
+                                   (image-context-menu)))))
 
 
 ;;;###autoload
@@ -702,7 +728,9 @@ insert-sliced-image
 	  (add-text-properties start (point)
 			       `(display ,(list (list 'slice x y dx dy) image)
 					 rear-nonsticky (display)
-                                         keymap ,image-map))
+                                         keymap ,image-map
+                                         context-menu-functions
+                                         (image-context-menu)))
 	  (setq x (+ x dx))))
       (setq x 0.0
 	    y (+ y dy))
diff --git a/lisp/mouse.el b/lisp/mouse.el
index 6b9accd6758..2cd2840bad9 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -368,9 +368,10 @@ context-menu-map
 the function `context-menu-filter-function'."
   (let* ((menu (make-sparse-keymap (propertize "Context Menu" 'hide t)))
          (click (or click last-input-event))
-         (window (posn-window (event-start click)))
-         (fun (mouse-posn-property (event-start click)
-                                   'context-menu-function)))
+         (start (event-start click))
+         (window (posn-window start))
+         (fun (mouse-posn-property start 'context-menu-function))
+         (funs (mouse-posn-property start 'context-menu-functions)))
 
     (unless (eq (selected-window) window)
       (select-window window))
@@ -380,7 +381,9 @@ context-menu-map
       (run-hook-wrapped 'context-menu-functions
                         (lambda (fun)
                           (setq menu (funcall fun menu click))
-                          nil)))
+                          nil))
+      (dolist (fun funs)
+        (setq menu (funcall fun menu click))))
 
     ;; Remove duplicate separators as well as ones at the beginning or
     ;; end of the menu.

bug#62250: 29.0.60; Allow context menu from text properties to not override everything

[Juri Linkov]

close 62250 30.0.50
thanks

>>> Just for the record, this would be useful to create a
>>> context menu for images.
>>
>> The patch below will allow using the text property like
>>
>>   'context-menu-functions '(image-context-menu)
>
> Here is a complete patch that uses the new text property
> for create a context menu for images:

So now the tested patch is pushed to master and closed.