Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Robert Pluim]

>>>>> On Thu,  1 Aug 2019 15:54:05 -0400 (EDT), larsi@gnus.org (Lars Ingebrigtsen) said:

    Lars> branch: master
    Lars> commit fe939b36f901645e976bf016d8766c3a1300e45c
    Lars> Author: Lars Ingebrigtsen <larsi@gnus.org>
    Lars> Commit: Lars Ingebrigtsen <larsi@gnus.org>

    Lars>     Fix reference to `tags-loop-continue' in doc string
    
    Lars>     * lisp/dired-aux.el (dired-do-search): Refer to
    Lars>     `fileloop-continue' instead of the obsolete `tags-loop-continue'
    Lars>     (bug#21475).

Hmm, in the interests of backwards compatibility, could
'xref-pop-marker-stack' not check 'last-command'?

Robert

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

>     Lars>     Fix reference to `tags-loop-continue' in doc string
>
>     Lars>     * lisp/dired-aux.el (dired-do-search): Refer to
>     Lars>     `fileloop-continue' instead of the obsolete `tags-loop-continue'
>     Lars>     (bug#21475).
>
> Hmm, in the interests of backwards compatibility, could
> 'xref-pop-marker-stack' not check 'last-command'?

Hm...  would that help?  The UI previously here was that you could just
hit `M-,' (or was it `M-.'?) and go to the next hit -- no matter what
the previous command was?  I think?

But `xref-pop-marker-stack' could perhaps possibly check whether
`fileloop-continue' has something to offer instead of erroring out?

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

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Robert Pluim]

>>>>> On Fri, 02 Aug 2019 12:49:19 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Robert Pluim <rpluim@gmail.com> writes:
    Lars> Fix reference to `tags-loop-continue' in doc string
    >> 
    Lars> * lisp/dired-aux.el (dired-do-search): Refer to
    Lars> `fileloop-continue' instead of the obsolete `tags-loop-continue'
    Lars> (bug#21475).
    >> 
    >> Hmm, in the interests of backwards compatibility, could
    >> 'xref-pop-marker-stack' not check 'last-command'?

    Lars> Hm...  would that help?  The UI previously here was that you could just
    Lars> hit `M-,' (or was it `M-.'?) and go to the next hit -- no matter what
    Lars> the previous command was?  I think?

I think youʼre right. Unfortunately fileloop has no "fileloop is in
progress" flag, nor would it help, because sometimes you do want the
xref behaviour.

    Lars> But `xref-pop-marker-stack' could perhaps possibly check whether
    Lars> `fileloop-continue' has something to offer instead of erroring out?

Ah, you mean like this? I hope I have the eval-when-compile stuff
right. And if you do 'dired-do-search' followed by
'xref-find-definitions' it would be easy to confuse yourself.

We could find a new binding for fileloop-continue, which also breaks
backwards compatibility, but then at least thereʼs a default binding.

diff --git i/lisp/progmodes/xref.el w/lisp/progmodes/xref.el
index 8dc4f3c471..d9245e2822 100644
--- i/lisp/progmodes/xref.el
+++ w/lisp/progmodes/xref.el
@@ -70,6 +70,7 @@
 (require 'eieio)
 (require 'ring)
 (require 'project)
+(eval-when-compile (declare-function fileloop-continue "fileloop" ()))
 
 (defgroup xref nil "Cross-referencing commands"
   :version "25.1"
@@ -380,14 +381,15 @@ xref-pop-marker-stack
   "Pop back to where \\[xref-find-definitions] was last invoked."
   (interactive)
   (let ((ring xref--marker-ring))
-    (when (ring-empty-p ring)
-      (user-error "Marker stack is empty"))
-    (let ((marker (ring-remove ring 0)))
-      (switch-to-buffer (or (marker-buffer marker)
-                            (user-error "The marked buffer has been deleted")))
-      (goto-char (marker-position marker))
-      (set-marker marker nil nil)
-      (run-hooks 'xref-after-return-hook))))
+    (if (ring-empty-p ring)
+        ;; Just in case we were in a fileloop sequence
+        (fileloop-continue)
+      (let ((marker (ring-remove ring 0)))
+        (switch-to-buffer (or (marker-buffer marker)
+                              (user-error "The marked buffer has been deleted")))
+        (goto-char (marker-position marker))
+        (set-marker marker nil nil)
+        (run-hooks 'xref-after-return-hook)))))
 
 (defvar xref--current-item nil)
 

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> Ah, you mean like this? I hope I have the eval-when-compile stuff
> right. And if you do 'dired-do-search' followed by
> 'xref-find-definitions' it would be easy to confuse yourself.
>
> We could find a new binding for fileloop-continue, which also breaks
> backwards compatibility, but then at least thereʼs a default binding.

[...]

> +(eval-when-compile (declare-function fileloop-continue "fileloop" ()))

I don't think this is necessary -- you can just have the
`declare-function' without any `eval-when-compile'.  But you should
probably include the arg list?

> +    (if (ring-empty-p ring)
> +        ;; Just in case we were in a fileloop sequence
> +        (fileloop-continue)

Yes, that's what I wondered would work.  Are there any drawbacks to
doing something like this?  Perhaps the interface becomes a bit...
unpredictable?

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

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Robert Pluim]

>>>>> On Fri, 02 Aug 2019 20:27:22 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Robert Pluim <rpluim@gmail.com> writes:
    >> Ah, you mean like this? I hope I have the eval-when-compile stuff
    >> right. And if you do 'dired-do-search' followed by
    >> 'xref-find-definitions' it would be easy to confuse yourself.
    >> 
    >> We could find a new binding for fileloop-continue, which also breaks
    >> backwards compatibility, but then at least thereʼs a default binding.

    Lars> [...]

    >> +(eval-when-compile (declare-function fileloop-continue "fileloop" ()))

    Lars> I don't think this is necessary -- you can just have the
    Lars> `declare-function' without any `eval-when-compile'.  But you should
    Lars> probably include the arg list?

I did :-)

fileloop.el:

    (defun fileloop-continue ()

    >> +    (if (ring-empty-p ring)
    >> +        ;; Just in case we were in a fileloop sequence
    >> +        (fileloop-continue)

    Lars> Yes, that's what I wondered would work.  Are there any drawbacks to
    Lars> doing something like this?  Perhaps the interface becomes a bit...
    Lars> unpredictable?

Itʼs perfectly deterministic, just potentially confusing :-)

It begs the question why xrefs replaced a 'do the next thing' type
binding with a 'go back' type binding in the first place. Iʼm tempted
to say that xref-pop-marker-stack should use a different binding, and
M-, should be fileloop-continue, but the xref one has existed for 5
years now.

Robert

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

>     >> +(eval-when-compile (declare-function fileloop-continue "fileloop" ()))
>
>     Lars> I don't think this is necessary -- you can just have the
>     Lars> `declare-function' without any `eval-when-compile'.  But you should
>     Lars> probably include the arg list?
>
> I did :-)
>
> fileloop.el:
>
>     (defun fileloop-continue ()

Oops.  I thought I checked.

> It begs the question why xrefs replaced a 'do the next thing' type
> binding with a 'go back' type binding in the first place. Iʼm tempted
> to say that xref-pop-marker-stack should use a different binding, and
> M-, should be fileloop-continue, but the xref one has existed for 5
> years now.

Yeah, I think it's too late to make xref-pop-marker-stack go to a
different binding.

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

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Robert Pluim]

>>>>> On Sun, 04 Aug 2019 14:09:06 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:
    >> It begs the question why xrefs replaced a 'do the next thing' type
    >> binding with a 'go back' type binding in the first place. Iʼm tempted
    >> to say that xref-pop-marker-stack should use a different binding, and
    >> M-, should be fileloop-continue, but the xref one has existed for 5
    >> years now.

    Lars> Yeah, I think it's too late to make xref-pop-marker-stack go to a
    Lars> different binding.

Then either we make xref-pop-marker-stack try fileloop-continue, or we
add a new binding for fileloop-continue along with a mea culpa in NEWS.

Robert

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> Then either we make xref-pop-marker-stack try fileloop-continue, or we
> add a new binding for fileloop-continue along with a mea culpa in NEWS.

I guess it's possible to both have an xref going as well as something
that `fileloop-continue' would react to?

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

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Dmitry Gutov]

On 8/4/19 3:24 PM, Robert Pluim wrote:
> Then either we make xref-pop-marker-stack try fileloop-continue, or we
> add a new binding for fileloop-continue along with a mea culpa in NEWS.

We do neither.

We don't have any default bindings for commands that use fileloop 
(AFAIK), and if the user sets them up, they can also add a 
fileloop-continue key binding in their init script.

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> We don't have any default bindings for commands that use fileloop
> (AFAIK), and if the user sets them up, they can also add a
> fileloop-continue key binding in their init script.

This is from a discussion of `dired-do-search', which does use
fileloop-continue...

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

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>> We don't have any default bindings for commands that use fileloop
>> (AFAIK), and if the user sets them up, they can also add a
>> fileloop-continue key binding in their init script.
>
> This is from a discussion of `dired-do-search', which does use
> fileloop-continue...

Ah, but that command doesn't have a key binding.  Never mind.

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

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Robert Pluim]

>>>>> On Mon, 05 Aug 2019 12:54:05 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Lars Ingebrigtsen <larsi@gnus.org> writes:
    >> Dmitry Gutov <dgutov@yandex.ru> writes:
    >> 
    >>> We don't have any default bindings for commands that use fileloop
    >>> (AFAIK), and if the user sets them up, they can also add a
    >>> fileloop-continue key binding in their init script.
    >> 
    >> This is from a discussion of `dired-do-search', which does use
    >> fileloop-continue...

    Lars> Ah, but that command doesn't have a key binding.  Never mind.

No, but 'tags-loop-continue' did.

Robert

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Dmitry Gutov]

On 8/4/19 3:03 PM, Robert Pluim wrote:
> It begs the question why xrefs replaced a 'do the next thing' type
> binding with a 'go back' type binding in the first place.

Because it's more useful. The xref UI presents you with all matches 
right away, and you choose before visiting, so you don't really need the 
"go next" binding 98% of the time.

There's also 'next-error', which navigates between the matches too.

> Iʼm tempted
> to say that xref-pop-marker-stack should use a different binding, and
> M-, should be fileloop-continue, but the xref one has existed for 5
> years now.

Exactly. That ship has sailed. If you are still not convinced, please go 
ahead and look up the older discussions for our reasoning.

RE: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Drew Adams]

> > It begs the question why xrefs replaced a 'do the next thing' type
> > binding with a 'go back' type binding in the first place.
> 
> Because it's more useful. The xref UI presents you with all matches
> right away, and you choose before visiting, so you don't really need
> the "go next" binding 98% of the time.

I would say "also useful" and "differently useful",
or "sometimes more useful" - not "more useful".

Is `occur' (show all matches, let you get directly
to any, in any order) more useful than incremental
search?

Or do you use them both, for different things, in
different contexts?  I use them both.

FWIW, I was the first to provide a way to get
occur-like behavior during completion (in Icicles),
to let you search or search-&-replace without
needing to visit each matching occurrence, in turn.

So I fully recognize the advantages of direct
access to search/find hits over the more exhaustive
approach of visit-each-in-order.

But I also recognize that a single tool - even a
good, flexible one - is not necessarily the best
tool for all jobs.

It was fine to add `xref' to the tool kit.  Great.
It was not so fine (IMHO) to have it take over the
longstanding keys used for the existing `do the
next thing' commands (e.g., `dired-do-search',
`dired-do-query-replace').

Addition, not replacement, would have been TRT.
And yes, I think it was done precipitously, in
addition to not being necessary.

 "xrefs replaced a 'do the next thing' type
  binding with a 'go back' type binding"

> > xref-pop-marker-stack should use a different
> > binding, and M-, should be fileloop-continue,
> > but the xref one has existed for 5 years now.
> 
> Exactly. That ship has sailed.

The bindings were repurposed, and perhaps it's
too late to back out that mistake.

But why is it too late to rehabilitate the
`do the next thing' commands and their doc,
and give them key bindings once again?  Why
promote only `xref', essentially deprecating
the others?

Why not promote (document, bind, etc.) both
approaches?  We don't denigrate Isearch just
because we have `occur'.  Why was it good to
_replace_ the `do the next' commands with
`xref'?

> If you are still not convinced, please go
> ahead and look up the older discussions for
> our reasoning.

I read it all at the time, argued against
repurposing those longstanding keys at the
time, and I'm still not convinced that Emacs
did the right thing in this regard.

I'm convinced of the utility of `xref' (and
I always was).  I'm not convinced that the
existing commands/keys had to be sacrificed
in order to promote what you see as "more
useful" (even perhaps as best for all uses).

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Dmitry Gutov]

On 8/5/19 6:05 PM, Drew Adams wrote:

> I would say "also useful" and "differently useful",
> or "sometimes more useful" - not "more useful".
> 
> Is `occur' (show all matches, let you get directly
> to any, in any order) more useful than incremental
> search?

The question was about xref, not general Emacs usage. Since M-. is bound 
to xref-find-definitions, M-, should do the action that makes most sense 
for the former.

> But I also recognize that a single tool - even a
> good, flexible one - is not necessarily the best
> tool for all jobs.

That is true, and we provide different commands and ways of interaction. 
But the number of useful, short key bindings is limited, and we have to 
make a good, consistent choices in that regard that would benefit the 
most users.

> But why is it too late to rehabilitate the
> `do the next thing' commands and their doc,
> and give them key bindings once again?  Why
> promote only `xref', essentially deprecating
> the others?

I'm not seeing a specific proposal, like which keys exactly would you 
want to use. Maybe make a bug report specifically about that.

IIRC pop-tag-mark was bound to 'M-*'? It seems unused.

> Why not promote (document, bind, etc.) both
> approaches?  We don't denigrate Isearch just
> because we have `occur'.

Because bindings loads of different commands to many different, long 
keybindings is not as useful as some people seem to think.

> I'm convinced of the utility of `xref' (and
> I always was).  I'm not convinced that the
> existing commands/keys had to be sacrificed
> in order to promote what you see as "more
> useful" (even perhaps as best for all uses).

I wouldn't say that any commands were sacrificed.

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Juri Linkov]

>>> xref-pop-marker-stack should use a different
>>> binding, and M-, should be fileloop-continue,

> But why is it too late to rehabilitate the
> `do the next thing' commands and their doc,
> and give them key bindings once again?

M-. runs the command xref-find-definitions
M-, runs the command xref-pop-marker-stack
C-M-. runs the command xref-find-apropos
C-M-, is undefined

so C-M-, is free to use for fileloop-continue

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> M-. runs the command xref-find-definitions
> M-, runs the command xref-pop-marker-stack
> C-M-. runs the command xref-find-apropos
> C-M-, is undefined
>
> so C-M-, is free to use for fileloop-continue

That seems like a logical key binding for that command.

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

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Dmitry Gutov]

On 07.08.2019 21:33, Lars Ingebrigtsen wrote:
> Juri Linkov <juri@linkov.net> writes:
> 
>> M-. runs the command xref-find-definitions
>> M-, runs the command xref-pop-marker-stack
>> C-M-. runs the command xref-find-apropos
>> C-M-, is undefined
>>
>> so C-M-, is free to use for fileloop-continue
> 
> That seems like a logical key binding for that command.

Even though it's not related to any of the other commands in the list above?

I'm not really against it, but it doesn't sound particularly "logical".

M-*, on the other hand, seems somewhat pertinent: an asterisk is usually 
related to a set of items, and e.g. in Vim '*' jumps between occurrences 
of a symbol.

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Juri Linkov]

>>> M-. runs the command xref-find-definitions
>>> M-, runs the command xref-pop-marker-stack
>>> C-M-. runs the command xref-find-apropos
>>> C-M-, is undefined
>>>
>>> so C-M-, is free to use for fileloop-continue
>>
>> That seems like a logical key binding for that command.
>
> Even though it's not related to any of the other commands in the list above?
>
> I'm not really against it, but it doesn't sound particularly "logical".
>
> M-*, on the other hand, seems somewhat pertinent: an asterisk is usually
> related to a set of items, and e.g. in Vim '*' jumps between occurrences of
> a symbol.

I agree, M-* is mnemonic as well.

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > I would say "also useful" and "differently useful",

Whatever meaning is intended, "differently useful" is a clumsy way
to word it.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: master fe939b3 1/2: Fix reference to `tags-loop-continue' in doc string

[Dmitry Gutov]

On 8/2/19 9:27 PM, Lars Ingebrigtsen wrote:
> Yes, that's what I wondered would work.  Are there any drawbacks to
> doing something like this?  Perhaps the interface becomes a bit...
> unpredictable?

I'm firmly against doing something like this. One is a "go back" action, 
another is a "do next" action. They don't mix.