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

[Juri Linkov <juri@linkov.net>]

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

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?

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

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.

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

Right, this is better.  Thanks.