From: Dmitry Gutov <dgutov@yandex.ru>
To: Eli Zaretskii <eliz@gnu.org>
Cc: 35702@debbugs.gnu.org, juri@linkov.net
Subject: bug#35702: xref revert-buffer
Date: Fri, 24 May 2019 18:15:57 +0300 [thread overview]
Message-ID: <9869e4ac-1b36-b605-22a8-b8bab4910132@yandex.ru> (raw)
In-Reply-To: <831s0o54g7.fsf@gnu.org>
[-- Attachment #1: Type: text/plain, Size: 6062 bytes --]
On 24.05.2019 17:10, Eli Zaretskii wrote:
>> I'm not sure we should document that in the command's docstring, though,
>> because I think we'll end up with a more different UI for
>> xref-find-definitions results, with a different major mode and a keymap
>> where 'g' is simply not bound.
>
> I'm not a great fan of obfuscating the docs, except for reasons of
> hiding internal implementation details.
First: my point is, it's possible that it *will* always work wherever
you are able to invoke it with 'g'. The Xref buffers where it wouldn't
work wouldn't bind 'g'.
Second: we can "obfuscate" the docs for things we're not sure about.
Think forward: if we expose the Xref UI as a library for other packages
in the future (and we probably will), should we allow some of them to
opt out of revert-ability (for simplicity, let's say), or not?
And if we do, we won't really be able to enumerate all the commands that
opted out in xref--revert-xref-buffer's docstring.
> Why is it a problem to say
> that XREF buffers created by xref-find-definitions currently don't
> support 'g'?
Because it feels ad-hoc and kinda weird. The intention is to support
reverting in "search results" buffers, not intentionally refuse support
it in xref-find-definitions.
But we can do that.
> For that matter, why shouldn't the error message be
> explicit about that, instead of saying something technically accurate
> but quite obscure from user's POV?
How would you phrase it?
>> *** Refreshing the search results
>> When an Xref buffer shows search results (e.g. from
>> xref-find-references or project-find-regexp) you can type 'g' to
>> repeat the search and refresh the results.
>
> This should say explicitly that only some Xref functions can support
> refreshing the results. "E.g." can be interpreted as just an example,
> not that some other examples don't support this.
The intent was to introduce a kind of classification. That is, to call
all commands that do a "wide" search as "commands that show search results".
xref-find-definitions, meanwhile, might be considered a "jump with
completions" command.
>> Right, but how often would the event of updading the TAGS file with
>> coincide with you wanting to refresh the xref-find-definitions results?
>
> When the original M-. doesn't show what I expected, for example.
Hmm. So it's something you would really find useful?
>> Wouldn't you just do the search again with 'M-.'?
>
> Yes, but that argument is true for any 'revert' action as well, isn't
> it? And we still have revert actions.
Not exactly: first, M-. is easier to invoke and re-invoke than
project-find-regexp, and you are likely to edit the regexp before searching.
Second, I quite frequently do something like this:
- Do a search for a string with project-find-regexp,
- Do *something* with the results, e.g. rename them all,
- Repeat the search, to make sure I have dealt with all occurrences.
So 'g' helps considerably there. And I don't have any similar scenarios
for xref-find-definitions.
To be clear, though, we *can* add support for reverting to
xref-find-definitions as well, if you want. Just at the cost of a
certain increase of complexity, see if you like the patch.
xref-find-defitions-revertable.diff is attached.
> OK, but (a) the heading sentence should end in a period; (b) please
> use 2 spaces between sentences.
OK.
>> You can probably see a certain level of duplication there, though.
>
> I don't. I see one entry referencing another, that's all.
Just to be clear: I'm referring to two of the three entries I've showed
in the previous email mentioning "search-type Xref commands".
>> This idea is pretty simple: instead of passing a list of search results
>> to xref--show-xrefs, we pass an anonymous function that can be called to
>> do the search, as well as repeat it, and returns the said list.
>
> <rant>
> The idea is simple and clear, but trying to understand what it does in
> any particular invocation isn't. I tried to figure out what happens
> after M-., which required me to understand when is the xref--fetcher
> variable set and to what values. There's no easy answer to that
> question, neither in comments nor in the doc strings.
Would a docstring saying "Function that returns a list of xrefs
containing the search results" change things?
> The value is
> set from a function passed as an argument to a couple of other
> internal functions, so now I need to go deeper into the rabbit hole
> and understand when and how these two functions are called, and with
> what function as that argument.
Where the fetcher is created is not to hard to find, I think (only a few
local variables with that name).
> Etc. etc. -- it feels like following
> a deeply OO C++ code, where basically the only way to figure out how
> the code works is to step through the code with a debugger.
It's actually more "functional programming" than OOP, and not the worst
example of it (very little function composition, for instance).
I can feel your pain (there's a lot of the code I have difficulty
following as well), but I'm not sure where I could add comments where
they wouldn't be self-evident. A couple of docstrings wouldn't hurt, though.
> Except
> that Edebug doesn't support generics well enough to do that, so I'm
> screwed even if I have the time and energy to go down that hole.
Well, at least in this case there are no generic calls between
xref-find-references and xref--show-xrefs and the fetcher creation.
> It used to be possible to understand what happens in a Lisp program by
> just reading the code, but it no longer is, in many cases.
Err, my experience of reading old Lisp code in various packages doesn't
really agree.
> Useful comments can go a long way towards making such investigations
> much easier and more efficient, but I guess I will hear
> counter-arguments about the need to keep comments up-to-date with the
> code...
It's not like I'm against those, but it might take a different person to
identify the places that need to be commented.
[-- Attachment #2: xref-find-definitions-revertable.diff --]
[-- Type: text/x-patch, Size: 3228 bytes --]
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 6a4906a232..0fd59dc330 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -782,7 +782,10 @@ xref--analyze
#'equal))
(defun xref--show-xref-buffer (fetcher alist)
- (let* ((xrefs (if (functionp fetcher) (funcall fetcher) fetcher))
+ (let* ((xrefs
+ (or
+ (assoc-default 'xrefs alist)
+ (funcall fetcher)))
(xref-alist (xref--analyze xrefs)))
(with-current-buffer (get-buffer-create xref-buffer-name)
(setq buffer-undo-list nil)
@@ -817,13 +820,16 @@ xref--revert-xref-buffer
'face 'error))))
(goto-char (point-min)))))
-(defun xref--show-defs-buffer (xrefs alist)
- (cond
- ((not (cdr xrefs))
- (xref--pop-to-location (car xrefs)
- (assoc-default 'display-action alist)))
- (t
- (xref--show-xref-buffer xrefs alist))))
+(defun xref--show-defs-buffer (fetcher alist)
+ (let ((xrefs (funcall fetcher)))
+ (cond
+ ((not (cdr xrefs))
+ (xref--pop-to-location (car xrefs)
+ (assoc-default 'display-action alist)))
+ (t
+ (xref--show-xref-buffer fetcher
+ (cons (cons 'xrefs xrefs)
+ alist))))))
\f
(defvar xref-show-xrefs-function 'xref--show-xref-buffer
@@ -885,28 +891,29 @@ xref--read-identifier
;;; Commands
(defun xref--find-xrefs (input kind arg display-action)
+ (xref--show-xrefs
+ (xref--create-fetcher input kind arg)
+ display-action))
+
+(defun xref--find-definitions (id display-action)
+ (xref--show-defs
+ (xref--create-fetcher id 'definitions id)
+ display-action))
+
+(defun xref--create-fetcher (input kind arg)
(let* ((orig-buffer (current-buffer))
(orig-position (point))
(backend (xref-find-backend))
- (method (intern (format "xref-backend-%s" kind)))
- (fetcher (lambda ()
- (save-excursion
- (when (buffer-live-p orig-buffer)
- (set-buffer orig-buffer)
- (ignore-errors (goto-char orig-position)))
- (let ((xrefs (funcall method backend arg)))
- (unless xrefs
- (xref--not-found-error kind input))
- xrefs)))))
- (xref--show-xrefs fetcher display-action)))
-
-(defun xref--find-definitions (id display-action)
- (let ((xrefs (funcall #'xref-backend-definitions
- (xref-find-backend)
- id)))
- (unless xrefs
- (xref--not-found-error 'definitions id))
- (xref--show-defs xrefs display-action)))
+ (method (intern (format "xref-backend-%s" kind))))
+ (lambda ()
+ (save-excursion
+ (when (buffer-live-p orig-buffer)
+ (set-buffer orig-buffer)
+ (ignore-errors (goto-char orig-position)))
+ (let ((xrefs (funcall method backend arg)))
+ (unless xrefs
+ (xref--not-found-error kind input))
+ xrefs)))))
(defun xref--not-found-error (kind input)
(user-error "No %s found for: %s" (symbol-name kind) input))
next prev parent reply other threads:[~2019-05-24 15:15 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-12 19:45 bug#35702: xref revert-buffer Juri Linkov
2019-05-24 1:51 ` Dmitry Gutov
2019-05-24 8:36 ` Eli Zaretskii
2019-05-24 10:09 ` Dmitry Gutov
2019-05-24 12:25 ` Eli Zaretskii
2019-05-24 12:57 ` Dmitry Gutov
2019-05-24 14:10 ` Eli Zaretskii
2019-05-24 14:26 ` Dmitry Gutov
2019-05-24 15:02 ` Eli Zaretskii
2019-05-24 22:35 ` Dmitry Gutov
2019-05-24 15:15 ` Dmitry Gutov [this message]
2019-05-24 19:35 ` Eli Zaretskii
2019-05-24 20:51 ` Dmitry Gutov
2019-05-25 7:39 ` Eli Zaretskii
2019-05-25 15:47 ` Dmitry Gutov
2019-05-25 16:06 ` Eli Zaretskii
2019-05-25 16:14 ` Dmitry Gutov
2019-05-25 16:49 ` Eli Zaretskii
2019-05-25 21:33 ` Dmitry Gutov
2019-05-26 16:44 ` Eli Zaretskii
2019-05-27 14:54 ` Dmitry Gutov
2019-05-27 16:31 ` Eli Zaretskii
2019-05-28 14:10 ` Dmitry Gutov
2019-05-28 18:41 ` Eli Zaretskii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9869e4ac-1b36-b605-22a8-b8bab4910132@yandex.ru \
--to=dgutov@yandex.ru \
--cc=35702@debbugs.gnu.org \
--cc=eliz@gnu.org \
--cc=juri@linkov.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.