bug#34908: Push mark in xref-push-marker-stack

bug#34908: Push mark in xref-push-marker-stack

[Juri Linkov]

X-Debbugs-CC: João Távora <joaotavora@gmail.com>, Dmitry Gutov <dgutov@yandex.ru>

Shouldn't xref-push-marker-stack push the mark like all normal commands do?

I know there is ‘M-,’ but why not allow using the standard keys
‘C-x C-SPC’ (pop-global-mark) and ‘C-u C-SPC’ (in the same file)
as well?

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 6974d00048..861e24a85f 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -365,6 +365,7 @@ xref-set-marker-ring-length
 
 (defun xref-push-marker-stack (&optional m)
   "Add point M (defaults to `point-marker') to the marker stack."
+  (push-mark nil t)
   (ring-insert xref--marker-ring (or m (point-marker))))
 

bug#34908: Push mark in xref-push-marker-stack

[João Távora]

This looks good to me, but I'm no expert

On a tangent, I'm a little ashamed that I didn't know
about C-u C-SPC until now (the docstring could be
clearer). It does basically a `pop-to-mark-command`
right? I wonder how many more of these nuggets
of original Emacs UI are hidden and where...

Anyway, thanks!
João

On Mon, Mar 18, 2019 at 9:25 PM Juri Linkov <juri@linkov.net> wrote:
>
> X-Debbugs-CC: João Távora <joaotavora@gmail.com>, Dmitry Gutov <dgutov@yandex.ru>
>
> Shouldn't xref-push-marker-stack push the mark like all normal commands do?
>
> I know there is ‘M-,’ but why not allow using the standard keys
> ‘C-x C-SPC’ (pop-global-mark) and ‘C-u C-SPC’ (in the same file)
> as well?
>
> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> index 6974d00048..861e24a85f 100644
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -365,6 +365,7 @@ xref-set-marker-ring-length
>
>  (defun xref-push-marker-stack (&optional m)
>    "Add point M (defaults to `point-marker') to the marker stack."
> +  (push-mark nil t)
>    (ring-insert xref--marker-ring (or m (point-marker))))
>
>
>


-- 
João Távora

bug#34908: Push mark in xref-push-marker-stack

[Dmitry Gutov]

On 18.03.2019 23:12, Juri Linkov wrote:
> X-Debbugs-CC: João Távora <joaotavora@gmail.com>, Dmitry Gutov <dgutov@yandex.ru>
> 
> Shouldn't xref-push-marker-stack push the mark like all normal commands do?

It's not a command, though. Right?

> I know there is ‘M-,’ but why not allow using the standard keys
> ‘C-x C-SPC’ (pop-global-mark) and ‘C-u C-SPC’ (in the same file)
> as well?

IMO that separation of marks between local and global ones, and 
navigation between them (where you have to remember whether your 
previous navigation was between files or inside one file) is extremely 
counter-intuitive, so I don't have a lot of experience with that facility.

Even so, I think it's been nice enough that every command can choose 
whether it pushes the mark to the local/global buffer rights, and/or it 
adds it to the xref marker stack. Do we have any particular guidelines 
in the manual for when either should happen?

xref-push-marker-stack is used externally as well as a replacement for 
find-tag-marker-ring (which is now marked obsolete). And any command 
that replaced the usage of the latter with the former, and also intended 
to push mark, probably does as a separate action.

> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> index 6974d00048..861e24a85f 100644
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -365,6 +365,7 @@ xref-set-marker-ring-length
>   
>   (defun xref-push-marker-stack (&optional m)
>     "Add point M (defaults to `point-marker') to the marker stack."
> +  (push-mark nil t)
>     (ring-insert xref--marker-ring (or m (point-marker))))

At the very least, it's missing a docstring update.

bug#34908: Push mark in xref-push-marker-stack

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Mon, 18 Mar 2019 23:12:50 +0200
> Cc: joão távora <joaotavora@gmail.com>,
> 	dmitry gutov <dgutov@yandex.ru>
> 
> Shouldn't xref-push-marker-stack push the mark like all normal commands do?

Fine with me to add that (I think etags.el-based M-. did this), but
please make sure the manual and NEWS are updated with this
information.

Thanks.

bug#34908: Push mark in xref-push-marker-stack

[Dmitry Gutov]

On 19.03.2019 8:16, Eli Zaretskii wrote:

