Re: Freezing frameset-restore

[Juanma Barranquero]

On Mon, Mar 10, 2014 at 5:29 AM, Stefan Monnier
<address@hidden> 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