Freezing frameset-restore

Freezing frameset-restore

[Juanma Barranquero]

I'm perfectly aware we're in a feature freeze and non-trivial changes
should be kept to a minimum or entirely avoided.

Even so, I'm deeply worried about freezing the current interface of
frameset-restore. While fixing the frameset-to-register bugs a couple
of weeks ago I realized that I had made a big mistake in the
REUSE-FRAMES argument, conflating two unrelated things:

- The policy to decide which frames to reuse.
- The cleanup policy for non-reused frames, after restoration.

If left as is, the list of values of REUSE-FRAMES will continue
creeping up as new useful cases are discovered. For example, I
suddenly noticed that something as sensible as "reuse existing frames
if possible, but do not delete non-reused ones" didn't exist and
should be added (I didn't). Also, reusing frames based in matching
frame ids (which should be the most common case for in-session
restore, as for example C-x r f R / C-x r j R) couldn't be directly
requested, which forced me to add extra complexity to
frameset--jump-to-register.

I think it makes perfect sense to split REUSE-FRAMES into two:

  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).
    PRED     A predicate function; it receives as argument a live frame,
               and must return t to allow reusing it, nil otherwise.

  CLEANUP allows to \"clean up\" the frame list after restoring a frameset:
    :delete  Delete all frames that weren't restored.  This is the default.
    :keep    Keep all frames.
    FUNC     A function called with two arguments:
             - FRAME, a live frame.
             - ACTION, which can be one of
               :rejected  Frame existed, but was not a candidate for reuse.
               :ignored   Frame existed, was a candidate, but wasn't reused.
               :reused    Frame existed, was a candidate, and restored upon.
               :created   Frame didn't exist, was created and restored upon.
             Return value is ignored.

This is not only cleaner, but also more flexible that the current API.

My motivation to ask for including this now is that it will be much
harder / uglier to do it afterwards, if we freeze the current
interface. Given that we're talking about a new feature anyway and so
there's no code out there relying on its current form, there's no
downside to fixing it at this time. The only two "users" of the
feature are frameset-to-register, which gets simplified by this, and
desktop.el, which only needs a minimal adaptation (included in the
patch below).

    Juanma



2014-03-05  Juanma Barranquero  <lekktu@gmail.com>

        * frameset.el: Separate options for reusing frames and cleaning up.
        (frameset--action-map): New variable.
        (frameset--restore-frame): Cache frame action.
        (frameset-restore): New keyword arg :cleanup, allows to select
        how to clean up the frame list after restoring.  Remove cleaning
        options from :reuse-frames.
        (frameset--clean-after-jump): New function.
        (frameset--jump-to-register): Simplify by using :cleanup.

        * desktop.el (desktop-restore-frameset): Pass new arg :cleanup.


=== modified file 'lisp/frameset.el'
--- lisp/frameset.el    2014-03-05 16:22:47 +0000
+++ lisp/frameset.el    2014-03-05 23:45:14 +0000
@@ -791,6 +791,11 @@
 Its value is only meaningful during execution of `frameset-restore'.
 Internal use only.")

+(defvar frameset--action-map nil
+  "Map of frames to actions.
+Its value is only meaningful during execution of `frameset-restore'.
+Internal use only.")
+
 (defun frameset-compute-pos (value left/top right/bottom)
   "Return an absolute positioning value for a frame.
 VALUE is the value of a positional frame parameter (`left' or `top').
@@ -982,16 +987,20 @@
        (push visible alt-cfg)
        (push (cons 'fullscreen fullscreen) alt-cfg)))

-    ;; Time to find or create a frame an apply the big bunch of parameters.
-    ;; If a frame needs to be created and it falls partially or fully
offscreen,
-    ;; sometimes it gets "pushed back" onscreen; however, moving it
afterwards is
-    ;; allowed.  So we create the frame as invisible and then reapply the full
-    ;; parameter alist (including position and size parameters).
-    (setq frame (or (and frameset--reuse-list
-                        (frameset--reuse-frame display filtered-cfg))
-                   (make-frame-on-display display
-                                          (cons '(visibility)
-
(frameset--initial-params filtered-cfg)))))
+    ;; Time to find or create a frame and apply the big bunch of parameters.
+    (setq frame (and frameset--reuse-list
+                    (frameset--reuse-frame display filtered-cfg)))
+    (if frame
+       (puthash frame :reused frameset--action-map)
+      ;; If a frame needs to be created and it falls partially or
fully offscreen,
+      ;; sometimes it gets "pushed back" onscreen; however, moving it
afterwards is
+      ;; allowed.  So we create the frame as invisible and then
reapply the full
+      ;; parameter alist (including position and size parameters).
+      (setq frame (make-frame-on-display display
+                                        (cons '(visibility)
+
(frameset--initial-params filtered-cfg))))
+      (puthash frame :created frameset--action-map))
+
     (modify-frame-parameters frame
                             (if (eq (frame-parameter frame
'fullscreen) fullscreen)
                                 ;; Workaround for bug#14949
@@ -1037,8 +1046,9 @@

 ;;;###autoload
 (cl-defun frameset-restore (frameset
-                           &key predicate filters reuse-frames
-                                force-display force-onscreen)
+                           &key predicate filters
+                                reuse-frames cleanup
+                                force-display force-onscreen)
   "Restore a FRAMESET into the current display(s).

 PREDICATE is a function called with two arguments, the parameter alist
@@ -1050,13 +1060,25 @@
 FILTERS is an alist of parameter filters; if nil, the value of
 `frameset-filter-alist' is used instead.

-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 t to allow reusing it, nil otherwise.
+
+CLEANUP allows to \"clean up\" the frame list after restoring a frameset:
+  :delete  Delete all frames that weren't restored.  This is the default.
+  :keep    Keep all frames.
+  FUNC     A function called with two arguments:
+           - FRAME, a live frame.
+           - ACTION, which can be one of
+             :rejected  Frame existed, but was not a candidate for reuse.
+             :ignored   Frame existed, was a candidate, but wasn't reused.
+             :reused    Frame existed, was a candidate, and restored upon.
+             :created   Frame didn't exist, was created and restored upon.
+           Return value is ignored.

 FORCE-DISPLAY can be:
   t        Frames are restored in the current display.
@@ -1079,29 +1101,55 @@

 Note the timing and scope of the operations described above: REUSE-FRAMES
 affects existing frames; PREDICATE, FILTERS and FORCE-DISPLAY affect the frame
-being restored before that happens; and FORCE-ONSCREEN affects the frame once
-it has been restored.
+being restored before that happens; FORCE-ONSCREEN affects the frame
once it has
+been restored; and CLEANUP affects all frames, after restoration has finished.

 All keyword parameters default to nil."

   (cl-assert (frameset-valid-p frameset))

-  (let (other-frames)
+  (let ((frames (frame-list))
+       (non-candidates nil))
+
+    (setq frameset--action-map (make-hash-table :test #'eq :weakness 'value))

     ;; frameset--reuse-list is a list of frames potentially reusable.  Later we
     ;; will decide which ones can be reused, and how to deal with any leftover.
     (pcase reuse-frames
-      ((or `nil `:keep)
+      ((or :all `nil)
+       (setq frameset--reuse-list frames))
+      (:none
        (setq frameset--reuse-list nil
-            other-frames (frame-list)))
+            non-candidates t))
+      (:match
+       (setq frameset--reuse-list
+            (cl-loop for (state) in (frameset-states frameset)
+                     when (frameset-frame-with-id (frameset-cfg-id
state) frames)
+                     collect it)
+            non-candidates t))
+      ((pred functionp)
+       (setq frameset--reuse-list (cl-remove-if
+                                  (lambda (frame)
+                                    (if (funcall reuse-frames frame)
+                                        nil
+                                      (puthash frame :rejected
frameset--action-map)))
+                                  frames)))
       ((pred consp)
        (setq frameset--reuse-list (copy-sequence reuse-frames)
-            other-frames (cl-delete-if (lambda (frame)
-                                         (memq frame frameset--reuse-list))
-                                       (frame-list))))
+            non-candidates t))
       (_
-       (setq frameset--reuse-list (frame-list)
-            other-frames nil)))
+       (error "Invalid arg :reuse-frames %s" reuse-frames)))
+
+    (when non-candidates
+      ;; If we didn't mark non-candidate frames on the fly, do it now.
+      (mapc (lambda (frame)
+             (puthash frame :rejected frameset--action-map))
+           (cl-set-difference frames frameset--reuse-list)))
+
+    ;; Mark candidates as :ignored; they will be reassigned later, if chosen.
+    (mapc (lambda (frame)
+           (puthash frame :ignored frameset--action-map))
+         frameset--reuse-list)

     ;; Sort saved states to guarantee that minibufferless frames will
be created
     ;; after the frames that contain their minibuffer windows.
@@ -1138,10 +1186,8 @@
                  ;; existing frame whose id matches a frame
configuration in the
                  ;; frameset.  Once the frame config is properly
restored, we can
                  ;; reset the old frame's id to nil.
-                 (setq duplicate (and other-frames
-                                      (or (eq reuse-frames :keep)
(consp reuse-frames))
-                                      (frameset-frame-with-id
(frameset-cfg-id frame-cfg)
-                                                              other-frames)))
+                 (setq duplicate (frameset-frame-with-id
(frameset-cfg-id frame-cfg)
+                                                         frames))
                  ;; Restore minibuffers.  Some of this stuff could be
