Re: [PATCH] tab-bar.el: add defcustoms for functions to call after opening and before closing

[Robert Cochran]

Juri Linkov <address@hidden> writes:

>> To be clear, `tab-bar-tab-pre-close-functions` didn't go away. I'm not
>> exactly sure of a situation where it really really matters from the
>> perspective of a hook function whether or not it's clean up task happens
>> before or after a tab is technically closed, but I'm not opposed to
>> having both a pre and post close hook. However, I do believe that
>> there's more information available to a hook function about a tab before
>> the tab is formally closed.
>
> Does tab-bar-tab-prevent-close-functions have the same information?
> Then why should we have two exactly the same pre-hooks?
>
> To me it makes more sense cleaning the workplace after the work is done,
> not before.  So killing the buffers is safer after closing the tab.
> If something goes wrong, then buffers are still here when the tab is not 
> closed.
> Or are there some technical problems with using a post-close hook for such 
> task?

pre-close-functions and prevent-close-functions get all of the same
information, yes. I decided to separate them into two separate things
because otherwise you run into function dependency issues.

For example, if tab-bar-pre-close-functions worked the way I had
original defined it, where it did double duty of what I'm now proposing
to be pre-close-functions and prevent-close-functions, then if the hook
were to look something like this:

;; From the examples in the last email
(#'tab-ex/kill-non-file-buffers-in-tab #'tab-ex/tab-has-unsaved-files)

then when closing a tab, you'd first kill all of the non-file buffers,
and then check to see if you should close the tab based on whether or
not there are any visible unsaved buffers. But at this point, you've
already run cleanup functions regardless of whether or not you
ultimately decide to close the tab. Mixing cleanup tasks and deciding
whether or not to close the tab leads to weird edge-cases and being
dependent on function ordering. I think that's too much complexity to
wrangle in a single hook.

The only 'technical' problem with a post-close hook is that the tab
information is now gone because it's been deleted from the list. A hook
gets slightly less contextual information if we decide to hold off on
running the hook until the end. While I'll agree that it might make a
little more sense to run the hook at the end, I'm going to counter by
saying that we should arrange for the hook to be able to get as much
information as it can get, because you'll never know when someone wants
all of it. It's better to have to filter out information you don't need
than to be stuck without the information you do need.

>> Perhaps then we should also split `tab-bar-tab-post-open-functions` into
>> pre and post variants (run before and after the tab is added to the
>> frame parameters respectively)? I won't deny that an arrangement like
>> that could possibly be confusing (Ex: "why is my hook not working to rename
>> my tab?" -> "oh, you have to put that in the pre open hook, not the post
>> open hook"),
>
> Why tab renaming works only in the pre-open hook, but not in the post-open 
> hook?
> I see no possible reasons why this shouldn't be possible.
>
>> but I personally find that a more attractive alternative
>> that expecting hooks to have to grovel through the frame parameters
>> themselves or to write a bunch of accessor functions to cover all the
>> possible use-cases.
>
> I think the user should have the final word - after the command that
> created a new tab and added it to the frame's tab list, the user
> should have freedom to customize the result of the command using hooks.
> This is possible only when the hook is run at the very end of the command.
>

I tested this again just now and found out that it actually does work to
just get the alist from frame-parameter and modify it directly. I had an
incorrect understanding about what did and did not work. My appologies;
I thought I had tested that better.  With that misunderstanding cleared
up, my original point is moot, and moving the call to
run-hooks-with-args to after the tab is put into the frame-parameters
list isn't problematic - life is still easy to modify tab parameters.
I've fixed it in this version of the patch.

Thanks,
-- 
~Robert Cochran

>From 0ec05e98ad28c5e5c96c2392dd971a14748fdcb9 Mon Sep 17 00:00:00 2001
From: Robert Cochran <address@hidden>
Date: Fri, 8 Nov 2019 11:29:43 -0800
Subject: [PATCH] Add hooks for after tab open, before close, and to prevent
 closing

* lisp/tab-bar.el (tab-bar-tab-post-open-functions,
tab-bar-tab-prevent-close-functions, tab-bar-tab-pre-close-functions):
New defcustoms
(tab-bar-new-tab-to, tab-bar-close-tab): Use new defcustoms
---
 lisp/tab-bar.el | 134 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 88 insertions(+), 46 deletions(-)

diff --git a/lisp/tab-bar.el b/lisp/tab-bar.el
index acc4304def..ba4bc0afcd 100644
--- a/lisp/tab-bar.el
+++ b/lisp/tab-bar.el
@@ -692,6 +692,15 @@ tab-bar-new-tab-to
   :group 'tab-bar
   :version "27.1")
 
+(defcustom tab-bar-tab-post-open-functions nil
+  "List of functions to call after creating a new tab.
+The current tab is supplied as an argument. Any modifications
+made to the tab argument will be applied after all functions are
+called."
+  :type '(repeat function)
+  :group 'tab-bar
+  :version "27.1")
+
 (defun tab-bar-new-tab-to (&optional to-index)
   "Add a new tab at the absolute position TO-INDEX.
 TO-INDEX counts from 1.  If no TO-INDEX is specified, then add
@@ -726,9 +735,13 @@ tab-bar-new-tab-to
                           ('right (1+ (or from-index 0)))))))
       (setq to-index (max 0 (min (or to-index 0) (length tabs))))
       (cl-pushnew to-tab (nthcdr to-index tabs))
+
       (when (eq to-index 0)
         ;; pushnew handles the head of tabs but not frame-parameter
-        (set-frame-parameter nil 'tabs tabs)))
+        (set-frame-parameter nil 'tabs tabs))
+
+      (run-hook-with-args 'tab-bar-tab-post-open-functions
+                          (nth to-index tabs)))
 
     (when (and (not tab-bar-mode)
                (or (eq tab-bar-show t)
@@ -780,6 +793,24 @@ tab-bar-close-last-tab-choice
   :group 'tab-bar
   :version "27.1")
 
+(defcustom tab-bar-tab-prevent-close-functions nil
+  "List of functions to call to determine whether to close a tab.
+The tab to be closed and a boolean indicating whether or not it
+is the only tab in the frame are supplied as arguments. If any
+function returns a non-nil value, the tab will not be closed."
+  :type '(repeat function)
+  :group 'tab-bar
+  :version "27.1")
+
+(defcustom tab-bar-tab-pre-close-functions nil
+  "List of functions to call before closing a tab.
+The tab to be closed and a boolean indicating whether or not it
+is the only tab in the frame are supplied as arguments,
+respectively."
+  :type '(repeat function)
+  :group 'tab-bar
+  :version "27.1")
+
 (defun tab-bar-close-tab (&optional arg to-index)
   "Close the tab specified by its absolute position ARG.
 If no ARG is specified, then close the current tab and switch
@@ -792,52 +823,63 @@ tab-bar-close-tab
   (interactive "P")
   (let* ((tabs (funcall tab-bar-tabs-function))
          (current-index (tab-bar--current-tab-index tabs))
-         (close-index (if (integerp arg) (1- arg) current-index)))
-    (if (= 1 (length tabs))
-        (pcase tab-bar-close-last-tab-choice
-          ('nil
-           (signal 'user-error '("Attempt to delete the sole tab in a frame")))
-          ('delete-frame
-           (delete-frame))
-          ('tab-bar-mode-disable
-           (tab-bar-mode -1))
-          ((pred functionp)
-           ;; Give the handler function the full extent of the tab's
-           ;; data, not just it's name and explicit-name flag.
-           (funcall tab-bar-close-last-tab-choice (tab-bar--tab))))
-
-      ;; More than one tab still open
-      (when (eq current-index close-index)
-        ;; Select another tab before deleting the current tab
-        (let ((to-index (or (if to-index (1- to-index))
-                            (pcase tab-bar-close-tab-select
-                              ('left (1- current-index))
-                              ('right (if (> (length tabs) (1+ current-index))
-                                          (1+ current-index)
-                                        (1- current-index)))
-                              ('recent (tab-bar--tab-index-recent 1 tabs))))))
-          (setq to-index (max 0 (min (or to-index 0) (1- (length tabs)))))
-          (tab-bar-select-tab (1+ to-index))
-          ;; Re-read tabs after selecting another tab
-          (setq tabs (funcall tab-bar-tabs-function))))
-
-      (let ((close-tab (nth close-index tabs)))
-        (push `((frame . ,(selected-frame))
-                (index . ,close-index)
-                (tab . ,(if (eq (car close-tab) 'current-tab)
-                            (tab-bar--tab)
-                          close-tab)))
-              tab-bar-closed-tabs)
-        (set-frame-parameter nil 'tabs (delq close-tab tabs)))
-
-      (when (and tab-bar-mode
-                 (and (natnump tab-bar-show)
-                      (<= (length tabs) tab-bar-show)))
-        (tab-bar-mode -1))
+         (close-index (if (integerp arg) (1- arg) current-index))
+         (last-tab-p (= 1 (length tabs)))
+         (prevent-close (run-hook-with-args-until-success
+                         'tab-bar-tab-prevent-close-functions
+                         (nth close-index tabs)
+                         last-tab-p)))
+
+    (unless prevent-close
+      (run-hook-with-args 'tab-bar-tab-pre-close-functions
+                          (nth close-index tabs)
+                          last-tab-p)
+
+      (if last-tab-p
+          (pcase tab-bar-close-last-tab-choice
+            ('nil
+             (user-error "Attempt to delete the sole tab in a frame"))
+            ('delete-frame
+             (delete-frame))
+            ('tab-bar-mode-disable
+             (tab-bar-mode -1))
+            ((pred functionp)
+             ;; Give the handler function the full extent of the tab's
+             ;; data, not just it's name and explicit-name flag.
+             (funcall tab-bar-close-last-tab-choice (tab-bar--tab))))
+
+        ;; More than one tab still open
+        (when (eq current-index close-index)
+          ;; Select another tab before deleting the current tab
+          (let ((to-index (or (if to-index (1- to-index))
+                              (pcase tab-bar-close-tab-select
+                                ('left (1- current-index))
+                                ('right (if (> (length tabs) (1+ 
current-index))
+                                            (1+ current-index)
+                                          (1- current-index)))
+                                ('recent (tab-bar--tab-index-recent 1 
tabs))))))
+            (setq to-index (max 0 (min (or to-index 0) (1- (length tabs)))))
+            (tab-bar-select-tab (1+ to-index))
+            ;; Re-read tabs after selecting another tab
+            (setq tabs (funcall tab-bar-tabs-function))))
+
+        (let ((close-tab (nth close-index tabs)))
+          (push `((frame . ,(selected-frame))
+                  (index . ,close-index)
+                  (tab . ,(if (eq (car close-tab) 'current-tab)
+                              (tab-bar--tab)
+                            close-tab)))
+                tab-bar-closed-tabs)
+          (set-frame-parameter nil 'tabs (delq close-tab tabs)))
+
+        (when (and tab-bar-mode
+                   (and (natnump tab-bar-show)
+                        (<= (length tabs) tab-bar-show)))
+          (tab-bar-mode -1))
 
-      (force-mode-line-update)
-      (unless tab-bar-mode
-        (message "Deleted tab and switched to %s" tab-bar-close-tab-select)))))
+        (force-mode-line-update)
+        (unless tab-bar-mode
+          (message "Deleted tab and switched to %s" 
tab-bar-close-tab-select))))))
 
 (defun tab-bar-close-tab-by-name (name)
   "Close the tab by NAME."
-- 
2.23.0