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: Freezing frameset-restore Date: Mon, 10 Mar 2014 00:29:44 -0400 Message-ID: References: NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1394425798 13887 80.91.229.3 (10 Mar 2014 04:29:58 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 10 Mar 2014 04:29:58 +0000 (UTC) Cc: Emacs developers To: Juanma Barranquero Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Mar 10 05:30:06 2014 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 1WMrqm-0007Dh-GS for ged-emacs-devel@m.gmane.org; Mon, 10 Mar 2014 05:30:04 +0100 Original-Received: from localhost ([::1]:46640 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WMrql-0001xk-Lz for ged-emacs-devel@m.gmane.org; Mon, 10 Mar 2014 00:30:03 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:42046) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WMrqb-0001fG-PY for emacs-devel@gnu.org; Mon, 10 Mar 2014 00:30:01 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WMrqU-0003bo-Ef for emacs-devel@gnu.org; Mon, 10 Mar 2014 00:29:53 -0400 Original-Received: from ironport2-out.teksavvy.com ([206.248.154.181]:52174) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WMrqU-0003bj-AM for emacs-devel@gnu.org; Mon, 10 Mar 2014 00:29:46 -0400 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: Av4EABK/CFFMCppy/2dsb2JhbABEvw4Xc4IeAQEEAVYjBQsLDiYSFBgNJIgeBsEtkQoDpHqBXoMT X-IPAS-Result: Av4EABK/CFFMCppy/2dsb2JhbABEvw4Xc4IeAQEEAVYjBQsLDiYSFBgNJIgeBsEtkQoDpHqBXoMT X-IronPort-AV: E=Sophos;i="4.84,565,1355115600"; d="scan'208";a="50966114" Original-Received: from 76-10-154-114.dsl.teksavvy.com (HELO ceviche.home) ([76.10.154.114]) by ironport2-out.teksavvy.com with ESMTP/TLS/ADH-AES256-SHA; 10 Mar 2014 00:29:45 -0400 Original-Received: by ceviche.home (Postfix, from userid 20848) id E497B660A5; Mon, 10 Mar 2014 00:29:44 -0400 (EDT) In-Reply-To: (Juanma Barranquero's message of "Sun, 9 Mar 2014 00:45:19 +0100") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3.50 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 206.248.154.181 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:170241 Archived-At: > A variant of the last patch. Looks good, thanks. Comments below. > +(defvar frameset--action-map nil > + "Map of frames to actions. > +Its value is only meaningful during execution of `frameset-restore'. > +Internal use only.") Make it just (defvar frameset--action-map). > -REUSE-FRAMES selects the policy to use to reuse frames when restoring: > - t Reuse existing frames if possible, and delete those not reused. > - nil Restore frameset in new frames and delete existing frames. > - :keep Restore frameset in new frames and keep the existing ones. > +REUSE-FRAMES selects the policy to reuse frames when restoring: > + :all All existing frames can be reused. This is the default. > + :none No existing frame can be reused. > + :match Only frames with matching frame ids can be reused. > LIST A list of frames to reuse; only these are reused (if possible). > - Remaining frames in this list are deleted; other frames not > - included on the list are left untouched. > + PRED A predicate function; it receives as argument a live frame, > + and must return non-nil to allow reusing it, nil otherwise. So far, I only see :all, :none, and :match being used. I suggest the following changes: - Get rid of the LIST case (it's not used and the caller could use PRED instead anyway to get the same result). - I prefer it when keywords are used only for the argument names and not for their values, so you don't need to count them to know which is which. So I'd favor using `all', `none', and `match'. Tho that slightly clashes with the PRED case. So I'd then suggest to rename `all' to t, `none' to nil, and to get rid of `match' (i.e. use the PRED case for it, probably providing a handy function for it). > + (setq frameset--action-map nil) let-bind it, instead. > + ;; Mark candidates as :ignored; they will be reassigned later, if chosen. > + (mapc (lambda (frame) > + (push (cons frame :ignored) frameset--action-map)) > + frameset--reuse-list) Maybe a hash-table would be better than an alist. This would save us from the hideous cl-acons/assq-delete-all. > +by `frameset-restore' (which see). Optional arg ACTION-MAP is an alist > +\((ACTIONn . FUNCTIONn)...) mapping actions to their cleanup functions. > +ACTION can be an action, or a list of actions. Allowing a list of actions is over-engineering it. > + (frameset-restore-cleanup > + (frameset-restore (aref data 0) > + :filters frameset-session-filter-alist > + :reuse-frames (if current-prefix-arg nil :match)) > + (if current-prefix-arg > + ;; delete frames > + nil > + ;; iconify frames > + '((:rejected . iconify-frame) > + ;; In the unexpected case that a frame was a candidate (matching frame id) > + ;; and yet not restored, remove it because it is in fact a duplicate. > + (:ignored . delete-frame)))) Without frameset-restore-cleanup this could look like (pcase-dolist (`(,frame . ,action) (frameset-restore (aref data 0) :filters frameset-session-filter-alist :reuse-frames (if current-prefix-arg nil :match))) (with-demoted-errors "Error cleaning up frame: %S" (pcase action (`:rejected (if current-prefix-arg (delete-frame frame) (iconify-frame frame))) ;; In the unexpected case that a frame was a candidate (matching frame ;; id) and yet not restored, remove it because it is in fact ;; a duplicate. (`:ignored (delete-frame frame))))) It's admittedly longer than your version, but I'm not convinced it justifies the complexity of frameset-restore-cleanup. Stefan