bug#50256: thing-at-mouse

bug#50256: thing-at-mouse

[Juri Linkov]

Maybe I'm missing something, but thingatpt.el has the keywords

  ;; Keywords: extensions, matching, mouse

that contain the keyword "mouse", and yet there is no such function as
'thing-at-mouse'.

bug#50256: thing-at-mouse

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> Maybe I'm missing something, but thingatpt.el has the keywords
>
>   ;; Keywords: extensions, matching, mouse
>
> that contain the keyword "mouse", and yet there is no such function as
> 'thing-at-mouse'.

I'm not quite sure I understand how such a function would look like.
`thing-at-point' has the signature

 (thing-at-point THING)

Would the proposed new function have a signature

 (thing-at-mouse EVENT THING)

?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#50256: thing-at-mouse

[Juri Linkov]

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

> I'm not quite sure I understand how such a function would look like.
> `thing-at-point' has the signature
>
>  (thing-at-point THING)
>
> Would the proposed new function have a signature
>
>  (thing-at-mouse EVENT THING)
>
> ?

This looks like the right signature.  Then the implementation
and an example of usage could look like this:


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

diff --git a/lisp/thingatpt.el b/lisp/thingatpt.el
index ab17748df5..536eef5406 100644
--- a/lisp/thingatpt.el
+++ b/lisp/thingatpt.el
@@ -151,6 +151,15 @@ bounds-of-thing-at-point
 		(if (and (<= real-beg orig) (<= orig end) (< real-beg end))
 		    (cons real-beg end))))))))))
 
