From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Juanma Barranquero Newsgroups: gmane.emacs.devel Subject: Re: Freezing frameset-restore Date: Mon, 10 Mar 2014 06:26:43 +0100 Message-ID: References: NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 X-Trace: ger.gmane.org 1394429240 11727 80.91.229.3 (10 Mar 2014 05:27:20 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 10 Mar 2014 05:27:20 +0000 (UTC) Cc: Emacs developers To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Mar 10 06:27:29 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 1WMskL-0007jF-AH for ged-emacs-devel@m.gmane.org; Mon, 10 Mar 2014 06:27:29 +0100 Original-Received: from localhost ([::1]:46774 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WMskK-0001w3-Oe for ged-emacs-devel@m.gmane.org; Mon, 10 Mar 2014 01:27:28 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:51232) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WMskH-0001vv-DO for emacs-devel@gnu.org; Mon, 10 Mar 2014 01:27:26 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WMskG-0003hk-Bk for emacs-devel@gnu.org; Mon, 10 Mar 2014 01:27:25 -0400 Original-Received: from mail-yk0-x230.google.com ([2607:f8b0:4002:c07::230]:57027) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WMskG-0003h2-6F for emacs-devel@gnu.org; Mon, 10 Mar 2014 01:27:24 -0400 Original-Received: by mail-yk0-f176.google.com with SMTP id 19so17970424ykq.7 for ; Sun, 09 Mar 2014 22:27:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=E7C1nDqhcRYRdbHQC7tzNywNe3YtqfybqQyd3oIufQI=; b=J1OlU++fGb+XgnTuiRc3Y5XnMtZr11/D1b10yQWq+4K8LailmndnL+YZUiANvHH5q4 EfMWSk5Dm12604EpT1vnyD9ydz17isoq/g+snmpzm3q0+UpLVs28JVkKIzh4bOQBf2nL mGdZZqRQxmCAnEZAtJBBjc0cK1etQcr1KO9x/CaWF4rUtosw08y1aThUepfdBLPu07xQ 1MZ0/mqlWJL3+y/+3dhybeW34JYGxuvOlVtYCSrQ9IWXzzUduBq778LtEVHxq/3VjIkS CyEZXiQuCoOTO+JCLsa+U22HUqix4dCDo/qa7wrSCTfUsjV4cnKxtKHOpBkCDaf9fd/i gG9Q== X-Received: by 10.236.93.208 with SMTP id l56mr419135yhf.112.1394429243716; Sun, 09 Mar 2014 22:27:23 -0700 (PDT) Original-Received: by 10.170.163.3 with HTTP; Sun, 9 Mar 2014 22:26:43 -0700 (PDT) In-Reply-To: X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2607:f8b0:4002:c07::230 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:170243 Archived-At: On Mon, Mar 10, 2014 at 5:29 AM, Stefan Monnier wrote: > Make it just (defvar frameset--action-map). Plus let-bind later. OK. > 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). Well, the list is handy if you have a pre-made list or just want to reuse one frame, but I'm OK with this change. > - 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). I'd like to avoid this, if possible. I happen to really like keywords as argument values, instead of normal symbols, and I dislike the idea of providing a matching function for the match case. Also, keywords are also used in the variable `frameset-filter-alist' (and, though slight, the possibility of clash with a function called never or restore isn't null). This also would meant to change `desktop-restore-reuses-frames' and `desktop-restore-forces-onscreen'. If you really insist on getting rid of the keyword values, I'd prefer to keep 'match, 'all and 'none and let the user be smart enough not to use a PRED function with these names. Consider that reusing all seems generally more desirable that reusing none (reusing all is more likely to produce a better user experience by reducing the flicker of frame deletion / frame creation), but your change makes none (nil) the default. > Maybe a hash-table would be better than an alist. This would save us > from the hideous cl-acons/assq-delete-all. Funnily enough, my previous patch *used* a hash table. I changed it to an alist to avoid having to build the action-map list at the end. But I certainly prefer the hash table, so I'll revert that part of the change. > Allowing a list of actions is over-engineering it. Except that the presumably common case of deleting all non-restored is '(((:ignored :rejected) delete-frame)), which seems nicer that '((:ignored . delete-frame) (:rejected . delete-frame)). But I can change it if you feel strongly against it. > (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. I'm no fan of frameset-restore-cleanup (I much preferred having a CLEANUP argument, because separating this from frameset-restore seems to me to make an artificial distinction between two phases of the same process, which is, to restore a frameset), but I'm still less fan of the code you propose, which even uses an undocumented macro (pcase-dolist ;-). Doing it your way means repeating some logic, like the error checking, at each point of call of frameset-restore (yes, there's currently only two uses, but I'm designing this thinking that some other uses will be found). The call to frameset-restore you're rewriting here is admittedly ugly (in both your and my versions) because frame-configuration-to-register had this weird behavior of iconifying by default and deleting with C-u that I'm imitating. As I see it, for the cleanup there are only three "cases", two common (leave everything as it is; and delete everything that wasn't restored), and a less common one (which lumps everything else, like iconifying, or customized functions). That's why I'm strongly convinced that having (frameset-restore fs :cleanup 'keep) (frameset-restore fs :cleanup 'delete) (frameset-restore fs :cleanup #'my-custom-function) and let frameset-restore worry about looping, error protection and default cases is much, much cleaner that the alternatives we've been hashing for the past couple days (not to mention more efficient: the action-map is only built when needed) and more correct (the cleanup is called before checking whether there's still at least one visible frame, while with your proposal, it is possible for the cleanup code to do something to all visible frames and leave only non-visible ones). J