On Thu, Feb 11, 2021 at 1:14 PM Bastian Beranek wrote: > > 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 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 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. I was wrong about the "tab-bar-mode is on by default" part, so maybe I like your suggestion more. I added: diff --git a/lisp/tab-bar.el b/lisp/tab-bar.el index 7afbb96212..93573d8a75 100644 --- a/lisp/tab-bar.el +++ b/lisp/tab-bar.el @@ -276,8 +276,9 @@ tab-bar-show :initialize 'custom-initialize-default :set (lambda (sym val) (set-default sym val) - ;; Recalculate tab-bar-lines for all frames - (tab-bar--update-tab-bar-lines t)) + (if val + (tab-bar-mode 1) + (tab-bar--update-tab-bar-lines t))) :group 'tab-bar :version "27.1") with respect to v6, v7 is attached. > > > > > > @@ -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.