GNU bug report logs - #34908
Push mark in xref-push-marker-stack

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 34908 in the body.
You can then email your comments to 34908 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: bug-gnu-emacs <at> gnu.org
Subject: Push mark in xref-push-marker-stack
Date: Mon, 18 Mar 2019 23:12:50 +0200

X-Debbugs-CC: João Távora <joaotavora <at> gmail.com>, Dmitry Gutov <dgutov <at> 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))))
 

Message #8 received at 34908 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 34908 <at> debbugs.gnu.org, dmitry gutov <dgutov <at> yandex.ru>
Subject: Re: bug#34908: Push mark in xref-push-marker-stack
Date: Mon, 18 Mar 2019 21:50:58 +0000

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 <at> linkov.net> wrote:
>
> X-Debbugs-CC: João Távora <joaotavora <at> gmail.com>, Dmitry Gutov <dgutov <at> 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

Message #11 received at 34908 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Juri Linkov <juri <at> linkov.net>, 34908 <at> debbugs.gnu.org
Cc: joão távora <joaotavora <at> gmail.com>
Subject: Re: bug#34908: Push mark in xref-push-marker-stack
Date: Tue, 19 Mar 2019 01:14:26 +0200

On 18.03.2019 23:12, Juri Linkov wrote:
> X-Debbugs-CC: João Távora <joaotavora <at> gmail.com>, Dmitry Gutov <dgutov <at> 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.

Message #14 received at 34908 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 34908 <at> debbugs.gnu.org, joaotavora <at> gmail.com, dgutov <at> yandex.ru
Subject: Re: bug#34908: Push mark in xref-push-marker-stack
Date: Tue, 19 Mar 2019 08:16:35 +0200

> From: Juri Linkov <juri <at> linkov.net>
> Date: Mon, 18 Mar 2019 23:12:50 +0200
> Cc: joão távora <joaotavora <at> gmail.com>,
> 	dmitry gutov <dgutov <at> 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.

Message #17 received at 34908 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>, Juri Linkov <juri <at> linkov.net>
Cc: 34908 <at> debbugs.gnu.org, joaotavora <at> gmail.com
Subject: Re: bug#34908: Push mark in xref-push-marker-stack
Date: Tue, 19 Mar 2019 13:57:31 +0200

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.

Message #20 received at 34908 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 34908 <at> debbugs.gnu.org, joaotavora <at> gmail.com, dgutov <at> yandex.ru
Subject: Re: bug#34908: Push mark in xref-push-marker-stack
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-,’.  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.

Message #23 received at 34908 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 34908 <at> debbugs.gnu.org,
 joão távora <joaotavora <at> gmail.com>
Subject: Re: bug#34908: Push mark in xref-push-marker-stack
Date: Tue, 19 Mar 2019 23:02:43 +0200

>> 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.

Message #26 received at 34908 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Juri Linkov <juri <at> linkov.net>
Cc: 34908 <at> debbugs.gnu.org,
 joão távora <joaotavora <at> gmail.com>
Subject: Re: bug#34908: Push mark in xref-push-marker-stack
Date: Wed, 20 Mar 2019 03:47:32 +0200

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)

Message #29 received at 34908 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 34908 <at> debbugs.gnu.org, joaotavora <at> gmail.com, dgutov <at> yandex.ru
Subject: Re: bug#34908: Push mark in xref-push-marker-stack
Date: Wed, 20 Mar 2019 07:59:09 +0200

> From: Juri Linkov <juri <at> linkov.net>
> Cc: 34908 <at> debbugs.gnu.org,  joaotavora <at> gmail.com,  dgutov <at> 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.

Message #32 received at 34908 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 34908 <at> debbugs.gnu.org, joaotavora <at> gmail.com, dgutov <at> yandex.ru
Subject: Re: bug#34908: Push mark in xref-push-marker-stack
Date: Wed, 20 Mar 2019 23:49:09 +0200

>> 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)" ?

Message #35 received at 34908 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 34908 <at> debbugs.gnu.org,
 joão távora <joaotavora <at> gmail.com>
Subject: Re: bug#34908: Push mark in xref-push-marker-stack
Date: Wed, 20 Mar 2019 23:59:49 +0200

>> 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.

Message #38 received at 34908 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Juri Linkov <juri <at> linkov.net>
Cc: 34908 <at> debbugs.gnu.org,
 joão távora <joaotavora <at> gmail.com>
Subject: Re: bug#34908: Push mark in xref-push-marker-stack
Date: Thu, 21 Mar 2019 02:59:18 +0200

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.

Message #41 received at 34908 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 34908 <at> debbugs.gnu.org, joaotavora <at> gmail.com, dgutov <at> yandex.ru
Subject: Re: bug#34908: Push mark in xref-push-marker-stack
Date: Thu, 21 Mar 2019 05:35:37 +0200

> From: Juri Linkov <juri <at> linkov.net>
> Cc: 34908 <at> debbugs.gnu.org,  joaotavora <at> gmail.com,  dgutov <at> 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.

Message #46 received at 34908-done <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 34908-done <at> debbugs.gnu.org, joaotavora <at> gmail.com, dgutov <at> yandex.ru
Subject: Re: bug#34908: Push mark in xref-push-marker-stack
Date: Sun, 24 Mar 2019 23:20:19 +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.

Done and pushed to master.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.