From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: code review request for saveplace with dired buffer Date: Fri, 07 Jun 2013 15:12:11 -0400 Message-ID: References: <87ppvy4e0m.fsf@kanis.fr> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1370632341 27176 80.91.229.3 (7 Jun 2013 19:12:21 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 7 Jun 2013 19:12:21 +0000 (UTC) Cc: kfogel@red-bean.com, Emacs Development List To: Ivan Kanis Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Jun 07 21:12:21 2013 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Ul25F-0000Rx-1N for ged-emacs-devel@m.gmane.org; Fri, 07 Jun 2013 21:12:21 +0200 Original-Received: from localhost ([::1]:41838 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ul25E-0003T0-Iy for ged-emacs-devel@m.gmane.org; Fri, 07 Jun 2013 15:12:20 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:36302) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ul259-0003Oz-Ty for emacs-devel@gnu.org; Fri, 07 Jun 2013 15:12:17 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ul257-0007Te-MF for emacs-devel@gnu.org; Fri, 07 Jun 2013 15:12:15 -0400 Original-Received: from chene.dit.umontreal.ca ([132.204.246.20]:51902) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ul257-0007S7-Et for emacs-devel@gnu.org; Fri, 07 Jun 2013 15:12:13 -0400 Original-Received: from faina.iro.umontreal.ca (lechon.iro.umontreal.ca [132.204.27.242]) by chene.dit.umontreal.ca (8.14.1/8.14.1) with ESMTP id r57JCBXh008577; Fri, 7 Jun 2013 15:12:11 -0400 Original-Received: by faina.iro.umontreal.ca (Postfix, from userid 20848) id 2133DB417C; Fri, 7 Jun 2013 15:12:11 -0400 (EDT) In-Reply-To: <87ppvy4e0m.fsf@kanis.fr> (Ivan Kanis's message of "Fri, 07 Jun 2013 08:43:21 +0200") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3.50 (gnu/linux) X-NAI-Spam-Flag: NO X-NAI-Spam-Threshold: 5 X-NAI-Spam-Score: 0 X-NAI-Spam-Rules: 1 Rules triggered RV4602=0 X-NAI-Spam-Version: 2.3.0.9362 : core <4602> : streams <975800> : uri <1441269> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 132.204.246.20 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:160248 Archived-At: Thanks Ivan. See some stylistic comments below, but otherwise, feel free to install it. Stefan > +(eval-when-compile (require 'saveplace)) I don't see any need for it. Such a compile-time require is only needed if you use macros from that package (or if your macros use functions from that package). > ;; FIXME document whatever dired-x is doing. > (defun dired-initial-position (dirname) > "Where point should go in a new listing of DIRNAME. > @@ -2762,7 +2765,8 @@ Point assumed at beginning of new subdir line." > (end-of-line) > (and (featurep 'dired-x) dired-find-subdir > (dired-goto-subdir dirname)) > - (if dired-trivial-filenames (dired-goto-next-nontrivial-file))) > + (if dired-trivial-filenames (dired-goto-next-nontrivial-file)) > + (if (featurep 'saveplace) (save-place-position-dired-cursor))) Maybe a better alternative is to add a dired-initial-position-hook. This way dired doesn't need any saveplace-specific code and instead saveplace can simply add `save-place-position-dired-cursor' to `dired-initial-position-hook'. > ;; file. If not, do so, then feel free to modify the alist. It > ;; will be saved again when Emacs is killed. > (or save-place-loaded (load-save-place-alist-from-file)) > - (when (and buffer-file-name > - (or (not save-place-ignore-files-regexp) > - (not (string-match save-place-ignore-files-regexp > - buffer-file-name)))) > - (let ((cell (assoc buffer-file-name save-place-alist)) > - (position (if (not (eq major-mode 'hexl-mode)) > - (point) > - (with-no-warnings > - (1+ (hexl-current-address)))))) > - (if cell > - (setq save-place-alist (delq cell save-place-alist))) > - (if (and save-place > - (not (= position 1))) ;; Optimize out the degenerate case. > - (setq save-place-alist > - (cons (cons buffer-file-name position) > - save-place-alist)))))) > + (let ((item (or buffer-file-name > + (when dired-directory (expand-file-name dired-directory)))) > + cell position) > + (when (and item > + (or (not save-place-ignore-files-regexp) > + (not (string-match save-place-ignore-files-regexp > + item)))) > + (setq cell (assoc item save-place-alist) > + position (if (not (eq major-mode 'hexl-mode)) > + (point) > + (with-no-warnings > + (1+ (hexl-current-address))))) I think it's preferable to keep the cell and position binding in a separate let. Two "let"s are more efficient than such a "single let plus subsequent setqs". Also they're cleaner since they avoid `setq'. > There might be a convention of using `when' instead of `if', when What Glenn said. Stefan