Re: fix for bug#29935 copyright-update inserts year at random places

[Stephen Leake <stephen_leake@stephe-leake.org>]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stephen Leake <stephen_leake@stephe-leake.org>
>> Date: Mon, 01 Jan 2018 17:29:27 -0600
>>
>> Ok to commit to emacs-26?
>
> How important is it to have this in Emacs 26?
>
>> +  (let ((copyright-end (point)))
>> +    (setq copyright-current-year (format-time-string "%Y"))
>> +    (unless (string= (buffer-substring (- (match-end 3) 2) (match-end 3))
>> +		     (substring copyright-current-year -2))
>> +      (if (or noquery
>> +	      (save-window-excursion
>> +	        ;; Fixes some point-moving oddness (bug#2209, bug#29935).
>> +	        (save-excursion
>> +	          (switch-to-buffer (current-buffer))
>> +                  ;; Ensure the copyright line is displayed.
>> +                  (goto-char copyright-end)
>> +		  (y-or-n-p (if replace
>> +			        (concat "Replace copyright year(s) by "
>> +				        copyright-current-year "? ")
>> +			      (concat "Add " copyright-current-year
>> +				      " to copyright? "))))))
>
> Why does this need to use save-window-excursion?

'switch-to-buffer' can pop up a new frame, or use a different window,
depending on various settings, so save-window-excursion is needed. 

'(goto-char copyright-end)' is needed because in general, a buffer does
not have a well-defined 'point'; is has a point in each window it is
displayed in, and recent Emacsen preserve that point when the buffer is
swapped in and out of that window (which is a nice feature).
'copyright-find-end' changes point in the buffer, but that's not used if
it's not displayed in the current window.

You suggest "using other facilities", but isn't this precisely what
save-window-excursion and save-excursion are for?

Here's a new patch, with improved comments:

--- a/lisp/emacs-lisp/copyright.el
+++ b/lisp/emacs-lisp/copyright.el
@@ -181,44 +181,54 @@ copyright-update-year
   ;; This uses the match-data from copyright-find-copyright/end.
   (goto-char (match-end 1))
   (copyright-find-end)
-  (setq copyright-current-year (format-time-string "%Y"))
-  (unless (string= (buffer-substring (- (match-end 3) 2) (match-end 3))
-		   (substring copyright-current-year -2))
-    (if (or noquery
-	    (save-window-excursion
-	      (switch-to-buffer (current-buffer))
-	      ;; Fixes some point-moving oddness (bug#2209).
-	      (save-excursion
-		(y-or-n-p (if replace
-			      (concat "Replace copyright year(s) by "
-				      copyright-current-year "? ")
-			    (concat "Add " copyright-current-year
-				    " to copyright? "))))))
-	(if replace
-	    (replace-match copyright-current-year t t nil 3)
-	  (let ((size (save-excursion (skip-chars-backward "0-9"))))
-	    (if (and (eq (% (- (string-to-number copyright-current-year)
-			       (string-to-number (buffer-substring
-						  (+ (point) size)
-						  (point))))
-			    100)
-			 1)
-		     (or (eq (char-after (+ (point) size -1)) ?-)
-			 (eq (char-after (+ (point) size -2)) ?-)))
-		;; This is a range so just replace the end part.
-		(delete-char size)
-	      ;; Insert a comma with the preferred number of spaces.
-	      (insert
-	       (save-excursion
-		 (if (re-search-backward "[0-9]\\( *, *\\)[0-9]"
-					 (line-beginning-position) t)
-		     (match-string 1)
-		   ", ")))
-	      ;; If people use the '91 '92 '93 scheme, do that as well.
-	      (if (eq (char-after (+ (point) size -3)) ?')
-		  (insert ?')))
-	    ;; Finally insert the new year.
-	    (insert (substring copyright-current-year size)))))))
+  (let ((copyright-end (point)))
+    (setq copyright-current-year (format-time-string "%Y"))
+    (unless (string= (buffer-substring (- (match-end 3) 2) (match-end 3))
+		     (substring copyright-current-year -2))
+      (if (or noquery
+              ;; ‘switch-to-buffer’ can pop up a new frame, or use
+              ;; another window, so preserve the current window
+              ;; config.
+              (save-window-excursion
+	        ;; Fixes some point-moving oddness (bug#2209, bug#29935).
+	        (save-excursion
+	          (switch-to-buffer (current-buffer))
+                  ;; Ensure the copyright line is displayed;
+                  ;; switch-to-buffer has moved point to where it was
+                  ;; when this buffer was last displayed in this
+                  ;; window.
+                  (goto-char copyright-end)
+		  (y-or-n-p (if replace
+			        (concat "Replace copyright year(s) by "
+				        copyright-current-year "? ")
+			      (concat "Add " copyright-current-year
+				      " to copyright? "))))))
+	  (if replace
+	      (replace-match copyright-current-year t t nil 3)
+	    (let ((size (save-excursion (skip-chars-backward "0-9"))))
+	      (if (and (eq (% (- (string-to-number copyright-current-year)
+			         (string-to-number (buffer-substring
+						    (+ (point) size)
+						    (point))))
+			      100)
+			   1)
+		       (or (eq (char-after (+ (point) size -1)) ?-)
+			   (eq (char-after (+ (point) size -2)) ?-)))
+		  ;; This is a range so just replace the end part.
+		  (delete-char size)
+	        ;; Insert a comma with the preferred number of spaces.
+	        (insert
+	         (save-excursion
+		   (if (re-search-backward "[0-9]\\( *, *\\)[0-9]"
+					   (line-beginning-position) t)
+		       (match-string 1)
+		     ", ")))
+	        ;; If people use the '91 '92 '93 scheme, do that as well.
+	        (if (eq (char-after (+ (point) size -3)) ?')
+		    (insert ?')))
+	      ;; Finally insert the new year.
+	      (insert (substring copyright-current-year size))))
+        ))))
 
 ;;;###autoload
 (defun copyright-update (&optional arg interactivep)

Ok to commit?

--
-- Stephe