done in a filter
                  ;; function, but it would be messy because restoring
minibuffers affects
                  ;; global state; it's best to do it here than add a
bunch of global
@@ -1183,23 +1229,30 @@
            (error
             (delay-warning 'frameset (error-message-string err) :error))))))

-    ;; In case we try to delete the initial frame, we want to make sure that
-    ;; other frames are already visible (discussed in thread for bug#14841).
-    (sit-for 0 t)
-
-    ;; Delete remaining frames, but do not fail if some resist being deleted.
-    (unless (eq reuse-frames :keep)
-      (dolist (frame (sort (nconc (if (listp reuse-frames) nil other-frames)
-                                 frameset--reuse-list)
+    ;; Clean up the frame list.
+    (unless (eq cleanup :keep)
+      ;; In case we try to delete the initial frame, we want to make sure that
+      ;; other frames are already visible (discussed in thread for bug#14841).
+      (sit-for 0 t)
+      ;; :delete is the default action for CLEANUP
+      (when (memq cleanup '(nil :delete))
+       (setq cleanup (lambda (frame action)
+                       (when (memq action '(:ignored :rejected))
+                         (delete-frame frame)))))
+      ;; Call the user action (or the default :delete one) for every frame.
+      (dolist (frame (sort (frame-list)
                           ;; Minibufferless frames must go first to avoid
                           ;; errors when attempting to delete a frame whose
                           ;; minibuffer window is used by another frame.
                           #'frameset-minibufferless-first-p))
        (condition-case err
-           (delete-frame frame)
+           (funcall cleanup frame (gethash frame frameset--action-map))
          (error
-          (delay-warning 'frameset (error-message-string err))))))
+          (delay-warning 'frameset (error-message-string err) :warning)))))
+
+    ;; Clean temporary caches
     (setq frameset--reuse-list nil
+         frameset--action-map nil
          frameset--target-display nil)

     ;; Make sure there's at least one visible frame.
@@ -1209,27 +1262,26 @@

 ;; Register support

+(defun frameset--clean-after-jump (frame action)
+  "Iconify `:rejected' FRAMEs.
+Intended to be called as `:cleanup' argument of `frameset-restore'.
+Internal use only."
+  (cl-case action
+    (:rejected (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.
+    (:unused (ignore-errors (delete-frame frame)))))
+
 (defun frameset--jump-to-register (data)
   "Restore frameset from DATA stored in register.
 Called from `jump-to-register'.  Internal use only."
-  (let ((fs (aref data 0))
-       reuse-frames iconify-list)
-    (if current-prefix-arg
-       ;; Reuse all frames and delete any left unused
-       (setq reuse-frames t)
-      ;; Reuse matching frames and leave others to be iconified
-      (setq iconify-list (frame-list))
-      (dolist (state (frameset-states fs))
-       (let ((frame (frameset-frame-with-id (frameset-cfg-id (car state))
-                                            iconify-list)))
-         (when frame
-           (push frame reuse-frames)
-           (setq iconify-list (delq frame iconify-list))))))
-    (frameset-restore fs
+  (let ((reuse-frames (if current-prefix-arg nil :match)))
+    (frameset-restore (aref data 0)
                      :filters frameset-session-filter-alist
-                     :reuse-frames reuse-frames)
-    (mapc #'iconify-frame iconify-list))
-
+                     :reuse-frames reuse-frames
+                     :cleanup (and reuse-frames
+                                   ;; Iconify remaining frames
+                                   #'frameset--clean-after-jump)))
   ;; Restore selected frame, buffer and point.
   (let ((frame (frameset-frame-with-id (aref data 1)))
        buffer window)

=== modified file 'lisp/desktop.el'
--- lisp/desktop.el     2014-02-22 02:10:49 +0000
+++ lisp/desktop.el     2014-03-05 23:01:49 +0000
@@ -1057,10 +1057,13 @@
 This function depends on the value of `desktop-saved-frameset'
 being set (usually, by reading it from the desktop)."
   (when (desktop-restoring-frameset-p)
-    (frameset-restore desktop-saved-frameset
-                     :reuse-frames desktop-restore-reuses-frames
-                     :force-display desktop-restore-in-current-display
-                     :force-onscreen desktop-restore-forces-onscreen)))
+    (let ((reuse (if (eq desktop-restore-reuses-frames t) :all :none))
+         (cleanup (if (eq desktop-restore-reuses-frames :keep) :keep :delete)))
+      (frameset-restore desktop-saved-frameset
+                       :reuse-frames reuse
+                       :cleanup cleanup
+                       :force-display desktop-restore-in-current-display
+                       :force-onscreen desktop-restore-forces-onscreen))))

 ;; Just to silence the byte compiler.
 ;; Dynamically bound in `desktop-read'.

Re: Freezing frameset-restore

[martin rudalics]

 >   CLEANUP allows to \"clean up\" the frame list after restoring a frameset:
 >     :delete  Delete all frames that weren't restored.  This is the default.

Why the term "restored" here and not "reused"?

 >     :keep    Keep all frames.
 >     FUNC     A function called with two arguments:
 >              - FRAME, a live frame.
 >              - ACTION, which can be one of
 >                :rejected  Frame existed, but was not a candidate for reuse.
 >                :ignored   Frame existed, was a candidate, but wasn't reused.
 >                :reused    Frame existed, was a candidate, and restored upon.
 >                :created   Frame didn't exist, was created and restored upon.

IIUC "cleanup" means to take care of frames that were not "reused".  I
understand that we should take care of "rejected" or "ignored" frames.
But why care about "reused" and "created" ones?

martin

Re: Freezing frameset-restore

[Juanma Barranquero]

On Thu, Mar 6, 2014 at 6:27 PM, martin rudalics <rudalics@gmx.at> wrote:

> Why the term "restored" here and not "reused"?

"Restored frames" describes (or tries to) all frames that are the
result of restoration, that is, reused frames and new ones (created
specifically to be restored upon).

Please suggest a better wording, my brain has an overdose of the words
"frame", "window", "restore" and "reuse" and cannot think straight.

> IIUC "cleanup" means to take care of frames that were not "reused".

No, the meaning of CLEANUP is to take care of all frames. It just that
most of the time, reused and created frames will be left alone. But...

> understand that we should take care of "rejected" or "ignored" frames.
> But why care about "reused" and "created" ones?

...what if you want to iconify all restored frames too, or just count
them and do something about it?

Again, perhaps CLEANUP is not the best name for the argument. Suggestions?

   J

Re: Freezing frameset-restore

[martin rudalics]

 >> Why the term "restored" here and not "reused"?
 >
 > "Restored frames" describes (or tries to) all frames that are the
 > result of restoration, that is, reused frames and new ones (created
 > specifically to be restored upon).
 >
 > Please suggest a better wording, my brain has an overdose of the words
 > "frame", "window", "restore" and "reuse" and cannot think straight.

Still IMO in

     :delete  Delete all frames that weren't restored.  This is the default.

a "frame that was not restored" refers to a frame stored in a past
session or configuration.  But IIUC you mean here a frame that existed
in the current session or configuration just before you started to
restore frames and you were not able to use profitably.  At least use
"restored upon" instead.

 >> IIUC "cleanup" means to take care of frames that were not "reused".
 >
 > No, the meaning of CLEANUP is to take care of all frames. It just that
 > most of the time, reused and created frames will be left alone. But...
 >
 >> understand that we should take care of "rejected" or "ignored" frames.
 >> But why care about "reused" and "created" ones?
 >
 > ...what if you want to iconify all restored frames too, or just count
 > them and do something about it?

Then CLEANUP is too narrow.  Maybe something like POST-RESTORE (which
clearly isn't a good term either) to emphasize that it does _not only_
deal with frames that were rejected or ignored.

martin

Re: Freezing frameset-restore

[Juanma Barranquero]

On Thu, Mar 6, 2014 at 7:16 PM, martin rudalics <rudalics@gmx.at> wrote:

> But IIUC you mean here a frame that existed
> in the current session or configuration just before you started to
> restore frames and you were not able to use profitably.

Yes. After restoration, there's a set of frames (old and/or new) which
correspond to what was in the frameset. And another, possibly empty,
set of pre-existing frames that do not correspond to the frameset,
they are remnants of the pre-restoration configuration. In some cases
they are kept (C-x r j R), in some cases they are deleted (C-u C-x r j
R).

> At least use "restored upon" instead.

I'll do that.

> Then CLEANUP is too narrow.  Maybe something like POST-RESTORE (which
> clearly isn't a good term either) to emphasize that it does _not only_
> deal with frames that were rejected or ignored.

Well, the term CLEANUP was meant to refer to cleaning up the whole
frame configuration afterwards. But I don't mind at all changing the
argument name, if someone can come up with an alternative less ugly
than POST-RESTORE.

Thanks for your comments, BTW.

    J

Re: Freezing frameset-restore

[Juanma Barranquero]

On Thu, Mar 6, 2014 at 7:16 PM, martin rudalics <rudalics@gmx.at> wrote:

> Then CLEANUP is too narrow.  Maybe something like POST-RESTORE

BTW, the reason for calling this function with its
:ignored/:rejected/:reused/:created action argument, is that this info
is hard (or at least, ugly) to get afterwards. Once the frames are
created and restored, etc. is not immediately obvious whether a frame
was reused or not, for example.

    J

Re: Freezing frameset-restore

[martin rudalics]

 >> Then CLEANUP is too narrow.  Maybe something like POST-RESTORE
 >
 > BTW, the reason for calling this function with its
 > :ignored/:rejected/:reused/:created action argument, is that this info
 > is hard (or at least, ugly) to get afterwards. Once the frames are
 > created and restored, etc. is not immediately obvious whether a frame
 > was reused or not, for example.

I think you should install the change leaving CLEANUP in, but telling
the user somewhere that cleanup also means to investigate new frames
that have just been created for restoring the state.

martin

Re: Freezing frameset-restore

[Juanma Barranquero]

On Fri, Mar 7, 2014 at 8:38 AM, martin rudalics <rudalics@gmx.at> wrote:

> I think you should install the change leaving CLEANUP in

Let's wait for Stefan...

> but telling
> the user somewhere that cleanup also means to investigate new frames
> that have just been created for restoring the state.

Care to suggest a wording?

    J

Re: Freezing frameset-restore

[martin rudalics]

 > Care to suggest a wording?

Something like "...  and CLEANUP affects all frames live at the time
restoration finished, including those that have been reused or created
anew."

martin

Re: Freezing frameset-restore

[Stefan Monnier]

> I think it makes perfect sense to split REUSE-FRAMES into two:

>   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).
>     PRED     A predicate function; it receives as argument a live frame,
>                and must return t to allow reusing it, nil otherwise.

>   CLEANUP allows to \"clean up\" the frame list after restoring a frameset:
>     :delete  Delete all frames that weren't restored.  This is the default.
>     :keep    Keep all frames.
>     FUNC     A function called with two arguments:
>              - FRAME, a live frame.
>              - ACTION, which can be one of
>                :rejected  Frame existed, but was not a candidate for reuse.
>                :ignored   Frame existed, was a candidate, but wasn't reused.
>                :reused    Frame existed, was a candidate, and restored upon.
>                :created   Frame didn't exist, was created and restored upon.
>              Return value is ignored.

Could we instead make frameset-restore return a list of (FRAME
. ACTION), so we can then use a plain dolist if we want to do some
extra cleanup.


        Stefan

Re: Freezing frameset-restore

[Juanma Barranquero]

On Fri, Mar 7, 2014 at 9:32 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> Could we instead make frameset-restore return a list of (FRAME
> . ACTION), so we can then use a plain dolist if we want to do some
> extra cleanup.

We could do that, but a priori I think the most common use case for
frameset-restore is to substitute the current frame(s) for the ones
restored from the frameset, so with your proposal the default call for
frameset restore would go from

  (frameset-restore fs)  ; all the current defaults make sense in this case

to

(dolist (frame-action (frameset-restore fs))
  (with-demoted-errors "Error deleting frame: %s"
    (when (memq (cdr frame-action) '(:ignored :rejected))
      (delete-frame (car frame-action)))))

i.e., we're offloading complexity from the occasional use of CLEANUP's
FUNC variant to the common case. To avoid that we would still need a
way to tell frameset-restore to default to :delete, and then we're
back to CLEANUP.

    J

Re: Freezing frameset-restore

[Juanma Barranquero]

On Fri, Mar 7, 2014 at 10:34 PM, Juanma Barranquero <lekktu@gmail.com> wrote:

> frameset-restore is to substitute the current frame(s) for the ones
> restored from the frameset

"Substitute X for Y" is one of these English idioms that are reversed
in Spanish, I think. I mean that the default case is to replace all
the current frames with the ones from the frameset.

   J

Re: Freezing frameset-restore

[Stefan Monnier]

> i.e., we're offloading complexity from the occasional use of CLEANUP's
> FUNC variant to the common case.

Right.  But we're simplifying the "interface".  And we can provide
a frameset-restore-cleanup function which does the default thing.
Tho the main question is: how many callers are we talking about?
So far I see 2 calls to this function in Emacs's trunk, so moving some
complexity to the caller doesn't sound so terrible.


        Stefan

Re: Freezing frameset-restore

[Juanma Barranquero]

On Sat, Mar 8, 2014 at 1:34 AM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> Right.  But we're simplifying the "interface".

Hm. On one hand, the interface is already complex; on the other hand,
it's not more complex than make-network-process or defcustom ;-)

> And we can provide
> a frameset-restore-cleanup function which does the default thing.

I don't understand what you mean, i.e, a function for the common case?

  (defun frameset-restore-cleanup (frame-action)
    "Delete frame from FRAME-ACTION if it was not restored upon.
  FRAME-ACTION is a pair (FRAME . ACTION) as returned
  by `frameset-restore' (which see)."
    (when (memq (cdr action) '(:ignored :rejected))
      (delete-frame (car frame))))

  (mapc 'frameset-restore-cleanup (frameset-restore fs))

Or a generic one

  (defun frameset-restore-cleanup (frame-action action-map)
    "Clean up frame from FRAME-ACTION according to ACTION-MAP.
  FRAME-ACTION is a pair (FRAME . ACTION) as returned by
  `frameset-restore' (which see).  ACTION-MAP is an alist
  \((ACTIONn . FUNCTIONn)...) mapping actions to their cleanup
  functions.  ACTIONn can be an action, or a list of actions.
  Each FUNCTIONn, if called, gets a single argument FRAME,
  and its return value is ignored."
    (let* ((action (cdr frame-action))
         (func (cl-find-if (lambda (item)
                             (if (consp item)
                                 (memq action item)
                               (eq action item)))
                           action-map :key #'car)))
      (when func
        (funcall (cdr func) (car frame-action)))))

  (dolist (frame-action (frameset-restore fs))
    (frameset-restore-cleanup frame-action
                            '((:rejected :ignored) . #'delete-frame)))


I'm not sure we gain anything with this. As a net loss, we must
construct (and sort) the return list even if it is not going to be
used, while in the CLEANUP argument case, no list is constructed; in
fact, if CLEANUP = :keep, not even (frame-list) is called at that
point.

> Tho the main question is: how many callers are we talking about?
> So far I see 2 calls to this function in Emacs's trunk

Well, yes, of course. This is an un-released feature. I imagine that
once it is released in 24.4, some other uses will be found, but not
that many; it is a specialized feature anyway, and its main use case
(and the reason it was implemented) is already covered by desktop.el.

> so moving some
> complexity to the caller doesn't sound so terrible.

I don't find it terrible, but neither do I find it cleaner. Anyway,
it's your call.

   J

Re: Freezing frameset-restore

[Juanma Barranquero]

>   (dolist (frame-action (frameset-restore fs))
>     (frameset-restore-cleanup frame-action
>                             '((:rejected :ignored) . #'delete-frame)))

Note, BTW, that I left aside error checking. But in general, failure
to clean up one frame shouldn't stop the cleanup, so the users will
have to wrap the call to frameset-restore inside
`with-demoted-errors', `ignore-errors' or somesuch (my preference is
`condition-case' with a call to `delay-warning').

   J

Re: Freezing frameset-restore

[Stefan Monnier]

>> Right.  But we're simplifying the "interface".
> Hm.  On one hand, the interface is already complex; on the other hand,
> it's not more complex than make-network-process or defcustom ;-)

The fact that it's bad already or that there's worse out there is no excuse.

>> And we can provide a frameset-restore-cleanup function which does the
>> default thing.
> I don't understand what you mean, i.e, a function for the common case?

Right (I'd expect that function to take the whole list, BTW, but
if mapc can be used, it's not critical).

Of course, with only 2 users so far, it might be tricky to decide what
is "common".

> I'm not sure we gain anything with this. As a net loss, we must
> construct (and sort)

I don't see why sorting would be needed.

> the return list even if it is not going to be used, while in the
> CLEANUP argument case, no list is constructed; in fact, if CLEANUP
> = :keep, not even (frame-list) is called at that point.

Clearly, the returned list does not need to give an action for
all frames.  Only for those which we did consider.  So we don't need to
call `frames-list' just so as to build the return value.

> that many; it is a specialized feature anyway, and its main use case
> (and the reason it was implemented) is already covered by desktop.el.

Exactly.

> I don't find it terrible, but neither do I find it cleaner.  Anyway,
> it's your call.

I think it's cleaner.


        Stefan

Re: Freezing frameset-restore

[Juanma Barranquero]

On Sat, Mar 8, 2014 at 5:31 AM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> The fact that it's bad already or that there's worse out there is no excuse.

I wasn't saying that as an excuse, just pointing out that I don't find
it terribly complex.

> Right (I'd expect that function to take the whole list, BTW, but
> if mapc can be used, it's not critical).

Oh, I see. mapc can be used, but if you're going to add error checking
(and I want to), it's not as clean as (mapc #'delete-frame
(frameset-restore fs)). I've changed the function to process the list
instead of individual items, and do error-checking. Now it defaults to
an action-map of '(((:ignored :rejected) . delete-frame)), but it can
be passed another action-map if desired.

> I don't see why sorting would be needed.

If the action is to delete, frames must be sorted so minibufferless
frames are deleted first; otherwise, we could try to delete a frame
whose minibuffer window is still used by another frame. Yes, I do
think that life would be easier if minibufferless frames didn't exist,
but obviously they have their fans (hi, Drew!).

> Clearly, the returned list does not need to give an action for
> all frames.  Only for those which we did consider. So we don't need to
> call `frames-list' just so as to build the return value.

I'm not sure what do you mean here by "those which we did consider"
(there are several possible interpretations: candidates, frames
included/not included in LIST passed to REUSE-FRAMES, etc.), but in
any case, I suspect that I strongly disagree. For example, you could
pass REUSE-FRAMES = :none, which wouldn't "consider" any existing
frame at all, and yet want to delete all previously-existing frames.

And even in our scarce two uses cases, already one of them
(frameset--jump-to-register) does need a cleanup other than
delete-frame, because its default action (copied from
frame-configuration-to-register) is to iconify.

> I think it's cleaner.

Here's the code (untabified, so not 'bzr patch'able).

    J



2014-03-08  Juanma Barranquero  <lekktu@gmail.com>

        * frameset.el: Separate frame-list cleaning from reusing.
        (frameset--action-map): New variable.
        (frameset--restore-frame): Cache frame action.
        (frameset-restore): Redesign REUSE-FRAMES arg by removing cleanup
        options and adding new PRED option.  Return alist of frames and
        actions carried on them.
        (frameset-restore-cleanup): New function.
        (frameset--jump-to-register): Use it.

        * desktop.el (desktop-restore-frameset): Use frameset-restore-cleanup.




=== modified file 'lisp/frameset.el'
--- lisp/frameset.el    2014-03-05 16:22:47 +0000
+++ lisp/frameset.el    2014-03-08 05:43:12 +0000
@@ -791,6 +791,11 @@
 Its value is only meaningful during execution of `frameset-restore'.
 Internal use only.")

+(defvar frameset--action-map nil
+  "Map of frames to actions.
+Its value is only meaningful during execution of `frameset-restore'.
+Internal use only.")
+
 (defun frameset-compute-pos (value left/top right/bottom)
   "Return an absolute positioning value for a frame.
 VALUE is the value of a positional frame parameter (`left' or `top').
@@ -982,16 +987,20 @@
        (push visible alt-cfg)
        (push (cons 'fullscreen fullscreen) alt-cfg)))

-    ;; Time to find or create a frame an apply the big bunch of parameters.
-    ;; If a frame needs to be created and it falls partially or fully
offscreen,
-    ;; sometimes it gets "pushed back" onscreen; however, moving it
afterwards is
-    ;; allowed.  So we create the frame as invisible and then reapply the full
-    ;; parameter alist (including position and size parameters).
-    (setq frame (or (and frameset--reuse-list
-                        (frameset--reuse-frame display filtered-cfg))
-                   (make-frame-on-display display
-                                          (cons '(visibility)
-
(frameset--initial-params filtered-cfg)))))
+    ;; Time to find or create a frame and apply the big bunch of parameters.
+    (setq frame (and frameset--reuse-list
+                    (frameset--reuse-frame display filtered-cfg)))
+    (if frame
+       (puthash frame :reused frameset--action-map)
+      ;; If a frame needs to be created and it falls partially or
fully offscreen,
+      ;; sometimes it gets "pushed back" onscreen; however, moving it
afterwards is
+      ;; allowed.  So we create the frame as invisible and then
reapply the full
+      ;; parameter alist (including position and size parameters).
+      (setq frame (make-frame-on-display display
+                                        (cons '(visibility)
+
(frameset--initial-params filtered-cfg))))
+      (puthash frame :created frameset--action-map))
+
     (modify-frame-parameters frame
                             (if (eq (frame-parameter frame
'fullscreen) fullscreen)
                                 ;; Workaround for bug#14949
@@ -1050,13 +1059,13 @@
 FILTERS is an alist of parameter filters; if nil, the value of
 `frameset-filter-alist' is used instead.

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

 FORCE-DISPLAY can be:
   t        Frames are restored in the current display.
@@ -1077,31 +1086,67 @@
           - a list (LEFT TOP WIDTH HEIGHT), describing the workarea.
           It must return non-nil to force the frame onscreen, nil otherwise.

+All keyword parameters default to nil.
+
 Note the timing and scope of the operations described above: REUSE-FRAMES
 affects existing frames; PREDICATE, FILTERS and FORCE-DISPLAY affect the frame
 being restored before that happens; and FORCE-ONSCREEN affects the frame once
 it has been restored.

-All keyword parameters default to nil."
+Returns an alist ((FRAMEn . ACTIONn)...) mapping all live frames to the
+actions carried on them during restoration.  ACTION is one of these:
+  :rejected   Frame existed, but was not a candidate for reuse.
+  :ignored    Frame existed, was a candidate, but wasn't reused.
+  :reused     Frame existed, was a candidate, and restored upon.
+  :created    Frame didn't exist, was created and restored upon.
+
+Function `frameset-restore-cleanup' can be useful to process this list."

   (cl-assert (frameset-valid-p frameset))

-  (let (other-frames)
+  (let ((frames (frame-list))
+       (non-candidates nil)
+       cleanup)
+
+    (setq frameset--action-map (make-hash-table :test #'eq :weakness 'value))

     ;; frameset--reuse-list is a list of frames potentially reusable.  Later we
     ;; will decide which ones can be reused, and how to deal with any leftover.
     (pcase reuse-frames
-      ((or `nil `:keep)
+      ((or :all `nil)
+       (setq frameset--reuse-list frames))
+      (:none
        (setq frameset--reuse-list nil
-            other-frames (frame-list)))
+            non-candidates t))
+      (:match
+       (setq frameset--reuse-list
+            (cl-loop for (state) in (frameset-states frameset)
+                     when (frameset-frame-with-id (frameset-cfg-id
state) frames)
+                     collect it)
+            non-candidates t))
+      ((pred functionp)
+       (setq frameset--reuse-list (cl-remove-if
+                                  (lambda (frame)
+                                    (if (funcall reuse-frames frame)
+                                        nil
+                                      (puthash frame :rejected
frameset--action-map)))
+                                  frames)))
       ((pred consp)
        (setq frameset--reuse-list (copy-sequence reuse-frames)
-            other-frames (cl-delete-if (lambda (frame)
-                                         (memq frame frameset--reuse-list))
-                                       (frame-list))))
+            non-candidates t))
       (_
-       (setq frameset--reuse-list (frame-list)
-            other-frames nil)))
+       (error "Invalid arg :reuse-frames %s" reuse-frames)))
+
+    (when non-candidates
+      ;; If we didn't mark non-candidate frames on the fly, do it now.
+      (mapc (lambda (frame)
+             (puthash frame :rejected frameset--action-map))
+           (cl-set-difference frames frameset--reuse-list)))
+
+    ;; Mark candidates as :ignored; they will be reassigned later, if chosen.
+    (mapc (lambda (frame)
+           (puthash frame :ignored frameset--action-map))
+         frameset--reuse-list)

     ;; Sort saved states to guarantee that minibufferless frames will
be created
     ;; after the frames that contain their minibuffer windows.
@@ -1138,10 +1183,8 @@
                  ;; existing frame whose id matches a frame
configuration in the
                  ;; frameset.  Once the frame config is properly
restored, we can
                  ;; reset the old frame's id to nil.
-                 (setq duplicate (and other-frames
-                                      (or (eq reuse-frames :keep)
(consp reuse-frames))
-                                      (frameset-frame-with-id
(frameset-cfg-id frame-cfg)
-                                                              other-frames)))
+                 (setq duplicate (frameset-frame-with-id
(frameset-cfg-id frame-cfg)
+                                                         frames))
                  ;; Restore minibuffers.  Some of this stuff could be
done in a filter
                  ;; function, but it would be messy because restoring
minibuffers affects
                  ;; global state; it's best to do it here than add a
bunch of global
@@ -1187,48 +1230,66 @@
     ;; other frames are already visible (discussed in thread for bug#14841).
     (sit-for 0 t)

-    ;; Delete remaining frames, but do not fail if some resist being deleted.
-    (unless (eq reuse-frames :keep)
-      (dolist (frame (sort (nconc (if (listp reuse-frames) nil other-frames)
-                                 frameset--reuse-list)
-                          ;; Minibufferless frames must go first to avoid
-                          ;; errors when attempting to delete a frame whose
-                          ;; minibuffer window is used by another frame.
-                          #'frameset-minibufferless-first-p))
-       (condition-case err
-           (delete-frame frame)
-         (error
-          (delay-warning 'frameset (error-message-string err))))))
+    ;; Compute the frame-action list.
+    (dolist (frame (sort (frame-list) #'frameset-minibufferless-first-p))
+      ;; We are generating the list in reverse order.  Will nreverse later.
+      (push (cons frame (gethash frame frameset--action-map)) cleanup))
+
+    ;; Clean temporary caches
     (setq frameset--reuse-list nil
+         frameset--action-map nil
          frameset--target-display nil)

     ;; Make sure there's at least one visible frame.
     (unless (or (daemonp) (visible-frame-list))
-      (make-frame-visible (car (frame-list))))))
-
+      (make-frame-visible (car (frame-list))))
+
+    ;; Minibufferless frames must go first to avoid errors when attempting
+    ;; to delete a frame whose minibuffer window is used by another frame.
+    (nreverse cleanup)))
+
+(defun frameset-restore-cleanup (frame-action-list &optional action-map)
+  "Clean up the frames in FRAME-ACTION-LIST according to ACTION-MAP.
+FRAME-ACTION-LIST is an alist of conses (FRAME . ACTION) as returned
+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.  Each FUNCTION, if called,
+gets a single argument FRAME, and its return value is ignored.
+ACTION-MAP defaults to deleting all :ignored and :rejected frames."
+  (unless action-map
+    (setq action-map '(((:ignored :rejected) . delete-frame))))
+  (dolist (frame-action frame-action-list)
+    (condition-case nil
+       (let* ((action (cdr frame-action))
+              (found (cl-assoc-if (lambda (item)
+                                    (if (consp item)
+                                        (memq action item)
+                                      (eq action item)))
+                                  action-map)))
+         (when found
+           (funcall (cdr found) (car frame-action))))
+      (error
+       (delay-warning 'frameset
+                     (format "Error cleaning up frame %s" (car frame-action))
+                     :warning)))))

 ;; Register support

 (defun frameset--jump-to-register (data)
   "Restore frameset from DATA stored in register.
 Called from `jump-to-register'.  Internal use only."
-  (let ((fs (aref data 0))
-       reuse-frames iconify-list)
-    (if current-prefix-arg
-       ;; Reuse all frames and delete any left unused
-       (setq reuse-frames t)
-      ;; Reuse matching frames and leave others to be iconified
-      (setq iconify-list (frame-list))
-      (dolist (state (frameset-states fs))
-       (let ((frame (frameset-frame-with-id (frameset-cfg-id (car state))
-                                            iconify-list)))
-         (when frame
-           (push frame reuse-frames)
-           (setq iconify-list (delq frame iconify-list))))))
-    (frameset-restore fs
-                     :filters frameset-session-filter-alist
-                     :reuse-frames reuse-frames)
-    (mapc #'iconify-frame iconify-list))
+  (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))))

   ;; Restore selected frame, buffer and point.
   (let ((frame (frameset-frame-with-id (aref data 1)))

=== modified file 'lisp/desktop.el'
--- lisp/desktop.el     2014-02-22 02:10:49 +0000
+++ lisp/desktop.el     2014-03-08 05:22:21 +0000
@@ -1057,10 +1057,14 @@
 This function depends on the value of `desktop-saved-frameset'
 being set (usually, by reading it from the desktop)."
   (when (desktop-restoring-frameset-p)
-    (frameset-restore desktop-saved-frameset
-                     :reuse-frames desktop-restore-reuses-frames
-                     :force-display desktop-restore-in-current-display
-                     :force-onscreen desktop-restore-forces-onscreen)))
+    (let* ((reuse desktop-restore-reuses-frames)
+          (cleanup-list (frameset-restore
+                         desktop-saved-frameset
+                         :reuse-frames (if (eq reuse t) :all :none)
+                         :force-display desktop-restore-in-current-display
+                         :force-onscreen desktop-restore-forces-onscreen)))
+      (unless (eq reuse :keep)
+       (frameset-restore-cleanup cleanup-list)))))

 ;; Just to silence the byte compiler.
 ;; Dynamically bound in `desktop-read'.

Re: Freezing frameset-restore

[Stefan Monnier]

>> Clearly, the returned list does not need to give an action for
>> all frames.  Only for those which we did consider. So we don't need to
>> call `frames-list' just so as to build the return value.
> I'm not sure what do you mean here by "those which we did consider"

The function returns the frames that it considered and what action it
took with it.  It will not necessarily return an entry for each
existing frame.

> any case, I suspect that I strongly disagree. For example, you could
> pass REUSE-FRAMES = :none, which wouldn't "consider" any existing
> frame at all, and yet want to delete all previously-existing frames.

I don't see why that would be problem.  The returned list simply
wouldn't mention any of the existing frames, so if you want to delete
those you'll have to call `frame-list'.

> And even in our scarce two uses cases, already one of them
> (frameset--jump-to-register) does need a cleanup other than
> delete-frame, because its default action (copied from
> frame-configuration-to-register) is to iconify.

Look at it not as "move stuff from frameset-restore to its caller" but
as "split frameset-restore into two functions".


        Stefan

Re: Freezing frameset-restore

[Juanma Barranquero]

On Sat, Mar 8, 2014 at 3:27 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> The function returns the frames that it considered and what action it
> took with it.  It will not necessarily return an entry for each
> existing frame.

What is the gain? In most cases, it already has to consider every
frame, for several reasons, chief among them that it tries to match
the frameset id. The only cases that is not so is if you explicitly
pass a LIST to REUSE-FRAMES, or :none.

>> any case, I suspect that I strongly disagree. For example, you could
>> pass REUSE-FRAMES = :none, which wouldn't "consider" any existing
>> frame at all, and yet want to delete all previously-existing frames.
>
> I don't see why that would be problem.  The returned list simply
> wouldn't mention any of the existing frames, so if you want to delete
> those you'll have to call `frame-list'.

And check against the returned list (you don't want to delete the just
restored frames). Messier than have the full list in the first place.

frameset-restore is not a high-performance function to be called many
times per second, it's a user-level function, it operates in human
time. It already has to call frame-list, build and filter out frame
parameter lists, etc.

> Look at it not as "move stuff from frameset-restore to its caller" but
> as "split frameset-restore into two functions".

That's what I've done with frameset-restore-cleanup. I think the
current interface (in my last patch) is good and clear enough.

    J

Re: Freezing frameset-restore

[Juanma Barranquero]

A variant of the last patch.

frameset-restore still returns an action-map for all frames, but it
builds it on the fly while processing the frames, so it avoids
recomputing it at the end and calling again frame-list unnecessarily.

    J



=== modified file 'lisp/desktop.el'
--- lisp/desktop.el     2014-02-22 02:10:49 +0000
+++ lisp/desktop.el     2014-03-08 22:31:50 +0000
@@ -1057,10 +1057,14 @@
 This function depends on the value of `desktop-saved-frameset'
 being set (usually, by reading it from the desktop)."
   (when (desktop-restoring-frameset-p)
-    (frameset-restore desktop-saved-frameset
-                     :reuse-frames desktop-restore-reuses-frames
-                     :force-display desktop-restore-in-current-display
-                     :force-onscreen desktop-restore-forces-onscreen)))
+    (let* ((reuse desktop-restore-reuses-frames)
+          (cleanup-list (frameset-restore
+                         desktop-saved-frameset
+                         :reuse-frames (if (eq reuse t) :all :none)
+                         :force-display desktop-restore-in-current-display
+                         :force-onscreen desktop-restore-forces-onscreen)))
+      (unless (eq reuse :keep)
+       (frameset-restore-cleanup cleanup-list)))))

 ;; Just to silence the byte compiler.
 ;; Dynamically bound in `desktop-read'.

=== modified file 'lisp/frameset.el'
--- lisp/frameset.el    2014-03-08 22:26:20 +0000
+++ lisp/frameset.el    2014-03-08 23:20:07 +0000
@@ -791,6 +791,11 @@
 Its value is only meaningful during execution of `frameset-restore'.
 Internal use only.")

+(defvar frameset--action-map nil
+  "Map of frames to actions.
+Its value is only meaningful during execution of `frameset-restore'.
+Internal use only.")
+
 (defun frameset-compute-pos (value left/top right/bottom)
   "Return an absolute positioning value for a frame.
 VALUE is the value of a positional frame parameter (`left' or `top').
@@ -982,16 +987,23 @@
        (push visible alt-cfg)
        (push (cons 'fullscreen fullscreen) alt-cfg)))

-    ;; Time to find or create a frame an apply the big bunch of parameters.
-    ;; If a frame needs to be created and it falls partially or fully
offscreen,
-    ;; sometimes it gets "pushed back" onscreen; however, moving it
afterwards is
-    ;; allowed.  So we create the frame as invisible and then reapply the full
-    ;; parameter alist (including position and size parameters).
-    (setq frame (or (and frameset--reuse-list
-                        (frameset--reuse-frame display filtered-cfg))
-                   (make-frame-on-display display
+    ;; Time to find or create a frame and apply the big bunch of parameters.
+    (setq frame (and frameset--reuse-list
+                    (frameset--reuse-frame display filtered-cfg)))
+    (let (action)
+      (if frame
+         (setq action :reused)
+       ;; If a frame needs to be created and it falls partially or
fully offscreen,
+       ;; sometimes it gets "pushed back" onscreen; however, moving
it afterwards is
+       ;; allowed.  So we create the frame as invisible and then
reapply the full
+       ;; parameter alist (including position and size parameters).
+       (setq frame (make-frame-on-display display
                                           (cons '(visibility)
-
(frameset--initial-params filtered-cfg)))))
+
(frameset--initial-params filtered-cfg)))
+             action :created))
+      (setq frameset--action-map (cl-acons frame action
+                                          (assq-delete-all frame
frameset--action-map))))
+
     (modify-frame-parameters frame
                             (if (eq (frame-parameter frame
'fullscreen) fullscreen)
                                 ;; Workaround for bug#14949
@@ -1050,13 +1062,13 @@
 FILTERS is an alist of parameter filters; if nil, the value of
 `frameset-filter-alist' is used instead.

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

 FORCE-DISPLAY can be:
   t        Frames are restored in the current display.
@@ -1077,31 +1089,68 @@
           - a list (LEFT TOP WIDTH HEIGHT), describing the workarea.
           It must return non-nil to force the frame onscreen, nil otherwise.

+All keyword parameters default to nil.
+
 Note the timing and scope of the operations described above: REUSE-FRAMES
 affects existing frames; PREDICATE, FILTERS and FORCE-DISPLAY affect the frame
 being restored before that happens; and FORCE-ONSCREEN affects the frame once
 it has been restored.

-All keyword parameters default to nil."
+Returns an alist ((FRAMEn . ACTIONn)...) mapping all live frames to the
+actions carried on them during restoration.  ACTION is one of these:
+  :rejected   Frame existed, but was not a candidate for reuse.
+  :ignored    Frame existed, was a candidate, but wasn't reused.
+  :reused     Frame existed, was a candidate, and restored upon.
+  :created    Frame didn't exist, was created and restored upon.
+
+Function `frameset-restore-cleanup' can be useful to process this list."

   (cl-assert (frameset-valid-p frameset))

-  (let (other-frames)
+  (let ((frames (frame-list))
+       (non-candidates nil)
+       cleanup)
+
+    (setq frameset--action-map nil)

     ;; frameset--reuse-list is a list of frames potentially reusable.  Later we
     ;; will decide which ones can be reused, and how to deal with any leftover.
     (pcase reuse-frames
-      ((or `nil `:keep)
+      ((or :all `nil)
+       (setq frameset--reuse-list frames))
+      (:none
        (setq frameset--reuse-list nil
-            other-frames (frame-list)))
+            non-candidates t))
+      (:match
+       (setq frameset--reuse-list
+            (cl-loop for (state) in (frameset-states frameset)
+                     when (frameset-frame-with-id (frameset-cfg-id
state) frames)
+                     collect it)
+            non-candidates t))
+      ((pred functionp)
+       (setq frameset--reuse-list (cl-remove-if
+                                  (lambda (frame)
+                                    (if (funcall reuse-frames frame)
+                                        nil
+                                      (push (cons frame :rejected)
+                                            frameset--action-map)))
+                                  frames)))
       ((pred consp)
        (setq frameset--reuse-list (copy-sequence reuse-frames)
-            other-frames (cl-delete-if (lambda (frame)
-                                         (memq frame frameset--reuse-list))
-                                       (frame-list))))
+            non-candidates t))
       (_
-       (setq frameset--reuse-list (frame-list)
-            other-frames nil)))
+       (error "Invalid arg :reuse-frames %s" reuse-frames)))
+
+    (when non-candidates
+      ;; If we didn't mark non-candidate frames on the fly, do it now.
+      (mapc (lambda (frame)
+             (push (cons frame :rejected) frameset--action-map))
+           (cl-set-difference frames frameset--reuse-list)))
+
+    ;; Mark candidates as :ignored; they will be reassigned later, if chosen.
+    (mapc (lambda (frame)
+           (push (cons frame :ignored) frameset--action-map))
+         frameset--reuse-list)

     ;; Sort saved states to guarantee that minibufferless frames will
be created
     ;; after the frames that contain their minibuffer windows.
@@ -1138,10 +1187,8 @@
                  ;; existing frame whose id matches a frame
configuration in the
                  ;; frameset.  Once the frame config is properly
restored, we can
                  ;; reset the old frame's id to nil.
-                 (setq duplicate (and other-frames
-                                      (or (eq reuse-frames :keep)
(consp reuse-frames))
-                                      (frameset-frame-with-id
(frameset-cfg-id frame-cfg)
-                                                              other-frames)))
+                 (setq duplicate (frameset-frame-with-id
(frameset-cfg-id frame-cfg)
+                                                         frames))
                  ;; Restore minibuffers.  Some of this stuff could be
done in a filter
                  ;; function, but it would be messy because restoring
minibuffers affects
                  ;; global state; it's best to do it here than add a
bunch of global
@@ -1187,48 +1234,67 @@
     ;; other frames are already visible (discussed in thread for bug#14841).
     (sit-for 0 t)

-    ;; Delete remaining frames, but do not fail if some resist being deleted.
-    (unless (eq reuse-frames :keep)
-      (dolist (frame (sort (nconc (if (listp reuse-frames) nil other-frames)
-                                 frameset--reuse-list)
-                          ;; Minibufferless frames must go first to avoid
-                          ;; errors when attempting to delete a frame whose
-                          ;; minibuffer window is used by another frame.
-                          #'frameset-minibufferless-first-p))
-       (condition-case err
-           (delete-frame frame)
-         (error
-          (delay-warning 'frameset (error-message-string err))))))
+    ;; Prepare the pre-computed frame-action map
+    (setq cleanup (cl-sort frameset--action-map
+                          ;; Minibufferless frames must go first to
avoid errors
+                          ;; when attempting to delete a frame whose minibuffer
+                          ;; window is used by another frame.
+                          #'frameset-minibufferless-first-p
+                          :key #'car))
+
+    ;; Clean temporary caches
     (setq frameset--reuse-list nil
+         frameset--action-map nil
          frameset--target-display nil)

     ;; Make sure there's at least one visible frame.
     (unless (or (daemonp) (visible-frame-list))
-      (make-frame-visible (selected-frame)))))
-
+      (make-frame-visible (selected-frame)))
+
+    cleanup))
+
+(defun frameset-restore-cleanup (frame-action-list &optional action-map)
+  "Clean up the frames in FRAME-ACTION-LIST according to ACTION-MAP.
+FRAME-ACTION-LIST is an alist of conses (FRAME . ACTION) as returned
+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.  Each FUNCTION, if called,
+gets a single argument FRAME, and its return value is ignored.
+ACTION-MAP defaults to deleting all :ignored and :rejected frames."
+  (unless action-map
+    (setq action-map '(((:ignored :rejected) . delete-frame))))
+  (dolist (frame-action frame-action-list)
+    (condition-case-unless-debug nil
+       (let* ((action (cdr frame-action))
+              (found (cl-assoc-if (lambda (item)
+                                    (if (consp item)
+                                        (memq action item)
+                                      (eq action item)))
+                                  action-map)))
+         (when found
+           (funcall (cdr found) (car frame-action))))
+      (error
+       (delay-warning 'frameset
+                     (format "Error cleaning up frame %s" (car frame-action))
+                     :warning)))))

 ;; Register support

 (defun frameset--jump-to-register (data)
   "Restore frameset from DATA stored in register.
 Called from `jump-to-register'.  Internal use only."
-  (let ((fs (aref data 0))
-       reuse-frames iconify-list)
-    (if current-prefix-arg
-       ;; Reuse all frames and delete any left unused
-       (setq reuse-frames t)
-      ;; Reuse matching frames and leave others to be iconified
-      (setq iconify-list (frame-list))
-      (dolist (state (frameset-states fs))
-       (let ((frame (frameset-frame-with-id (frameset-cfg-id (car state))
-                                            iconify-list)))
-         (when frame
-           (push frame reuse-frames)
-           (setq iconify-list (delq frame iconify-list))))))
-    (frameset-restore fs
-                     :filters frameset-session-filter-alist
-                     :reuse-frames reuse-frames)
-    (mapc #'iconify-frame iconify-list))
+  (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))))

   ;; Restore selected frame, buffer and point.
   (let ((frame (frameset-frame-with-id (aref data 1)))

Re: Freezing frameset-restore

[Stefan Monnier]

> What is the gain?

You yourself mentioned the downside of having to uselessly call
frame-list when :keep would make that unnecessary.

> frameset-restore is not a high-performance function to be called many
> times per second, it's a user-level function, it operates in human
> time. It already has to call frame-list, build and filter out frame
> parameter lists, etc.

Fine by me,


        Stefan

Re: Freezing frameset-restore

[Stefan Monnier]

> 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

Re: Freezing frameset-restore

[Juanma Barranquero]

On Mon, Mar 10, 2014 at 5:29 AM, Stefan Monnier
<monnier@iro.umontreal.ca> 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

Re: Freezing frameset-restore

[Juanma Barranquero]

On Mon, Mar 10, 2014 at 6:26 AM, Juanma Barranquero <lekktu@gmail.com> wrote:

> 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

The more I think about it, the more I believe it makes no sense to
separate cleanup from frameset-restore. It is not an additional,
optional part of frameset restoration, it is an integral part of it,
even in these cases where the cleanup consists in doing nothing.

If frameset-restore just created new frames, separating both
operations would be reasonable. But frameset restoration isn't like
that, it already can reuse exiting frames; it moves them, changes
their visibility, resizes them. The whole experience of restoring a
frameset includes deciding what to do with the remaining frames.

Re: Freezing frameset-restore

[Stefan Monnier]

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

Re: Freezing frameset-restore

[Juanma Barranquero]

On Mon, Mar 10, 2014 at 3:19 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:

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

OK. Ill change that, too.

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

Hmm. I think you're right. That's two in a row. ;-)

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

Eh, no. As I said already, processing the frames in random (maphash)
order just isn't going to cut it if you want to delete frames.

> 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 unconvinced, but it's not a big deal. It's a much bigger deal
that, as I've argued again in my next message, I think we shouldn't be
extracting cleanup from frameset-restore.

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

That would be an argument to split every long function along its
length, just by looking for appropriate places where it can be
"cleanly" done, but that's not an argument that I can accept.
Restoring a frameset is going from one frame configuration to another
one; cleanup isn't just something that gets accidentally done
afterwards.

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

See above.

And please don't say that we can return a hash and leave the sorting
for the caller. The caller is us, we already call delete-frame, so
we'll have to add yet *more* ugly code at the point of call of
frameset-restore.

We keep having this conversation where you extract conclusions from
our two use cases; but I'm trying to design an interface for those
people out there who will perhaps have ideas and uses we cannot think
of right now. That's why there is a frameset.el library, isn't it? To
allow people to be creative with it?

> That's pretty close to the definition of over-engineering.

That's also pretty close to the definition of writing a library, as
opposed to a tightly knitted piece of code.

> Personally, I'd just ditch frameset-restore-cleanup.

I'd ditch it too, for a CLEANUP arg to frameset-restore.

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

You can create an invisible frame, save it in a frameset, then delete
it (so you have just one frame, visible), and then restore the
frameset over your only frame. Presto! If frameset-restore doesn't
protect against it, you end with just one, quite invisible, frame. So
frameset-restore checks that (except in daemon mode) at least one
frame is visible after restoration. IMO, that check should be (and it
was until now) the last step in restoration, after everything else
that affects the existing frames.

   J

Re: Freezing frameset-restore

[Juanma Barranquero]

This is the patch I'm happy with. Incorporates most of your
suggestions, but deletes frameset-restore-cleanup and re-adds the
CLEANUP (now CLEANUP-FRAMES) argument. I think it offers the best of
both worlds: it offers a simple interface, and puts the (relatively
small) complexity back in the bag, instead of scattered at the point
of call.

Hope you accept this and we can put the poor beast to rest.

    J


=== modified file 'lisp/desktop.el'
--- lisp/desktop.el     2014-03-10 02:18:29 +0000
+++ lisp/desktop.el     2014-03-10 18:09:09 +0000
@@ -404,12 +404,12 @@

 (defcustom desktop-restore-forces-onscreen t
   "If t, offscreen frames are restored onscreen instead.
-If `:all', frames that are partially offscreen are also forced onscreen.
+If `all', frames that are partially offscreen are also forced onscreen.
 NOTE: Checking of frame boundaries is only approximate and can fail
 to reliably detect frames whose onscreen/offscreen state depends on a
 few pixels, especially near the right / bottom borders of the screen."
   :type '(choice (const :tag "Only fully offscreen frames" t)
-                (const :tag "Also partially offscreen frames" :all)
+                (const :tag "Also partially offscreen frames" all)
                 (const :tag "Do not force frames onscreen" nil))
   :group 'desktop
   :version "24.4")
@@ -417,7 +417,7 @@
 (defcustom desktop-restore-reuses-frames t
   "If t, restoring frames reuses existing frames.
 If nil, existing frames are deleted.
-If `:keep', existing frames are kept and not reused."
+If `keep', existing frames are kept and not reused."
   :type '(choice (const :tag "Reuse existing frames" t)
                 (const :tag "Delete existing frames" nil)
                 (const :tag "Keep existing frames" :keep))
@@ -1058,7 +1058,8 @@
 being set (usually, by reading it from the desktop)."
   (when (desktop-restoring-frameset-p)
     (frameset-restore desktop-saved-frameset
-                     :reuse-frames desktop-restore-reuses-frames
+                     :reuse-frames (eq desktop-restore-reuses-frames t)
+                     :cleanup-frames (not (eq
desktop-restore-reuses-frames 'keep))
                      :force-display desktop-restore-in-current-display
                      :force-onscreen desktop-restore-forces-onscreen)))


=== modified file 'lisp/frameset.el'
--- lisp/frameset.el    2014-03-08 22:26:20 +0000
+++ lisp/frameset.el    2014-03-10 18:25:41 +0000
@@ -786,10 +786,8 @@

 ;; Restoring framesets

-(defvar frameset--reuse-list nil
-  "The list of frames potentially reusable.
-Its value is only meaningful during execution of `frameset-restore'.
-Internal use only.")
+(defvar frameset--reuse-list)
+(defvar frameset--action-map)

 (defun frameset-compute-pos (value left/top right/bottom)
   "Return an absolute positioning value for a frame.
@@ -871,7 +869,7 @@
          (modify-frame-parameters frame params))))))

 (defun frameset--find-frame-if (predicate display &rest args)
-  "Find a frame in `frameset--reuse-list' satisfying PREDICATE.
+  "Find a reusable frame satisfying PREDICATE.
 Look through available frames whose display property matches DISPLAY
 and return the first one for which (PREDICATE frame ARGS) returns t.
 If PREDICATE is nil, it is always satisfied.  Internal use only."
@@ -982,16 +980,20 @@
        (push visible alt-cfg)
        (push (cons 'fullscreen fullscreen) alt-cfg)))

-    ;; Time to find or create a frame an apply the big bunch of parameters.
-    ;; If a frame needs to be created and it falls partially or fully
offscreen,
-    ;; sometimes it gets "pushed back" onscreen; however, moving it
afterwards is
-    ;; allowed.  So we create the frame as invisible and then reapply the full
-    ;; parameter alist (including position and size parameters).
-    (setq frame (or (and frameset--reuse-list
-                        (frameset--reuse-frame display filtered-cfg))
-                   (make-frame-on-display display
-                                          (cons '(visibility)
-
(frameset--initial-params filtered-cfg)))))
+    ;; Time to find or create a frame and apply the big bunch of parameters.
+    (setq frame (and frameset--reuse-list
+                    (frameset--reuse-frame display filtered-cfg)))
+    (if frame
+       (puthash frame :reused frameset--action-map)
+      ;; If a frame needs to be created and it falls partially or
fully offscreen,
+      ;; sometimes it gets "pushed back" onscreen; however, moving it
afterwards is
+      ;; allowed.  So we create the frame as invisible and then
reapply the full
+      ;; parameter alist (including position and size parameters).
+      (setq frame (make-frame-on-display display
+                                        (cons '(visibility)
+
(frameset--initial-params filtered-cfg))))
+      (puthash frame :created frameset--action-map))
+
     (modify-frame-parameters frame
                             (if (eq (frame-parameter frame
'fullscreen) fullscreen)
                                 ;; Workaround for bug#14949
@@ -1038,7 +1040,8 @@
 ;;;###autoload
 (cl-defun frameset-restore (frameset
                            &key predicate filters reuse-frames
-                                force-display force-onscreen)
+                                force-display force-onscreen
+                                cleanup-frames)
   "Restore a FRAMESET into the current display(s).

 PREDICATE is a function called with two arguments, the parameter alist
@@ -1050,58 +1053,79 @@
 FILTERS is an alist of parameter filters; if nil, the value of
 `frameset-filter-alist' is used instead.

-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.
-  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.
+REUSE-FRAMES selects the policy to reuse frames when restoring:
+  t        All existing frames can be reused.
+  nil      No existing frame can be reused.
+  match    Only frames with matching frame ids can be reused.
+  PRED     A predicate function; it receives as argument a live frame,
+             and must return non-nil to allow reusing it, nil otherwise.

 FORCE-DISPLAY can be:
   t        Frames are restored in the current display.
   nil      Frames are restored, if possible, in their original displays.
-  :delete  Frames in other displays are deleted instead of restored.
+  delete   Frames in other displays are deleted instead of restored.
   PRED     A function called with two arguments, the parameter alist and
             the window state (in that order).  It must return t, nil or
-            `:delete', as above but affecting only the frame that will
+            `delete', as above but affecting only the frame that will
             be created from that parameter alist.

 FORCE-ONSCREEN can be:
   t        Force onscreen only those frames that are fully offscreen.
   nil      Do not force any frame back onscreen.
-  :all     Force onscreen any frame fully or partially offscreen.
+  all      Force onscreen any frame fully or partially offscreen.
   PRED     A function called with three arguments,
           - the live frame just restored,
           - a list (LEFT TOP WIDTH HEIGHT), describing the frame,
           - a list (LEFT TOP WIDTH HEIGHT), describing the workarea.
           It must return non-nil to force the frame onscreen, nil otherwise.

+CLEANUP-FRAMES allows to \"clean up\" the frame list after restoring
a frameset:
+ t        Delete all frames that were not created or restored upon.
+ nil      Keep all frames.
+ FUNC     A function called with two arguments:
+          - FRAME, a live frame.
+          - ACTION, which can be one of
+            :rejected  Frame existed, but was not a candidate for reuse.
+            :ignored   Frame existed, was a candidate, but wasn't reused.
+            :reused    Frame existed, was a candidate, and restored upon.
+            :created   Frame didn't exist, was created and restored upon.
+          Return value is ignored.
+
 Note the timing and scope of the operations described above: REUSE-FRAMES
 affects existing frames; PREDICATE, FILTERS and FORCE-DISPLAY affect the frame
-being restored before that happens; and FORCE-ONSCREEN affects the frame once
-it has been restored.
+being restored before that happens; FORCE-ONSCREEN affects the frame once
+it has been restored; and CLEANUP-FRAMES affects all frames alive after the
+restoration, including those that have been reused or created anew.

 All keyword parameters default to nil."

   (cl-assert (frameset-valid-p frameset))

-  (let (other-frames)
+  (let* ((frames (frame-list))
+        (frameset--action-map (make-hash-table :test #'eq))
+        ;; frameset--reuse-list is a list of frames potentially
reusable.  Later we
+        ;; will decide which ones can be reused, and how to deal with
any leftover.
+        (frameset--reuse-list
+         (pcase reuse-frames
+           (`t
+            frames)
+           (`nil
+            nil)
+           (`match
+            (cl-loop for (state) in (frameset-states frameset)
+                     when (frameset-frame-with-id (frameset-cfg-id
state) frames)
+                     collect it))
+           ((pred functionp)
+            (cl-remove-if-not reuse-frames frames))
+           (_
+            (error "Invalid arg :reuse-frames %s" reuse-frames)))))

-    ;; frameset--reuse-list is a list of frames potentially reusable.  Later we
-    ;; will decide which ones can be reused, and how to deal with any leftover.
-    (pcase reuse-frames
-      ((or `nil `:keep)
-       (setq frameset--reuse-list nil
-            other-frames (frame-list)))
-      ((pred consp)
-       (setq frameset--reuse-list (copy-sequence reuse-frames)
-            other-frames (cl-delete-if (lambda (frame)
-                                         (memq frame frameset--reuse-list))
-                                       (frame-list))))
-      (_
-       (setq frameset--reuse-list (frame-list)
-            other-frames nil)))
+    ;; Mark existing frames in the map; candidates to reuse are
marked as :ignored;
+    ;; they will be reassigned later, if chosen.
+    (dolist (frame frames)
+      (puthash frame
+              (if (memq frame frameset--reuse-list) :ignored :rejected)
+              frameset--action-map))

     ;; Sort saved states to guarantee that minibufferless frames will
be created
     ;; after the frames that contain their minibuffer windows.
@@ -1131,17 +1155,15 @@
                ;; - we're switching displays, and the user chose the
option to delete, or
                ;; - we're switching to tty, and the frame to restore
is minibuffer-only.
                (unless (and frameset--target-display
-                            (or (eq force-display :delete)
+                            (or (eq force-display 'delete)
                                 (and to-tty
                                      (eq (cdr (assq 'minibuffer
frame-cfg)) 'only))))
                  ;; To avoid duplicating frame ids after restoration,
we note any
                  ;; existing frame whose id matches a frame
configuration in the
                  ;; frameset.  Once the frame config is properly
restored, we can
                  ;; reset the old frame's id to nil.
-                 (setq duplicate (and other-frames
-                                      (or (eq reuse-frames :keep)
(consp reuse-frames))
-                                      (frameset-frame-with-id
(frameset-cfg-id frame-cfg)
-                                                              other-frames)))
+                 (setq duplicate (frameset-frame-with-id
(frameset-cfg-id frame-cfg)
+                                                         frames))
                  ;; Restore minibuffers.  Some of this stuff could be
done in a filter
                  ;; function, but it would be messy because restoring
minibuffers affects
                  ;; global state; it's best to do it here than add a
bunch of global
@@ -1187,20 +1209,30 @@
     ;; other frames are already visible (discussed in thread for bug#14841).
     (sit-for 0 t)

-    ;; Delete remaining frames, but do not fail if some resist being deleted.
-    (unless (eq reuse-frames :keep)
-      (dolist (frame (sort (nconc (if (listp reuse-frames) nil other-frames)
-                                 frameset--reuse-list)
-                          ;; Minibufferless frames must go first to avoid
-                          ;; errors when attempting to delete a frame whose
-                          ;; minibuffer window is used by another frame.
-                          #'frameset-minibufferless-first-p))
-       (condition-case err
-           (delete-frame frame)
-         (error
-          (delay-warning 'frameset (error-message-string err))))))
-    (setq frameset--reuse-list nil
-         frameset--target-display nil)
+    ;; Clean temporary caches
+    (setq frameset--target-display nil)
+
+    (when cleanup-frames
+      (let ((map nil)
+           (cleanup (if (eq cleanup-frames t)
+                        (lambda (frame action)
+                          (when (memq action '(:rejected :ignored))
+                            (delete-frame frame)))
+                      cleanup-frames)))
+       ;; Prepare the frame-action map
+       (maphash (lambda (frame action)
+                  (push (cons frame action) map))
+                frameset--action-map)
+       ;; Clean frame list
+       (dolist (frame-action (cl-sort map
+                                     ;; Minibufferless frames must go
first to avoid
+                                     ;; errors when attempting to
delete a frame whose
+                                     ;; minibuffer window is used by
another frame.
+
#'frameset-minibufferless-first-p :key #'car))
+         (condition-case-unless-debug err
+             (funcall cleanup (car frame-action) (cdr frame-action))
+           (error
+            (delay-warning 'frameset (error-message-string err) :warning))))))

     ;; Make sure there's at least one visible frame.
     (unless (or (daemonp) (visible-frame-list))
@@ -1212,23 +1244,21 @@
 (defun frameset--jump-to-register (data)
   "Restore frameset from DATA stored in register.
 Called from `jump-to-register'.  Internal use only."
-  (let ((fs (aref data 0))
-       reuse-frames iconify-list)
-    (if current-prefix-arg
-       ;; Reuse all frames and delete any left unused
-       (setq reuse-frames t)
-      ;; Reuse matching frames and leave others to be iconified
-      (setq iconify-list (frame-list))
-      (dolist (state (frameset-states fs))
-       (let ((frame (frameset-frame-with-id (frameset-cfg-id (car state))
-                                            iconify-list)))
-         (when frame
-           (push frame reuse-frames)
-           (setq iconify-list (delq frame iconify-list))))))
-    (frameset-restore fs
-                     :filters frameset-session-filter-alist
-                     :reuse-frames reuse-frames)
-    (mapc #'iconify-frame iconify-list))
+  (frameset-restore
+   (aref data 0)
+   :filters frameset-session-filter-alist
+   :reuse-frames (if current-prefix-arg t 'match)
+   :cleanup-frames (if current-prefix-arg
+                      ;; delete frames
+                      nil
+                    ;; iconify frames
+                    (lambda (frame action)
+                      (pcase action
+                        (`rejected (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))))))

   ;; Restore selected frame, buffer and point.
   (let ((frame (frameset-frame-with-id (aref data 1)))

Re: Freezing frameset-restore

[Stefan Monnier]

> Eh, no.  As I said already, processing the frames in random (maphash)
> order just isn't going to cut it if you want to delete frames.

Ah, then we need to fix the docstring and specify that the order of
elements in the returned list is not arbitrary but provides some kind of
guarantee (which we need to describe).  Yuck.

> That would be an argument to split every long function along its
> length, just by looking for appropriate places where it can be
> "cleanly" done,

Indeed, it is.

> but that's not an argument that I can accept.
> Restoring a frameset is going from one frame configuration to another
> one; cleanup isn't just something that gets accidentally done
> afterwards.

And starting a new process is "one operation" but some people think it's
much cleaner to do it as a bunch of simple syscalls rather than as one
giant can-do-anything monster.

Yes, every caller of frame-restore will probably want to cleanup
afterwards, but that doesn't mean we should provide "one function that
does it all".

>> Personally, I'd just ditch frameset-restore-cleanup.
> I'd ditch it too, for a CLEANUP arg to frameset-restore.

OK, but then make it very simple: it should be a function that takes
a frame and a symbol (the action that was taken on that frame) and its
return value is ignored.

> You can create an invisible frame, save it in a frameset, then delete
> it (so you have just one frame, visible), and then restore the
> frameset over your only frame.

But your frameset has 2 frames, so after restoration you should also
have 2 frames, no?


        Stefan

Re: Freezing frameset-restore

[Juanma Barranquero]

On Mon, Mar 10, 2014 at 9:32 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:

> Ah, then we need to fix the docstring and specify that the order of
> elements in the returned list is not arbitrary but provides some kind of
> guarantee (which we need to describe).  Yuck.

I'll add that to the appropriate place, once we agree about it.

> Yes, every caller of frame-restore will probably want to cleanup
> afterwards, but that doesn't mean we should provide "one function that
> does it all".

If you excuse my analogy, it's like changing a flat tyre. Of course it
can be divided into removing the old one, and putting the new one.
Other than a bit of sharing (the jack carries over from the first
operation to the second), they are separate operations. Yet no one
thinks of it that way because the user experience is "fixing the tyre
so the car can go again".

You think of frameset restoration like two operations: 1) restoring
frames, and 2) cleaning up frames. But frameset-restore is just one
operation: going from a frame configuration in your display(s) to
another, different one. 2) is as part of it as 1), and in most cases
(certainly, in *our* use cases) it is a non-null operation.

> OK, but then make it very simple: it should be a function that takes
> a frame and a symbol (the action that was taken on that frame) and its
> return value is ignored.

Currently (in my last patch), it's just that, plus the options t and
nil to simplify common use cases. Do you want me to remove the t/nil
options?

> But your frameset has 2 frames, so after restoration you should also
> have 2 frames, no?

If you're doing it via desktop.el, yes. But you can save a frameset
from a partial set of the current frames. And just in case you think
it's too artificial, I discovered the problem because it happened to
me (though I don't remember what was I doing at the time). And even if
you are saving the full frame list with desktop.el, if you switch
displays or something like that you can end restoring less frames than
originally saved, so it can definitely happen. It's better to protect
against the case, however unlikely, that present the user with an
invisible running Emacs.

    J

Re: Freezing frameset-restore

[Stefan Monnier]

> This is the patch I'm happy with.

Looks good, thank you,


        Stefan

Re: Freezing frameset-restore

[Juanma Barranquero]

On Tue, Mar 11, 2014 at 12:10 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:

> Looks good, thank you,

Great, thanks.  Committed.

    J