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

[Robert Cochran]

Juri Linkov <address@hidden> writes:

>> 1. Trying to make a single hook do two separate things (determining
>> whether to prevent tab closing and tasks on tab close) just made things
>> overly complicated. Tab close prevention has been split off into a new
>> hook, `tab-bar-prevent-close-functions'.
>
> Then better to have two hooks:
> tab-bar-tab-prevent-close-functions,
> tab-bar-tab-post-close-functions.
>
> The first can be used not only for close prevention, but also
> for other tasks when necessary.  But the primary hook for tasks
> on closing the tab could be the post hook invoked after the tab is closed.
> This helps to run tasks such as killing the buffers when no other
> remaining tabs still use the buffers after closing the tab.

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.

>> 2. I realized that it's possible when creating a new tab to just delay
>> the actual creation of the tab in the frame's tab list. This makes it
>> possible to directly modify the tab passed in to
>> tab-bar-tab-post-open-functions, ie `(setf (alist-get 'name tab) "New Name")`
>> from within a hook function. This means it's not really
>> necessary to make new accessors.
>
> It would be better to run the hook after the tab is added to
> the frame's tab list, not before.  This will widen the usability
> of the hook.  For example, when display-buffer-in-tab from
> (info "(gnus) Tabbed Interface") creates a new tab for Gnus Summary,
> I want to use such a new hook to move a new tab to the predefined place
> (to keep the same order of Summary tabs as groups are sorted in the
> Group buffer).  When the hook will be called after a new tab is added
> to the frame's tab list, it can be used to move a new tab automatically.

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"), 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.

>> New patch, as well as a file of examples for the new hooks, follow.
>> Again, these new hooks still need to be documented in the manual,
>> which I will be glad to do as soon as a design is nailed down.
>
> Thanks for the new patch.  An additional comment below.
>
>> +         (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 (= 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))))
>
> There is the same condition '(= 1 (length tabs))' used twice.
> This suggests that the code containing 'pcase tab-bar-close-last-tab-choice'
> could be moved to a new function added to tab-bar-tab-pre-close-functions
> by default.  It could check for '(= 1 (length tabs))', then does its pcase
> and returns nil to prevent other code in tab-bar-close-tab from running.
> This is just an optimization.

It's probably better to reuse the variable last-tab-p in this case,
yes. I'll make sure that gets fixed if the code remains in the next
patch revision, as well as simplify the signaling of the user-error. I'm
shying away from putting that code into a hook though because:

A) It might confuse someone who, for whatever reason, has managed to
remove the function handling `tab-bar-close-last-tab-choice` and is now
left wondering why the variable isn't being respected.

B) Part of the point of this particular segment of code was to fix a bug
where tabs lost their explicit names when the user attempted to close
them. The solution was having a well-defined point at which the function
is short-circuited if there is only one tab. This part of the logic is
dispatched by a defcustom because there were 3-4 ideas on what the right
answer was to the user closing the last time.

>> (defun tab-ex/tab-has-unsaved-files (tab _lastp)
>>   ;; Don't let the user close a tab if they have unsaved buffers, to
>>   ;; make sure they don't lose track of unsaved buffers, for
>>   ;; example. In this case, it doesn't matter if this tab is the last
>>   ;; one or not.
>>   (cl-loop for buffer in (tab-ex/tab-buffers tab)
>>            always (and (with-current-buffer buffer buffer-file-name)
>>                        (not (buffer-modified-p buffer)))))
>
> Good example - some editors also prepend "*" to the tab name
> of an unsaved file, and don't allow closing it without saving.

Sidenote for later, but it sounds like having hooks of the nature that
we're hashing out here would also be useful for tab-line-mode as
well. That's a different discussion though.

Thanks,
-- 
~Robert Cochran