Re: Freezing frameset-restore

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

> 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