Re: Freezing frameset-restore

[Stefan Monnier <monnier@iro.umontreal.ca>]

>> - 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,

I'm not opposed to keywords as arguments in general:  I'm opposed to
them when the argument is itself specified via a keyword.

I.e. for a given function call, the keywords should either all be actual
arguments passed positionally or they should all be "argument names".

> deletion / frame creation), but your change makes none (nil) the
> default.

This choice is not based on an estimate of which case will be more
common, but rather I think that given the argument name ":reuse-frames"
a nil value which says "don't reuse any frames" is more intuitive than
one that says "reuse all frames".  Hence the change.

>> 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.

Don't convert to an alist at the end: just return the hashtable.

>> 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 than
> '((:ignored . delete-frame) (:rejected . delete-frame)).  But I can
> change it if you feel strongly against it.

Again, we're talking about very few callers.  You want the code to be as
simple as possible, since the rare callers can pay the extra price, if needed.

> 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),

It's only artificial in your mind: the code already did it in two phases
(just within the same function).

> but I'm still less fan of the code you propose, which even uses an
> undocumented macro (pcase-dolist ;-).

Once you change the code to return a hashtable, pcase-dolist will be
replaced with maphash anyway ;-)

> 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).

That's pretty close to the definition of over-engineering.
Personally, I'd just ditch frameset-restore-cleanup.

> 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.

Aha!  Can you expand on this?  What do you mean exactly by
"non-visible"?


        Stefan