From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Juanma Barranquero Newsgroups: gmane.emacs.devel Subject: Freezing frameset-restore Date: Thu, 6 Mar 2014 01:03:30 +0100 Message-ID: NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 X-Trace: ger.gmane.org 1394064251 18357 80.91.229.3 (6 Mar 2014 00:04:11 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 6 Mar 2014 00:04:11 +0000 (UTC) To: Emacs developers Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Mar 06 01:04:20 2014 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1WLLnP-0002Pl-Da for ged-emacs-devel@m.gmane.org; Thu, 06 Mar 2014 01:04:19 +0100 Original-Received: from localhost ([::1]:55109 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLLnP-0007u2-0a for ged-emacs-devel@m.gmane.org; Wed, 05 Mar 2014 19:04:19 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:33958) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLLnK-0007tt-Rv for emacs-devel@gnu.org; Wed, 05 Mar 2014 19:04:17 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WLLnI-0004Xy-Rj for emacs-devel@gnu.org; Wed, 05 Mar 2014 19:04:14 -0500 Original-Received: from mail-yk0-x231.google.com ([2607:f8b0:4002:c07::231]:49148) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLLnI-0004X1-Km for emacs-devel@gnu.org; Wed, 05 Mar 2014 19:04:12 -0500 Original-Received: by mail-yk0-f177.google.com with SMTP id q200so4745447ykb.8 for ; Wed, 05 Mar 2014 16:04:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:from:date:message-id:subject:to:content-type; bh=kS2KxD40ROZlJL/P2kapKcdJOH+DWAIQBskJS8oJsWA=; b=GjNMUwodqFEyYzYciMG+Ssln9ChLcJm/6zLw8jIc2tUtGu3viZqLOmmnOAnjIVmbvf Bw6ChNUMHUKBDA4WwgKgXb9qbVfU8veAoyZOYk6TaDeD2IFGXFMoPt5DQTayPKZvrsNr USvm7oCUxNl6XDnAJsHk1verk8qEhhmPLhaxEVGIG4UKZF7/CvsJdNo/0w5tz1t4t60F KhCLrP17T4H22VtS3ng34hFzJ+gS/Zon62WfZCRyZXNpLNcz+tICGNMbTKh55KOVTmWK GocSjsznPop1pdIwq2hDKJ2ORVK65B1GbgcRld6wScTNUS+BlIuamuWvnU23ASUlBiXl WeQw== X-Received: by 10.236.30.2 with SMTP id j2mr10305666yha.73.1394064251642; Wed, 05 Mar 2014 16:04:11 -0800 (PST) Original-Received: by 10.170.163.3 with HTTP; Wed, 5 Mar 2014 16:03:30 -0800 (PST) X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2607:f8b0:4002:c07::231 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:170169 Archived-At: 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 * 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'.