Package: auctex;
Reported by: "Paul D. Nelson" <ultrono <at> gmail.com>
Date: Thu, 27 Feb 2025 18:31:02 UTC
Severity: normal
Found in version 14.0.9
Done: Stefan Monnier <monnier <at> iro.umontreal.ca>
Bug is archived. No further changes may be made.
Message #65 received at 76615 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: "Paul D. Nelson" <ultrono <at> gmail.com> Cc: ikumi <at> ikumi.que.jp, arash <at> gnu.org, 76615 <at> debbugs.gnu.org Subject: Re: bug#76615: 14.0.9; error with amsmath style hooks Date: Wed, 02 Apr 2025 18:13:33 -0400
> I agree that the right approach is to make RefTeX work properly in
> non-file buffers. To explore this, I implemented the approach you
> suggested (see the attached patch).
Looks pretty good. Matches my expectations fairly well.
Should we push it to `master`?
AFAICT it should be safe in the sense that it should not affect behavior
in file buffers. See comments below.
> This bug report arose from a simpler issue: C-x v v C-c C-w in a tex
> document (with AUCTeX installed) makes vc activate LaTeX-mode in a
> temporary non-file buffer, where AUCTeX style hooks attempt to load
> RefTeX (catastrophically). For this narrow issue, the workaround
> provided by Ikumi suffices.
Yes, that was clear, thanks.
Stefan
> @@ -456,7 +458,7 @@ reftex-isearch-isearch-search
> ;; beginning/end of the file list, depending of the search direction.
> (defun reftex-isearch-switch-to-next-file (crt-buf &optional wrapp)
> (reftex-access-scan-info)
> - (let ((cb (buffer-file-name crt-buf))
> + (let ((cb (reftex-get-buffer-identifier crt-buf))
> (flist (reftex-all-document-files)))
> (when flist
> (if wrapp
I think we had better use the buffer object here instead of using
"buffer:<NAME>". More generally, I think we should never convert
"buffer:<NAME>" back to a buffer. I guess that means such a conversion
would only ever be used when displaying info for a human to read, in
which case we could just as well use `prin1`s format `#<buffer ...>`.
> --- a/lisp/textmodes/reftex-ref.el
> +++ b/lisp/textmodes/reftex-ref.el
> @@ -73,8 +73,13 @@ reftex-label-info-update
> (file (nth 3 cell))
> (comment (nth 4 cell))
> (note (nth 5 cell))
> - (buf (reftex-get-file-buffer-force
> - file (not (eq t reftex-keep-temporary-buffers)))))
> + (buf (cond
> + ((and (stringp file)
> + (string-match "\\`buffer:\\(.*\\)" file))
> + (get-buffer (match-string 1 file)))
> + (file (reftex-get-file-buffer-force
> + file (not (eq t reftex-keep-temporary-buffers))))
> + (t nil))))
So here presumably we'd use
(cond
((bufferp file) file)
[...]
and I wouldn't test `file` before calling `reftex-get-file-buffer-force`
since we didn't test it before either.
> +;;; =========================================================================
> +;;;
> +;;; Helper functions for handling both file paths and buffer objects
> +;;;
> +
> +(defun reftex-get-buffer-identifier (&optional buffer)
> + "Return a buffer file name or a buffer identifier.
> +For file buffers, returns `buffer-file-name'.
> +For non-file buffers, returns a string with format
> +\"buffer:BUFFER-NAME\". When BUFFER is nil, use the current buffer."
> + (with-current-buffer (or buffer (current-buffer))
> + (or buffer-file-name (concat "buffer:" (buffer-name)))))
So I think what I said above means this would become:
(or (buffer-file-name buffer) buffer (current-buffer))
> +(defun reftex-get-buffer-for-master (master)
> + "Get the buffer associated with MASTER.
> +MASTER can be a file path or a buffer object."
> + (cond
> + ((bufferp master) master)
> + ((stringp master) (find-file-noselect master))
> + (t nil)))
I wouldn't test `(stringp master)`, so we get a "clean" error if
`master` is neither a buffer nor a string.
> +(defun reftex-get-directory-for-master (master)
> + "Get the directory associated with MASTER.
> +MASTER can be a file path or a buffer object."
> + (cond
> + ((bufferp master) (with-current-buffer master default-directory))
> + ((stringp master) (file-name-directory master))
> + (t default-directory)))
Same here and in most other functions.
> +(defun reftex-file-or-buffer-name (master)
> + "Get a display name for MASTER.
> +Returns the filename for file masters, or the buffer name for buffer
> +masters."
> + (cond
> + ((bufferp master) (buffer-name master))
> + ((stringp master) (file-name-nondirectory master))
> + (t "")))
This doesn't seem to be used.
> +(defun reftex-abbreviate-or-get-name (master)
> + "Get a nice display name for MASTER.
> +For files, returns the abbreviated file name.
> +For buffers, returns the buffer name."
> + (cond
> + ((bufferp master) (buffer-name master))
> + ((stringp master) (abbreviate-file-name master))
> + (t "")))
I think I'd use (prin1-to-string master) in the buffer case, just to
avoid confusing the user in case a buffer name looks like a file name.
And I'd drop the `(stringp master)` test.
> +(defun reftex-get-basename-for-master (master)
> + "Get the base name (without extension) for MASTER.
> +For file masters, returns the file name without directory and extension.
> +For buffer objects, returns a sanitized version of the buffer name
> +suitable for use in LaTeX labels."
> + (cond
> + ((bufferp master)
> + (let ((name (buffer-name master)))
> + ;; Remove extension if present
> + (if (string-match "\\(.*\\)\\.[^.]*\\'" name)
> + (match-string 1 name)
> + name)))
Why not (file-name-base (buffer-name master))?
> ;;; =========================================================================
> ;;;
> ;;; Multibuffer Variables
> @@ -282,8 +354,11 @@ reftex-next-multifile-index
> (defun reftex-tie-multifile-symbols ()
> "Tie the buffer-local symbols to globals connected with the master file.
> If the symbols for the current master file do not exist, they are created."
> - (let* ((master (file-truename (reftex-TeX-master-file)))
> - (index (assoc master reftex-master-index-list))
> + (let* ((master (reftex-TeX-master-file))
> + (master-key (if (bufferp master)
> + master
> + (file-truename master)))
I think you can use `reftex-get-truename-for-master` here, no?
I'd also keep the name of the second var as `master` (which just hides
the previous var of the same name), so we don't need to change the rest
of the code.
> @@ -293,7 +368,7 @@ reftex-tie-multifile-symbols
> ;; Get a new index and add info to the alist.
> (setq index (reftex-next-multifile-index)
> newflag t)
> - (push (cons master index) reftex-master-index-list))
> + (push (cons master-key index) reftex-master-index-list))
BTW, while this works, it's a bit more at risk of creating a memory leak
where `reftex-master-index-list` keeps references to long-dead buffers.
Maybe a `kill-buffer-hook` could be used to remove entries from this list?
> @@ -377,7 +452,7 @@ reftex-TeX-master-file
> (buffer-file-name)))))
> (cond
> ((null master)
> - (error "Need a filename for this buffer, please save it first"))
> + (current-buffer))
> ((or (file-exists-p (concat master ".tex"))
> (find-buffer-visiting (concat master ".tex")))
> ;; Ahh, an extra .tex was missing...
IIRC the `master` of one buffer may end up stored in a `TeX-master`
variable of a related buffer (see `reftex-find-duplicate-labels`), so we
need to adjust the code that consults `TeX-master` to account for the
fact that it can be a buffer. Also, it seems that `tex-main-file`
signals an error if `buffer-file-name` is nil, so we presumably need to
wrap the call to that function in the same kind of `condition-case` as
we do when calling `TeX-master`.
> (cond
> + ;; For non-file buffers, skip file operations but allow initialization
> + ((and is-buffer (memq action '(readable read kill)))
> + ;; Return appropriate values for buffer objects
> + (cond ((eq action 'readable) nil)
> + ((eq action 'read) nil)
> + ((eq action 'kill) t)))
> +
> ((eq action 'readable)
> - (file-readable-p file))
> + (and file (file-readable-p file)))
Here we know `file` is not a buffer, so I'd skip the `file` test.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.