Hello Juri,

I have updated my patch (see v6 attached). Please find my comments inline.

I have also finished the copyright assignment procedure now.

On Wed, Feb 10, 2021 at 7:41 PM Juri Linkov <juri@linkov.net> wrote:
> I don't know if Martin will agree, but most frame functions
> interpret their optional FRAME argument in such a way that
> if it's nil or omitted, FRAME defaults to the selected frame.
>
> For example, 'set-frame-font' documents this:
>
>   If FRAMES is nil, apply the font to the selected frame only.
>   If FRAMES is non-nil, it should be a list of frames to act upon,
>   or t meaning all existing graphical frames.
>
> and uses such implementation:
>
>    (let ((frame-lst (cond ((null frames)
>                            (list (selected-frame)))
>                           ((eq frames t)
>                            (frame-list))
>                           (t frames))))
>       (dolist (f frame-lst)

That's true and I noticed this inconsistency as well. So thanks for
the suggestion, I have updated the patch accordingly.

On Wed, Feb 10, 2021 at 7:41 PM Juri Linkov <juri@linkov.net> wrote:
>
> > Hope this is satisfactory, if not please feel free to adjust as you wish.
>
> Thanks, please see more comments:
>
> > +(defun tab-bar--tab-bar-lines-for-frame (frame)
> > +  (if (not tab-bar-mode)
> > +      0
> > +    (cond
> > +     ((eq tab-bar-show t) 1)
> > +     ((natnump tab-bar-show)
> > +      (if (> (length (funcall tab-bar-tabs-function frame)) tab-bar-show) 1 0))
> > +     (t 0))))
>
> A small optimization:
>
>   ((not tab-bar-mode) 0)
>
> could be added as the first condition of the same 'cond'.

Thanks! Done.

>
> >    :set (lambda (sym val)
> >           (set-default sym val)
> >           ;; Preload button images
> > +         ;; Note: tab-bar-mode updates tab-bar-lines as well.
> > +         (tab-bar-mode 1))
>
> Not sure whether the users would want to enable tab-bar-mode
> unconditionally after customizing tab-bar-show.
>
> Maybe when customized tab-bar-show to nil, only call
> tab-bar--update-tab-bar-lines in all frames?
> Or maybe simply to disable the tab bar with (tab-bar-mode 0)
> when customized to nil?

I am not sure either. I think the best is to just leave tab-bar-mode
as it is to be honest. All this entanglement doesn't seem very clean
to me. Yes this would mean that the user needs to manually enable
tab-bar-mode after customizing the variable, but on the other hand
tab-bar-mode is on by default, so the user must have switched it off
in his .emacs by choice. So I just added the call the
tab-bar--update-tab-bar-lines for all frames, because this is
necessary for sure. On the other hand I don't fully understand the
comment about 'Preload button images'. I think the images and
keybindings are loaded when tab-bar-mode is switched on and afterwards
whenever a new tab is created in tab-bar-new-to, so it seems
independent of tab-bar-show. When tab-bar-show is customized they are
either already loaded because tab-bar-mode is on, or if it is not they
are not required and will be loaded when tab-bar-mode is activated.

>
> > @@ -852,16 +867,15 @@ After the tab is created, the hooks in
> > +    ;; Switch on tab-bar-mode, since a tab was created
> > +    (when tab-bar-show
> >        (tab-bar-mode 1))
> > +
> > +    ;; Recalculate tab-bar-lines and update frames
> > +    (tab-bar--update-tab-bar-lines (selected-frame))
> > +    (when tab-bar-mode
> > +      (tab-bar--load-buttons)
> > +      (tab-bar--define-keys))
>
> Would you agree that here in tab-bar-new-tab-to, the first call of
> tab-bar-mode should already do all these calls: tab-bar--update-tab-bar-lines,
> tab-bar--load-buttons, tab-bar--define-keys?  So maybe it should be
> sufficient just to leave these 2 lines here:
>
>     (when tab-bar-show
>        (tab-bar-mode 1))

Yes I agree that tab-bar--update-tab-bar-lines is not needed. It
happens in the line before when tab-bar-show is not nil and doesn't
matter otherwise. I have left these two lines, though:

    (when tab-bar-mode
      (tab-bar--load-buttons)
      (tab-bar--define-keys))

Because I think defining the keys is useful even if tab-bar-show is
nil, so you can switch to another tab using the key bindings even if
you can't see the tab-bar. As for the buttons, I think it makes sense
to load them so that in case tab-bar-show is customized to another
value afterwards they are available directly.