[PATCH] Push mark before jumping to new location for xref

[Kelly Dean <kelly@prtime.org>]

[-- Attachment #1: Type: text/plain, Size: 1446 bytes --]

When following a link in the help page for a function or variable to open the source code file, and the file is already open, Emacs moves point in that buffer, but it doesn't push the mark first, so the user loses his previous place. It should push the mark first.

The find-tag command does push the mark, but it's too trigger-happy: it pushes the mark even if the file was _not_ already open. Since the mark ring is for the user's use to keep track of where he was in the buffer, the extraneous push leaves the user later wondering whether that file had already been open for some reason, or whether Emacs was just being sloppy. This affects me when I look at several xref targets in the same files; if Emacs is fixed to avoid the extraneous push, then I can just try to go to the previous mark, and if it says there's no mark set, I immediately know that I looked at no other targets in the current buffer.

The attached patch solves both issues. It pushes the mark for both help xref and find-tag if the buffer is already open, and it avoids pushing otherwise. (Applies to 24.4; not updated for trunk yet.)

I posted an earlier version of this patch in 2012, but it was poorly designed, incomplete, and buggy, and was therefore ignored.

Maybe this updated version will be considered poorly designed too, because it still uses a dynamic variable as an implicit return value. But if not this way, then what other practical solution is there?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: xref-push-mark-24.4.patch --]
[-- Type: text/x-diff, Size: 5713 bytes --]

--- lisp/files.el
+++ lisp/files.el
@@ -1816,12 +1816,15 @@
 If a buffer exists visiting FILENAME, return that one, but
 verify that the file has not changed since visited or saved.
 The buffer is not selected, just returned to the caller.
+If a buffer exists visiting FILENAME, and the dynamic variable
+buffer-preexisted is bound, set it to t.
 Optional second arg NOWARN non-nil means suppress any warning messages.
 Optional third arg RAWFILE non-nil means the file is read literally.
 Optional fourth arg WILDCARDS non-nil means do wildcard processing
 and visit all the matching files.  When wildcards are actually
 used and expanded, return a list of buffers that are visiting
 the various files."
+  (defvar buffer-preexisted) ; Tell byte compiler about free dynamic var
   (setq filename
 	(abbreviate-file-name
 	 (expand-file-name filename)))
@@ -1981,6 +1984,8 @@
 		      (error (if rawfile "File already visited non-literally"
 			       "File already visited literally"))))))
 	      ;; Return the buffer we are using.
+	      (if (boundp 'buffer-preexisted)
+		  (setq buffer-preexisted t))
 	      buf)
 	  ;; Create a new buffer.
 	  (setq buf (create-file-buffer filename))
