Subject: Re: 25.0.50; [PATCH] Allow M-x man to reuse an existing window

From: martin rudalics <[email-masked]>
Date: Tue, 01 Mar 2016 18:04:08 +0100

> Well, it took a bit of time but here I am ! Could you please review the
 > following patch ?

Thanks for the patch!

+(defun display-buffer-reuse-mode-window (buffer alist)
+  "Return a window based on the mode of the buffer it displays.
+Display BUFFER in the returned window. Return nil if no usable
                                        ^
+window is found.
+
+If ALIST contains a `mode' entry, its value is a major mode (a
+symbol) or a list of modes.  A window is a candidate if it
+displays a buffer that derives from one of the given modes. When
                                                             ^
+ALIST contains no `mode' entry, the current major mode of BUFFER
+is used.

Please consistently use two spaces after each sentence in doc-strings,
comments etc.

+      (dolist (window windows)
+        (let ((window-mode (with-current-buffer
+                               (window-buffer window)
+                             major-mode))
+              mode? frame?)
+          (setq mode?
+                (cond ((memq window-mode allowed-modes)
+                       'same)
+                      ((let ((major-mode window-mode))
+                         (derived-mode-p allowed-modes))
+                       'derived)))

It's not nice to bind a buffer-local variable like ‘major-mode’ in a
completely unrelated buffer just for calling ‘derived-mode-p’.  Please
do that either within the ‘with-current-buffer’ form above, somehow like

       (dolist (window windows)
         (let (window-mode window-derived-mode mode? frame?)
	  (with-current-buffer (window-buffer window)
	    (setq window-mode major-mode)
	    (setq window-derived-mode (derived-mode-p allowed-modes)))
           (setq mode?
                 (cond ((memq window-mode allowed-modes)
                        'same)
                       (window-derived-mode
		       'derived)))

or simply write

                       ((with-current-buffer (window-buffer window))
                          (derived-mode-p allowed-modes))
                        'derived)))

instead.

+      (let ((window (first (nconc same-mode-same-frame

This gets me

In end of data:
window.el:8617:1:Warning: the function ‘first’ is not known to be defined.

here.

Thanks again, martin