emacs-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Freezing frameset-restore


From: Juanma Barranquero
Subject: Freezing frameset-restore
Date: Thu, 6 Mar 2014 01:03:30 +0100

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  <address@hidden>

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]