--- lisp/emacs-lisp/find-func.el
+++ lisp/emacs-lisp/find-func.el
@@ -212,6 +212,7 @@
 		      (indirect-function
 		       (find-function-advised-original fun-or-var)))))
   (with-current-buffer (find-file-noselect file)
+    (save-excursion
     (goto-char (point-min))
     (unless (re-search-forward
 	     (if type
@@ -223,7 +224,7 @@
 		       "\""))
 	     nil t)
       (error "Can't find source for %s" fun-or-var))
-    (cons (current-buffer) (match-beginning 0))))
+      (cons (current-buffer) (match-beginning 0)))))
 
 ;;;###autoload
 (defun find-library (library)
@@ -283,6 +284,7 @@
     (let* ((filename (find-library-name library))
 	   (regexp-symbol (cdr (assq type find-function-regexp-alist))))
       (with-current-buffer (find-file-noselect filename)
+	(save-excursion
 	(let ((regexp (format (symbol-value regexp-symbol)
 			      ;; Entry for ` (backquote) macro in loaddefs.el,
 			      ;; (defalias (quote \`)..., has a \ but
@@ -310,7 +312,7 @@
 		(progn
 		  (beginning-of-line)
 		  (cons (current-buffer) (point)))
-	      (cons (current-buffer) nil))))))))
+		(cons (current-buffer) nil)))))))))
 
 ;;;###autoload
 (defun find-function-noselect (function &optional lisp-only)
--- lisp/help-mode.el
+++ lisp/help-mode.el
@@ -198,11 +198,16 @@
 			   (help-C-file-name (indirect-function fun) 'fun)))
 		   ;; Don't use find-function-noselect because it follows
 		   ;; aliases (which fails for built-in functions).
-		   (let ((location
+		   (defvar buffer-preexisted)
+		   (let* (buffer-preexisted
+			 (location
 			  (find-function-search-for-symbol fun nil file)))
 		     (pop-to-buffer (car location))
 		     (if (cdr location)
-			 (goto-char (cdr location))
+			 (progn (if (and (/= (cdr location) (point))
+					 buffer-preexisted)
+				    (push-mark))
+			  (goto-char (cdr location)))
 		       (message "Unable to find location in file"))))
   'help-echo (purecopy "mouse-2, RET: find function's definition"))
 
@@ -211,8 +216,11 @@
   'help-function (lambda (fun file)
 		   (setq file (locate-library file t))
 		   (if (and file (file-readable-p file))
-		       (progn
+		       (defvar buffer-preexisted)
+		       (let* ((buffer-preexisted
+			       (find-buffer-visiting file)))
 			 (pop-to-buffer (find-file-noselect file))
+			 (if buffer-preexisted (push-mark))
 			 (goto-char (point-min))
 			 (if (re-search-forward
 			      (format "^[ \t]*(\\(cl-\\)?define-compiler-macro[ \t]+%s"
@@ -227,10 +235,15 @@
   'help-function (lambda (var &optional file)
 		   (when (eq file 'C-source)
 		     (setq file (help-C-file-name var 'var)))
-		   (let ((location (find-variable-noselect var file)))
+		   (defvar buffer-preexisted)
+		   (let* (buffer-preexisted
+			 (location (find-variable-noselect var file)))
 		     (pop-to-buffer (car location))
 		     (if (cdr location)
-		       (goto-char (cdr location))
+			 (progn (if (and (/= (cdr location) (point))
+					 buffer-preexisted)
+				    (push-mark))
+			  (goto-char (cdr location)))
 		       (message "Unable to find location in file"))))
   'help-echo (purecopy "mouse-2, RET: find variable's definition"))
 
--- lisp/progmodes/etags.el
+++ lisp/progmodes/etags.el
@@ -1171,9 +1171,11 @@
 
       ;; Get the local value in the tags table buffer before switching buffers.
       (setq goto-func goto-tag-location-function)
+      (defvar buffer-preexisted)
+      (let (buffer-preexisted)
       (tag-find-file-of-tag-noselect file)
       (widen)
-      (push-mark)
+	(if buffer-preexisted (push-mark)))
       (funcall goto-func tag-info)
 
       ;; Return the buffer where the tag was found.
@@ -1183,6 +1185,7 @@
   "Find the right line in the specified FILE."
   ;; If interested in compressed-files, search files with extensions.
   ;; Otherwise, search only the real file.
+  (defvar buffer-preexisted) ; Tell byte compiler about free dynamic var
   (let* ((buffer-search-extensions (if auto-compression-mode
 				       tags-compression-info-list
 				     '("")))
@@ -1198,6 +1201,8 @@
     (while (and (not the-buffer) buffer-search-extensions)
       (setq the-buffer (find-buffer-visiting (concat file (car buffer-search-extensions))))
       (setq buffer-search-extensions (cdr buffer-search-extensions)))
+    (if (and the-buffer (boundp 'buffer-preexisted))
+	(setq buffer-preexisted t))
     ;; if found a buffer but file modified, ensure we re-read !
     (if (and the-buffer (not (verify-visited-file-modtime the-buffer)))
 	(find-file-noselect (buffer-file-name the-buffer)))