> Fine with me to add that (I think etags.el-based M-. did this), but
> please make sure the manual and NEWS are updated with this
> information.

The closer counterpart to find-tag-in-order calling push-mark would be 
to add that call at the beginning of xref--show-xrefs.

bug#34908: Push mark in xref-push-marker-stack

[Juri Linkov]

>> Shouldn't xref-push-marker-stack push the mark like all normal commands do?
>
> Fine with me to add that (I think etags.el-based M-. did this), but
> please make sure the manual and NEWS are updated with this
> information.

I don't want to advertise this fix because the primary way to get back
for xref should be ‘M-,’.  Setting the mark is only needed for users
who expect that every significant change in position should leave a trace
in the global mark ring.

bug#34908: Push mark in xref-push-marker-stack

[Juri Linkov]

>> I know there is ‘M-,’ but why not allow using the standard keys
>> ‘C-x C-SPC’ (pop-global-mark) and ‘C-u C-SPC’ (in the same file)
>> as well?
>
> IMO that separation of marks between local and global ones, and navigation
> between them (where you have to remember whether your previous navigation
> was between files or inside one file) is extremely counter-intuitive, so
> I don't have a lot of experience with that facility.

Yes, its inconvenience is that you have to remember whether a previous
position was in the same file or not, and depending on this decide
what command to use: global or local pop.

> Even so, I think it's been nice enough that every command can choose
> whether it pushes the mark to the local/global buffer rights, and/or it
> adds it to the xref marker stack. Do we have any particular guidelines in
> the manual for when either should happen?

I think it should push to both.

> xref-push-marker-stack is used externally as well as a replacement for
> find-tag-marker-ring (which is now marked obsolete). And any command that
> replaced the usage of the latter with the former, and also intended to push
> mark, probably does as a separate action.

> The closer counterpart to find-tag-in-order calling push-mark would be to
> add that call at the beginning of xref--show-xrefs.

I'm not sure where to call push-mark: closer to the command,
or closer to ring-insert.  It seems a suitable place for
push-mark is in xref-push-marker-stack as its name suggests.

bug#34908: Push mark in xref-push-marker-stack

[Dmitry Gutov]

On 19.03.2019 23:02, Juri Linkov wrote:

> Yes, its inconvenience is that you have to remember whether a previous
> position was in the same file or not, and depending on this decide
> what command to use: global or local pop.

Sooner or later, I believe we should rethink that UI.

>> Even so, I think it's been nice enough that every command can choose
>> whether it pushes the mark to the local/global buffer rights, and/or it
>> adds it to the xref marker stack. Do we have any particular guidelines in
>> the manual for when either should happen?
> 
> I think it should push to both.

OK.

> I'm not sure where to call push-mark: closer to the command,
> or closer to ring-insert.  It seems a suitable place for
> push-mark is in xref-push-marker-stack as its name suggests.

I'd rather have a more localized change, at least for now. And leave 
xref-push-marker-stack to only modify its own data structure.