+;;;###autoload
+(defun thing-at-mouse (event thing &optional no-properties)
+  "Return the THING at mouse click.
+Like `thing-at-point', but reacts to the event
+where the mouse button is clicked."
+  (save-excursion
+    (mouse-set-point last-input-event)
+    (thing-at-point thing no-properties)))
+
 ;;;###autoload
 (defun thing-at-point (thing &optional no-properties)
   "Return the THING at point.
diff --git a/lisp/net/dictionary.el b/lisp/net/dictionary.el
index f33cbaf112..c9b2bdeaf1 100644
--- a/lisp/net/dictionary.el
+++ b/lisp/net/dictionary.el
@@ -1368,5 +1367,14 @@ global-dictionary-tooltip-mode
                     (if on #'dictionary-tooltip-track-mouse #'ignore))
     on))
 
+(defun context-menu-dictionary (menu)
+  "Dictionary context menu."
+  (when (thing-at-mouse last-input-event 'word)
+    (define-key menu [dictionary-separator] menu-bar-separator)
+    (define-key menu [dictionary-search-word-at-mouse]
+      '(menu-item "Dictionary Search" dictionary-search-word-at-mouse
+                  :help "Search the word at mouse click in dictionary")))
+  menu)
+
 (provide 'dictionary)
 ;;; dictionary.el ends here

bug#50256: thing-at-mouse

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> This looks like the right signature.  Then the implementation
> and an example of usage could look like this:

[...]

> +(defun context-menu-dictionary (menu)
> +  "Dictionary context menu."
> +  (when (thing-at-mouse last-input-event 'word)
> +    (define-key menu [dictionary-separator] menu-bar-separator)
> +    (define-key menu [dictionary-search-word-at-mouse]
> +      '(menu-item "Dictionary Search" dictionary-search-word-at-mouse
> +                  :help "Search the word at mouse click in dictionary")))
> +  menu)

Ah, I see.  Yes, that makes perfect sense to me -- go ahead and push
(perhaps with some documentation).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#50256: thing-at-mouse

[Juri Linkov]

>> This looks like the right signature.  Then the implementation
>> and an example of usage could look like this:
>
> [...]
>
>> +(defun context-menu-dictionary (menu)
>> +  "Dictionary context menu."
>> +  (when (thing-at-mouse last-input-event 'word)
>> +    (define-key menu [dictionary-separator] menu-bar-separator)
>> +    (define-key menu [dictionary-search-word-at-mouse]
>> +      '(menu-item "Dictionary Search" dictionary-search-word-at-mouse
>> +                  :help "Search the word at mouse click in dictionary")))
>> +  menu)
>
> Ah, I see.  Yes, that makes perfect sense to me -- go ahead and push
> (perhaps with some documentation).

In bug#50067 you suggested to add a new arg with the click event
to all context menu functions.  This will make code more readable:

  (defun context-menu-dictionary (menu click)
    (when (thing-at-mouse click 'word)

so I will change the call to provide the arg from last-input-event,
and maybe later it will be possible to acquire the same event
more directly, not from last-input-event, and all callers will get it.

Now the problem found in bug#9923:

1. 'C-h m' temporarily switches from the original *info* buffer
   to the *Help* buffer;

2. thing-at-mouse (called from context-menu-dictionary)
   uses (save-excursion (mouse-set-point event))

3. mouse-set-point calls (posn-set-point (event-end event))

4. in posn-set-point:
4.1. (select-window (posn-window position))
     switches back to the original *info* buffer
     that was selected in the window;

4.2. (goto-char (posn-point position))
     goes to the position that event-end got from the *Help* buffer,
     so in the *info* buffer moves to the position from the *Help* buffer.

So maybe need to add the same condition that I already added to 2 places:

    (eq (window-buffer (posn-window (event-start event)))
                       (current-buffer))

but I have no idea at what level to add it: in mouse-set-point?
Or deeper in posn-set-point?  Both are quite low-level,
and I don't know if this might break something.

bug#50256: thing-at-mouse

[Juri Linkov]

> 3. mouse-set-point calls (posn-set-point (event-end event))
>
> 4. in posn-set-point:
> 4.1. (select-window (posn-window position))
>      switches back to the original *info* buffer
>      that was selected in the window;
>
> 4.2. (goto-char (posn-point position))
>      goes to the position that event-end got from the *Help* buffer,
>      so in the *info* buffer moves to the position from the *Help* buffer.
>
> So maybe need to add the same condition that I already added to 2 places:
>
>     (eq (window-buffer (posn-window (event-start event)))
>                        (current-buffer))
>
> but I have no idea at what level to add it: in mouse-set-point?
> Or deeper in posn-set-point?  Both are quite low-level,
> and I don't know if this might break something.

There are more context-menu-functions that use mouse-set-point
that fails with 'C-h m', e.g.:

  (defun context-menu-ffap (menu)
    "File at point menu."
    (save-excursion
      (mouse-set-point last-input-event)
      (when (ffap-guess-file-name-at-point)

So maybe the check for current-buffer above should be added
to mouse-set-point.  Or maybe it's not backward-compatible,
I'm not sure.

bug#50256: thing-at-mouse

[Juri Linkov]

> So maybe the check for current-buffer above should be added
> to mouse-set-point.

Now I found the real root of the problems.  All reported problems
can be solved by this short patch:

diff --git a/lisp/subr.el b/lisp/subr.el
index 0a31ef2b29..0b3b8d0e0f 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -1510,8 +1510,8 @@ event-start
 
 For more information, see Info node `(elisp)Click Events'."
   (if (consp event) (nth 1 event)
-    (or (posn-at-point)
-        (list (selected-window) (point) '(0 . 0) 0))))
+    (or (posn-at-point (window-point))
+        (list (selected-window) (window-point) '(0 . 0) 0))))
 
 (defun event-end (event)
   "Return the ending position of EVENT.
@@ -1519,8 +1519,8 @@ event-end
 
 See `event-start' for a description of the value returned."
   (if (consp event) (nth (if (consp (nth 2 event)) 2 1) event)
-    (or (posn-at-point)
-        (list (selected-window) (point) '(0 . 0) 0))))
+    (or (posn-at-point (window-point))
+        (list (selected-window) (window-point) '(0 . 0) 0))))

Both 'event-start' and 'event-end' created an event
with the window equal to the selected window,
but point from some random buffer,
not from selected window's buffer.

One question still remains: maybe this fix should be implemented
at a deeper level in posn-at-point when its arg POS is nil?

But actually, posn-at-point just uses this line:

  tem = Fpos_visible_in_window_p (pos, window, Qt);

So maybe this fix should be implemented in Fpos_visible_in_window_p,
i.e. at the end of this code

  if (EQ (pos, Qt))
    posint = -1;
  else if (!NILP (pos))
    posint = fix_position (pos);
  else if (w == XWINDOW (selected_window))
    posint = PT;
  else
    posint = marker_position (w->pointm);

it should get position from the selected window's buffer?

bug#50256: thing-at-mouse

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> So maybe this fix should be implemented in Fpos_visible_in_window_p,
> i.e. at the end of this code
>
>   if (EQ (pos, Qt))
>     posint = -1;
>   else if (!NILP (pos))
>     posint = fix_position (pos);
>   else if (w == XWINDOW (selected_window))
>     posint = PT;
>   else
>     posint = marker_position (w->pointm);
>
> it should get position from the selected window's buffer?

I've tried to follow the logic here, and I think you're right, but it's
difficult to really get a handle on what the repercussions here might be
in various scenarios.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#50256: thing-at-mouse

[martin rudalics]

 > So maybe this fix should be implemented in Fpos_visible_in_window_p,
 > i.e. at the end of this code
 >
 >    if (EQ (pos, Qt))
 >      posint = -1;
 >    else if (!NILP (pos))
 >      posint = fix_position (pos);
 >    else if (w == XWINDOW (selected_window))
 >      posint = PT;
 >    else
 >      posint = marker_position (w->pointm);

Using the position of point of the current buffer when WINDOW is
specified doesn't make any sense.  Moreover the Elisp manual says that

   "The argument POSITION defaults to the current position of point in
   WINDOW"

which doesn't make sense if point is taken from a buffer not shown in
WINDOW.  So the fix should be implemented in Fpos_visible_in_window_p.

 > it should get position from the selected window's buffer?

I don't understand this last sentence though.

martin

bug#50256: thing-at-mouse

[Eli Zaretskii]

> From: martin rudalics <rudalics@gmx.at>
> Date: Wed, 1 Sep 2021 11:18:01 +0200
> Cc: 50256@debbugs.gnu.org
> 
> Using the position of point of the current buffer when WINDOW is
> specified doesn't make any sense.  Moreover the Elisp manual says that
> 
>    "The argument POSITION defaults to the current position of point in
>    WINDOW"
> 
> which doesn't make sense if point is taken from a buffer not shown in
> WINDOW.

That function works only with positions shown in some window, so why
do you say the above makes no sense?  The implementation emulates
display, so it must have a window to make layout decisions.

> So the fix should be implemented in Fpos_visible_in_window_p.

The snippet cited in the original message _was_ from
Fpos_visible_in_window_p.  But I don't understand what kind of fix did
you have in mind, perhaps because I don't understand how the original
problem reported by Juri happened in the first place.

bug#50256: thing-at-mouse

[martin rudalics]

 > That function works only with positions shown in some window, so why
 > do you say the above makes no sense?  The implementation emulates
 > display, so it must have a window to make layout decisions.
 >
 >> So the fix should be implemented in Fpos_visible_in_window_p.
 >
 > The snippet cited in the original message _was_ from
 > Fpos_visible_in_window_p.  But I don't understand what kind of fix did
 > you have in mind, perhaps because I don't understand how the original
 > problem reported by Juri happened in the first place.

With emacs -Q put into *scratch* and evaluate via C-x c-e

(progn
   (with-current-buffer "*Messages*"
     (goto-char (point-min)))
   (pop-to-buffer "*Messages*")
   (with-current-buffer "*scratch*"
     (pos-visible-in-window-p nil nil t)))

This reports visibility for the position of point in *scratch* and not
that of the window point of the selected *Messages* window.

martin

bug#50256: thing-at-mouse

[Eli Zaretskii]

> Cc: juri@linkov.net, larsi@gnus.org, 50256@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Wed, 1 Sep 2021 16:25:20 +0200
> 
> (progn
>    (with-current-buffer "*Messages*"
>      (goto-char (point-min)))
>    (pop-to-buffer "*Messages*")
>    (with-current-buffer "*scratch*"
>      (pos-visible-in-window-p nil nil t)))
> 
> This reports visibility for the position of point in *scratch* and not
> that of the window point of the selected *Messages* window.

But you've switched to *scratch* in the selected window, so why is
that a problem?

bug#50256: thing-at-mouse

[martin rudalics]

 >> (progn
 >>     (with-current-buffer "*Messages*"
 >>       (goto-char (point-min)))
 >>     (pop-to-buffer "*Messages*")
 >>     (with-current-buffer "*scratch*"
 >>       (pos-visible-in-window-p nil nil t)))
 >>
 >> This reports visibility for the position of point in *scratch* and not
 >> that of the window point of the selected *Messages* window.
 >
 > But you've switched to *scratch* in the selected window, so why is
 > that a problem?

I have only run `with-current-buffer'.  The selected window shows
*Messages*.  (pos-visible-in-window-p nil nil t) should never return
nil as it does in the above recipe.

martin

bug#50256: thing-at-mouse

[Eli Zaretskii]

> Cc: juri@linkov.net, larsi@gnus.org, 50256@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Wed, 1 Sep 2021 18:21:00 +0200
> 
>  >> (progn
>  >>     (with-current-buffer "*Messages*"
>  >>       (goto-char (point-min)))
>  >>     (pop-to-buffer "*Messages*")
>  >>     (with-current-buffer "*scratch*"
>  >>       (pos-visible-in-window-p nil nil t)))
>  >>
>  >> This reports visibility for the position of point in *scratch* and not
>  >> that of the window point of the selected *Messages* window.
>  >
>  > But you've switched to *scratch* in the selected window, so why is
>  > that a problem?
> 
> I have only run `with-current-buffer'.  The selected window shows
> *Messages*.

Then what is the semantics of the code snippet above, and why did you
call with-current-buffer the second time?  What did you want to
accomplish, except make a point?

IOW, what kind of real-life situation needs such a code?

> (pos-visible-in-window-p nil nil t) should never return nil as it
> does in the above recipe.

Why not?

bug#50256: thing-at-mouse

[Juri Linkov]

>> I have only run `with-current-buffer'.  The selected window shows
>> *Messages*.
>
> Then what is the semantics of the code snippet above, and why did you
> call with-current-buffer the second time?  What did you want to
> accomplish, except make a point?
>
> IOW, what kind of real-life situation needs such a code?

In bug#9923 'C-h m' switched to another buffer before calling
mouse-set-point.

bug#50256: thing-at-mouse

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: martin rudalics <rudalics@gmx.at>,  larsi@gnus.org,  50256@debbugs.gnu.org
> Date: Wed, 01 Sep 2021 20:59:59 +0300
> 
> >> I have only run `with-current-buffer'.  The selected window shows
> >> *Messages*.
> >
> > Then what is the semantics of the code snippet above, and why did you
> > call with-current-buffer the second time?  What did you want to
> > accomplish, except make a point?
> >
> > IOW, what kind of real-life situation needs such a code?
> 
> In bug#9923 'C-h m' switched to another buffer before calling
> mouse-set-point.

Then it's a bug in that command, I'd say.  You assume something about
last-input and what mouse-set-point and posn-set-point will do when
last-input is not a click event.  And that assumption turns out to be
false.  So instead of making that assumption, why not give the code a
valid event to work with instead?

posn-at-point cannot work correctly when current buffer and the
selected window's buffer are not the same, because they use display
code which is based on that contract.  If you break the contract by
the likes of with-current-buffer, you will be lucky not to crash, let
alone cause errors.

bug#50256: thing-at-mouse

[Juri Linkov]

>> In bug#9923 'C-h m' switched to another buffer before calling
>> mouse-set-point.
>
> Then it's a bug in that command, I'd say.  You assume something about
> last-input and what mouse-set-point and posn-set-point will do when
> last-input is not a click event.  And that assumption turns out to be
> false.  So instead of making that assumption, why not give the code a
> valid event to work with instead?

Currently event-start and event-end that use mouse-set-point and posn-set-point
create an invalid event when some code inadvertently switches the buffer.

> posn-at-point cannot work correctly when current buffer and the
> selected window's buffer are not the same, because they use display
> code which is based on that contract.  If you break the contract by
> the likes of with-current-buffer, you will be lucky not to crash, let
> alone cause errors.

So the conclusion is following?  There is a bug in the low-level
function, but we ask users to be careful and to take precautions
against stumbling on this bug.  Then the problem is that such warning
should be documented somewhere.

bug#50256: thing-at-mouse

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: rudalics@gmx.at,  larsi@gnus.org,  50256@debbugs.gnu.org
> Date: Thu, 02 Sep 2021 09:16:03 +0300
> 
> >> In bug#9923 'C-h m' switched to another buffer before calling
> >> mouse-set-point.
> >
> > Then it's a bug in that command, I'd say.  You assume something about
> > last-input and what mouse-set-point and posn-set-point will do when
> > last-input is not a click event.  And that assumption turns out to be
> > false.  So instead of making that assumption, why not give the code a
> > valid event to work with instead?
> 
> Currently event-start and event-end that use mouse-set-point and posn-set-point
> create an invalid event when some code inadvertently switches the buffer.

Yes.  Then either mouse-set-point should be fixed to avoid that when
it runs inside with-current-buffer, or, when the event is not a click
event, we should concoct the data corresponding to event-start/end
differently, in a way that is tolerant to this situation.

And I'm not yet clear what would be the desired result in this case:

> (progn
>    (with-current-buffer "*Messages*"
>      (goto-char (point-min)))
>    (pop-to-buffer "*Messages*")
>    (with-current-buffer "*scratch*"
>      (pos-visible-in-window-p nil nil t)))

What position would you like this to report on?  Would you like it to
report on the value of point in the selected window, or should it
report on the value of point in the current buffer?  Since these two
are different, it is no longer a trivial question to answer.

> > posn-at-point cannot work correctly when current buffer and the
> > selected window's buffer are not the same, because they use display
> > code which is based on that contract.  If you break the contract by
> > the likes of with-current-buffer, you will be lucky not to crash, let
> > alone cause errors.
> 
> So the conclusion is following?  There is a bug in the low-level
> function, but we ask users to be careful and to take precautions
> against stumbling on this bug.  Then the problem is that such warning
> should be documented somewhere.

It's fine with me to document that restriction, yes.

bug#50256: thing-at-mouse

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> So the conclusion is following?  There is a bug in the low-level
> function, but we ask users to be careful and to take precautions
> against stumbling on this bug.  Then the problem is that such warning
> should be documented somewhere.

I think we should fix the low-level function, myself.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#50256: thing-at-mouse

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  rudalics@gmx.at,  50256@debbugs.gnu.org
> Date: Thu, 02 Sep 2021 09:23:54 +0200
> 
> Juri Linkov <juri@linkov.net> writes:
> 
> > So the conclusion is following?  There is a bug in the low-level
> > function, but we ask users to be careful and to take precautions
> > against stumbling on this bug.  Then the problem is that such warning
> > should be documented somewhere.
> 
> I think we should fix the low-level function, myself.

Which low-level function you'd like to fix?

bug#50256: thing-at-mouse

[martin rudalics]

 > Then what is the semantics of the code snippet above, and why did you
 > call with-current-buffer the second time?  What did you want to
 > accomplish, except make a point?

I tried to write a simple example with emacs -Q showing how code breaks
a contract stated in a manual.

 > IOW, what kind of real-life situation needs such a code?

Any call of `pos-visible-in-window-p' might happen within the context of
`with-current-buffer'.  Just check how often these forms are used in the
Emacs code base.

 >> (pos-visible-in-window-p nil nil t) should never return nil as it
 >> does in the above recipe.
 >
 > Why not?

Because in the manual we say that

   "The argument POSITION defaults to the current position
    of point in WINDOW; WINDOW defaults to the selected window."

If the current buffer is not shown in WINDOW, the first part of this
sentence is wrong.

martin

bug#50256: thing-at-mouse

[Eli Zaretskii]

> Cc: juri@linkov.net, larsi@gnus.org, 50256@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Thu, 2 Sep 2021 08:48:15 +0200
> 
>  >> (pos-visible-in-window-p nil nil t) should never return nil as it
>  >> does in the above recipe.
>  >
>  > Why not?
> 
> Because in the manual we say that
> 
>    "The argument POSITION defaults to the current position
>     of point in WINDOW; WINDOW defaults to the selected window."
> 
> If the current buffer is not shown in WINDOW, the first part of this
> sentence is wrong.

So the only problem is that of inaccurate documentation?

bug#50256: thing-at-mouse

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> Because in the manual we say that
>> 
>>    "The argument POSITION defaults to the current position
>>     of point in WINDOW; WINDOW defaults to the selected window."
>> 
>> If the current buffer is not shown in WINDOW, the first part of this
>> sentence is wrong.
>
> So the only problem is that of inaccurate documentation?

The doc string makes sense to me (that is, if it did what it said, it'd
be good).  That it doesn't do this is a bug, in my opinion.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#50256: thing-at-mouse

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: martin rudalics <rudalics@gmx.at>,  juri@linkov.net,  50256@debbugs.gnu.org
> Date: Thu, 02 Sep 2021 09:32:56 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >>    "The argument POSITION defaults to the current position
> >>     of point in WINDOW; WINDOW defaults to the selected window."
> >> 
> >> If the current buffer is not shown in WINDOW, the first part of this
> >> sentence is wrong.
> >
> > So the only problem is that of inaccurate documentation?
> 
> The doc string makes sense to me (that is, if it did what it said, it'd
> be good).  That it doesn't do this is a bug, in my opinion.

Our doc strings didn't descend on us from Mt Sinai, and aren't carved
in stone.  They were written by people, and may include hidden
assumptions those people had in mind that could make other people
interpret them incorrectly.  So to decide that the code is wrong and
the documentation is right, we need more evidence than just what the
current documentation literally says.

If we want to support the current documentation to the letter, the
only way of doing that I know of is to force WINDOW to display the
current buffer, at least internally, i.e. to switch to the WINDOW's
buffer for the duration of pos-visible-in-window-p.  If that leaves
everyone happy, it could be done relatively easily, but then I wonder
why does the code in question with-current-buffer, and what would
break when pos-visible-in-window-p internally switches back to the
buffer shown in WINDOW?

bug#50256: thing-at-mouse

[martin rudalics]

 > If we want to support the current documentation to the letter, the
 > only way of doing that I know of is to force WINDOW to display the
 > current buffer, at least internally, i.e. to switch to the WINDOW's
 > buffer for the duration of pos-visible-in-window-p.  If that leaves
 > everyone happy, it could be done relatively easily, but then I wonder
 > why does the code in question with-current-buffer, and what would
 > break when pos-visible-in-window-p internally switches back to the
 > buffer shown in WINDOW?

pos_visible_p already does

   if (XBUFFER (w->contents) != current_buffer)
     {
       old_buffer = current_buffer;
       set_buffer_internal_1 (XBUFFER (w->contents));
     }

martin

bug#50256: thing-at-mouse

[Eli Zaretskii]

> Cc: juri@linkov.net, 50256@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Thu, 2 Sep 2021 10:54:44 +0200
> 
>  > If we want to support the current documentation to the letter, the
>  > only way of doing that I know of is to force WINDOW to display the
>  > current buffer, at least internally, i.e. to switch to the WINDOW's
>  > buffer for the duration of pos-visible-in-window-p.  If that leaves
>  > everyone happy, it could be done relatively easily, but then I wonder
>  > why does the code in question with-current-buffer, and what would
>  > break when pos-visible-in-window-p internally switches back to the
>  > buffer shown in WINDOW?
> 
> pos_visible_p already does
> 
>    if (XBUFFER (w->contents) != current_buffer)
>      {
>        old_buffer = current_buffer;
>        set_buffer_internal_1 (XBUFFER (w->contents));
>      }

I'm asking if this is the desired behavior, when Lisp runs this inside
with-current-buffer?

bug#50256: thing-at-mouse

[martin rudalics]

 >> pos_visible_p already does
 >>
 >>     if (XBUFFER (w->contents) != current_buffer)
 >>       {
 >>         old_buffer = current_buffer;
 >>         set_buffer_internal_1 (XBUFFER (w->contents));
 >>       }
 >
 > I'm asking if this is the desired behavior, when Lisp runs this inside
 > with-current-buffer?

It is the _necessary_ behavior when WINDOW is not the selected window
and its buffer is not current.  Regardless of whether it happens inside
`with-current-buffer' or not.

martin

bug#50256: thing-at-mouse

[Eli Zaretskii]

> Cc: larsi@gnus.org, juri@linkov.net, 50256@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Thu, 2 Sep 2021 14:42:15 +0200
> 
>  >> pos_visible_p already does
>  >>
>  >>     if (XBUFFER (w->contents) != current_buffer)
>  >>       {
>  >>         old_buffer = current_buffer;
>  >>         set_buffer_internal_1 (XBUFFER (w->contents));
>  >>       }
>  >
>  > I'm asking if this is the desired behavior, when Lisp runs this inside
>  > with-current-buffer?
> 
> It is the _necessary_ behavior when WINDOW is not the selected window
> and its buffer is not current.

But in the case in point, WINDOWS _was_ the selected window.  That's
why pos-visible-in-window-p used PT.  Which is what you said was the
bug.  Or am I missing something?

bug#50256: thing-at-mouse

[martin rudalics]

 >>   >> pos_visible_p already does
 >>   >>
 >>   >>     if (XBUFFER (w->contents) != current_buffer)
 >>   >>       {
 >>   >>         old_buffer = current_buffer;
 >>   >>         set_buffer_internal_1 (XBUFFER (w->contents));
 >>   >>       }
 >>   >
 >>   > I'm asking if this is the desired behavior, when Lisp runs this inside
 >>   > with-current-buffer?
 >>
 >> It is the _necessary_ behavior when WINDOW is not the selected window
 >> and its buffer is not current.
 >
 > But in the case in point, WINDOWS _was_ the selected window.  That's
 > why pos-visible-in-window-p used PT.  Which is what you said was the
 > bug.  Or am I missing something?

No.  The above was meant in response to your earlier:

   If we want to support the current documentation to the letter, the
   only way of doing that I know of is to force WINDOW to display the
   current buffer, at least internally, i.e. to switch to the WINDOW's
   buffer for the duration of pos-visible-in-window-p.

I mean that we needn't do that because pos_visible_p already does it.
So I'd just propose to do the trivial (line numbers are not trunk's);

diff --git a/src/window.c b/src/window.c
index cb8fe5fcdb..bfbed01749 100644
--- a/src/window.c
+++ b/src/window.c
@@ -2199,8 +2199,6 @@ DEFUN ("pos-visible-in-window-p", Fpos_visible_in_window_p,
      posint = -1;
    else if (!NILP (pos))
      posint = fix_position (pos);
-  else if (w == XWINDOW (selected_window))
-    posint = PT;
    else
      posint = marker_position (w->pointm);

martin

bug#50256: thing-at-mouse

[Juri Linkov]

> I mean that we needn't do that because pos_visible_p already does it.
> So I'd just propose to do the trivial (line numbers are not trunk's);
>
> diff --git a/src/window.c b/src/window.c
> index cb8fe5fcdb..bfbed01749 100644
> --- a/src/window.c
> +++ b/src/window.c
> @@ -2199,8 +2199,6 @@ DEFUN ("pos-visible-in-window-p", Fpos_visible_in_window_p,
>      posint = -1;
>    else if (!NILP (pos))
>      posint = fix_position (pos);
> -  else if (w == XWINDOW (selected_window))
> -    posint = PT;
>    else
>      posint = marker_position (w->pointm);

I confirm this fixes the reported issues.

bug#50256: thing-at-mouse

[Juri Linkov]

>> I mean that we needn't do that because pos_visible_p already does it.
>> So I'd just propose to do the trivial (line numbers are not trunk's);
>>
>> diff --git a/src/window.c b/src/window.c
>> index cb8fe5fcdb..bfbed01749 100644
>> --- a/src/window.c
>> +++ b/src/window.c
>> @@ -2199,8 +2199,6 @@ DEFUN ("pos-visible-in-window-p", Fpos_visible_in_window_p,
>>      posint = -1;
>>    else if (!NILP (pos))
>>      posint = fix_position (pos);
>> -  else if (w == XWINDOW (selected_window))
>> -    posint = PT;
>>    else
>>      posint = marker_position (w->pointm);
>
> I confirm this fixes the reported issues.

Actually, whereas it fixes the reported issue,
it breaks everything else: moving point up and down
always jumps to the fixed column like goal-column,
selecting a completion from the Completions buffer
always says "No completion here", 'C-c C-c' in diff-mode
jumps to the wrong hunk, etc.

bug#50256: thing-at-mouse

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Thu, 02 Sep 2021 21:28:32 +0300
> Cc: 50256@debbugs.gnu.org, larsi@gnus.org
> 
> >> --- a/src/window.c
> >> +++ b/src/window.c
> >> @@ -2199,8 +2199,6 @@ DEFUN ("pos-visible-in-window-p", Fpos_visible_in_window_p,
> >>      posint = -1;
> >>    else if (!NILP (pos))
> >>      posint = fix_position (pos);
> >> -  else if (w == XWINDOW (selected_window))
> >> -    posint = PT;
> >>    else
> >>      posint = marker_position (w->pointm);
> >
> > I confirm this fixes the reported issues.
> 
> Actually, whereas it fixes the reported issue,
> it breaks everything else: moving point up and down
> always jumps to the fixed column like goal-column,
> selecting a completion from the Completions buffer
> always says "No completion here", 'C-c C-c' in diff-mode
> jumps to the wrong hunk, etc.

Figures out, because the window's point didn't get updated yet (AFAIR,
it is updated by redisplay), whereas pos-visible-in-window-p is
expected to work before that.

I think we need to special-case the case of the current buffer.  But
I'd still like to talk about the semantics of calling
pos-visible-in-window-p when WINDOWS is the selected window, but the
WINDOW's buffer is not the current one.  Who "wins" in that case, for
the purpose of the default value of position: the window or the
buffer?

bug#50256: thing-at-mouse

[martin rudalics]

 > I think we need to special-case the case of the current buffer.  But
 > I'd still like to talk about the semantics of calling
 > pos-visible-in-window-p when WINDOWS is the selected window, but the
 > WINDOW's buffer is not the current one.  Who "wins" in that case, for
 > the purpose of the default value of position: the window or the
 > buffer?

If in the manual we say "The argument POSITION defaults to the current
position of point in WINDOW", this means that the window should win.
Whether that's reasonable is another question.  Note that a similar
disputable case already happens when we do

(pos-visible-in-window-p nil (next-window) t)

and the next window does not show the current buffer.  We there silently
override the "nil" with WINDOW's point.  Maybe we should signal an error
in either of these cases when WINDOW's buffer is not the current buffer.

IIUC we use `pos-visible-in-window-p' mainly for three purposes:

(1) Detect whether a buffer position that typically does _not_ equal
     window point is visible in window and, if it isn't, do something
     (scroll, enlarge the window) to make it visible.

(2) Detect whether window point is only partially visible.

(3) Get the coordinates of window point and move the mouse or pop up a
     menu there.

In all these cases, callers simply don't care about which buffer is
current when calling the function - the buffer in question is WINDOW's
buffer.

A different use were to check whether a position of an arbitrary BUFFER
would be visible in a WINDOW if BUFFER were displayed there with the
start of the window set to some valid BUFFER position.  I don't know
whether anyone ever used such a functionality.  To make it work, a
caller would have to set WINDOW's buffer and start position first, call
`pos-visible-in-window-p' and restore the original state afterwards.
Even in this hypothetical case, the caller wouldn't care about which
buffer is current.

martin

bug#50256: thing-at-mouse

[Eli Zaretskii]

> Cc: 50256@debbugs.gnu.org, larsi@gnus.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Fri, 3 Sep 2021 09:40:36 +0200
> 
> If in the manual we say "The argument POSITION defaults to the current
> position of point in WINDOW", this means that the window should win.
> Whether that's reasonable is another question.  Note that a similar
> disputable case already happens when we do
> 
> (pos-visible-in-window-p nil (next-window) t)
> 
> and the next window does not show the current buffer.  We there silently
> override the "nil" with WINDOW's point.  Maybe we should signal an error
> in either of these cases when WINDOW's buffer is not the current buffer.
> 
> IIUC we use `pos-visible-in-window-p' mainly for three purposes:
> 
> (1) Detect whether a buffer position that typically does _not_ equal
>      window point is visible in window and, if it isn't, do something
>      (scroll, enlarge the window) to make it visible.
> 
> (2) Detect whether window point is only partially visible.
> 
> (3) Get the coordinates of window point and move the mouse or pop up a
>      menu there.
> 
> In all these cases, callers simply don't care about which buffer is
> current when calling the function - the buffer in question is WINDOW's
> buffer.
> 
> A different use were to check whether a position of an arbitrary BUFFER
> would be visible in a WINDOW if BUFFER were displayed there with the
> start of the window set to some valid BUFFER position.  I don't know
> whether anyone ever used such a functionality.  To make it work, a
> caller would have to set WINDOW's buffer and start position first, call
> `pos-visible-in-window-p' and restore the original state afterwards.
> Even in this hypothetical case, the caller wouldn't care about which
> buffer is current.

Thanks.  There's one more use case I can think of: when WINDOW is not
a selected one, but its buffer is also displayed in the selected
window, which could mean its point is different from WINDOW's point.

Anyway, after thinking for some time about this, I concluded that the
only sane way forward, especially since we are going to cut the
emacs-28 branch soon, is to leave the default behavior of
pos-visible-in-window-p and posn-at-point as it is today, and add one
more optional argument to force the possible alternative behavior(s).
The proposed change to event-start and event-end are new code, so they
should have no trouble passing this new optional argument to
posn-at-point.

That means to add an argument to pos-visible-in-window-p that would
cause it to select one of the following 3 alternatives: WINDOW's
point, WINDOW's buffer's point, and (in case WINDOW is the selected
window) the current buffer's point.  The default should stay as it is
today: when WINDOW is the selected window, use the current buffer's
point.

Anything else IMNSHO risks introducing many bugs into existing
well-tested code that we will never be able to discover and fix in
time for Emacs 28.1 release.

bug#50256: thing-at-mouse

[martin rudalics]

 > Thanks.  There's one more use case I can think of: when WINDOW is not
 > a selected one, but its buffer is also displayed in the selected
 > window, which could mean its point is different from WINDOW's point.

You mean this would constitute a reasonable and legitimate scenario
where we should use the current buffer's point via a nil argument as we
do with the present code.  It can be easily accommodated via

diff --git a/src/window.c b/src/window.c
index cb8fe5fcdb..9296b12499 100644
--- a/src/window.c
+++ b/src/window.c
@@ -2199,7 +2199,7 @@ DEFUN ("pos-visible-in-window-p", Fpos_visible_in_window_p,
      posint = -1;
    else if (!NILP (pos))
      posint = fix_position (pos);
-  else if (w == XWINDOW (selected_window))
+  else if (XBUFFER (w->contents) == current_buffer)
      posint = PT;
    else
      posint = marker_position (w->pointm);

 > Anyway, after thinking for some time about this, I concluded that the
 > only sane way forward, especially since we are going to cut the
 > emacs-28 branch soon, is to leave the default behavior of
 > pos-visible-in-window-p and posn-at-point as it is today, and add one
 > more optional argument to force the possible alternative behavior(s).
 > The proposed change to event-start and event-end are new code, so they
 > should have no trouble passing this new optional argument to
 > posn-at-point.

There's no need doing that - these function could just pass an explicit
POS argument instead of using nil.

 > That means to add an argument to pos-visible-in-window-p that would
 > cause it to select one of the following 3 alternatives: WINDOW's
 > point, WINDOW's buffer's point, and (in case WINDOW is the selected
 > window) the current buffer's point.  The default should stay as it is
 > today: when WINDOW is the selected window, use the current buffer's
 > point.
 >
 > Anything else IMNSHO risks introducing many bugs into existing
 > well-tested code that we will never be able to discover and fix in
 > time for Emacs 28.1 release.

Agreed.  But the solution you propose appears pure overkill to me.
Instead of adding another argument (and trying to explain its meaning)
we should rather tell that using nil for POS is ambiguous and should be
avoided because at the time this function is called, the current buffer
and WINDOW's buffer might not be the same.

Also, your proposed solution will not catch bugs in existing but
no-so-well-tested code.  To catch those it might reasonable to do:

diff --git a/src/window.c b/src/window.c
index cb8fe5fcdb..18ada851fe 100644
--- a/src/window.c
+++ b/src/window.c
@@ -2199,10 +2199,16 @@ DEFUN ("pos-visible-in-window-p", Fpos_visible_in_window_p,
      posint = -1;
    else if (!NILP (pos))
      posint = fix_position (pos);
-  else if (w == XWINDOW (selected_window))
-    posint = PT;
    else
-    posint = marker_position (w->pointm);
+    {
+      if (XBUFFER (w->contents) != current_buffer)
+	message ("`pos-visible-in-window' called with POS nil but WINDOW's buffer is not current");
+
+      if (w == XWINDOW (selected_window))
+	posint = PT;
+      else
+	posint = marker_position (w->pointm);
+    }

    /* If position is above window start or outside buffer boundaries,
       or if window start is out of range, position is not visible.  */

martin

bug#50256: thing-at-mouse

[Eli Zaretskii]

> Cc: juri@linkov.net, 50256@debbugs.gnu.org, larsi@gnus.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sat, 4 Sep 2021 09:34:17 +0200
> 
>  > Thanks.  There's one more use case I can think of: when WINDOW is not
>  > a selected one, but its buffer is also displayed in the selected
>  > window, which could mean its point is different from WINDOW's point.
> 
> You mean this would constitute a reasonable and legitimate scenario
> where we should use the current buffer's point via a nil argument as we
> do with the present code.

I don't know what is reasonable in that case, all I know is that there
could be 2 alternatives, each one not necessarily senseless.

>  > Anyway, after thinking for some time about this, I concluded that the
>  > only sane way forward, especially since we are going to cut the
>  > emacs-28 branch soon, is to leave the default behavior of
>  > pos-visible-in-window-p and posn-at-point as it is today, and add one
>  > more optional argument to force the possible alternative behavior(s).
>  > The proposed change to event-start and event-end are new code, so they
>  > should have no trouble passing this new optional argument to
>  > posn-at-point.
> 
> There's no need doing that - these function could just pass an explicit
> POS argument instead of using nil.

They could, but (posn-at-point) is a very frequent usage, and the same
with pos-visible-in-window-p.  I want to avoid the need of auditing
all of those (and there are a lot of them not in Emacs, so we
practically can't) and adjusting them for the different semantics we
want to introduce to handle what mouse-set-point does, or should do,
for context menus.

>  > That means to add an argument to pos-visible-in-window-p that would
>  > cause it to select one of the following 3 alternatives: WINDOW's
>  > point, WINDOW's buffer's point, and (in case WINDOW is the selected
>  > window) the current buffer's point.  The default should stay as it is
>  > today: when WINDOW is the selected window, use the current buffer's
>  > point.
>  >
>  > Anything else IMNSHO risks introducing many bugs into existing
>  > well-tested code that we will never be able to discover and fix in
>  > time for Emacs 28.1 release.
> 
> Agreed.  But the solution you propose appears pure overkill to me.

It could be, but it is safe, and it does the job.  The allegedly
"buggy" code in pos-visible-in-window-p was there since long ago, so
its behavior is now a de-facto standard, even though the doc string
fails to describe the subtlety.  I cannot see any sense in changing
that behavior now, just because some new (and quite tricky) code made
some assumption about that behavior that happens to be false.

> Instead of adding another argument (and trying to explain its meaning)
> we should rather tell that using nil for POS is ambiguous and should be
> avoided because at the time this function is called, the current buffer
> and WINDOW's buffer might not be the same.

"We should rather tell" means documentation changes, right?  That
cannot fix the problems I'm worried about: if we break code out there,
telling we documented that won't get us any points.  We should avoid
breaking the code in the first place.

> Also, your proposed solution will not catch bugs in existing but
> no-so-well-tested code.

That doesn't make the situation worse, does it?  "Don't do harm" is a
great principle in these cases, IME.

> To catch those it might reasonable to do:
> 
> diff --git a/src/window.c b/src/window.c
> index cb8fe5fcdb..18ada851fe 100644
> --- a/src/window.c
> +++ b/src/window.c
> @@ -2199,10 +2199,16 @@ DEFUN ("pos-visible-in-window-p", Fpos_visible_in_window_p,
>       posint = -1;
>     else if (!NILP (pos))
>       posint = fix_position (pos);
> -  else if (w == XWINDOW (selected_window))
> -    posint = PT;
>     else
> -    posint = marker_position (w->pointm);
> +    {
> +      if (XBUFFER (w->contents) != current_buffer)
> +	message ("`pos-visible-in-window' called with POS nil but WINDOW's buffer is not current");
> +
> +      if (w == XWINDOW (selected_window))
> +	posint = PT;
> +      else
> +	posint = marker_position (w->pointm);
> +    }

That's orthogonal.  (And it seems to issue the warning in an unrelated
case, handled by the 'else' clause?)  We should first decide how to
support the context menus with this stuff, and then we can talk how to
find code out there which makes similar assumptions.  (But if there is
such code out there, how was it working till now?)

bug#50256: thing-at-mouse

[martin rudalics]

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

 > That's orthogonal.  (And it seems to issue the warning in an unrelated
 > case, handled by the 'else' clause?)

Which case?

 > We should first decide how to
 > support the context menus with this stuff, and then we can talk how to
 > find code out there which makes similar assumptions.  (But if there is
 > such code out there, how was it working till now?)

Maybe I have not been making myself clear.  What I would apply is the
attached (modulo any errors it contains): No change in behavior, just a
warning in the doc-string and suspicious calls.

martin

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: pos-visible-in-window-p.diff --]
[-- Type: text/x-patch; name="pos-visible-in-window-p.diff", Size: 1445 bytes --]

diff --git a/src/window.c b/src/window.c
index cb8fe5fcdb..a7ad4d5599 100644
--- a/src/window.c
+++ b/src/window.c
@@ -2172,6 +2172,10 @@ DEFUN ("pos-visible-in-window-p", Fpos_visible_in_window_p,
 first.  POS defaults to point in WINDOW; WINDOW defaults to the
 selected window.

+Do not pass nil as POS unless you are sure that WINDOW displays the
+current buffer.  This function will emit a warning when POS is neither a
+marker, a number or t and WINDOW does not display the current buffer.
+
 If POS is visible, return t if PARTIALLY is nil; if PARTIALLY is non-nil,
 the return value is a list of 2 or 6 elements (X Y [RTOP RBOT ROWH VPOS]),
 where X and Y are the pixel coordinates relative to the top left corner
@@ -2199,10 +2203,16 @@ DEFUN ("pos-visible-in-window-p", Fpos_visible_in_window_p,
     posint = -1;
   else if (!NILP (pos))
     posint = fix_position (pos);
-  else if (w == XWINDOW (selected_window))
-    posint = PT;
   else
-    posint = marker_position (w->pointm);
+    {
+      if (XBUFFER (w->contents) != current_buffer)
+	message ("`pos-visible-in-window' called with POS nil but WINDOW's buffer is not current");
+
+      if (w == XWINDOW (selected_window))
+	posint = PT;
+      else
+	posint = marker_position (w->pointm);
+    }

   /* If position is above window start or outside buffer boundaries,
      or if window start is out of range, position is not visible.  */

bug#50256: thing-at-mouse

[Eli Zaretskii]

> Cc: juri@linkov.net, 50256@debbugs.gnu.org, larsi@gnus.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sat, 4 Sep 2021 13:10:43 +0200
> 
>  > That's orthogonal.  (And it seems to issue the warning in an unrelated
>  > case, handled by the 'else' clause?)
> 
> Which case?

This one:

> +      else
> +	   posint = marker_position (w->pointm);

>  > We should first decide how to
>  > support the context menus with this stuff, and then we can talk how to
>  > find code out there which makes similar assumptions.  (But if there is
>  > such code out there, how was it working till now?)
> 
> Maybe I have not been making myself clear.  What I would apply is the
> attached (modulo any errors it contains): No change in behavior, just a
> warning in the doc-string and suspicious calls.

That doesn't fix the problem which started this thread, does it?

bug#50256: thing-at-mouse

[Juri Linkov]

>> Maybe I have not been making myself clear.  What I would apply is the
>> attached (modulo any errors it contains): No change in behavior, just a
>> warning in the doc-string and suspicious calls.
>
> That doesn't fix the problem which started this thread, does it?

Indeed, it displays a warning, but doesn't fix the problem.

bug#50256: thing-at-mouse

[martin rudalics]

 > Indeed, it displays a warning, but doesn't fix the problem.

But you now know that there is a problem and how to fix it.  Right?

martin

bug#50256: thing-at-mouse

[martin rudalics]

 >>   > That's orthogonal.  (And it seems to issue the warning in an unrelated
 >>   > case, handled by the 'else' clause?)
 >>
 >> Which case?
 >
 > This one:
 >
 >> +      else
 >> +	   posint = marker_position (w->pointm);

You mean when the caller asks for using point in a non-selected window
that does not show the current buffer.  Why do you think this case is
unrelated and/or should be handled differently?

 >> Maybe I have not been making myself clear.  What I would apply is the
 >> attached (modulo any errors it contains): No change in behavior, just a
 >> warning in the doc-string and suspicious calls.
 >
 > That doesn't fix the problem which started this thread, does it?

Right.  We have two ways to fix that problem:

(1) Fix it in a general way.  You don't want to do that because it might
     harm calls of that function that silently worked until now.

(2) Fix it by modifying the calls of `pos-visible-in-window-p'.  In that
     case the warning should help us find the calls that should be fixed.

martin

bug#50256: thing-at-mouse

[Eli Zaretskii]

> Cc: juri@linkov.net, 50256@debbugs.gnu.org, larsi@gnus.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sun, 5 Sep 2021 09:50:14 +0200
> 
>  >>   > That's orthogonal.  (And it seems to issue the warning in an unrelated
>  >>   > case, handled by the 'else' clause?)
>  >>
>  >> Which case?
>  >
>  > This one:
>  >
>  >> +      else
>  >> +	   posint = marker_position (w->pointm);
> 
> You mean when the caller asks for using point in a non-selected window
> that does not show the current buffer.  Why do you think this case is
> unrelated and/or should be handled differently?

I see no reason to display a warning in that case.  It's completely
legitimate; moreover, a non-selected window will almost always display
a buffer that is not the current buffer, and annoying warnings in
those cases will just ... annoy.

>  >> Maybe I have not been making myself clear.  What I would apply is the
>  >> attached (modulo any errors it contains): No change in behavior, just a
>  >> warning in the doc-string and suspicious calls.
>  >
>  > That doesn't fix the problem which started this thread, does it?
> 
> Right.  We have two ways to fix that problem:
> 
> (1) Fix it in a general way.  You don't want to do that because it might
>      harm calls of that function that silently worked until now.
> 
> (2) Fix it by modifying the calls of `pos-visible-in-window-p'.  In that
>      case the warning should help us find the calls that should be fixed.

Once again, the warning is an orthogonal issue.  I'm not opposed to
displaying a warning in cases where the caller might have intended
something else.  But let's please first fix the actual issue that
started this thread, and that wasn't the lack of warning.

bug#50256: thing-at-mouse

[martin rudalics]

 > legitimate; moreover, a non-selected window will almost always display
 > a buffer that is not the current buffer, and annoying warnings in
 > those cases will just ... annoy.

OK.

 > Once again, the warning is an orthogonal issue.  I'm not opposed to
 > displaying a warning in cases where the caller might have intended
 > something else.  But let's please first fix the actual issue that
 > started this thread, and that wasn't the lack of warning.

I probably don't understand the actual issue then.

martin

bug#50256: thing-at-mouse

[Eli Zaretskii]

> Cc: juri@linkov.net, 50256@debbugs.gnu.org, larsi@gnus.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sun, 5 Sep 2021 11:39:06 +0200
> 
>  > Once again, the warning is an orthogonal issue.  I'm not opposed to
>  > displaying a warning in cases where the caller might have intended
>  > something else.  But let's please first fix the actual issue that
>  > started this thread, and that wasn't the lack of warning.
> 
> I probably don't understand the actual issue then.

The actual issue is that pos-visible-in-window-p and posn-at-point
produced unexpected results when the buffer shown in the selected
window was momentarily changed by with-current-buffer.

bug#50256: thing-at-mouse

[martin rudalics]

 > The actual issue is that pos-visible-in-window-p and posn-at-point
 > produced unexpected results when the buffer shown in the selected
 > window was momentarily changed by with-current-buffer.

Whatever `with-current-buffer' does, it does not "momentarily change the
buffer shown in the selected window".  I've re-read this thread and now
think too that we should not change the code of neither `posn-at-point'
nor `pos-visible-in-window-p'.  Given that a function that calls any of
these can have used `with-current-buffer', `set-window-point' and
`goto-char', it's easy to see that when passing nil as POS argument DTRT
never works out.

Suppose I have two windows, the selected one showing *scratch*, the
other one showing *Messages*, and I managed to make position 0 in the
*Messages* window vertically scrolled out of view.  If I now do

(let ((window (next-window)))
   (set-window-point window 0)
   (pos-visible-in-window-p nil window))

I get nil as response although in the resulting configuration position 0
is clearly visible.  If OTOH I do

(with-current-buffer (window-buffer (next-window))
   (goto-char 0)
   (pos-visible-in-window-p nil (next-window)))

I get t although in the resulting configuration position 0 is clearly
out of view.  We may call both pilot errors but I see no evidence that
the coder did anything wrong wrt our docs of `pos-visible-in-window-p'.

If I do instead

(let ((window (next-window)))
   (set-window-point window 0)
   (pos-visible-in-window-p (window-point) window))

and

(with-current-buffer (window-buffer (next-window))
   (goto-char 0)
   (pos-visible-in-window-p (point) (next-window)))

I get the results I would have expected.

So I would, in general, recommend against using nil as POS argument for
`pos-visible-in-window-p'.  Given how fairly rare in our code base the
use of nil is, such recommendation seems feasible to me.  For Emacs 29
it should be then even possible to warn whenever it's used.

martin

bug#50256: thing-at-mouse

[Juri Linkov]

> (1) Fix it in a general way.  You don't want to do that because it might
>     harm calls of that function that silently worked until now.
>
> (2) Fix it by modifying the calls of `pos-visible-in-window-p'.  In that
>     case the warning should help us find the calls that should be fixed.

Since there is no way to fix (1), I sent a fix for (2):

diff --git a/lisp/subr.el b/lisp/subr.el
index 6ae6d242a4..bf0177a846 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -1510,8 +1510,8 @@ event-start
 
 For more information, see Info node `(elisp)Click Events'."
   (if (consp event) (nth 1 event)
-    (or (posn-at-point)
-        (list (selected-window) (point) '(0 . 0) 0))))
+    (or (posn-at-point (window-point))
+        (list (selected-window) (window-point) '(0 . 0) 0))))
 
 (defun event-end (event)
   "Return the ending position of EVENT.
@@ -1519,8 +1519,8 @@ event-end
 
 See `event-start' for a description of the value returned."
   (if (consp event) (nth (if (consp (nth 2 event)) 2 1) event)
-    (or (posn-at-point)
-        (list (selected-window) (point) '(0 . 0) 0))))
+    (or (posn-at-point (window-point))
+        (list (selected-window) (window-point) '(0 . 0) 0))))
 
 (defsubst event-click-count (event)
   "Return the multi-click count of EVENT, a click or drag event.

bug#50256: thing-at-mouse

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  50256@debbugs.gnu.org,  larsi@gnus.org
> Date: Sun, 05 Sep 2021 19:13:15 +0300
> 
> > (1) Fix it in a general way.  You don't want to do that because it might
> >     harm calls of that function that silently worked until now.
> >
> > (2) Fix it by modifying the calls of `pos-visible-in-window-p'.  In that
> >     case the warning should help us find the calls that should be fixed.
> 
> Since there is no way to fix (1), I sent a fix for (2):

Thanks, but is that the best fix?  It would mean any other Lisp
program that needs to do something similar will have to use such
kludges.

But if I'm the only one who thinks this isn't clean enough, I won't
object to this solution.  Just be sure to add comments explaining why
you do something as strange as that.

bug#50256: thing-at-mouse

[martin rudalics]

 > Thanks, but is that the best fix?  It would mean any other Lisp
 > program that needs to do something similar will have to use such
 > kludges.
 >
 > But if I'm the only one who thinks this isn't clean enough, I won't
 > object to this solution.  Just be sure to add comments explaining why
 > you do something as strange as that.

Please explain why you think Juri's solution is a kludge and why it is
not clean.

martin

bug#50256: thing-at-mouse

[Eli Zaretskii]

> Cc: 50256@debbugs.gnu.org, larsi@gnus.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Mon, 6 Sep 2021 10:31:10 +0200
> 
>  > Thanks, but is that the best fix?  It would mean any other Lisp
>  > program that needs to do something similar will have to use such
>  > kludges.
>  >
>  > But if I'm the only one who thinks this isn't clean enough, I won't
>  > object to this solution.  Just be sure to add comments explaining why
>  > you do something as strange as that.
> 
> Please explain why you think Juri's solution is a kludge and why it is
> not clean.

Because it extremely non-obvious why the code does something strange
like that.

bug#50256: thing-at-mouse

[martin rudalics]

 >> Please explain why you think Juri's solution is a kludge and why it is
 >> not clean.
 >
 > Because it extremely non-obvious why the code does something strange
 > like that.

If you intend the use of (posn-at-point (window-point)) instead of
(posn-at-point) we strongly disagree.

martin

bug#50256: thing-at-mouse

[Eli Zaretskii]

> Cc: juri@linkov.net, 50256@debbugs.gnu.org, larsi@gnus.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Mon, 6 Sep 2021 16:08:47 +0200
> 
>  >> Please explain why you think Juri's solution is a kludge and why it is
>  >> not clean.
>  >
>  > Because it extremely non-obvious why the code does something strange
>  > like that.
> 
> If you intend the use of (posn-at-point (window-point)) instead of
> (posn-at-point) we strongly disagree.

Then we'll have to disagree.

bug#50256: thing-at-mouse

[Juri Linkov]

>>  > Thanks, but is that the best fix?  It would mean any other Lisp
>>  > program that needs to do something similar will have to use such
>>  > kludges.
>>  >
>>  > But if I'm the only one who thinks this isn't clean enough, I won't
>>  > object to this solution.  Just be sure to add comments explaining why
>>  > you do something as strange as that.
>>
>> Please explain why you think Juri's solution is a kludge and why it is
>> not clean.
>
> Because it extremely non-obvious why the code does something strange
> like that.

I see no other way to fix it.

bug#50256: thing-at-mouse

[Juri Linkov]

> But if I'm the only one who thinks this isn't clean enough, I won't
> object to this solution.  Just be sure to add comments explaining why
> you do something as strange as that.

So I added comments explaining why such an arg is needed, and pushed.

bug#50256: thing-at-mouse

[martin rudalics]

 > Actually, whereas it fixes the reported issue,
 > it breaks everything else: moving point up and down
 > always jumps to the fixed column like goal-column,
 > selecting a completion from the Completions buffer
 > always says "No completion here", 'C-c C-c' in diff-mode
 > jumps to the wrong hunk, etc.

Is the below better?

diff --git a/src/window.c b/src/window.c
index cb8fe5fcdb..e86f50e600 100644
--- a/src/window.c
+++ b/src/window.c
@@ -2199,7 +2199,8 @@ DEFUN ("pos-visible-in-window-p", Fpos_visible_in_window_p,
      posint = -1;
    else if (!NILP (pos))
      posint = fix_position (pos);
-  else if (w == XWINDOW (selected_window))
+  else if (w == XWINDOW (selected_window)
+	   && XBUFFER (w->contents) == current_buffer)
      posint = PT;
    else
      posint = marker_position (w->pointm);

martin

bug#50256: thing-at-mouse

[Juri Linkov]

>> Actually, whereas it fixes the reported issue,
>> it breaks everything else: moving point up and down
>> always jumps to the fixed column like goal-column,
>> selecting a completion from the Completions buffer
>> always says "No completion here", 'C-c C-c' in diff-mode
>> jumps to the wrong hunk, etc.
>
> Is the below better?
>
> @@ -2199,7 +2199,8 @@ DEFUN ("pos-visible-in-window-p", Fpos_visible_in_window_p,
> -  else if (w == XWINDOW (selected_window))
> +  else if (w == XWINDOW (selected_window)
> +	   && XBUFFER (w->contents) == current_buffer)

Thanks, now it fixes the reported issue while not breaking other things.

bug#50256: thing-at-mouse

[Eli Zaretskii]

> Cc: larsi@gnus.org, juri@linkov.net, 50256@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Thu, 2 Sep 2021 16:43:02 +0200
> 
>  >>   >> pos_visible_p already does
>  >>   >>
>  >>   >>     if (XBUFFER (w->contents) != current_buffer)
>  >>   >>       {
>  >>   >>         old_buffer = current_buffer;
>  >>   >>         set_buffer_internal_1 (XBUFFER (w->contents));
>  >>   >>       }
>  >>   >
>  >>   > I'm asking if this is the desired behavior, when Lisp runs this inside
>  >>   > with-current-buffer?
>  >>
>  >> It is the _necessary_ behavior when WINDOW is not the selected window
>  >> and its buffer is not current.
>  >
>  > But in the case in point, WINDOWS _was_ the selected window.  That's
>  > why pos-visible-in-window-p used PT.  Which is what you said was the
>  > bug.  Or am I missing something?
> 
> No.  The above was meant in response to your earlier:
> 
>    If we want to support the current documentation to the letter, the
>    only way of doing that I know of is to force WINDOW to display the
>    current buffer, at least internally, i.e. to switch to the WINDOW's
>    buffer for the duration of pos-visible-in-window-p.
> 
> I mean that we needn't do that because pos_visible_p already does it.
> So I'd just propose to do the trivial (line numbers are not trunk's);

Yes, but I'd prefer that we first established what is the desired
behavior in this case:

>    (pop-to-buffer "*Messages*")
>    (with-current-buffer "*scratch*"
>      (pos-visible-in-window-p nil nil t)))

IOW, what to do when WINDOW is the selected window, but the current
buffer is not the buffer shown in WINDOW?

Would you please humor me with an answer to my question?  I'd also
like to hear from Lars and Juri about this.  Without the agreement
about the behavior in that use case, I don't see how we can reason
about the fix.

bug#50256: thing-at-mouse

[Juri Linkov]

>> So maybe this fix should be implemented in Fpos_visible_in_window_p,
>> i.e. at the end of this code
>>
>>    if (EQ (pos, Qt))
>>      posint = -1;
>>    else if (!NILP (pos))
>>      posint = fix_position (pos);
>>    else if (w == XWINDOW (selected_window))
>>      posint = PT;
>>    else
>>      posint = marker_position (w->pointm);
>
> Using the position of point of the current buffer when WINDOW is
> specified doesn't make any sense.  Moreover the Elisp manual says that
>
>   "The argument POSITION defaults to the current position of point in
>   WINDOW"
>
> which doesn't make sense if point is taken from a buffer not shown in
> WINDOW.  So the fix should be implemented in Fpos_visible_in_window_p.

Actually, WINDOW is not specified, but defaults to the selected window.
But still you point is valid: using the position of point of the current buffer
for the selected window doesn't make sense.

>> it should get position from the selected window's buffer?
>
> I don't understand this last sentence though.

I meant to set posint to (window-point), but written in C.

bug#50256: thing-at-mouse

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Wed, 01 Sep 2021 18:42:35 +0300
> Cc: 50256@debbugs.gnu.org, Lars Ingebrigtsen <larsi@gnus.org>
> 
> >>    if (EQ (pos, Qt))
> >>      posint = -1;
> >>    else if (!NILP (pos))
> >>      posint = fix_position (pos);
> >>    else if (w == XWINDOW (selected_window))
> >>      posint = PT;
> >>    else
> >>      posint = marker_position (w->pointm);
> >
> > Using the position of point of the current buffer when WINDOW is
> > specified doesn't make any sense.  Moreover the Elisp manual says that
> >
> >   "The argument POSITION defaults to the current position of point in
> >   WINDOW"
> >
> > which doesn't make sense if point is taken from a buffer not shown in
> > WINDOW.  So the fix should be implemented in Fpos_visible_in_window_p.
> 
> Actually, WINDOW is not specified, but defaults to the selected window.
> But still you point is valid: using the position of point of the current buffer
> for the selected window doesn't make sense.

It makes perfect sense, because this code cannot work if the current
buffer and the selected window's buffer are different.

bug#50256: thing-at-mouse

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Wed, 01 Sep 2021 10:17:56 +0300
> Cc: 50256@debbugs.gnu.org
> 
> > So maybe the check for current-buffer above should be added
> > to mouse-set-point.
> 
> Now I found the real root of the problems.  All reported problems
> can be solved by this short patch:
> 
> diff --git a/lisp/subr.el b/lisp/subr.el
> index 0a31ef2b29..0b3b8d0e0f 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -1510,8 +1510,8 @@ event-start
>  
>  For more information, see Info node `(elisp)Click Events'."
>    (if (consp event) (nth 1 event)
> -    (or (posn-at-point)
> -        (list (selected-window) (point) '(0 . 0) 0))))
> +    (or (posn-at-point (window-point))
> +        (list (selected-window) (window-point) '(0 . 0) 0))))

I don't understand this: are you saying that in a selected window you
see point different from window-point?  How does that happen?

> Both 'event-start' and 'event-end' created an event
> with the window equal to the selected window,
> but point from some random buffer,
> not from selected window's buffer.

Can you show the recipe that produces this strange result?  (I looked
at bug#9923, which you say is where this was reported, but couldn't
find a recipe there for which you described the sequence of calls.)

> One question still remains: maybe this fix should be implemented
> at a deeper level in posn-at-point when its arg POS is nil?
> 
> But actually, posn-at-point just uses this line:
> 
>   tem = Fpos_visible_in_window_p (pos, window, Qt);
> 
> So maybe this fix should be implemented in Fpos_visible_in_window_p,
> i.e. at the end of this code
> 
>   if (EQ (pos, Qt))
>     posint = -1;
>   else if (!NILP (pos))
>     posint = fix_position (pos);
>   else if (w == XWINDOW (selected_window))
>     posint = PT;
>   else
>     posint = marker_position (w->pointm);
> 
> it should get position from the selected window's buffer?

Once again, in a selected window, point (a.k.a. "PT" in C) should be
identical to the window-point of that window.  So I don't think I
understand why this would need any change.  Please tell more.

bug#50256: thing-at-mouse

[Juri Linkov]

> I don't understand this: are you saying that in a selected window you
> see point different from window-point?  How does that happen?
>
>> Both 'event-start' and 'event-end' created an event
>> with the window equal to the selected window,
>> but point from some random buffer,
>> not from selected window's buffer.
>
> Can you show the recipe that produces this strange result?  (I looked
> at bug#9923, which you say is where this was reported, but couldn't
> find a recipe there for which you described the sequence of calls.)

Martin sent a test case for pos-visible-in-window-p, and here is
the test case for mouse-set-point that uses pos-visible-in-window-p.
In emacs -Q evaluate:

  (with-current-buffer "*Messages*"
    (mouse-set-point last-input-event))

and it moves point in *scratch* to the position of point of
the *Messages* buffer.

bug#50256: thing-at-mouse

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: larsi@gnus.org,  50256@debbugs.gnu.org
> Date: Wed, 01 Sep 2021 18:44:49 +0300
> 
> In emacs -Q evaluate:
> 
>   (with-current-buffer "*Messages*"
>     (mouse-set-point last-input-event))
> 
> and it moves point in *scratch* to the position of point of
> the *Messages* buffer.

I don't follow: what do you expect to be in last-input-event in this
case, and how is that relevant to mouse clicks, when the mouse is not
even involved in this scenario?

I fear I'm missing something very important here.

bug#50256: thing-at-mouse

[Juri Linkov]

>>   (with-current-buffer "*Messages*"
>>     (mouse-set-point last-input-event))
>>
>> and it moves point in *scratch* to the position of point of
>> the *Messages* buffer.
>
> I don't follow: what do you expect to be in last-input-event in this
> case, and how is that relevant to mouse clicks, when the mouse is not
> even involved in this scenario?

Normally, last-input-event should be a mouse click event.
But sometimes it gets a non-mouse event (e.g. a number).
Then in mouse-set-point, event-end produces a fake mouse event
using posn-at-point.  But such constructed event
contains position from a random buffer (that was current
at the time of invocation). It should contain the position
of point in the buffer displayed in the selected window.

bug#50256: thing-at-mouse

[Juri Linkov]

> In bug#50067 you suggested to add a new arg with the click event
> to all context menu functions.  This will make code more readable:
>
>   (defun context-menu-dictionary (menu click)
>     (when (thing-at-mouse click 'word)
>
> so I will change the call to provide the arg from last-input-event,
> and maybe later it will be possible to acquire the same event
> more directly, not from last-input-event, and all callers will get it.

So now a new arg 'click' was added to all context-menu-functions.

bug#50256: thing-at-mouse

[Juri Linkov]

tags 50256 fixed
close 50256 28.0.50
quit

>> +(defun context-menu-dictionary (menu)
>> +  "Dictionary context menu."
>> +  (when (thing-at-mouse last-input-event 'word)
>> +    (define-key menu [dictionary-separator] menu-bar-separator)
>> +    (define-key menu [dictionary-search-word-at-mouse]
>> +      '(menu-item "Dictionary Search" dictionary-search-word-at-mouse
>> +                  :help "Search the word at mouse click in dictionary")))
>> +  menu)
>
> Ah, I see.  Yes, that makes perfect sense to me -- go ahead and push
> (perhaps with some documentation).

Now pushed with NEWS.  I don't know if it needs to be mentioned in Info manual.