* bug#34908: Push mark in xref-push-marker-stack
@ 2019-03-18 21:12 Juri Linkov
2019-03-18 21:50 ` João Távora
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Juri Linkov @ 2019-03-18 21:12 UTC (permalink / raw
To: 34908; +Cc: joão távora, dmitry gutov
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))))
^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#34908: Push mark in xref-push-marker-stack
2019-03-18 21:12 bug#34908: Push mark in xref-push-marker-stack Juri Linkov
@ 2019-03-18 21:50 ` João Távora
2019-03-18 23:14 ` Dmitry Gutov
2019-03-19 6:16 ` Eli Zaretskii
2 siblings, 0 replies; 14+ messages in thread
From: João Távora @ 2019-03-18 21:50 UTC (permalink / raw
To: Juri Linkov; +Cc: 34908, dmitry gutov
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#34908: Push mark in xref-push-marker-stack
2019-03-18 21:12 bug#34908: Push mark in xref-push-marker-stack Juri Linkov
2019-03-18 21:50 ` João Távora
@ 2019-03-18 23:14 ` Dmitry Gutov
2019-03-19 21:02 ` Juri Linkov
2019-03-19 6:16 ` Eli Zaretskii
2 siblings, 1 reply; 14+ messages in thread
From: Dmitry Gutov @ 2019-03-18 23:14 UTC (permalink / raw
To: Juri Linkov, 34908; +Cc: joão távora
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#34908: Push mark in xref-push-marker-stack
2019-03-18 21:12 bug#34908: Push mark in xref-push-marker-stack Juri Linkov
2019-03-18 21:50 ` João Távora
2019-03-18 23:14 ` Dmitry Gutov
@ 2019-03-19 6:16 ` Eli Zaretskii
2019-03-19 11:57 ` Dmitry Gutov
2019-03-19 20:59 ` Juri Linkov
2 siblings, 2 replies; 14+ messages in thread
From: Eli Zaretskii @ 2019-03-19 6:16 UTC (permalink / raw
To: Juri Linkov; +Cc: 34908, joaotavora, dgutov
> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#34908: Push mark in xref-push-marker-stack
2019-03-19 6:16 ` Eli Zaretskii
@ 2019-03-19 11:57 ` Dmitry Gutov
2019-03-19 20:59 ` Juri Linkov
1 sibling, 0 replies; 14+ messages in thread
From: Dmitry Gutov @ 2019-03-19 11:57 UTC (permalink / raw
To: Eli Zaretskii, Juri Linkov; +Cc: 34908, joaotavora
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#34908: Push mark in xref-push-marker-stack
2019-03-19 6:16 ` Eli Zaretskii
2019-03-19 11:57 ` Dmitry Gutov
@ 2019-03-19 20:59 ` Juri Linkov
2019-03-20 5:59 ` Eli Zaretskii
1 sibling, 1 reply; 14+ messages in thread
From: Juri Linkov @ 2019-03-19 20:59 UTC (permalink / raw
To: Eli Zaretskii; +Cc: 34908, joaotavora, dgutov
>> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#34908: Push mark in xref-push-marker-stack
2019-03-18 23:14 ` Dmitry Gutov
@ 2019-03-19 21:02 ` Juri Linkov
2019-03-20 1:47 ` Dmitry Gutov
0 siblings, 1 reply; 14+ messages in thread
From: Juri Linkov @ 2019-03-19 21:02 UTC (permalink / raw
To: Dmitry Gutov; +Cc: 34908, joão távora
>> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#34908: Push mark in xref-push-marker-stack
2019-03-19 21:02 ` Juri Linkov
@ 2019-03-20 1:47 ` Dmitry Gutov
2019-03-20 21:59 ` Juri Linkov
0 siblings, 1 reply; 14+ messages in thread
From: Dmitry Gutov @ 2019-03-20 1:47 UTC (permalink / raw
To: Juri Linkov; +Cc: 34908, joão távora
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)
^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#34908: Push mark in xref-push-marker-stack
2019-03-19 20:59 ` Juri Linkov
@ 2019-03-20 5:59 ` Eli Zaretskii
2019-03-20 21:49 ` Juri Linkov
0 siblings, 1 reply; 14+ messages in thread
From: Eli Zaretskii @ 2019-03-20 5:59 UTC (permalink / raw
To: Juri Linkov; +Cc: 34908, joaotavora, dgutov
> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#34908: Push mark in xref-push-marker-stack
2019-03-20 5:59 ` Eli Zaretskii
@ 2019-03-20 21:49 ` Juri Linkov
2019-03-21 3:35 ` Eli Zaretskii
0 siblings, 1 reply; 14+ messages in thread
From: Juri Linkov @ 2019-03-20 21:49 UTC (permalink / raw
To: Eli Zaretskii; +Cc: 34908, joaotavora, dgutov
>> 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)" ?
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#34908: Push mark in xref-push-marker-stack
2019-03-20 1:47 ` Dmitry Gutov
@ 2019-03-20 21:59 ` Juri Linkov
2019-03-21 0:59 ` Dmitry Gutov
0 siblings, 1 reply; 14+ messages in thread
From: Juri Linkov @ 2019-03-20 21:59 UTC (permalink / raw
To: Dmitry Gutov; +Cc: 34908, joão távora
>> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#34908: Push mark in xref-push-marker-stack
2019-03-20 21:59 ` Juri Linkov
@ 2019-03-21 0:59 ` Dmitry Gutov
0 siblings, 0 replies; 14+ messages in thread
From: Dmitry Gutov @ 2019-03-21 0:59 UTC (permalink / raw
To: Juri Linkov; +Cc: 34908, joão távora
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#34908: Push mark in xref-push-marker-stack
2019-03-20 21:49 ` Juri Linkov
@ 2019-03-21 3:35 ` Eli Zaretskii
2019-03-24 21:20 ` Juri Linkov
0 siblings, 1 reply; 14+ messages in thread
From: Eli Zaretskii @ 2019-03-21 3:35 UTC (permalink / raw
To: Juri Linkov; +Cc: 34908, joaotavora, dgutov
> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#34908: Push mark in xref-push-marker-stack
2019-03-21 3:35 ` Eli Zaretskii
@ 2019-03-24 21:20 ` Juri Linkov
0 siblings, 0 replies; 14+ messages in thread
From: Juri Linkov @ 2019-03-24 21:20 UTC (permalink / raw
To: Eli Zaretskii; +Cc: 34908-done, joaotavora, dgutov
>> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-03-24 21:20 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-18 21:12 bug#34908: Push mark in xref-push-marker-stack Juri Linkov
2019-03-18 21:50 ` João Távora
2019-03-18 23:14 ` Dmitry Gutov
2019-03-19 21:02 ` Juri Linkov
2019-03-20 1:47 ` Dmitry Gutov
2019-03-20 21:59 ` Juri Linkov
2019-03-21 0:59 ` Dmitry Gutov
2019-03-19 6:16 ` Eli Zaretskii
2019-03-19 11:57 ` Dmitry Gutov
2019-03-19 20:59 ` Juri Linkov
2019-03-20 5:59 ` Eli Zaretskii
2019-03-20 21:49 ` Juri Linkov
2019-03-21 3:35 ` Eli Zaretskii
2019-03-24 21:20 ` Juri Linkov
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.