So how about this?

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 6974d00048..015ea16f34 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -797,6 +797,7 @@ xref--read-identifier-history
  (defvar xref--read-pattern-history nil)

  (defun xref--show-xrefs (xrefs display-action &optional always-show-list)
+  (push-mark nil t)
    (cond
     ((and (not (cdr xrefs)) (not always-show-list))
      (xref-push-marker-stack)

bug#34908: Push mark in xref-push-marker-stack

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 34908@debbugs.gnu.org,  joaotavora@gmail.com,  dgutov@yandex.ru
> Date: Tue, 19 Mar 2019 22:59:18 +0200
> 
> >> Shouldn't xref-push-marker-stack push the mark like all normal commands do?
> >
> > Fine with me to add that (I think etags.el-based M-. did this), but
> > please make sure the manual and NEWS are updated with this
> > information.
> 
> I don't want to advertise this fix because the primary way to get back
> for xref should be ‘M-,’.

Well, it must be in NEWS, since this is a change in behavior.

As for the manuals, it might be okay not to mention that, although I
admit I don't understand your reasoning: something that is not a
primary use doesn't automatically mean it shouldn't be documented.

> Setting the mark is only needed for users who expect that every
> significant change in position should leave a trace in the global
> mark ring.

I believe most of our commands that move to a far away position do set
the mark.

bug#34908: Push mark in xref-push-marker-stack

[Juri Linkov]

>> I don't want to advertise this fix because the primary way to get back
>> for xref should be ‘M-,’.
>
> Well, it must be in NEWS, since this is a change in behavior.
>
> As for the manuals, it might be okay not to mention that, although I
> admit I don't understand your reasoning: something that is not a
> primary use doesn't automatically mean it shouldn't be documented.
>
>> Setting the mark is only needed for users who expect that every
>> significant change in position should leave a trace in the global
>> mark ring.
>
> I believe most of our commands that move to a far away position do set
> the mark.

Yes, most of these commands set the mark, but none of them mention this fact
in the manual.  I looked at

  (info "(emacs) Moving Point")
  (info "(emacs) Moving by Defuns")
  etc.

there is nothing about setting the mark, because it's so obvious
that doesn't need a special reminder for every command.

So I don't know what to write in NEWS, something along the lines:
"like all commands that move to a far away position
 xref-find-definitions now sets the mark as well
 but please remember that the primary way to pop xref mark
 is still M-, (xref-pop-marker-stack)" ?

bug#34908: Push mark in xref-push-marker-stack

[Juri Linkov]

>> I'm not sure where to call push-mark: closer to the command,
>> or closer to ring-insert.  It seems a suitable place for
>> push-mark is in xref-push-marker-stack as its name suggests.
>
> I'd rather have a more localized change, at least for now. And leave
> xref-push-marker-stack to only modify its own data structure.

Do you think other commands that use xref-push-marker-stack
won't need setting the mark?

> So how about this?
>
> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> index 6974d00048..015ea16f34 100644
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -797,6 +797,7 @@ xref--read-identifier-history
>  (defvar xref--read-pattern-history nil)
>
>  (defun xref--show-xrefs (xrefs display-action &optional always-show-list)
> +  (push-mark nil t)

push-mark is usually called closer to the end of the command.

>    (cond
>     ((and (not (cdr xrefs)) (not always-show-list))
>      (xref-push-marker-stack)

Some commands like e.g. ‘goto-line’ use such idiom:

  (or (region-active-p) (push-mark))

documented in its docstring as

  Prior to moving point, this function sets the mark (without
  activating it), unless Transient Mark mode is enabled and the
  mark is already active.

bug#34908: Push mark in xref-push-marker-stack

[Dmitry Gutov]

On 20.03.2019 23:59, Juri Linkov wrote:

> Do you think other commands that use xref-push-marker-stack
> won't need setting the mark?

Like I said, they probably already do. And this is a matter of a 
function changing into its related data structure.

If we're going to make xref-push-marker-stack a 
single-stop-to-modify-all-stacks, that should be discussed first. Like I 
said, I don't have a lot of clarity about how and why people use the 
global/local marker stacks.

>>   (defun xref--show-xrefs (xrefs display-action &optional always-show-list)
>> +  (push-mark nil t)
> 
> push-mark is usually called closer to the end of the command.

You mean text-wise? I don't think it will make any practical difference 
in this case.

>>     (cond
>>      ((and (not (cdr xrefs)) (not always-show-list))
>>       (xref-push-marker-stack)
> 
> Some commands like e.g. ‘goto-line’ use such idiom:
> 
>    (or (region-active-p) (push-mark))

Sure, fine by me.

We'll still want to call xref-push-marker-stack unconditionally, though.

bug#34908: Push mark in xref-push-marker-stack

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 34908@debbugs.gnu.org,  joaotavora@gmail.com,  dgutov@yandex.ru
> Date: Wed, 20 Mar 2019 23:49:09 +0200
> 
> So I don't know what to write in NEWS, something along the lines:
> "like all commands that move to a far away position
>  xref-find-definitions now sets the mark as well
>  but please remember that the primary way to pop xref mark
>  is still M-, (xref-pop-marker-stack)" ?

Just

  xref-find-definitions now sets the mark at the buffer position where
  it was invoked

will do.

Thanks.

bug#34908: Push mark in xref-push-marker-stack

[Juri Linkov]

>> So I don't know what to write in NEWS, something along the lines:
>> "like all commands that move to a far away position
>>  xref-find-definitions now sets the mark as well
>>  but please remember that the primary way to pop xref mark
>>  is still M-, (xref-pop-marker-stack)" ?
>
> Just
>
>   xref-find-definitions now sets the mark at the buffer position where
>   it was invoked
>
> will do.

Done and pushed to master.